Found by the ChatGPT Codex review of PR #151 at head 64bcb82, and correct.
`clear-cell`, `yank`, `paste` and `drill-into-cell` are registered with
`register`, and their INTERACTIVE arms took `|_args, ctx|`: they discarded the
arguments and used `ctx.cell_key()`. That was harmless while a script only ever
reached the pure arm, which reads the coordinates. The moment the headless loop
re-parsed interactively, `clear-cell Region/East` passed the syntax gate and
then cleared whatever the cursor was sitting on — the origin, `Region/North`.
So this ticket's own lesson has a third instance, inside its own fix: two
construction arms that mean different things demote whichever caller reaches the
weaker one, and here the "weaker" one was the arm that had the context. The four
arms now go through `key_from_args_or_cursor` — explicit coordinates win, the
cursor is the fallback, which is what the keymap path wants anyway since a
binding carries no arguments.
`an_explicit_coordinate_still_beats_the_cursor` writes three cells and clears
the one the cursor is NOT on, so it separates "cleared the named cell" from
"cleared the cursor cell". Restoring the arg-discarding arm fails it.
Evidence: context/test-evidence/cli.md.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
12 KiB
Test evidence — the headless CLI (src/main.rs, tests/headless_cli.rs)
Keyed by exact test name, per design-principles §7. A renamed test orphans its
entry; python3 scripts/check-comment-runs.py --check-evidence catches a
heading that names no test.
tests/headless_cli.rs — the contract at the process boundary
improvise-q914 / improvise-j6x8 / improvise-z05h. The reasoning is
context/code/cli.md; this file records what broke and what each test can see.
Why a spawned binary rather than a unit test. The three things under test
are an exit code, a choice of stream, and whether a file was written — none of
which exists below std::process::exit. Per design-principles §7 this is a
boundary contract, so it is pinned once, slowly and honestly, and the
command/cmd/ unit tests assume it. Measured on the orchestrator's machine the
whole binary runs in ~0.5s, twelve process spawns included, which is well
inside the per-binary budget.
What was broken. run_headless_commands applied effects and never read
app.view_state.status_msg, and set exit_code only for parse errors. So a
successful command and a refused one produced byte-identical output — nothing —
and exit 0 for both. Measured at bf83294 before the fix: six of the twelve
tests below failed.
a_refused_command_says_what_went_wrong
add-item Nonexistent Thing against a document that has no such category.
Workbook::add_item's Err reaches status_msg and the effect does not
set abort_effects, which is what makes this the load-bearing case: it is a
genuine refusal that no flag in the codebase classifies as one.
Asserts the category's name appears, not merely that something was printed —
"something was printed" is satisfied by a fixed ok on every command.
a_command_with_nothing_to_say_says_nothing
The rival this exists to kill, and it is the one the brief named: a fix
that prints on the happy path and stays silent on the refusal. Injected as
if said.trim().is_empty() { eprintln!("ok") } — the exact inversion — and it
fails this test and a_refused_command_says_what_went_wrong, five tests in
all. A test that only asked "was anything printed" passes that rival.
add-category Region is the input because it sets no status at all, so the
assertion is about the writer rather than about this command's vocabulary.
an_aborted_batch_exits_non_zero_and_says_why
:w into a directory that does not exist. effect::Save reports through
report_file_write, which sets the status and abort_effects (gotcha 21),
so this is the one arm the exit code can honestly read. Before the fix this
exited 0 with no output, which is what made improvise cmd … && next-step
proceed over a failed write.
a_command_that_succeeds_while_reporting_still_exits_zero
The rival this exists to kill: making any emitted status a non-zero exit,
which is the obvious way to widen the previous test and is wrong. effect::Save
on success also writes a status, as does cmd/fit.rs — whose status carries
the seed that reproduces the fit. So "it spoke" cannot mean "it failed".
The first assertion is a check-the-checker: if the save ever stops reporting, the test proves nothing and says so rather than passing quietly.
a_parse_error_still_exits_non_zero
The pre-existing arm, kept so the widening cannot silently drop it. Also
asserts the message names the word typed, which the Unknown command arm
reports post-alias-resolution.
what_a_command_says_goes_to_stderr_and_stdout_stays_empty
Pins the stream choice itself. Both halves are asserted: a rival that writes to both streams passes an stderr-only check.
a_script_reports_exactly_as_cmd_does
improvise script is the same function, so this cannot fail while cmd
passes — which is the point. It is here because the ticket asked whether the
hole was shared, and a claim that two paths are one is worth one cheap
assertion rather than a sentence. The script also carries a # comment, so the
comment-skipping arm is exercised on the way through.
a_run_that_changes_nothing_does_not_rewrite_the_document
move-cursor down is ViewOnly, so nothing is dirty. This test passed
before the fix — a freshly written document re-serialises byte-identically —
and it is kept as the control that says so. The discriminating one is below.
a_no_op_run_leaves_an_unnormalised_literal_alone
The reported instance. A hand-written document carrying 1e20 is rewritten as
100000000000000000000 by a run asked to change nothing, because
persistence::save ran unconditionally. It fails before the fix where its
sibling does not, which is exactly why both are here: the sibling cannot see a
normalisation, only a reordering.
The first assertion checks the fixture still contains 1e20 before the run, so
a future grammar change that rejects the literal fails loudly instead of making
the test vacuous.
a_run_that_changed_the_document_still_writes_it
The regression the dirty-gate could have introduced. effect::Save sets
dirty = false, so a gate reading the flag once at the end would see
[add-item East, w other.improv] as unchanged and silently drop the edit from
the -f document. Accumulating changed per command is what this asserts; a
gate written the obvious way fails it.
the_cmd_help_does_not_advertise_a_language_it_refuses
Three clap doc strings said JSON three times over. Checks both subcommands,
because script's <PATH> help carried the same claim and a fix to cmd
alone reads as complete.
the_language_the_help_names_is_the_one_it_accepts
The other direction, and the reason the test above is not merely a spell check:
it demonstrates the : command line works end to end and that the JSON form
the help advertised is genuinely refused. Without this half, deleting the word
"JSON" from the help would satisfy the check while the CLI still accepted
neither language.
Scratch (helper)
A per-test temp directory named from the test's own tag and the process id, so
the twelve tests cannot collide, with a Drop that removes it. Not
tempfile::TempDir only because the fixture also has to be readable by a
spawned child at a stable path.
seeded (helper)
Runs add-category Region to create the document. It asserts the file exists
afterwards rather than trusting the seed, because every test that follows would
otherwise misreport as "the command did nothing" when the seed was what failed.
a_script_can_move_the_cursor_and_the_next_command_sees_it
improvise cmd and improvise script resolved every command through
CmdRegistry::parse — the pure arm, which builds CursorState::default()
(row 0, col 0, zero rows, zero columns). The interactive arm, which builds
CursorState::from_ctx(ctx), was never reached from a script, so every Move
snapshotted a zeroed cursor at parse time, clamped to (0, 0) and emitted
SetSelected(0, 0) — worse than a no-op, since it resets the cursor
(improvise-6uva). The reasoning is context/code/cli.md section 7.
Reproduced at a9cad15 before the fix, on a seeded document with Region on
row carrying North, South, East: move-selection 2 0 . hide-selected-row-item
and a bare hide-selected-row-item both wrote hidden: Region/North. After the
fix the first writes hidden: Region/East.
Why the test has two halves. Asserting only that the moved script hides
East cannot separate "the move worked" from "the origin happens to be East".
The second document asserts the origin is North, so the first assertion is
discriminating, and its message says what breaks if that ever changes — three
identical replays of a fixture whose moves are silent no-ops prove nothing,
which is how this survived five months.
Two faults were injected, one per layer, and both fail this test:
parse_segment_interactivecallsctx.registry.parseinstead of.interactive— the bug itself, one level down.left: "hidden: Region/North" right: "hidden: Region/East".run_headless_commandsbuilds the segment up front withparse_linerather than per segment against a liveCmdContext— the composition half. Same failure. The unit test inparse.rspasses under this second one, which is why both tests exist.
a_script_cursor_move_survives_the_line_that_made_it
The same property across two -f arguments rather than one dot-separated line,
because improvise cmd takes several and improvise script is one per line:
the cursor is App state and a line boundary is not a reset.
a_segment_is_built_against_the_cursor_it_will_run_against
The unit test for the layer that owns the bug. It seats a CmdContext at
(1, 0) in a two-by-two grid and asserts move-selection 0 1 executes to
SetSelected(1, 1).
Its second half is what keeps it honest: it builds the same command through
registry.parse and asserts that one gives SetSelected(0, 0). If the pure arm
ever learns the cursor, the first assertion stops discriminating and this half
fails first, saying so.
a_pure_commands_two_arms_agree_on_no_arguments
A second instance of the same shape, found by the first fix rather than
predicted by the ticket. CmdRegistry::register_pure built an interactive arm
that refused empty args while its own pure arm accepted them, so w with no
path — save to the current document — parsed one way and refused the other. Once
a script re-parsed through the interactive arm, improvise cmd 'w' and :w
both broke; the suite caught it as
a_failed_colon_write_lands_in_normal_where_its_error_is_drawn reporting
":w — w: this command requires arguments".
Both arms are now parse, which is what "needs no interactive context" was
supposed to mean. Restoring the empty-args guard fails this test on the first
expect: the interactive arm refuses a bare w the pure arm accepts.
segments_drops_comments_and_blanks
segments is the split half of parse_line_with, extracted so the headless
loop can re-parse one segment at a time. It answers the same three cases the
whole-line parser did — a # comment, a // comment and a blank line are no
commands — plus the empty segment in a . . b.
an_unknown_segment_is_an_error_not_a_silent_skip
parse_segment_interactive returns Ok(None) for a segment with no word, which
is a shape an unknown word must not be able to reach: a silently skipped
command in a script is the partial-and-silent failure this repo keeps finding.
The error also names the word, matching what CmdRegistry::parse does.
an_explicit_coordinate_still_beats_the_cursor
Found by the ChatGPT Codex review of PR #151 at head 64bcb82, and correct: the
first version of improvise-6uva's fix introduced a regression it was not looking
for. Four registry entries — clear-cell, yank, paste, drill-into-cell —
were registered with register, and their interactive arms took |_args, ctx|: they discarded the arguments and used ctx.cell_key(). That was harmless
while a script only ever reached the pure arm, which reads the coordinates. The
moment the headless loop re-parsed interactively, clear-cell Region/East passed
the syntax gate and then cleared whatever the cursor was sitting on — the origin,
Region/North.
So improvise-6uva's own lesson had a third instance in its own fix: two
construction arms that mean different things demote whichever caller reaches the
weaker one, and here the "weaker" one was the arm that had the context. The four
arms now go through key_from_args_or_cursor: explicit coordinates win, the
cursor is the fallback, which is also what the keymap path wants since a binding
carries no arguments.
The fixture writes three cells and clears the one the cursor is not on, so
the assertion separates "cleared the named cell" from "cleared the cursor cell"
— clearing North by name would have passed against the broken code. Restoring
the arg-discarding arm fails it: the cursor cell was cleared instead of the named one: ["Region=East = 3", "Region=South = 2"].