refactor(help): a chord is recognised by its modifiers, not its spelling (improvise-thpe)

`compact` tested whether a label it had just formatted began with "Ctrl-" or
"Alt-". No false positive today — the reviewer enumerated every
`format_key_label` output — so this is the shape, not the behaviour.

`Hint::keys` is now `(KeyCode, KeyModifiers)` and `compact` takes the pairs, so
the string is not in scope to parse; `Hint::labels` and the help pages format
last through the one spelling in `keymap::key_label`. The regression guard is
the signature, as it is for `Checked`.

`Keymap::labels_for` — the "throw the modifiers away" step the ticket names —
has no caller left and is deleted: a consolidation that keeps its old path keeps
the defect available.

The rule also got wider and total over the bitset: a chord is any modifier
beyond shift. Injected a temporary `KeyModifiers::SUPER` spelled `Super-x` and
ran it — the old filter keeps it, `compact` drops it. Binding dump byte-identical
across the change (4406 lines).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH

context/code/protocol.md

@@ -1364,7 +1364,12 @@ That is the price of the keys being true.
 
 ### The three rules the rendering follows
 
-1. `hint_line` **drops chorded keys** (`Ctrl-`/`Alt-` prefixed). `T` advertises
+1. `hint_line` **drops chorded keys** — any key held with a modifier beyond
+   shift, asked of the **modifiers** and never of the label (improvise-thpe).
+   `Hint::keys` is `(KeyCode, KeyModifiers)` and `compact` takes the pairs, so
+   the string it used to parse is not in scope to parse; `Hint::labels` and the
+   help pages format last, through the one spelling in `keymap::key_label`.
+   `T` advertises
    tile select; its four Ctrl-arrow aliases would push the rest of the line off
    the screen and teach nobody anything. A client wanting all of them reads
    `hints()`, which keeps them.

context/test-evidence/protocol.md

@@ -462,3 +462,43 @@ against an enum that really did have the variant.
 
 Proved discriminating by re-adding the variant with an empty `apply` arm: the
 retired spelling parses again and the test fails on `expect_err`.
+
+### a_chord_is_recognised_by_its_modifiers_and_not_by_its_label
+
+`help::compact` dropped a chorded key from a status-bar hint by testing whether
+the formatted label started with `"Ctrl-"` or `"Alt-"` — parsing a string it had
+just formatted. Finding 3 of the adversarial review of PR #50, filed as
+improvise-thpe and deliberately not fixed there: the reviewer enumerated every
+`format_key_label` output and none begins with either, so there was **no false
+positive**. The defect was the shape.
+
+**The regression guard is the signature**, as it is for `Checked` (gotcha 43):
+`compact` now takes `&[(KeyCode, KeyModifiers)]`, so the string it used to parse
+is not in scope to parse. `keys_for` returns the pairs and `Hint::keys` carries
+them; `help.rs` formats last, through `Hint::labels` and the one spelling in
+`keymap::key_label`. `Keymap::labels_for` — the "throw the modifiers away into a
+string" step the ticket names — has **no caller left and is deleted**, because a
+consolidation that keeps its old path keeps the defect available
+(`doctrine(a_consolidation_that_keeps_its_compatibility_test_is_unfinished)`).
+
+The rule also got wider, and that is the part a test can state: a chord is any
+modifier **beyond shift**, which is total over the bitset, where the string test
+enumerated two prefixes. Shift alone is not a chord — it is how a key is named
+rather than how it is held, which `S-Tab` is the case for.
+
+**Injected fault, run rather than argued.** A `KeyModifiers::SUPER` bit and a
+`key_label` arm spelling it `Super-x` were added temporarily. The old filter
+keeps it — `["T", "Super-x"]`, two survivors where one is wanted — while
+`compact` returns `"T"`. That is the ticket's own reproduction, executed, and it
+is the only way to see the difference: with `KeyModifiers` as shipped, every
+chord's label *does* begin with `Ctrl-` or `Alt-`, so no permanent test can
+separate the two rules. Both the bit and the probe were removed afterwards.
+
+One behaviour that had to be preserved deliberately: `keys_for` used to `dedup`
+**labels**, and deduping the pairs instead would let two patterns that spell the
+same key both reach the status bar. It now dedups by label explicitly.
+
+The binding dump is byte-identical across this change (4406 lines, `diff`
+clean), which is what `doctrine(binding_dump_diff)` asks of anything touching
+the keymap: the dump's prefix rows go through `Hint`, so they had to be checked
+rather than assumed.

crates/improvise-command/src/command/keymap.rs

@@ -861,7 +861,7 @@ mod binding_dump {
                             let rows: Vec<(Vec<String>, &str)> =
                                 improvise_protocol::help::which_key(sub)
                                     .into_iter()
-                                    .map(|hint| (hint.keys, hint.command))
+                                    .map(|hint| (hint.labels(), hint.command))
                                     .collect();
                             format!("prefix {rows:?}")
                         }

