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
This commit is contained in:
Edward Langley
2026-09-22 22:08:42 -07:00
parent 1c4a27243d
commit 8772db2e7b
2 changed files with 47 additions and 12 deletions
+21
View File
@@ -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`.
+19 -5
View File
@@ -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,10 +362,10 @@ mod key_row {
(!keys.is_empty()).then_some(Self { keys, description })
}
}
}
/// A keys row, looked up in `mode`.
fn 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]>,
@@ -373,6 +374,7 @@ fn keys(
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.
fn prefixed_labels(mode: &ModeKey, command: &str, args: Option<&[String]>) -> Vec<String> {
@@ -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]