fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: guard improvise-mai9 at the call site, and correct §3's group_of

Browse Source 
Review of PR #77 found the mai9 fix unprotected and one doc paragraph
contradicting another. Both were right; both are addressed here.

The guard was aimed at the wrong thing
--------------------------------------
The mai9 defect was in the WIRING — `draw_bottom_bar` built
`format!("{}{}", mb.prompt, buf)` plus the cursor glyph and never consulted
`status_msg`. Both tests called the composing helper `minibuffer_line`
DIRECTLY, so nothing exercised the call site. Reverting that one line restored
the bug in full with the suite green at 1411, reproduced here before rewriting.
The helper did not even go dead: the test module's `use` kept it alive while it
asserted nothing about the product.

`a_status_message_reaches_the_bottom_row_while_a_minibuffer_is_open` now renders
`draw_bottom_bar` through a real `Terminal<TestBackend>` and reads the row the
terminal received. That is the only assertion that can fail for the real defect.
`the_minibuffer_prompt_is_not_displaced_by_the_message` is beside it because a
"fix" that dropped to `draw_status` would satisfy the first on the message
alone. Injected fault: the pre-mai9 `format!` — both fail, the first naming the
missing message and printing the row it did compose.

And the negative arm could not fail, twice over
-----------------------------------------------
`fill_line` computes `pad = width - left.len() - right.len()`, so with an empty
status an unconditional `format!("  {msg}  ")` produces a BYTE-IDENTICAL row at
any width where the line fits. The `match status_msg.is_empty()` is observable
only in the band where the prompt fits but `left.len() + 4` does not.

Rewriting it against that band was not enough: the first attempt derived its
lower bound by calling `minibuffer_line` itself, so the injected fault moved the
bound with it and the loop skipped the failing band entirely. The bound is now
spelled locally. A bound read off the function under test is not a bound.

Injected fault: `right` built unconditionally. Now fails at width 16 with
"an empty status still composed 20 columns".

context/code/protocol.md contradicted itself, 270 lines apart
-------------------------------------------------------------
§3 still said `group_of` answers "nearest preceding header", DELIBERATELY, and
that encoding an end boundary "is filed rather than smuggled into" the anr9
migration — while §3a, added by this branch, says `GroupEnd` is the boundary. A
reader arriving at §3 first would conclude this PR smuggled in exactly what that
paragraph forbids. It did not; improvise-gd83 IS the filed ticket. §3a was
written as an addition where it needed to be a CORRECTION, and gotcha 48 turned
on its author — worse than the `bindings.rs` case only because both copies sit
in one file.

§3 now records that the rule changed and which ticket changed it, and §3a points
at the correction rather than restating the history. The neighbouring paragraph
was stale too: `body_from`'s argument no longer exists "only because the two
front ends scroll differently".

Filed, not fixed
----------------
  * improvise-1wl0: `#[serde(default)]` on `AxisFrame::first` forges the
    permissive claim — `first: 0` asserts "this frame leads the axis", which is
    what `body_from` acts on, so a frame that loses the field deserializes as
    leading. Same argument that gave `Basis` no `Default`. No skew ships today.
  * improvise-h0iv: `group_runs` coalesces by name alone where `FrameGroup` has
    `cat` to fix it. Right call for a consolidation — both prior copies did the
    same, and changing the rule while moving it would have cost the agreement
    test its evidence — but a single home reads as decided, which is the new
    risk and the reason to file.

Both are cited in `context/code/protocol.md` §3a, so the single home does not
read as settled.

