From 8772db2e7b8bb43550df61f5cbe96123ae4f7753 Mon Sep 17 00:00:00 2001
From: Edward Langley <el-github@elangley.org>
Date: Tue, 22 Sep 2026 22:08:42 -0700
Subject: [PATCH] A keys row is drawn only if the builders asked for it (ssoe)

KeyRow::looked_up is private to key_row, whose keys() is the lookup
pages() hands the builders; the reviewer's direct call is E0624. A new
test asserts the drawn keys rows equal help_bindings(), which catches
the remaining self::keys bypass.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
---
 context/test-evidence/protocol.md     | 21 +++++++++++++++
 crates/improvise-protocol/src/help.rs | 38 ++++++++++++++++++---------
 2 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md
index 53729978..db8c4ada 100644
--- a/context/test-evidence/protocol.md
+++ b/context/test-evidence/protocol.md
@@ -1088,3 +1088,24 @@ failed: ex_usage_is_placeholders("q  or press Q")`. The rule cost one row:
 `:add-items <cat> a b c ...` had three bare words and is now
 `:add-items <cat> <item> ...`. This test pins the rule the macro asks, in both
 directions, including that shipped usage and the rival.
+
+### help::tests::every_keys_row_drawn_is_one_the_builders_asked_for
+
+improvise-ssoe, found in re-review of #180 at 544744c. `KeyRow` closed
+hand-typed keys, but `KeyRow::looked_up` was `pub(super)`, so a page builder
+could call it directly instead of through its `keys` parameter — and
+`help_bindings()` records only what goes through the parameter. Injected into
+`editing()`: `KeyRow::looked_up(n, "clear-cell", None, "Edit the current
+cell")`, which captions `x` as editing the cell — the exact lie `jhdf`'s test
+exists for — and the whole workspace stayed green, because both guards that
+would object read `help_bindings()`.
+
+Two changes. `looked_up` is now private to the `key_row` module, whose one
+caller is the module's own `keys` — the lookup `pages()` hands every builder —
+so the reviewer's injection is `E0624: associated function looked_up is
+private`. That narrows the door without closing it: a builder can still reach
+the module-level `keys` by path (`self::keys(..)`) rather than through its
+shadowing parameter. So this test states the premise directly — the number of
+keys rows drawn equals the number of rows the builders asked for (every asked
+row finds its keys, by `every_keys_row_the_pages_ask_for_finds_its_keys`) —
+and the `self::keys` bypass fails it `51 != 50`.
diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs
index 017bff2a..9382a557 100644
--- a/crates/improvise-protocol/src/help.rs
+++ b/crates/improvise-protocol/src/help.rs
@@ -324,11 +324,12 @@ pub struct HelpPage {
 }
 
 pub use key_row::KeyRow;
+use key_row::keys;
 
 /// The one door to a keys row: its fields are private to this module, and its
 /// only constructor looks the keys up.
 mod key_row {
-    use super::{ModeKey, key_label, keys_for, prefixed_labels};
+    use super::{HelpLine, ModeKey, key_label, keys_for, prefixed_labels};
 
     #[derive(Debug, Clone, PartialEq, Eq)]
     pub struct KeyRow {
@@ -347,7 +348,7 @@ mod key_row {
 
         /// An unbound command yields no keys and no row, so the page never
         /// advertises a key that does not exist.
-        pub(super) fn looked_up(
+        fn looked_up(
             mode: &ModeKey,
             command: &str,
             args: Option<&[String]>,
@@ -361,17 +362,18 @@ mod key_row {
             (!keys.is_empty()).then_some(Self { keys, description })
         }
     }
-}
 
-/// A keys row, looked up in `mode`.
-fn keys(
-    mode: &ModeKey,
-    command: &'static str,
-    args: Option<&[&str]>,
-    description: &'static str,
-) -> Option<HelpLine> {
-    let args: Option<Vec<String>> = args.map(|a| a.iter().map(|s| (*s).to_string()).collect());
-    KeyRow::looked_up(mode, command, args.as_deref(), description).map(HelpLine::Keys)
+    /// A keys row, looked up in `mode`: the one caller of [`KeyRow::looked_up`],
+    /// and the lookup `pages` hands every builder (improvise-ssoe).
+    pub(super) fn keys(
+        mode: &ModeKey,
+        command: &'static str,
+        args: Option<&[&str]>,
+        description: &'static str,
+    ) -> Option<HelpLine> {
+        let args: Option<Vec<String>> = args.map(|a| a.iter().map(|s| (*s).to_string()).collect());
+        KeyRow::looked_up(mode, command, args.as_deref(), description).map(HelpLine::Keys)
+    }
 }
 
 /// `gg`, `yy`, `ZZ`: a prefix key and the key after it, both off the keymap.
@@ -1021,6 +1023,18 @@ mod tests {
         }
     }
 
+    /// improvise-ssoe: a keys row drawn but never asked for through the builders'
+    /// parameter is invisible to every guard. See `context/test-evidence/protocol.md`.
+    #[test]
+    fn every_keys_row_drawn_is_one_the_builders_asked_for() {
+        let drawn = pages()
+            .iter()
+            .flat_map(|page| page.lines.iter())
+            .filter(|line| matches!(line, HelpLine::Keys(_)))
+            .count();
+        assert_eq!(drawn, help_bindings().len());
+    }
+
     /// A row whose lookup finds no key is dropped silently, so a broken lookup
     /// looks like a shorter page. See `context/test-evidence/protocol.md`.
     #[test]