crates/improvise-protocol/src/help.rs

@@ -1,19 +1,34 @@
 //! What a binding is called and what it does, in words: the status-bar hint,
 //! the which-key popup and the help pages. See `context/code/protocol.md` §8.
 
-use crate::keymap::{Binding, KeyPattern, Keymap, Keymaps, ModeKey};
+use crate::key::{KeyCode, KeyModifiers};
+use crate::keymap::{Binding, KeyPattern, Keymap, Keymaps, ModeKey, key_label};
 use crate::{AppMode, HELP_PAGE_COUNT};
 
 /// One advertised binding. `command` is the semantic value — a localized or
 /// non-terminal front end keys off it and ignores `caption`.
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct Hint {
-    /// The keys that reach it, derived from the keymap and never spelled.
+    /// The keys that reach it, as the keymap holds them. A renderer spells them
-    pub keys: Vec<String>,
+    /// with [`Hint::labels`]; nothing reads a modifier back out of a label.
+    pub keys: Vec<(KeyCode, KeyModifiers)>,
     pub command: &'static str,
     pub caption: &'static str,
 }
 
+impl Hint {
+    /// This hint's keys, spelled. The one spelling is `keymap::key_label`.
+    pub fn labels(&self) -> Vec<String> {
+        self.keys.iter().map(|(c, m)| key_label(c, *m)).collect()
+    }
+}
+
+/// A key held with a modifier the status bar will not spell. Asked of the
+/// modifiers, never of the label — `context/code/protocol.md` §8.
+fn is_chord(mods: KeyModifiers) -> bool {
+    mods != KeyModifiers::NONE && mods != KeyModifiers::SHIFT
+}
+
 /// Short uppercase name for the title bar. Moved off [`AppMode`] so the enum
 /// that crosses the wire carries no English (improvise-k099).
 pub fn mode_label(mode: &AppMode) -> &'static str {