1412 tests pass (was 1411; +3 wiring tests, -2 helper-only tests).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
Edward Langley
2026-09-20 21:01:02 -07:00
parent 8f3a9768b7
commit bd3967cc7c
3 changed files with 158 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/code/protocol.md
+42 -18
View File
@@ -477,20 +477,30 @@ the top edge (improvise-jjx).
**`body_from(idx)`** drops the group header that owns the first drawn item: a
frame opening mid-group carries that header so `group_of` can answer for it,
but it is *above* the first drawn row and must not be painted as one. This is
the same cut `GridLayout::data_row_to_visual` has always made. Its argument
exists only because the two front ends scroll differently — a browser's frame
is already windowed and passes `0`, the TUI's covers the whole grid and passes
its row offset (gotcha 44) — **not** because the rule differs.
the same cut `GridLayout::data_row_to_visual` has always made.
**`group_of` answers "nearest preceding header", not membership, and
deliberately so: it is what the TUI has always painted.** `expand_category`
emits a header on *entering* a group and no boundary on leaving one, so an
ungrouped item following a grouped run is answered with the run's group — and
the TUI's column-group row spans it under that label today. Encoding
membership, or an end boundary, means changing `AxisEntry` in
`improvise-core` and changing what the TUI paints: a fix to the layout, not to
this projection of it, and it is filed rather than smuggled into the migration
that made the two ends agree (improvise-anr9, raised by Copilot on PR #23).
Two things about its argument, and the first of them **used to be the whole
story and was wrong**. It exists partly because the two front ends scroll
differently — a browser's frame is already windowed and passes `0`, the TUI's
covers the whole grid and passes its row offset (gotcha 44). But `idx` alone
cannot say whether the frame *leads the axis*, and at the top there is no
header "above" to drop: it is content, and a collapsed first group has no
other affordance. That is `AxisFrame::first` and improvise-0o6x — §3a.
**`group_of` answers membership**, and the paragraph that stood here said the
opposite: that it answers "nearest preceding header", deliberately, because
that is what the TUI had always painted, and that encoding an end boundary
would mean changing `AxisEntry` in `improvise-core` and was "filed rather than
smuggled into" improvise-anr9. All of that was true when written. **The filed
ticket is improvise-gd83 and it has landed** — `AxisEntry::GroupEnd` closes a
run, `groups()` clears on it, and the TUI's painted span changed with it. The
mechanism and its two emission arms are §3a.
That correction is here rather than only in §3a on purpose. This paragraph
forbade, in terms, the change §3a describes; a reader arriving at the stale
copy first would conclude the rule had been smuggled past its own prohibition.
When a fact is stated twice, changing it is a grep (gotcha 48) — and the two
copies being in *one file* is what made this worth saying out loud.
### The width rule has one home
@@ -755,10 +765,12 @@ Nothing is defined for the help overlay: its content is static and
Two facts `AxisFrame` gained after `improvise-anr9`, both about group runs.
**`FrameEntry::GroupEnd` closes a run.** `group_of` was documented as answering
"nearest preceding header", which is not membership: `expand_category` emitted a
header on *entering* a group and nothing on leaving one, so an ungrouped item
after a grouped run inherited it (`improvise-gd83`). `AxisEntry::GroupEnd` is
**`FrameEntry::GroupEnd` closes a run.** `group_of` used to answer "nearest
preceding header", which is not membership: `expand_category` emitted a header
on *entering* a group and nothing on leaving one, so an ungrouped item after a
grouped run inherited it (`improvise-gd83`). The §3 paragraph that recorded the
old rule — and forbade this change — is corrected there, not merely superseded
here. `AxisEntry::GroupEnd` is
the boundary, carried onto the wire as `FrameEntry::GroupEnd`; `groups()` clears
`current` on it and `group_of` stops walking. It is emitted only on
`Some -> None`, never between two groups — the next header already ends the
@@ -780,11 +792,23 @@ The TUI builds over the whole grid so `first` is always `0` there, and the
browser's comes from the projected window. A frame built by hand in a test must
set it, which is why it is a plain field rather than derived.
It carries `#[serde(default)]`, and **that is a defect rather than a decision**
(improvise-1wl0): `first: 0` is not an absence but the claim "this frame leads
the axis", which is the claim `body_from` acts on — so a frame that loses the
field deserializes as leading. No skew ships today, both ends being one
workspace at one commit. Same argument as `Basis` having no `Default`.
**`group_runs` / `group_run_heads` are the coalescing rule's one home.** The TUI
and `render.rs::col_groups` each coalesced adjacent columns sharing a group,
kept in step by a comment (`improvise-8qlr`). Runs coalesce by group **name**,
so two adjacent runs under two different headers of the same name read as one —
that is what both ends did before and is preserved deliberately. The TUI's one
that is what both ends did before and is preserved deliberately, because
changing the rule in the commit that moves it would leave the agreement test
unable to tell "both ends agree" from "both ends changed together".
`FrameGroup` carries `cat` and could fix it, and **a single home reads as
decided** — which is the new risk and why it is filed as improvise-h0iv rather
than left as a note. The TUI's one
divergence is that it labels its first *visible* column even mid-run, so a
scrolled grid does not draw an unlabelled group line.
context/test-evidence/tui.md
+40 -14
View File
@@ -416,36 +416,62 @@ produce 1–4 items rather than 0. Each invocation is seeded from the current ti
plus the PID.
### a_status_message_set_while_a_minibuffer_is_open_is_on_the_bottom_row
### a_status_message_reaches_the_bottom_row_while_a_minibuffer_is_open
### with_no_message_pending_the_minibuffer_row_is_only_the_minibuffer
### the_minibuffer_prompt_is_not_displaced_by_the_message
### an_empty_message_never_pushes_a_row_that_fits_past_its_width
improvise-mai9. `draw_bottom_bar` rendered the minibuffer **instead of** the
status line, so a message written while a text-entry mode was open was never
drawn — not truncated, not overwritten. A test asserting `app.status_msg` was
set passed in every one of those cases, which is exactly how this survived
improvise-6t3's careful effect ordering and why the guard has to be aimed at
the *composed row* rather than at the field.
set passed in every one of those cases, which is how this survived
improvise-6t3's careful effect ordering.
Of the ticket's two candidate fixes, the row is **shared** rather than the
callers being made structurally unable to report: giving the status a second
row changes frame geometry (improvise-q0vb's cost), and "always leave the mode
before reporting" is what `CommitSave` does by hand and would have to be
repeated at every text-entry commit — the thing the ticket calls a workaround.
Sharing the row costs one function and generalises to all eight modes.
The second test is the negative arm and is not decoration: a composition that
appended the status unconditionally would pass the first and draw two trailing
spaces of nothing on every keystroke of every minibuffer.
**The first version of these tests could not fail for the bug, and that is the
entry worth reading.** They called the composing helper `minibuffer_line`
directly. The defect was in the *wiring* — `draw_bottom_bar` building its own
`format!` and never consulting `status_msg` — so reverting that one line
restored the bug in full with all 1411 tests green. The helper did not even go
dead: the test module's `use` kept it alive while it asserted nothing about the
product. Found by review (PR #77), reproduced here before rewriting.
So the guard is now `Buffer`-backed: a real `Terminal<TestBackend>` renders
`draw_bottom_bar` and the assertion reads the row the terminal received. That
is the only assertion that can fail for mai9's actual defect. Fault injected by
restoring `format!("{}{}", mb.prompt, buf)` plus the cursor glyph — both wiring
tests fail, the first naming the missing message and printing the row it did
compose.
The second test exists because a "fix" that dropped to `draw_status` would
satisfy the first on the message alone; it pins that the prompt still leads.
**The third test is the negative arm, and its first version was unfalsifiable
for arithmetic reasons.** `fill_line` computes `pad = width - left.len() -
right.len()`, so with an empty status an unconditional `format!(" {msg} ")`
produces a byte-identical row at any width where the line fits: `left` plus
`width - left.len()` spaces, either way. The `match status_msg.is_empty()` is
observable **only** in the band where the prompt fits but `left.len() + 4`
does not — widths 16..=19 for this fixture, where the unguarded arm returns a
row wider than the terminal.
That test then failed a second time in a more interesting way: it derived its
lower bound by *calling `minibuffer_line` itself*, so the injected fault moved
the bound with it and the loop skipped the failing band entirely. The bound is
now spelled locally. **A bound read off the function under test is not a
bound** — it is the fault grading its own exam.
Known and not fixed here: `fill_line` pads on `left.len()`, which is bytes, so
the right-hand message sits two cells further right than intended whenever the
left contains the multi-byte cursor glyph. `draw_status` has always done this
with its own multi-byte content; it is pre-existing and out of scope.
Fault injected by restoring `format!("{}{}the cursor glyph", prompt, buf)` (here: the
`false` arm returning `String::new()`): the first test fails naming the missing
message and printing the row it did compose.
with its own multi-byte content; it is pre-existing and out of scope. It is
also what makes the band above four columns wide rather than two.
### a_group_header_first_on_its_axis_reaches_both_front_ends
src/draw.rs
+76 -15
View File
@@ -540,32 +540,93 @@ fn draw_welcome(f: &mut Frame, area: Rect) {
#[cfg(test)]
mod bottom_bar_tests {
use super::minibuffer_line;
use super::{draw_bottom_bar, minibuffer_line};
use crate::ui::app::{App, AppMode};
use crate::workbook::Workbook;
use ratatui::{Terminal, backend::TestBackend, layout::Rect};
/// improvise-mai9: the message used not to be drawn at all — see
/// context/test-evidence/tui.md.
/// The bottom row as the terminal really receives it, so the assertion is
/// about `draw_bottom_bar`'s wiring and not about a helper it may not call.
fn bottom_row(app: &App, width: u16) -> String {
let mut terminal = Terminal::new(TestBackend::new(width, 1)).expect("test backend");
terminal
.draw(|f| draw_bottom_bar(f, Rect::new(0, 0, width, 1), app))
.expect("draw");
terminal
.backend()
.buffer()
.content()
.iter()
.map(|c| c.symbol())
.collect()
}
fn app_with_minibuffer_and_message() -> App {
let mut app = App::new(Workbook::new("T"), None);
app.view_state.mode = AppMode::category_add();
let key = app
.view_state
.mode
.minibuffer()
.expect("CategoryAdd carries a MinibufferConfig")
.buffer_key
.clone();
app.view_state.buffers.insert(key, "North".to_string());
app.view_state.status_msg = "category already exists".to_string();
app
}
/// improvise-mai9, and the ONE assertion that can fail for the real defect
/// — see context/test-evidence/tui.md.
#[test]
fn a_status_message_set_while_a_minibuffer_is_open_is_on_the_bottom_row() {
let line = minibuffer_line("Category: ", "North", "category already exists", 80);
fn a_status_message_reaches_the_bottom_row_while_a_minibuffer_is_open() {
let app = app_with_minibuffer_and_message();
let row = bottom_row(&app, 80);
assert!(
line.contains("Category: North"),
"the minibuffer still owns the left of the row: {line:?}"
row.contains("North"),
"the minibuffer must still own the left of the row: {row:?}"
);
assert!(
line.contains("category already exists"),
"the status message must reach the user: {line:?}"
row.contains("category already exists"),
"the status message is never drawn — the bug is in draw_bottom_bar, \
which used to render the minibuffer INSTEAD of the status line, so \
a test that calls the composing helper directly cannot see it: {row:?}"
);
}
/// The minibuffer is still the thing that owns the row: a fix that dropped
/// to `draw_status` would pass the assertion above on the message alone.
#[test]
fn with_no_message_pending_the_minibuffer_row_is_only_the_minibuffer() {
let line = minibuffer_line("Category: ", "North", "", 80);
assert_eq!(
line.trim_end(),
"Category: North\u{258c}",
"an empty status adds nothing but padding"
fn the_minibuffer_prompt_is_not_displaced_by_the_message() {
let app = app_with_minibuffer_and_message();
let row = bottom_row(&app, 80);
let prompt_at = row.find("North").expect("the buffer is drawn");
let message_at = row.find("category already exists").expect("the message");
assert!(
prompt_at < message_at,
"the prompt leads and the message follows it: {row:?}"
);
}
/// The guard's ONLY observable effect, in the narrow band where the two
/// arms differ at all. Why the obvious negative arm cannot: tui.md.
#[test]
fn an_empty_message_never_pushes_a_row_that_fits_past_its_width() {
// Spelled here, NOT taken from `minibuffer_line`: a bound read off the
// function under test moves with the fault and skips the failing band.
let fits_from = "Category: North\u{258c}".chars().count() as u16;
for width in fits_from..fits_from + 24 {
let line = minibuffer_line("Category: ", "North", "", width);
assert!(
line.chars().count() <= width as usize,
"width {width}: an empty status still composed {} columns \
from {line:?} — an unconditional append pads for a message \
that is not there",
line.chars().count()
);
}
}
}
#[cfg(test)]