@@ -187,11 +202,11 @@ pub fn hint_line(mode: &AppMode) -> String {
 
 /// The status bar's rendering of one hint's keys: chords dropped, the rest
 /// run together when every one is a single character. §8 says why.
-fn compact(keys: &[String]) -> String {
+fn compact(keys: &[(KeyCode, KeyModifiers)]) -> String {
-    let plain: Vec<&str> = keys
+    let plain: Vec<String> = keys
         .iter()
-        .map(String::as_str)
+        .filter(|(_, mods)| !is_chord(*mods))
-        .filter(|k| !k.starts_with("Ctrl-") && !k.starts_with("Alt-"))
+        .map(|(code, mods)| key_label(code, *mods))
         .collect();
     match plain.iter().all(|k| k.chars().count() == 1) {
         true => plain.concat(),
@@ -201,13 +216,19 @@ fn compact(keys: &[String]) -> String {
 
 /// Every key that reaches `command` with `args` in `mode`, parents included,
 /// sorted by label so the answer does not move between processes (gotcha 18).
-pub fn keys_for(mode: &ModeKey, command: &str, args: Option<&[String]>) -> Vec<String> {
+pub fn keys_for(
+    mode: &ModeKey,
+    command: &str,
+    args: Option<&[String]>,
+) -> Vec<(KeyCode, KeyModifiers)> {
     let mut keys = match Keymaps::shared().get(mode) {
-        Some(keymap) => keymap.labels_for(command, args),
+        Some(keymap) => keymap.keys_running(command, args),
         None => Vec::new(),
     };
-    keys.sort();
+    keys.sort_by_key(|(code, mods)| key_label(code, *mods));
-    keys.dedup();
+    // By label, not by pattern: two patterns can spell the same key, and the
+    // status bar drew it twice when they did.
+    keys.dedup_by_key(|(code, mods)| key_label(code, *mods));
     keys
 }
 
@@ -227,13 +248,13 @@ pub fn which_key(keymap: &Keymap) -> Vec<Hint> {
                 Binding::Prefix(_) => return None,
             };
             Some(Hint {
-                keys: vec![crate::keymap::key_label(code, *mods)],
+                keys: vec![(*code, *mods)],
                 command,
                 caption: describe(command),
             })
         })
         .collect();
-    hints.sort_by(|a, b| a.keys.cmp(&b.keys));
+    hints.sort_by_key(Hint::labels);
     hints
 }
 
@@ -317,7 +338,7 @@ fn keys(
     match found.is_empty() {
         true => None,
         false => Some(HelpLine::Keys {
-            keys: found,
+            keys: found.iter().map(|(c, m)| key_label(c, *m)).collect(),
             description,
         }),
     }
@@ -827,7 +848,7 @@ mod tests {
             .expect("Normal advertises the cursor");
         for key in ["h", "j", "k", "l"] {
             assert!(
-                nav.keys.iter().any(|k| k == key),
+                nav.labels().iter().any(|k| k == key),
                 "{key} is bound to move-selection and must appear: {:?}",
                 nav.keys
             );
@@ -864,19 +885,50 @@ mod tests {
     /// keys that are all chords leave nothing to draw.
     #[test]
     fn the_compact_rendering_drops_chords_and_says_nothing_when_all_are() {
-        let of = |ks: &[&str]| compact(&ks.iter().map(|k| (*k).to_string()).collect::<Vec<_>>());
+        const NONE: KeyModifiers = KeyModifiers::NONE;
-        assert_eq!(of(&["h", "j", "k", "l"]), "hjkl");
+        const CTRL: KeyModifiers = KeyModifiers::CONTROL;
-        assert_eq!(of(&["Esc", "Tab"]), "Esc/Tab");
+        const ALT: KeyModifiers = KeyModifiers::ALT;
+        let ch = |c: char, m| (KeyCode::Char(c), m);
+        assert_eq!(compact(&['h', 'j', 'k', 'l'].map(|c| ch(c, NONE))), "hjkl");
         assert_eq!(
-            of(&["T", "Ctrl-\u{2190}", "Alt-x"]),
+            compact(&[(KeyCode::Esc, NONE), (KeyCode::Tab, NONE)]),
+            "Esc/Tab"
+        );
+        assert_eq!(
+            compact(&[ch('T', NONE), (KeyCode::Left, CTRL), ch('x', ALT)]),
             "T",
             "a chord is not worth the width it costs in a status bar"
         );
         assert_eq!(
-            of(&["Ctrl-d", "Ctrl-u"]),
+            compact(&[ch('d', CTRL), ch('u', CTRL)]),
             "",
             "and all-chords is empty rather than a bare colon"
         );
+        assert_eq!(
+            compact(&[(KeyCode::BackTab, KeyModifiers::SHIFT)]),
+            "S-Tab",
+            "shift alone is not a chord — it is how a key is named, not held"
+        );
+    }
+
+    /// A chord is dropped by its MODIFIERS, not by its spelling — the defect
+    /// improvise-thpe closed. See `context/test-evidence/protocol.md`.
+    #[test]
+    fn a_chord_is_recognised_by_its_modifiers_and_not_by_its_label() {
+        let exotic = KeyModifiers::CONTROL | KeyModifiers::ALT | KeyModifiers::SHIFT;
+        assert!(
+            is_chord(exotic),
+            "any modifier beyond shift is a chord, whatever it is called"
+        );
+        assert_eq!(
+            compact(&[
+                (KeyCode::Char('T'), KeyModifiers::NONE),
+                (KeyCode::F(5), exotic)
+            ]),
+            "T",
+            "the label is built from the modifiers, so nothing may read them \
+             back out of it"
+        );
     }
 
     /// A caption is looked up by **command name**, so a localized front end

crates/improvise-protocol/src/keymap.rs

@@ -235,15 +235,6 @@ impl Keymap {
         keys
     }
 
-    /// The same keys, spelled. One walk, so a caller asking "which keys" and
-    /// a caller asking "how are they written" cannot get different answers.
-    pub fn labels_for(&self, command: &str, args: Option<&[String]>) -> Vec<String> {
-        self.keys_running(command, args)
-            .iter()
-            .map(|(code, mods)| key_label(code, *mods))
-            .collect()
-    }
-
     /// Look up the binding for a key in this keymap's own bindings.
     fn lookup_local(&self, key: KeyCode, mods: KeyModifiers) -> Option<&Binding> {
         self.bindings

src/ui/which_key.rs

@@ -28,7 +28,7 @@ impl Widget for WhichKeyWidget<'_> {
         let content_width = self
             .hints
             .iter()
-            .map(|hint| hint.keys.concat().len() + 2 + hint.caption.len())
+            .map(|hint| hint.labels().concat().len() + 2 + hint.caption.len())
             .max()
             .unwrap_or(10);
         let popup_w = (content_width as u16 + 4).min(area.width); // +4 for border + padding
@@ -58,7 +58,7 @@ impl Widget for WhichKeyWidget<'_> {
                 break;
             }
             let y = inner.y + i as u16;
-            buf.set_string(inner.x + 1, y, hint.keys.concat(), key_style);
+            buf.set_string(inner.x + 1, y, hint.labels().concat(), key_style);
             let cmd_x = inner.x + 4; // fixed column for what the key does
             if cmd_x < inner.x + inner.width {
                 buf.set_string(cmd_x, y, hint.caption, cmd_style);