From 6e3e51b0ac505a037cef1ff269ce1d1d643d4561 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:17:11 -0700 Subject: [PATCH 01/16] WIP: a model effect that cannot do what it names refuses its frame (improvise-bok4) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-command/src/ui/effect.rs | 93 +++++++++------- crates/improvise-command/src/ui/host.rs | 127 +++++++++++++++++++++- 2 files changed, 179 insertions(+), 41 deletions(-) diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 62a4f6a0..c6eccc26 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -44,6 +44,31 @@ pub enum ViewTarget { Named(String), } +/// A model effect that could not do what it names: it says why AND stops the +/// batch, so a frame containing it is refused, never accepted (improvise-bok4). +fn refused(app: &mut App, why: impl Into) { + app.view_state.status_msg = why.into(); + app.abort_effects = true; +} + +/// Write the view a geometry effect names, or refuse: a view the document no +/// longer has is a failure to report, not a no-op to swallow (improvise-bok4). +fn on_view(app: &mut App, target: &ViewTarget, write: impl FnOnce(&mut crate::view::View)) { + let missing = match app.view_mut_of(target) { + Some(view) => { + write(view); + None + } + None => Some(match target { + ViewTarget::Named(name) => format!("No view named \"{name}\""), + ViewTarget::Active => "No active view".to_string(), + }), + }; + if let Some(why) = missing { + refused(app, why); + } +} + /// A discrete state change produced by a command. /// Effects know how to apply themselves to the App. pub trait Effect: Debug { @@ -105,7 +130,7 @@ impl Effect for AddItem { .workbook .add_item(&self.category, &self.item) { - app.view_state.status_msg = e.to_string(); + refused(app, e.to_string()); } } fn kind(&self) -> EffectKind { @@ -126,7 +151,7 @@ impl Effect for AddItemInGroup { .workbook .add_item_in_group(&self.category, &self.item, &self.group) { - app.view_state.status_msg = e.to_string(); + refused(app, e.to_string()); } } fn kind(&self) -> EffectKind { @@ -191,9 +216,7 @@ impl Effect for AddFormula { } app.model_state.workbook.model.add_formula(formula); } - Err(e) => { - app.view_state.status_msg = format!("Formula error: {e}"); - } + Err(e) => refused(app, format!("Formula error: {e}")), } } fn kind(&self) -> EffectKind { @@ -252,9 +275,9 @@ impl Effect for EnterEditAtCursor { pub struct TogglePruneEmpty(pub ViewTarget); impl Effect for TogglePruneEmpty { fn apply(&self, app: &mut App) { - if let Some(v) = app.view_mut_of(&self.0) { + on_view(app, &self.0, |v| { v.prune_empty = !v.prune_empty; - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -318,7 +341,9 @@ impl Effect for CreateView { pub struct DeleteView(pub String); impl Effect for DeleteView { fn apply(&self, app: &mut App) { - let _ = app.model_state.workbook.delete_view(&self.0); + if let Err(e) = app.model_state.workbook.delete_view(&self.0) { + return refused(app, e.to_string()); + } // This client may have been the one on it. Idempotent for every other // view; the mirrors owe the same move (`Sessions::adopt_deleted_views`). app.view_state.active_view = app.view_name().to_string(); @@ -402,9 +427,9 @@ pub struct SetAxis { } impl Effect for SetAxis { fn apply(&self, app: &mut App) { - if let Some(v) = app.view_mut_of(&self.view) { + on_view(app, &self.view, |v| { v.set_axis(&self.category, self.axis); - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -419,9 +444,9 @@ pub struct SetPageSelection { } impl Effect for SetPageSelection { fn apply(&self, app: &mut App) { - if let Some(v) = app.view_mut_of(&self.view) { + on_view(app, &self.view, |v| { v.set_page_selection(&self.category, &self.item); - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -436,9 +461,9 @@ pub struct ToggleGroup { } impl Effect for ToggleGroup { fn apply(&self, app: &mut App) { - if let Some(v) = app.view_mut_of(&self.view) { + on_view(app, &self.view, |v| { v.toggle_group_collapse(&self.category, &self.group); - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -453,9 +478,9 @@ pub struct HideItem { } impl Effect for HideItem { fn apply(&self, app: &mut App) { - if let Some(v) = app.view_mut_of(&self.view) { + on_view(app, &self.view, |v| { v.hide_item(&self.category, &self.item); - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -470,9 +495,9 @@ pub struct ShowItem { } impl Effect for ShowItem { fn apply(&self, app: &mut App) { - if let Some(v) = app.view_mut_of(&self.view) { + on_view(app, &self.view, |v| { v.show_item(&self.category, &self.item); - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -483,9 +508,9 @@ impl Effect for ShowItem { pub struct TransposeAxes(pub ViewTarget); impl Effect for TransposeAxes { fn apply(&self, app: &mut App) { - if let Some(v) = app.view_mut_of(&self.0) { + on_view(app, &self.0, |v| { v.transpose_axes(); - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -497,9 +522,9 @@ pub struct CycleAxis(pub ViewTarget, pub String); impl Effect for CycleAxis { fn apply(&self, app: &mut App) { let category = self.1.clone(); - if let Some(v) = app.view_mut_of(&self.0) { + on_view(app, &self.0, |v| { v.cycle_axis(&category); - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -511,9 +536,9 @@ pub struct SetNumberFormat(pub ViewTarget, pub String); impl Effect for SetNumberFormat { fn apply(&self, app: &mut App) { let format = self.1.clone(); - if let Some(v) = app.view_mut_of(&self.0) { + on_view(app, &self.0, |v| { v.number_format = format; - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -921,9 +946,7 @@ impl Effect for LoadModel { app.install_workbook(loaded); app.view_state.status_msg = format!("Loaded from {}", self.0.display()); } - Err(e) => { - app.view_state.status_msg = format!("Load error: {e}"); - } + Err(e) => refused(app, format!("Load error: {e}")), } } fn kind(&self) -> EffectKind { @@ -980,8 +1003,7 @@ impl Effect for ImportJsonHeadless { let bytes = match app.storage.read(&self.path) { Ok(b) => b, Err(e) => { - app.view_state.status_msg = - format!("Cannot read '{}': {e}", self.path.display()); + refused(app, format!("Cannot read '{}': {e}", self.path.display())); return; } }; @@ -992,7 +1014,7 @@ impl Effect for ImportJsonHeadless { (recs, raw) } Err(e) => { - app.view_state.status_msg = format!("CSV error: {e}"); + refused(app, format!("CSV error: {e}")); return; } } @@ -1000,22 +1022,21 @@ impl Effect for ImportJsonHeadless { let content = match app.storage.read(&self.path) { Ok(c) => c, Err(e) => { - app.view_state.status_msg = - format!("Cannot read '{}': {e}", self.path.display()); + refused(app, format!("Cannot read '{}': {e}", self.path.display())); return; } }; let value: serde_json::Value = match serde_json::from_slice(&content) { Ok(v) => v, Err(e) => { - app.view_state.status_msg = format!("JSON parse error: {e}"); + refused(app, format!("JSON parse error: {e}")); return; } }; match json_import_records(&value, self.array_path.as_deref()) { Ok(recs) => (recs, value), Err(msg) => { - app.view_state.status_msg = msg; + refused(app, msg); return; } } @@ -1053,9 +1074,7 @@ impl Effect for ImportJsonHeadless { format!("Imported with warnings: {}", warnings.join("; ")) }; } - Err(e) => { - app.view_state.status_msg = format!("Import error: {e}"); - } + Err(e) => refused(app, format!("Import error: {e}")), } } fn kind(&self) -> EffectKind { diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 50d7b392..493e8541 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -1519,10 +1519,12 @@ mod tests { .any(|f| matches!(f.msg, ServerFrame::Snapshot(_))), "a failed load cost every client its cursors: {frames:?}" ); - assert!( - host.app().view_state.status_msg.starts_with("Load error"), - "the fixture did not actually fail the load: {:?}", - host.app().view_state.status_msg + // Since improvise-bok4 a failed load is REFUSED, so the refusal is the + // witness that it failed; the host's status line is rolled back with it. + assert_eq!( + to(&frames, asker), + vec![ServerFrame::Rejected(Rejection::MutationRefused)], + "the fixture did not actually fail the load" ); } @@ -1756,4 +1758,121 @@ mod tests { let json = serde_json::to_string(&inbound).unwrap(); assert_eq!(serde_json::from_str::(&json).unwrap(), inbound); } + + /// Two model commands, each caught up against the document as the client + /// saw it, so no basis tier can refuse the frame first (improvise-bok4). + fn two_command_frame(host: &Host, first: ModelCommand, then: ModelCommand) -> ClientFrame { + ClientFrame::Commands(vec![ + crate::ui::reduce::caught_up(host.app(), first), + crate::ui::reduce::caught_up(host.app(), then), + ]) + } + + /// improvise-bok4, the view-addressed case: the second command's view is + /// gone. Evidence: `context/test-evidence/command.md`. + #[test] + fn a_frame_writing_a_view_its_own_first_command_deleted_is_refused() { + let mut host = host(); + host.app_mut().model_state.workbook.create_view("Scratch"); + let (asker, _) = host.connect(10, 10); + + let answer = host.handle( + asker, + two_command_frame( + &host, + ModelCommand::DeleteView("Scratch".into()), + ModelCommand::SetNumberFormat { + view: "Scratch".into(), + format: ",.2f".into(), + }, + ), + ); + + assert_eq!( + to(&answer, asker), + vec![ServerFrame::Rejected(Rejection::MutationRefused)], + "half the frame did nothing and was reported accepted: {answer:?}" + ); + assert!( + host.app().model_state.workbook.views.contains_key("Scratch"), + "the refused frame's deletion must be undone" + ); + } + + /// improvise-bok4, the category-addressed case: it narrated its failure into + /// the HOST's status line and told the client nothing. + #[test] + fn a_frame_adding_an_item_to_a_category_its_first_command_removed_is_refused() { + let mut host = host(); + let (asker, _) = host.connect(10, 10); + + let answer = host.handle( + asker, + two_command_frame( + &host, + ModelCommand::RemoveCategory("Region".into()), + ModelCommand::AddItem { + category: "Region".into(), + item: "East".into(), + }, + ), + ); + + assert_eq!( + to(&answer, asker), + vec![ServerFrame::Rejected(Rejection::MutationRefused)], + "half the frame did nothing and was reported accepted: {answer:?}" + ); + assert!( + host.app().model_state.workbook.model.category("Region").is_some(), + "the refused frame's removal must be undone" + ); + } + + /// The one-command case of the same class: deleting the last view was + /// swallowed with `let _`, so the frame was accepted having done nothing. + #[test] + fn deleting_the_documents_last_view_is_refused_rather_than_accepted() { + let mut host = host(); + let (asker, _) = host.connect(10, 10); + let only = host.app().view_name().to_string(); + + let answer = host.model(asker, ModelCommand::DeleteView(only.clone())); + + assert_eq!( + to(&answer, asker), + vec![ServerFrame::Rejected(Rejection::MutationRefused)], + "a deletion the workbook refused was reported accepted: {answer:?}" + ); + assert!(host.app().model_state.workbook.views.contains_key(&only)); + } + + /// The control: two commands that both land are accepted, so the refusals + /// above are about a failure and not about the frame's shape. + #[test] + fn a_frame_whose_commands_all_land_is_accepted() { + let mut host = host(); + host.app_mut().model_state.workbook.create_view("Scratch"); + let (asker, _) = host.connect(10, 10); + + let answer = host.handle( + asker, + two_command_frame( + &host, + ModelCommand::SetNumberFormat { + view: "Scratch".into(), + format: ",.2f".into(), + }, + ModelCommand::AddItem { + category: "Region".into(), + item: "East".into(), + }, + ), + ); + + assert!( + to(&answer, asker).contains(&ServerFrame::Accepted), + "a frame with nothing to refuse was refused: {answer:?}" + ); + } } From 26abc3b9a17fc0f843728253ac7a29ba5e6ae7d1 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:18:18 -0700 Subject: [PATCH 02/16] WIP: bok4 docs, evidence and last-view refusal Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 60 +++++++++++++++++++++++++ context/test-evidence/command.md | 25 +++++++++++ crates/improvise-command/src/ui/host.rs | 13 +++++- 3 files changed, 96 insertions(+), 2 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index 987df348..d3eb1413 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -553,6 +553,66 @@ status line. one, and the limit is checked in exactly one place — `Model::add_category` — so nothing in the effect restates it. +### A model effect that cannot do what it names reports it: `refused` and `on_view` (improvise-bok4) + +`AddCategory` was the only wire-reachable model effect that aborted when it +failed. The others did one of two things: + +- **Narrated.** `AddItem`, `AddItemInGroup`, `AddFormula`, `LoadModel` and + `ImportJsonHeadless` wrote a sentence to the *host's* `status_msg` and + carried on. +- **Swallowed.** Every view-addressed effect (`SetAxis`, `SetPageSelection`, + `ToggleGroup`, `HideItem`, `ShowItem`, `TransposeAxes`, `CycleAxis`, + `SetNumberFormat`, `TogglePruneEmpty`) did nothing when `view_mut_of` found + no such view. `DeleteView` discarded "cannot delete the last view" with + `let _`. + +Either way the batch ran to its end, `batch_aborted` stayed false, and +`Host::mutating_frame` reported the frame **accepted**. That is clause 4's +partial-and-silent shape, one layer below the record tier that +`broken_chain` guards. The reviewer measured two cases. +`[DeleteView("Scratch"), SetNumberFormat { view: "Scratch" }]` was accepted +with half of it a no-op. `[RemoveCategory("Region"), AddItem { category: +"Region" }]` was accepted too, and its reason went only to the host's own +status line, so the client was told nothing at all. + +Two helpers in `effect.rs` are the one home for "this effect failed": + +- `refused(app, why)` writes the reason **and** sets `abort_effects`. +- `on_view(app, target, write)` runs `write` on the named view, or refuses + naming the missing view. + +An abort in a client's frame becomes `Rejected(MutationRefused)` through the +existing `mutating_frame` check. The frame is undone whole, and the client is +told, which is the channel the narrated reason never had. + +**This takes the ticket's first option, scoped to the effects the wire can +reach.** Failure is *reported* through the mechanism that already existed +(`abort_effects`), without changing `Effect::apply`'s signature for all ~58 +effects. The second option, growing `broken_chain` to ask whether every view, +category and item target still resolves, would have been a second description +of which targets each effect needs. That second copy would be free to disagree +with the effect itself, which is gotcha 24's reason for observing rather than +predicting. + +Three consequences, and the first one changes behaviour: + +- **The key path aborts too.** A failed `:e missing.improv . q` no longer + quits, and a formula that does not parse stops its batch. This is gotcha 21's + rule applied rather than bent: tidy-up is at the head of every batch, so the + mode and buffers are already settled when the abort lands. `CommitBuffer` + already emits the add effect *last* (gotcha 20). +- **A failed load is refused**, where it used to be accepted with a host status + line. So `a_load_that_fails_is_not_a_rebase` now witnesses the failure by + the refusal, not by `status_msg`. The frame's rollback also rolls the host's + status line back. +- **Not covered:** a no-op *inside* a view that does resolve. Examples are + `SetAxis` on a category the view does not have, `HideItem` of an absent + item, and `RemoveItem`/`RemoveCategory`/`RemoveFormula` of something already + gone. The removals are idempotent by intent. The others are the remaining + instances of this class, and each needs its own `View` method to say whether + it changed anything. + ### `AddFormula` and non-`_Measure` targets For non-`_Measure` targets the item is added to the category so it appears in the diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index efcda9b0..805de176 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -4903,3 +4903,28 @@ The control for the test above, and it is stated separately rather than folded in so that a change making the fixture's load stop rebasing fails *here*, where the message says what actually broke, rather than turning the rollback test green by emptying it. + +### ui::host::tests::a_frame_writing_a_view_its_own_first_command_deleted_is_refused + +improvise-bok4. This is the reviewer's first measured case: +`[DeleteView("Scratch"), SetNumberFormat { view: "Scratch" }]`. Before the fix +the asker's reply was `[Commands([ForgetView("Scratch")]), Commands([Project…])]` +and then `Accepted`. The second command had done nothing, because +`view_mut_of` found no view and every view-addressed effect swallowed the +`None`. Now `on_view` refuses, the abort becomes +`Rejected(MutationRefused)`, and the assertion that `Scratch` still exists +proves the deletion was undone with the rest of the frame. + +Its sibling, `a_frame_adding_an_item_to_a_category_its_first_command_removed_is_refused`, +is the reviewer's second case: `[RemoveCategory("Region"), AddItem { category: +"Region" }]`. It was accepted as well, with `AddItem`'s error written only to +the host's `status_msg`. Both tests failed first against the unfixed effects. + +`deleting_the_documents_last_view_is_refused_rather_than_accepted` covers the +one-command case, which `DeleteView`'s `let _` swallowed. The fault injected +was to put the `let _` back. The test then failed with a projection batch +where the refusal should have been, and it was reverted. + +The control is `a_frame_whose_commands_all_land_is_accepted`, the same two +commands in an order where both land. Without it, a host that refused every +two-command frame would pass all three tests. diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 493e8541..45148d21 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -1794,7 +1794,11 @@ mod tests { "half the frame did nothing and was reported accepted: {answer:?}" ); assert!( - host.app().model_state.workbook.views.contains_key("Scratch"), + host.app() + .model_state + .workbook + .views + .contains_key("Scratch"), "the refused frame's deletion must be undone" ); } @@ -1824,7 +1828,12 @@ mod tests { "half the frame did nothing and was reported accepted: {answer:?}" ); assert!( - host.app().model_state.workbook.model.category("Region").is_some(), + host.app() + .model_state + .workbook + .model + .category("Region") + .is_some(), "the refused frame's removal must be undone" ); } From 4f6e033a3549d476a774a2d1aa5f8f1838e4be93 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:19:39 -0700 Subject: [PATCH 03/16] WIP yf7p: the cursor is the only context a registration fills Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .../improvise-command/src/command/cmd/core.rs | 53 ++++++-- .../improvise-command/src/command/cmd/mod.rs | 16 +++ .../src/command/cmd/panel.rs | 100 ++++++--------- .../src/command/cmd/registry.rs | 117 ++++-------------- 4 files changed, 124 insertions(+), 162 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/core.rs b/crates/improvise-command/src/command/cmd/core.rs index 491718cb..9944f2de 100644 --- a/crates/improvise-command/src/command/cmd/core.rs +++ b/crates/improvise-command/src/command/cmd/core.rs @@ -3,6 +3,8 @@ use std::fmt::Debug; use crate::command::key::KeyCode; +use super::navigation::CursorState; + use crate::model::Model; use crate::model::cell::{CellKey, CellValue}; use crate::ui::app::AppMode; @@ -139,13 +141,15 @@ pub trait Cmd: Debug + Send + Sync { /// Factory that constructs a Cmd from text arguments (headless/script). pub type ParseFn = fn(&[String]) -> Result, String>; -/// Fills the context-only fields of a command `parse` already built. It never -/// sees the args, so it cannot discard them — `context/code/cli.md` §8. -pub type ContextualizeFn = fn(C, &CmdContext) -> C; - /// Builds a command from context, reached ONLY for args `parse` refused. pub type FallbackFn = fn(&[String], &CmdContext) -> Result; +/// A command whose one context-only field is the cursor it moves from. The +/// registry fills it, and no registration can — `context/code/cli.md` §8. +pub trait AtCursor: Cmd { + fn cursor_mut(&mut self) -> &mut CursorState; +} + type BoxParseFn = Box Result, String>>; type BoxInteractiveFn = Box Result, String>>; @@ -186,25 +190,48 @@ impl CmdRegistry { name } - /// Register a command whose interactive arm is `parse` then `contextualize`, so - /// args `parse` accepts are honoured whole — `context/code/cli.md` §8. + /// Register a command whose interactive arm is `parse`, reaching `fallback` + /// only where `parse` refused — `context/code/cli.md` §8. pub fn register( &mut self, prototype: &C, parse: fn(&[String]) -> Result, - contextualize: ContextualizeFn, fallback: Option>, + ) { + self.push(prototype.name(), parse, fallback, |_, _| {}); + } + + /// As [`register`](Self::register), then the interactive arm moves the + /// command's cursor to the live one. The cursor is the only field it writes. + pub fn register_at_cursor( + &mut self, + prototype: &C, + parse: fn(&[String]) -> Result, + fallback: Option>, + ) { + self.push(prototype.name(), parse, fallback, |cmd, ctx| { + *cmd.cursor_mut() = CursorState::from_ctx(ctx); + }); + } + + fn push( + &mut self, + name: &'static str, + parse: fn(&[String]) -> Result, + fallback: Option>, + at_cursor: fn(&mut C, &CmdContext), ) { self.entries.push(CmdEntry { - name: prototype.name(), + name, parse: Box::new(move |args| parse(args).map(|c| Box::new(c) as Box)), interactive: Box::new(move |args, ctx| { - let built = match (parse(args), fallback) { - (Ok(cmd), _) => Ok(contextualize(cmd, ctx)), - (Err(_), Some(fallback)) => fallback(args, ctx), - (Err(err), None) => Err(err), + let mut built = match (parse(args), fallback) { + (Ok(cmd), _) => cmd, + (Err(_), Some(fallback)) => fallback(args, ctx)?, + (Err(err), None) => return Err(err), }; - built.map(|c| Box::new(c) as Box) + at_cursor(&mut built, ctx); + Ok(Box::new(built) as Box) }), }); } diff --git a/crates/improvise-command/src/command/cmd/mod.rs b/crates/improvise-command/src/command/cmd/mod.rs index 0000d0fd..cc51853f 100644 --- a/crates/improvise-command/src/command/cmd/mod.rs +++ b/crates/improvise-command/src/command/cmd/mod.rs @@ -117,6 +117,22 @@ pub(super) mod test_helpers { wb } + /// `two_cat_model` with one `_Measure` formula per target, in order. + pub fn with_formulas(targets: &[&str]) -> Workbook { + let mut wb = two_cat_model(); + for target in targets { + wb.model.add_formula(crate::formula::ast::Formula { + raw: format!("{target} = 0"), + target: target.to_string(), + target_category: "_Measure".to_string(), + expr: crate::formula::ast::Expr::Number(0.0), + filter: None, + format: None, + }); + } + wb + } + pub fn effects_debug(effects: &[Box]) -> String { format!("{:?}", effects) } diff --git a/crates/improvise-command/src/command/cmd/panel.rs b/crates/improvise-command/src/command/cmd/panel.rs index 1d27a42f..cfee15eb 100644 --- a/crates/improvise-command/src/command/cmd/panel.rs +++ b/crates/improvise-command/src/command/cmd/panel.rs @@ -192,11 +192,7 @@ mod tests { let layout = make_layout(&m); let reg = make_registry(); let ctx = make_ctx(&m, &layout, ®); - let cmd = CyclePanelFocus { - formula_open: false, - category_open: false, - view_open: false, - }; + let cmd = CyclePanelFocus; let effects = cmd.execute(&ctx); assert!(effects.is_empty()); } @@ -208,11 +204,7 @@ mod tests { let reg = make_registry(); let mut ctx = make_ctx(&m, &layout, ®); ctx.formula_panel_open = true; - let cmd = CyclePanelFocus { - formula_open: true, - category_open: false, - view_open: false, - }; + let cmd = CyclePanelFocus; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 1); let dbg = format!("{:?}", effects[0]); @@ -231,11 +223,7 @@ mod tests { let mut ctx = make_ctx(&m, &layout, ®); ctx.formula_panel_open = true; ctx.category_panel_open = true; - let cmd = CyclePanelFocus { - formula_open: true, - category_open: true, - view_open: false, - }; + let cmd = CyclePanelFocus; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 1); let dbg = effects_debug(&effects); @@ -256,11 +244,7 @@ mod tests { ctx.formula_panel_open = true; ctx.category_panel_open = true; ctx.mode = &AppMode::FormulaPanel; - let cmd = CyclePanelFocus { - formula_open: true, - category_open: true, - view_open: false, - }; + let cmd = CyclePanelFocus; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 1); let dbg = effects_debug(&effects); @@ -281,11 +265,7 @@ mod tests { ctx.category_panel_open = true; ctx.view_panel_open = true; ctx.mode = &AppMode::CategoryPanel; - let cmd = CyclePanelFocus { - formula_open: false, - category_open: true, - view_open: true, - }; + let cmd = CyclePanelFocus; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 1); let dbg = effects_debug(&effects); @@ -305,11 +285,7 @@ mod tests { ctx.formula_panel_open = true; ctx.view_panel_open = true; ctx.mode = &AppMode::ViewPanel; - let cmd = CyclePanelFocus { - formula_open: true, - category_open: false, - view_open: true, - }; + let cmd = CyclePanelFocus; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 1); let dbg = effects_debug(&effects); @@ -329,11 +305,7 @@ mod tests { let mut ctx = make_ctx(&m, &layout, ®); ctx.category_panel_open = true; ctx.mode = &AppMode::CategoryPanel; - let cmd = CyclePanelFocus { - formula_open: false, - category_open: true, - view_open: false, - }; + let cmd = CyclePanelFocus; let effects = cmd.execute(&ctx); let dbg = effects_debug(&effects); assert!( @@ -344,15 +316,13 @@ mod tests { #[test] fn move_panel_cursor_down_from_zero() { - let m = two_cat_model(); + let m = with_formulas(&["A", "B"]); let layout = make_layout(&m); let reg = make_registry(); let ctx = make_ctx(&m, &layout, ®); let cmd = MovePanelCursor { panel: effect::Panel::Formula, delta: 1, - current: 0, - max: 5, }; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 1); @@ -363,17 +333,35 @@ mod tests { ); } + /// Evidence: `context/test-evidence/command.md`. + #[test] + fn move_panel_cursor_moves_from_the_named_panels_own_cursor() { + let m = with_formulas(&["A", "B", "C", "D", "E"]); + let layout = make_layout(&m); + let reg = make_registry(); + let mut ctx = make_ctx(&m, &layout, ®); + ctx.formula_cursor = 1; + let args = ["formula".to_string(), "2".to_string()]; + let effects = reg + .interactive("move-panel-cursor", &args, &ctx) + .expect("move-panel-cursor resolves") + .execute(&ctx); + let dbg = effects_debug(&effects); + assert!( + dbg.contains("SetPanelCursor { panel: Formula, cursor: 3 }"), + "must move from the formula cursor (1) by the parsed delta (2), got: {dbg}" + ); + } + #[test] fn move_panel_cursor_clamps_at_zero() { - let m = two_cat_model(); + let m = with_formulas(&["A", "B"]); let layout = make_layout(&m); let reg = make_registry(); let ctx = make_ctx(&m, &layout, ®); let cmd = MovePanelCursor { panel: effect::Panel::Formula, delta: -1, - current: 0, - max: 5, }; let effects = cmd.execute(&ctx); assert!(effects.is_empty()); @@ -388,8 +376,6 @@ mod tests { let cmd = MovePanelCursor { panel: effect::Panel::Formula, delta: 1, - current: 0, - max: 0, }; let effects = cmd.execute(&ctx); assert!(effects.is_empty()); @@ -559,16 +545,15 @@ impl Cmd for TogglePanelAndFocus { #[derive(Debug)] pub struct TogglePanelVisibility { pub panel: Panel, - pub currently_open: bool, } impl Cmd for TogglePanelVisibility { fn name(&self) -> &'static str { "toggle-panel-visibility" } - fn execute(&self, _ctx: &CmdContext) -> Vec> { + fn execute(&self, ctx: &CmdContext) -> Vec> { vec![Box::new(effect::SetPanelOpen { panel: self.panel, - open: !self.currently_open, + open: !ctx.panel_open(self.panel), })] } } @@ -576,20 +561,16 @@ impl Cmd for TogglePanelVisibility { /// Tab through open panels Formula → Category → View, wrapping, starting from the /// focused one. One open panel self-transitions; none open is a no-op. #[derive(Debug)] -pub struct CyclePanelFocus { - pub formula_open: bool, - pub category_open: bool, - pub view_open: bool, -} +pub struct CyclePanelFocus; impl Cmd for CyclePanelFocus { fn name(&self) -> &'static str { "cycle-panel-focus" } fn execute(&self, ctx: &CmdContext) -> Vec> { let order = [ - (self.formula_open, AppMode::FormulaPanel), - (self.category_open, AppMode::CategoryPanel), - (self.view_open, AppMode::ViewPanel), + (ctx.formula_panel_open, AppMode::FormulaPanel), + (ctx.category_panel_open, AppMode::CategoryPanel), + (ctx.view_panel_open, AppMode::ViewPanel), ]; // Index of the first candidate: the panel after the focused one, // or the first panel when invoked from a non-panel mode. @@ -614,16 +595,17 @@ impl Cmd for CyclePanelFocus { pub struct MovePanelCursor { pub panel: Panel, pub delta: i32, - pub current: usize, - pub max: usize, } impl Cmd for MovePanelCursor { fn name(&self) -> &'static str { "move-panel-cursor" } - fn execute(&self, _ctx: &CmdContext) -> Vec> { - let cursor = self.current; - let max = self.max; + fn execute(&self, ctx: &CmdContext) -> Vec> { + let (cursor, max) = match self.panel { + Panel::Formula => (ctx.formula_cursor, ctx.model.formulas().len()), + Panel::Category => (ctx.cat_panel_cursor, ctx.cat_tree_len()), + Panel::View => (ctx.view_panel_cursor, ctx.workbook.views.len()), + }; if max == 0 { return vec![]; } diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index 6ef243dc..ef4e82b3 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -27,19 +27,21 @@ fn cursor_key(ctx: &CmdContext) -> Result { ctx.cell_key().clone().ok_or("no cell at cursor".into()) } -/// A `Move` built from args takes the live cursor as its origin. -fn move_from_cursor(cmd: Move, ctx: &CmdContext) -> Move { - Move { - cursor: CursorState::from_ctx(ctx), - ..cmd +impl AtCursor for Move { + fn cursor_mut(&mut self) -> &mut CursorState { + &mut self.cursor } } -/// A commit built from args still advances from the live cursor. -fn commit_from_cursor(cmd: CommitAndAdvance, ctx: &CmdContext) -> CommitAndAdvance { - CommitAndAdvance { - cursor: CursorState::from_ctx(ctx), - ..cmd +impl AtCursor for EnterAdvance { + fn cursor_mut(&mut self) -> &mut CursorState { + &mut self.cursor + } +} + +impl AtCursor for CommitAndAdvance { + fn cursor_mut(&mut self) -> &mut CursorState { + &mut self.cursor } } @@ -61,11 +63,6 @@ fn commit_at_cursor( }) } -/// For a command with no context-only fields: `parse`'s result is used as built. -fn as_parsed(cmd: C, _ctx: &CmdContext) -> C { - cmd -} - /// Build the default command registry with all commands. /// Registry names MUST match the `Cmd::name()` return value. pub fn default_registry() -> CmdRegistry { @@ -90,7 +87,6 @@ pub fn default_registry() -> CmdRegistry { key: parse_cell_key_from_args(args), }) }, - as_parsed, Some(|_, ctx| { Ok(ClearCellCommand { key: cursor_key(ctx)?, @@ -121,7 +117,7 @@ pub fn default_registry() -> CmdRegistry { r.register_pure(&HelpPagePrevCmd(vec![]), HelpPagePrevCmd::parse); // ── Navigation (unified Move) ────────────────────────────────────── - r.register( + r.register_at_cursor( &Move { kind: MoveKind::Relative(0, 0), cursor: CursorState::default(), @@ -137,13 +133,12 @@ pub fn default_registry() -> CmdRegistry { cmd_name: "move-selection", }) }, - move_from_cursor, None, ); // Jump-to-edge commands: first/last row/col macro_rules! reg_jump { ($r:expr, $is_row:expr, $to_end:expr, $name:expr) => { - $r.register( + $r.register_at_cursor( &Move { kind: if $to_end { MoveKind::ToEnd($is_row) @@ -164,7 +159,6 @@ pub fn default_registry() -> CmdRegistry { cmd_name: $name, }) }, - move_from_cursor, None, ); }; @@ -173,7 +167,7 @@ pub fn default_registry() -> CmdRegistry { reg_jump!(r, true, true, "jump-last-row"); reg_jump!(r, false, false, "jump-first-col"); reg_jump!(r, false, true, "jump-last-col"); - r.register( + r.register_at_cursor( &Move { kind: MoveKind::Relative(0, 0), cursor: CursorState::default(), @@ -188,10 +182,9 @@ pub fn default_registry() -> CmdRegistry { cmd_name: "scroll-rows", }) }, - move_from_cursor, None, ); - r.register( + r.register_at_cursor( &Move { kind: MoveKind::Page(0), cursor: CursorState::default(), @@ -206,10 +199,9 @@ pub fn default_registry() -> CmdRegistry { cmd_name: "page-scroll", }) }, - move_from_cursor, None, ); - r.register( + r.register_at_cursor( &EnterAdvance { cursor: CursorState::default(), }, @@ -227,9 +219,6 @@ pub fn default_registry() -> CmdRegistry { }, }) }, - |_, ctx| EnterAdvance { - cursor: CursorState::from_ctx(ctx), - }, None, ); @@ -246,7 +235,6 @@ pub fn default_registry() -> CmdRegistry { key: parse_cell_key_from_args(args), }) }, - as_parsed, Some(|_, ctx| { Ok(YankCell { key: cursor_key(ctx)?, @@ -265,7 +253,6 @@ pub fn default_registry() -> CmdRegistry { key: parse_cell_key_from_args(args), }) }, - as_parsed, Some(|_, ctx| { Ok(PasteCell { key: cursor_key(ctx)?, @@ -318,7 +305,6 @@ pub fn default_registry() -> CmdRegistry { key: parse_cell_key_from_args(args), }) }, - as_parsed, Some(|_, ctx| { Ok(DrillIntoCell { key: cursor_key(ctx)?, @@ -356,77 +342,30 @@ pub fn default_registry() -> CmdRegistry { }); Ok(TogglePanelAndFocus { panel, target }) }, - as_parsed, None, ); r.register( &TogglePanelVisibility { panel: Panel::Formula, - currently_open: false, }, |args| { require_args(args, 1)?; let panel = parse_panel(&args[0])?; - Ok(TogglePanelVisibility { - panel, - currently_open: false, - }) - }, - |cmd, ctx| TogglePanelVisibility { - currently_open: ctx.panel_open(cmd.panel), - ..cmd - }, - None, - ); - r.register( - &CyclePanelFocus { - formula_open: false, - category_open: false, - view_open: false, - }, - |_| { - Ok(CyclePanelFocus { - formula_open: false, - category_open: false, - view_open: false, - }) - }, - |_, ctx| CyclePanelFocus { - formula_open: ctx.formula_panel_open, - category_open: ctx.category_panel_open, - view_open: ctx.view_panel_open, + Ok(TogglePanelVisibility { panel }) }, None, ); + r.register_nullary(|| Box::new(CyclePanelFocus)); r.register( &MovePanelCursor { panel: Panel::Formula, delta: 0, - current: 0, - max: 0, }, |args| { require_args(args, 2)?; let panel = parse_panel(&args[0])?; let delta = args[1].parse::().map_err(|e| e.to_string())?; - Ok(MovePanelCursor { - panel, - delta, - current: 0, - max: 0, - }) - }, - |cmd, ctx| { - let (current, max) = match cmd.panel { - Panel::Formula => (ctx.formula_cursor, ctx.model.formulas().len()), - Panel::Category => (ctx.cat_panel_cursor, ctx.cat_tree_len()), - Panel::View => (ctx.view_panel_cursor, ctx.workbook.views.len()), - }; - MovePanelCursor { - current, - max, - ..cmd - } + Ok(MovePanelCursor { panel, delta }) }, None, ); @@ -493,7 +432,7 @@ pub fn default_registry() -> CmdRegistry { // ── Commit ─────────────────────────────────────────────────────────── // These take a mode-name arg as args[0]; the command never inspects ctx.mode. - r.register( + r.register_at_cursor( &CommitAndAdvance { key: CellKey::new(vec![]), value: String::new(), @@ -514,10 +453,9 @@ pub fn default_registry() -> CmdRegistry { edit_mode, }) }, - commit_from_cursor, Some(|args, ctx| commit_at_cursor(args, ctx, AdvanceDir::Down)), ); - r.register( + r.register_at_cursor( &CommitAndAdvance { key: CellKey::new(vec![]), value: String::new(), @@ -526,7 +464,6 @@ pub fn default_registry() -> CmdRegistry { edit_mode: AppMode::editing(), }, |_| Err("commit-and-advance-right requires context".into()), - commit_from_cursor, Some(|args, ctx| commit_at_cursor(args, ctx, AdvanceDir::Right)), ); r.register( @@ -538,7 +475,6 @@ pub fn default_registry() -> CmdRegistry { let kind = parse_commit_kind(&args[0], &AppMode::Normal)?; Ok(CommitBuffer { kind }) }, - as_parsed, Some(|args, ctx| { require_args(args, 1)?; let kind = parse_commit_kind(&args[0], ctx.mode)?; @@ -863,12 +799,13 @@ mod panel_open_tests { let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); for asked in Panel::ALL { let args = [asked.name().to_string()]; - let cmd = ctx + let effects = ctx .registry .interactive("toggle-panel-visibility", &args, &ctx) - .expect("toggle-panel-visibility resolves"); - let dbg = format!("{cmd:?}"); - let expected = format!("currently_open: {}", asked == opened); + .expect("toggle-panel-visibility resolves") + .execute(&ctx); + let dbg = format!("{effects:?}"); + let expected = format!("SetPanelOpen {{ panel: {asked:?}, open: {} }}", asked != opened); assert!( dbg.contains(&expected), "with only {opened:?} open, toggling {asked:?} read another \ From d4b60d7979a2442d9009a28b89bff929a8e3b0aa Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:23:43 -0700 Subject: [PATCH 04/16] yf7p: no registration can overwrite a parsed field CmdRegistry::register loses its contextualize parameter. Every contextualizer in the tree was one of two shapes: the command's CursorState, now filled by the registry itself through AtCursor (a slot typed &mut CursorState, so it cannot point at a parsed field); or a read of context copied into the command only to be read back (toggle-panel-visibility, cycle-panel-focus, move-panel-cursor), now read in execute. The five overwrites review injected are unwritable. Reasoning: context/code/cli.md section 8. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/cli.md | 69 ++++++++++++++----- context/code/command.md | 12 ++-- context/test-evidence/cli.md | 5 +- context/test-evidence/command.md | 23 +++++-- .../src/command/cmd/registry.rs | 5 +- 5 files changed, 82 insertions(+), 32 deletions(-) diff --git a/context/code/cli.md b/context/code/cli.md index d4cc9a68..fb4d5bd2 100644 --- a/context/code/cli.md +++ b/context/code/cli.md @@ -164,7 +164,7 @@ limitation that is not new: `ExecuteCommand` returns one effect batch, so the segments of `:a . b` all see the pre-batch cursor, exactly as a `Binding::Sequence` does (gotchas 21 and 23). -## 8 — the interactive arm is `parse`, then context; it is never a second parser +## 8 — the interactive arm is `parse`, then the cursor; it is never a second parser #151 fixed three instances of *a registry with two construction arms demotes whichever caller reaches the weaker one*, one patch each. A post-merge review @@ -178,31 +178,59 @@ and the key from the cursor. So a script that wrote 5 to `Region/East` The shape rather than the instances is what changed. `CmdRegistry::register` used to take two free functions, `parse(args)` and `interactive(args, ctx)`, and nothing related them; each command re-derived its arguments twice and every -disagreement was silent. It now takes three pieces, and the interactive arm is -**built from `parse`**, not beside it: +disagreement was silent. The interactive arm is now **built from `parse`**, not +beside it: - `parse(args) -> C` — the only function that reads arguments a caller supplied. -- `contextualize(C, &CmdContext) -> C` — fills the fields that are context and - never argument (a `Move`'s origin cursor, a `MovePanelCursor`'s bound). It is - handed the built command and **not the args**, so it cannot discard them. - `fallback: Option<(args, ctx) -> C>` — reached **only** when `parse` refused. This is where "no coordinates means the cursor" lives: `clear-cell` bare, and `commit-cell-edit ` bare, which is what the keymap binds. +- For `register_at_cursor` only, the registry itself then sets the command's + **cursor** — its `CursorState`, reached through `AtCursor::cursor_mut` — to the + live one. That is the whole of what the interactive arm adds to a parsed + command, and no registration supplies it. -Two consequences hold by construction rather than by test. **Whatever `parse` -accepts, the interactive arm accepts** — `contextualize` is infallible — so the -§7 gate cannot pass a segment the loop then refuses. And **an accepted argument -list cannot be overridden by the cursor**, because the only function that sees -both args and context never runs on an accepted list. `register_pure` and -`register_nullary` already had one arm; they are the `contextualize = identity, -fallback = None` case. +Three consequences hold by construction rather than by test. **Whatever `parse` +accepts, the interactive arm accepts** — nothing after `parse` can fail — so the +§7 gate cannot pass a segment the loop then refuses. **An accepted argument list +cannot be overridden by the cursor**, because the only function that sees both +args and context never runs on an accepted list. And **no field `parse` set can +be overwritten from context**, because nothing a registration writes can name +one: the cursor slot is typed `&mut CursorState`, and no command parses a +`CursorState` out of its arguments. `register_pure` and `register_nullary` +already had one arm; they are `register` with no fallback. + +That last one is improvise-yf7p, and it is why the registry lost a parameter. +#171 took a third function, `contextualize(C, &CmdContext) -> C`, handed the +built command and not the args, and stated honestly that it could still +**overwrite** a parsed field — naming the field was "a visible line in review". +Review then injected five such overwrites on commands other than the one with a +test (yank, paste and drill-into-cell taking their key from the cursor over the +parsed one; toggle-panel-visibility ignoring its parsed panel; move-panel-cursor +clamping its parsed delta to ±1) and the whole workspace stayed green. A hole +guarded by review discipline plus one command's test is not guarded. Every +contextualizer in the tree fell into one of two shapes, and each had an answer +that removes the function rather than checking it: + +- **The cursor** (`Move`, `EnterAdvance`, `CommitAndAdvance`). Always one + `CursorState` field, always `CursorState::from_ctx`. So the registry does it, + through a slot whose *type* is the guard: an `AtCursor` impl pointing at a + `CellKey` or a delta is a compile error, which was tried. +- **A read of context that belonged in `execute`** (`toggle-panel-visibility`'s + `currently_open`, `cycle-panel-focus`'s three flags, `move-panel-cursor`'s + cursor and bound). These copied context into the command only to read it back; + `execute` now reads `ctx` directly, the same answer `toggle-panel-and-focus` + got below. Those three are `register`/`register_nullary` now, so their + interactive arm is exactly `parse`. + +What `execute` does with a parsed field is still the command's own logic and is +tested as such — `toggle_panel_visibility_reads_the_panel_it_names` and +`move_panel_cursor_moves_from_the_named_panels_own_cursor` fail if the panel or +the delta is ignored there. The difference is that the registry is no longer a +second place such a bug can be written. What is still representable, stated rather than hidden: -- `contextualize` can overwrite a field `parse` set. It would have to name the - field, which is a visible line in review, where the old `|_args, ctx|` was an - invisible omission. `commit_cell_edit_honours_an_explicit_value_and_cell_over_the_cursor` - fails if `commit-cell-edit`'s does. - A `fallback` sees the args `parse` refused and could ignore some of them. `commit-cell-edit`'s refuses anything but a lone mode, so the malformed `commit-cell-edit editing 5` is an error on the `:` line — which has no pure @@ -210,9 +238,12 @@ What is still representable, stated rather than hidden: - The gate is **stricter** than the loop wherever a fallback exists: a bare `clear-cell` is refused in a script. That is the safe direction — the whole line is refused — and it is unchanged from before #151. +- A new command could declare a *second* `CursorState` field and parse it from + args; `cursor_mut` could then point at the wrong one. No command has one, and + a parsed cursor would be a new idea deserving its own argument. -`toggle-panel-and-focus` did not fit `contextualize`, because its pure arm -defaulted the omitted flags to `true` and a contextualizer cannot tell an +`toggle-panel-and-focus` did not fit #171's `contextualize` either, because its +pure arm defaulted the omitted flags to `true` and a contextualizer cannot tell an omitted flag from an explicit one. So the omission is now in the type — `target: Option<(bool, bool)>`, `None` meaning *toggle* — and the toggle is resolved in `execute` against the same context. That is the honest home: the diff --git a/context/code/command.md b/context/code/command.md index 987df348..bc985d33 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -2490,12 +2490,12 @@ available for the rare command that needs to enumerate all views, such as the vi panel. The interactive arm (keymap dispatch, scripts, the `:` line) is built **from** -`parse`: `register` takes `parse`, a `contextualize` that fills context-only -fields of the command `parse` built without seeing the args, and an optional -`fallback` reached only for argument lists `parse` refused. So an accepted -argument list is never overridden by context (improvise-2bac; the argument is -`context/code/cli.md` §8). `register_pure` is the case with nothing to -contextualize and no fallback: both arms are `parse`. +`parse`: `register` takes `parse` and an optional `fallback` reached only for +argument lists `parse` refused, and `register_at_cursor` additionally has the +registry set the command's `CursorState` to the live cursor. No registration +writes any other field, so an accepted argument list is never overridden by +context (improvise-2bac, improvise-yf7p; the argument is `context/code/cli.md` +§8). `register_pure` is the case with no fallback: both arms are `parse`. `write_buffer` is the effect that writes a named buffer — the mirror of `read_buffer`, and the only place that knows "search" is not an entry in the buffers diff --git a/context/test-evidence/cli.md b/context/test-evidence/cli.md index 4c89ddee..031e8780 100644 --- a/context/test-evidence/cli.md +++ b/context/test-evidence/cli.md @@ -298,8 +298,9 @@ The unit-level form of the headless test, against `CmdRegistry::interactive` directly: cursor on `Type=Food, Month=Jan`, a decoy `edit` buffer, and an explicit `5` for `Type/Clothing Month/Feb`. It asserts the built command's key and value. Rival (a) above — a `contextualize` that overwrites the parsed -fields — fails it; this is the one hole the new registry shape leaves -representable, so it is the one that needs a test. +fields — failed it. improvise-yf7p then removed `contextualize` altogether +(`context/code/cli.md` §8), so rival (a) no longer compiles; the test stays as +the behavioural statement of what the signature now guarantees. ### every_argument_list_the_gate_accepts_the_run_accepts_too diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index efcda9b0..cbfe697f 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -3106,10 +3106,13 @@ Injected fault: hardcode the `category_panel_open` propagator to `true`. Fails. ### toggle_panel_visibility_reads_the_panel_it_names -The behavioural half, and the one that reaches the registry. Its interactive arm -passes `currently_open` straight through, so with exactly one panel open the command -built for each of the three names carries its own panel's flag. Both injections above -fail here too. +The behavioural half, and the one that reaches the registry. With exactly one panel +open, the command built for each of the three names and executed must emit +`SetPanelOpen` for **its own** panel, flipping **its own** flag. Both injections above +fail here too. Since improvise-yf7p the flag is read in `execute` rather than copied +in by a registry contextualizer, so the assertion moved from the built command's +`Debug` to its effects; it also names the panel, so an `execute` that ignores the +parsed panel (`ctx.panel_open(Panel::Formula)`) fails it — injected and run. ### a_focused_open_panel_closes_and_a_closed_one_opens @@ -4694,6 +4697,18 @@ Its mirror, and the one that pins the wrap: from the first item `[` must land on `SetPanelCursor` carries a panel *and* a cursor and neither was checked. Both are now named, so moving the right cursor on the wrong panel fails. +### move_panel_cursor_moves_from_the_named_panels_own_cursor + +improvise-yf7p. `move-panel-cursor formula 2` through the registry's interactive +arm, with the formula cursor at **1** of five formulas and the category and view +cursors at 0, must land on **3**. Before yf7p the cursor and bound were copied in +by a registry `contextualize`, and review showed one that clamped the parsed delta +to `delta.signum()` left the whole workspace green: every test moved by ±1, where +a clamp is invisible. They are read in `execute` now, and this test is what sees a +bug there. Rival `self.delta.signum()` in `execute` — injected, run, fails (lands +on 2). Reading another panel's cursor fails too, because the fixture seats them at +different values — the index-0 fixture could not separate them. + ### delete_formula_at_cursor_with_formulas Asserts the `RemoveFormula` names `Profit` in `Type` — the formula the cursor is diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index ef4e82b3..0534709e 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -805,7 +805,10 @@ mod panel_open_tests { .expect("toggle-panel-visibility resolves") .execute(&ctx); let dbg = format!("{effects:?}"); - let expected = format!("SetPanelOpen {{ panel: {asked:?}, open: {} }}", asked != opened); + let expected = format!( + "SetPanelOpen {{ panel: {asked:?}, open: {} }}", + asked != opened + ); assert!( dbg.contains(&expected), "with only {opened:?} open, toggling {asked:?} read another \ From 4277cf6f84872f0e2015f7d819866c4474a8ce6e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:28:58 -0700 Subject: [PATCH 05/16] y1av: seat the at-cursor tests off index 0 delete-formula-at-cursor, switch-view-at-cursor and delete-view-at-cursor were asserted with the cursor at 0 of a one-element list, where the item under the cursor and the first item are one item. A DeleteFormulaAtCursor hard-wired to index 0 passed the whole workspace. Now each cursor sits at index 1 of a longer list; the index-0 rivals fail. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/command.md | 29 +++++++-- .../src/command/cmd/panel.rs | 62 ++++++++++++++----- 2 files changed, 72 insertions(+), 19 deletions(-) diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index fcbee537..34b87505 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -4721,17 +4721,38 @@ are now named, so moving the right cursor on the wrong panel fails. ### delete_formula_at_cursor_with_formulas -Asserts the `RemoveFormula` names `Profit` in `Type` — the formula the cursor is -on — rather than merely that a formula was removed. +Asserts the `RemoveFormula` names the formula the cursor is on, rather than merely +that a formula was removed. PR #148 named the field, but its fixture was **one** +formula with the cursor at **0**, where "the formula under the cursor" and "the +first formula" are the same formula (improvise-y1av). The rival it could not see — +`DeleteFormulaAtCursor` reading index `0` instead of `ctx.formula_cursor` — passed +the **whole workspace**, so `d` in the formula panel deleting the wrong formula had +no guard anywhere. Now two formulas, cursor at **1**: it must remove `Tax` and must +not name `Profit`. Rival `let cursor = 0usize.min(..)` injected, full suite run: +this is the only test that fails. ### switch_view_at_cursor_with_valid_cursor -Names `Default`, the view under the cursor. +Names the view under the cursor. Same fixture defect as above (improvise-y1av): +`two_cat_model` has one view, so the cursor at 0 named `Default` whether or not +the command read it. Now `Default, Other` with the cursor at **1**, asserting +`Other`. Rival `get(cursor).and(first())` — in bounds only, so the out-of-bounds +arm is unchanged and `switch_view_at_cursor_out_of_bounds_returns_empty` cannot be +what catches it — injected and run: fails here, in +`delete_view_at_cursor_deletes_the_view_under_a_nonzero_cursor`, and in the two +`wasm-client/tests/lockstep.rs` tests that were previously the only guard. + +### delete_view_at_cursor_deletes_the_view_under_a_nonzero_cursor + +improvise-y1av. `Default, Other, Third` with the cursor at **1**: deletes `Other` +and steps the panel cursor back to 0. The same rival as above fails it. ### delete_view_at_cursor_zero_does_not_adjust_cursor Names `Default` too. Its existing negative half — no `SetPanelCursor` at -position 0 — is unchanged. +position 0 — is unchanged. Position 0 is this test's subject, so it cannot +separate "the view under the cursor" from "the first view" and does not try to; +the test above does. ### commit_category_add_with_name_produces_add_effect diff --git a/crates/improvise-command/src/command/cmd/panel.rs b/crates/improvise-command/src/command/cmd/panel.rs index 1d27a42f..7c17c7f1 100644 --- a/crates/improvise-command/src/command/cmd/panel.rs +++ b/crates/improvise-command/src/command/cmd/panel.rs @@ -395,39 +395,50 @@ mod tests { assert!(effects.is_empty()); } + /// Evidence: `context/test-evidence/command.md`. #[test] fn delete_formula_at_cursor_with_formulas() { let mut m = two_cat_model(); - m.model.add_formula(crate::formula::ast::Formula { - raw: "Profit = Revenue - Cost".to_string(), - target: "Profit".to_string(), - target_category: "Type".to_string(), - expr: crate::formula::ast::Expr::Number(0.0), - filter: None, - format: None, - }); + for (target, raw) in [("Profit", "Profit = 1"), ("Tax", "Tax = 2")] { + m.model.add_formula(crate::formula::ast::Formula { + raw: raw.to_string(), + target: target.to_string(), + target_category: "Type".to_string(), + expr: crate::formula::ast::Expr::Number(0.0), + filter: None, + format: None, + }); + } let layout = make_layout(&m); let reg = make_registry(); - let ctx = make_ctx(&m, &layout, ®); + let mut ctx = make_ctx(&m, &layout, ®); + ctx.formula_cursor = 1; let effects = DeleteFormulaAtCursor.execute(&ctx); let dbg = effects_debug(&effects); assert!( - dbg.contains("RemoveFormula { target: \"Profit\", target_category: \"Type\" }"), - "must remove the formula AT THE CURSOR, got: {dbg}" + dbg.contains("RemoveFormula { target: \"Tax\", target_category: \"Type\" }"), + "must remove the formula AT THE CURSOR (Tax, index 1), got: {dbg}" + ); + assert!( + !dbg.contains("\"Profit\""), + "removed the first formula, not the one under the cursor: {dbg}" ); } + /// Evidence: `context/test-evidence/command.md`. #[test] fn switch_view_at_cursor_with_valid_cursor() { - let m = two_cat_model(); + let mut m = two_cat_model(); + m.create_view("Other"); let layout = make_layout(&m); let reg = make_registry(); - let ctx = make_ctx(&m, &layout, ®); + let mut ctx = make_ctx(&m, &layout, ®); + ctx.view_panel_cursor = 1; let effects = SwitchViewAtCursor.execute(&ctx); let dbg = effects_debug(&effects); assert!( - dbg.contains("SwitchView(\"Default\")"), - "must switch to the view UNDER THE CURSOR, got: {dbg}" + dbg.contains("SwitchView(\"Other\")"), + "must switch to the view UNDER THE CURSOR (Other, index 1), got: {dbg}" ); assert!(dbg.contains("Normal"), "Expected Normal mode, got: {dbg}"); } @@ -504,6 +515,27 @@ mod tests { ); } + /// Evidence: `context/test-evidence/command.md`. + #[test] + fn delete_view_at_cursor_deletes_the_view_under_a_nonzero_cursor() { + let mut m = two_cat_model(); + m.create_view("Other"); + m.create_view("Third"); + let layout = make_layout(&m); + let reg = make_registry(); + let mut ctx = make_ctx(&m, &layout, ®); + ctx.view_panel_cursor = 1; + let dbg = effects_debug(&DeleteViewAtCursor.execute(&ctx)); + assert!( + dbg.contains("DeleteView(\"Other\")"), + "must delete the view UNDER THE CURSOR (Other, index 1), got: {dbg}" + ); + assert!( + dbg.contains("SetPanelCursor { panel: View, cursor: 0 }"), + "a delete off position 0 steps the cursor back, got: {dbg}" + ); + } + #[test] fn delete_view_at_cursor_zero_does_not_adjust_cursor() { let m = two_cat_model(); From e0550fc51f7d5a844bb7c6dbc0ca61fd2c29953c Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:32:18 -0700 Subject: [PATCH 06/16] WIP pr-watch: failing cases for UNKNOWN mergeability (improvise-edpl) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- scripts/pr-watch-test.sh | 43 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/scripts/pr-watch-test.sh b/scripts/pr-watch-test.sh index baed93ae..1fdb22ec 100755 --- a/scripts/pr-watch-test.sh +++ b/scripts/pr-watch-test.sh @@ -73,5 +73,48 @@ run_case "a head that moves mid-wait is not a pass" 2 "$work/moved" --timeout 60 run_case "--expect refuses a different head" 2 "$work/green" --expect bbbbbbbb run_case "--expect accepts a prefix of the head" 0 "$work/green" --expect aaaa +# improvise-edpl. GitHub computes mergeability lazily: for a while after the +# base moves it reports UNKNOWN, and PRs #172 and #179 both exited 0 there. +# This is the shape both had: every check green, mergeability unresolved. +green_rollup='[{"name":"build","status":"COMPLETED","conclusion":"SUCCESS"}]' +red_rollup='[{"name":"build","status":"COMPLETED","conclusion":"FAILURE"}]' +# $1 dir, $2 file, $3 mergeable, $4 mergeStateStatus, $5 rollup (JSON values) +state() { + mkdir -p "$1" + printf '{"headRefOid":"aaaaaaaaaaaa","mergeable":%s,"mergeStateStatus":%s,"statusCheckRollup":%s}\n' \ + "$3" "$4" "$5" > "$1/$2" +} + +state "$work/unk-conflict" 1.json '"UNKNOWN"' '"UNKNOWN"' "$green_rollup" +state "$work/unk-conflict" last.json '"CONFLICTING"' '"DIRTY"' "$green_rollup" +run_case "UNKNOWN that resolves to CONFLICTING fails" 1 "$work/unk-conflict" --timeout 60 + +state "$work/unk-clean" 1.json '"UNKNOWN"' '"UNKNOWN"' "$green_rollup" +state "$work/unk-clean" 2.json '"MERGEABLE"' '"UNKNOWN"' "$green_rollup" +state "$work/unk-clean" last.json '"MERGEABLE"' '"CLEAN"' "$green_rollup" +run_case "UNKNOWN that resolves to MERGEABLE/CLEAN passes" 0 "$work/unk-clean" --timeout 60 + +state "$work/unk-forever" last.json '"UNKNOWN"' '"UNKNOWN"' "$green_rollup" +run_case "UNKNOWN until the timeout is a timeout, not a pass" 3 "$work/unk-forever" + +# A failed check fails whatever mergeability says; waiting on it would turn a +# known red into a timeout. +state "$work/unk-red" last.json '"UNKNOWN"' '"UNKNOWN"' "$red_rollup" +run_case "a failed check fails without waiting on mergeability" 1 "$work/unk-red" --timeout 0 + +# The whole table, enumerated rather than hand-picked, every check green. The +# rule it states is in context/code/tooling.md, "pr-watch and mergeability". +for m in MERGEABLE CONFLICTING UNKNOWN; do + for s in BEHIND BLOCKED CLEAN DIRTY DRAFT HAS_HOOKS UNKNOWN UNSTABLE SOMETHING_NEW null; do + if [ "$m" = CONFLICTING ]; then want=1 + elif [ "$m" = UNKNOWN ] || [ "$s" = UNKNOWN ]; then want=3 + else case "$s" in CLEAN|UNSTABLE|HAS_HOOKS) want=0 ;; *) want=1 ;; esac + fi + sv="\"$s\""; [ "$s" = null ] && sv=null + state "$work/table-$m-$s" last.json "\"$m\"" "$sv" "$green_rollup" + run_case "table: $m/$s" "$want" "$work/table-$m-$s" --timeout 0 + done +done + printf '\npr-watch-test: %d passed, %d failed\n' "$pass" "$fail" [ "$fail" -eq 0 ] From d1a9ef9add081ce412aae65c73ccbe9b1b511db1 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:34:07 -0700 Subject: [PATCH 07/16] pr-watch: wait out UNKNOWN mergeability; pass only an allowlisted state (improvise-edpl) GitHub reports UNKNOWN for a while after the base moves, and PRs #172 and #179 both exited 0 there. UNKNOWN is now waited on (exit 3 on timeout), a failed check or CONFLICTING ends the wait, and the pass condition is MERGEABLE with CLEAN, UNSTABLE or HAS_HOOKS. BLOCKED, DIRTY, BEHIND, DRAFT and any unshipped state fail closed. The suite enumerates the whole table. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/tooling.md | 45 ++++++++++++++++++++++++++++++++ context/test-evidence/tooling.md | 34 ++++++++++++++++++++++++ scripts/pr-watch | 38 ++++++++++++++++----------- 3 files changed, 102 insertions(+), 15 deletions(-) diff --git a/context/code/tooling.md b/context/code/tooling.md index bd1d9c6d..0667f408 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -259,6 +259,51 @@ the deploy's and pass. `README.md` therefore states the rule and deliberately **does not list the jobs**: a second copy with nothing holding it to the first is what went stale, twice. +### pr-watch and mergeability (improvise-edpl) + +`scripts/pr-watch` is what the orchestrator's merge procedure stands on, and +until improvise-edpl its verdict failed only on `mergeable == CONFLICTING`. +GitHub computes mergeability lazily: for a while after the base moves it reports +`UNKNOWN` for both `mergeable` and `mergeStateStatus`. PRs #172 and #179 both +exited 0 at `UNKNOWN/UNKNOWN … 0 failed`. A PR that has *just become* +conflicting is in exactly that window, so it could pass the watch. It is the +same shape as improvise-b4ks: an uncertain state read as success. + +The rule now: + +- **`UNKNOWN` in either field is not an answer, so it is waited on.** On + timeout it exits 3, the same code as a check still running. It never exits 0. +- **A known answer ends the wait early**: a failed check (exit 1 whatever + mergeability says, since waiting would turn a known red into a timeout), or + `mergeable == CONFLICTING`. +- **The pass condition is an allowlist**: `mergeable == MERGEABLE` *and* + `mergeStateStatus` in `CLEAN`, `UNSTABLE` or `HAS_HOOKS`. Anything else exits + 1. That includes `null` and any state GitHub adds later, so the check fails + closed. + +Why each state is where it is. The `main` ruleset (`gh api +repos/fiddlerwoaroof/improvise/rules/branches/main`, read 2026-09-22) requires +three status checks. It requires no approvals, and its required checks are not +strict. + +- `BLOCKED` with a `MERGEABLE` flag means a required check has not passed, + either unrun or failing. With zero required approvals nothing else produces it + here. It is refused, not waited on. A required check that never runs would + otherwise hold the watch until the timeout, and exit 1 is the "not mergeable" + answer. +- `DIRTY` can wear `MERGEABLE` for a moment while the two fields settle. It is + refused. +- `BEHIND` needs strict required checks, which this repo does not set. `DRAFT` + cannot be merged. Both are refused. +- `UNSTABLE` means a non-required check is unhappy. It stays a pass, as it was + before. Any failed check in the rollup has already exited 1, so in practice it + only means a non-required status still settling. +- `HAS_HOOKS` is GitHub Enterprise's "mergeable, with pre-receive hooks". It is + harmless to allow. + +The suite enumerates the whole `mergeable × mergeStateStatus` table, plus an +unshipped value and `null`, rather than hand-picking cases. Why: improvise-90ea. + ### Why the Pages deploy fails on "Found 0 artifact(s)" (improvise-864a) `actions/deploy-pages@v5` intermittently fails a push to `main` with *No diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 341a0a32..439ca4aa 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -610,3 +610,37 @@ to git. A later `add` is excused. After `git checkout --` and a hand loss, the its `.base`, `.ledger`, `.removed` and `.stamped` siblings and `.lock`. `git status --untracked-files=all` and `git add -A --dry-run` both show none of them, and `git check-ignore -v` names `.gitignore:40` (`prolog-sessions/*`). + +### The pr-watch mergeability cases — `scripts/pr-watch-test.sh` + +improvise-edpl. The cases are injected faults in themselves. Like the other +`scripts/` suites, none of their names is a Rust `fn`, so the case list is the +evidence. The reasoning is in `context/code/tooling.md`, "pr-watch and +mergeability". + +**The bug, reproduced before the fix.** Against the script as it stood at +`ae96b8c`, 19 of the 34 new cases failed and all 19 exited **0**. +`UNKNOWN that resolves to CONFLICTING fails` is PR #172's shape exactly: every +check green, `UNKNOWN/UNKNOWN` on the first poll, `CONFLICTING/DIRTY` on the +second. The old script broke out of the loop on the first poll and passed. Each +fixture is the real `gh pr view --json` payload with only the two mergeability +fields changed. A stub that errors instead would have exited 4, a wider shape +than the fault. + +**Rival 1: treat `UNKNOWN` as an answer.** The wait condition was replaced with +`if true`. 13 cases fail: every `UNKNOWN` row of the table, +`MERGEABLE/UNKNOWN`, the timeout case, and +`UNKNOWN that resolves to MERGEABLE/CLEAN passes`. That last one exits 1 under +this rival, so the fix cannot pass by refusing everything uncertain. It has to +wait. `UNKNOWN that resolves to CONFLICTING fails` still passes under this +rival, because the allowlist refuses `UNKNOWN` as well. The next rival is what +separates it. + +**Rival 2: the old verdict.** The allowlist was replaced with `exit 0` after the +`MERGEABLE` test. Six cases fail: `MERGEABLE/BEHIND`, `BLOCKED`, `DIRTY`, +`DRAFT`, `SOMETHING_NEW` and `null`. These are the states that look mergeable +by the flag and are not. + +`a failed check fails without waiting on mergeability` runs at `--timeout 0`. +A script that waited on `UNKNOWN` before reading the rollup would report that +known red as exit 3. diff --git a/scripts/pr-watch b/scripts/pr-watch index 37a0053e..e90dc799 100755 --- a/scripts/pr-watch +++ b/scripts/pr-watch @@ -1,6 +1,6 @@ #!/usr/bin/env bash # Waits for a PR's checks to finish and says what happened. Every rule in here -# is one this repo got wrong first; see context/code/pr-watch.md. +# is one this repo got wrong first; see context/code/tooling.md section 3. set -euo pipefail usage() { @@ -11,10 +11,11 @@ usage: pr-watch [--expect ] [--timeout ] [--interval --timeout give up after this long (default 1800) --interval seconds between polls (default 20) -exit 0 every check COMPLETED and none failed -exit 1 a check failed, or the PR is not mergeable +exit 0 every check COMPLETED, none failed, and GitHub says MERGEABLE with a + state of CLEAN, UNSTABLE or HAS_HOOKS +exit 1 a check failed, or the PR is not mergeable (any other known state) exit 2 the head moved during the wait, or is not --expect -exit 3 timed out with checks still running +exit 3 timed out with checks still running or mergeability still UNKNOWN exit 4 usage or environment error EOF exit 4 @@ -86,13 +87,25 @@ while :; do total="$(printf '%s' "$json" | jqf '[.statusCheckRollup[]?] | length')" running="$(printf '%s' "$json" | jqf '[.statusCheckRollup[]? | select(.status != "COMPLETED")] | length')" + failed="$(printf '%s' "$json" | jqf \ + '[.statusCheckRollup[]? | select(.conclusion != "SUCCESS" and .conclusion != "SKIPPED" and .conclusion != "NEUTRAL")] | length')" + mergeable="$(printf '%s' "$json" | jqf '.mergeable')" + state="$(printf '%s' "$json" | jqf '.mergeStateStatus')" + + # UNKNOWN is GitHub not having computed mergeability yet, so it is waited on; + # a red check or CONFLICTING is already an answer (improvise-edpl). if [ "$total" -gt 0 ] && [ "$running" -eq 0 ]; then - break + if [ "$failed" -gt 0 ] || [ "$mergeable" = CONFLICTING ]; then break; fi + if [ "$mergeable" != UNKNOWN ] && [ "$state" != UNKNOWN ]; then break; fi fi now=$(date +%s) if [ $((now - started)) -ge "$TIMEOUT" ]; then - echo "pr-watch: timed out after ${TIMEOUT}s with $running of $total still running" >&2 + if [ "$total" -gt 0 ] && [ "$running" -eq 0 ]; then + echo "pr-watch: timed out after ${TIMEOUT}s with mergeability $mergeable/$state" >&2 + else + echo "pr-watch: timed out after ${TIMEOUT}s with $running of $total still running" >&2 + fi exit 3 fi sleep "$INTERVAL" @@ -100,15 +113,10 @@ done printf '%s' "$json" | jqf '.statusCheckRollup[]? | select(.conclusion != "SKIPPED") | " \(.conclusion) \(.name)"' -failed="$(printf '%s' "$json" | jqf \ - '[.statusCheckRollup[]? | select(.conclusion != "SUCCESS" and .conclusion != "SKIPPED" and .conclusion != "NEUTRAL")] | length')" -mergeable="$(printf '%s' "$json" | jqf '.mergeable')" -state="$(printf '%s' "$json" | jqf '.mergeStateStatus')" - echo "pr-watch: head=$head $mergeable/$state $total checks, $failed failed" [ "$failed" -eq 0 ] || exit 1 -# UNSTABLE means a non-required check is unhappy; it is not a merge blocker on -# its own, so only an outright non-mergeable state fails here. -[ "$mergeable" != "CONFLICTING" ] || exit 1 -exit 0 +# An allowlist, so a state GitHub adds later fails closed. BLOCKED and DIRTY +# can wear a MERGEABLE flag: context/code/tooling.md, "pr-watch and mergeability". +[ "$mergeable" = MERGEABLE ] || exit 1 +case "$state" in CLEAN|UNSTABLE|HAS_HOOKS) exit 0 ;; *) exit 1 ;; esac From 2db69d927afc5d2eac2b75d59957b6fa37077e1e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:34:45 -0700 Subject: [PATCH 08/16] pr-watch skill: exit 0 now means mergeable; UNKNOWN is the fourth false green (improvise-edpl) Only the .agents copy is edited: .claude/skills/pr-watch becomes a symlink to it in the improvise-5fzx PR, and editing both here would make that PR's merge a modify/delete conflict. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .agents/skills/pr-watch/SKILL.md | 18 ++++++++++++++---- .agents/skills/wave/SKILL.md | 2 +- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/.agents/skills/pr-watch/SKILL.md b/.agents/skills/pr-watch/SKILL.md index cf304a2b..a213ffe6 100644 --- a/.agents/skills/pr-watch/SKILL.md +++ b/.agents/skills/pr-watch/SKILL.md @@ -10,26 +10,36 @@ scripts/pr-watch [--expect ] [--timeout ] [--interval ` whenever you know which commit you are asking about.** It is the difference between "this PR is green" and "the thing I pushed is green", and a PR here was once merged at a head that predated its own fix. ## Do not write the loop yourself -Three signals look like "finished" and are not: +Four signals look like "finished" and are not: - **"nothing pending"** — also true when no check has registered yet. Require a **non-zero total** as well. - **`conclusion == null`** — `conclusion` is a *string*, empty while running. Test `status == "COMPLETED"`. - **a verdict from before the head moved** — re-check the head, or `--expect`. +- **`mergeable: UNKNOWN`** — GitHub has not computed it yet. It is not a + pass, and PRs #172 and #179 both went green on it. `gh` is shell-aliased to a different tool in these shells; `pr-watch` locates a real one itself. Do not reintroduce a bare `gh` call in a script here. diff --git a/.agents/skills/wave/SKILL.md b/.agents/skills/wave/SKILL.md index fbb9bd74..b9b17b86 100644 --- a/.agents/skills/wave/SKILL.md +++ b/.agents/skills/wave/SKILL.md @@ -73,7 +73,7 @@ defect is a test that passes. ## Shepherding - Wait on checks with `scripts/pr-watch --expect `. Never hand-roll a - poll; see the `pr-watch` skill for the three ways they report green while wrong. + poll; see the `pr-watch` skill for the four ways they report green while wrong. - Review before merging. Copilot's quota may be exhausted and its request is a **silent no-op**; an absent review and an unrequested one both read as "pending". Dispatch an adversarial reviewer and give it the head SHA, the base, From 8fe55e0b4abfa434240a88a47c54fa10cea029f8 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:42:05 -0700 Subject: [PATCH 09/16] Witness every bok4 refusal with a frame test (improvise-9fpl) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-command/src/ui/host.rs | 85 +++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index fe6aa371..aa8fe87c 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -1906,4 +1906,89 @@ mod tests { "a frame with nothing to refuse was refused: {answer:?}" ); } + + /// A host whose storage holds one file per way `ImportJsonHeadless` can + /// fail, plus nothing at the two paths that must fail to read. + fn host_with_import_fixtures() -> Host { + let storage = MemoryFs::new(); + let files: [(&str, &[u8]); 4] = [ + // Every field numeric, so no category is accepted and the build fails. + ("measures.json", br#"[{"x": 1}, {"x": 2}]"#), + ("bad.json", b"{not json"), + ("nested.json", br#"{"data": [{"Region": "North"}]}"#), + // A record that is not UTF-8 is a CSV read error, not a warning. + ("bad.csv", b"Region,Revenue\n\xff\xfe,1\n"), + ]; + for (name, bytes) in files { + crate::storage::Storage::write(&storage, Path::new(name), bytes) + .expect("fixture writes"); + } + let mut app = App::new(workbook(), None); + app.storage = Box::new(storage); + app.rebuild_layout(); + Host::new(app) + } + + fn import(path: &str, array_path: Option<&str>) -> ModelCommand { + ModelCommand::ImportJsonHeadless { + path: PathBuf::from(path), + model_name: None, + array_path: array_path.map(String::from), + } + } + + /// improvise-9fpl: one case per refusal #181 converted that no other test + /// witnesses. Evidence: `context/test-evidence/command.md`. + #[test] + fn every_failing_model_command_refuses_its_whole_frame() { + let cases: Vec<(&str, ModelCommand)> = vec![ + ( + "AddItemInGroup into an absent category", + ModelCommand::AddItemInGroup { + category: "Ghost".into(), + item: "East".into(), + group: "Coastal".into(), + }, + ), + ( + "AddFormula that does not parse", + ModelCommand::AddFormula { + raw: "Total = (".into(), + target_category: "_Measure".into(), + }, + ), + ("ImportJsonHeadless of an absent JSON", import("absent.json", None)), + ("ImportJsonHeadless of an absent CSV", import("absent.csv", None)), + ("ImportJsonHeadless of a CSV that is not text", import("bad.csv", None)), + ("ImportJsonHeadless of JSON that does not parse", import("bad.json", None)), + ("ImportJsonHeadless at a path with no array", import("nested.json", Some("nope"))), + ("ImportJsonHeadless that builds no model", import("measures.json", None)), + ]; + for (what, failing) in cases { + let mut host = host_with_import_fixtures(); + let (asker, _) = host.connect(10, 10); + let landed = ModelCommand::AddItem { + category: "Region".into(), + item: "Landed".into(), + }; + + let answer = host.handle(asker, two_command_frame(&host, landed, failing)); + + assert_eq!( + to(&answer, asker), + vec![ServerFrame::Rejected(Rejection::MutationRefused)], + "{what}: the frame was not refused: {answer:?}" + ); + assert!( + !host + .app() + .model_state + .workbook + .model + .category("Region") + .is_some_and(|c| c.ordered_item_names().contains(&"Landed")), + "{what}: the half of the frame that landed was not undone" + ); + } + } } From 07d9732b7a1cca32ac4469a767e2e67ef97f8bdd Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:43:02 -0700 Subject: [PATCH 10/16] Refuse DeleteView of an absent view and AddFormula into an absent category (improvise-40j4) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-command/src/ui/effect.rs | 15 ++++++-- crates/improvise-command/src/ui/host.rs | 46 ++++++++++++++++++++++- 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index c6eccc26..556d3089 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -208,11 +208,13 @@ impl Effect for AddFormula { // grid; _Measure targets arrive via `measure_item_names()`. if !crate::model::category::CategoryKind::of_name(&formula.target_category) .is_measure() + && let Err(e) = app + .model_state + .workbook + .add_item(&formula.target_category, &formula.target) { - let _ = app - .model_state - .workbook - .add_item(&formula.target_category, &formula.target); + // Its category may be gone: store no formula for it (40j4). + return refused(app, e.to_string()); } app.model_state.workbook.model.add_formula(formula); } @@ -341,6 +343,11 @@ impl Effect for CreateView { pub struct DeleteView(pub String); impl Effect for DeleteView { fn apply(&self, app: &mut App) { + // `delete_view` answers Ok for a name it lacks, which is a failure to + // report, not a deletion (improvise-40j4). + if !app.model_state.workbook.views.contains_key(&self.0) { + return refused(app, format!("No view named \"{}\"", self.0)); + } if let Err(e) = app.model_state.workbook.delete_view(&self.0) { return refused(app, e.to_string()); } diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index aa8fe87c..da3771af 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -1907,6 +1907,38 @@ mod tests { ); } + /// improvise-40j4: the formula was stored for a category the frame's own + /// first command removed, and it survived save and reload. + #[test] + fn a_frame_adding_a_formula_to_a_category_its_first_command_removed_is_refused() { + let mut host = host(); + let (asker, _) = host.connect(10, 10); + + let answer = host.handle( + asker, + two_command_frame( + &host, + ModelCommand::RemoveCategory("Region".into()), + ModelCommand::AddFormula { + raw: "Total = 1".into(), + target_category: "Region".into(), + }, + ), + ); + + assert_eq!( + to(&answer, asker), + vec![ServerFrame::Rejected(Rejection::MutationRefused)], + "a formula for a vanished category was accepted: {answer:?}" + ); + let model = &host.app().model_state.workbook.model; + assert!(model.category("Region").is_some(), "the removal must be undone"); + assert!( + model.formulas().iter().all(|f| f.target != "Total"), + "and the formula must not be stored" + ); + } + /// A host whose storage holds one file per way `ImportJsonHeadless` can /// fail, plus nothing at the two paths that must fail to read. fn host_with_import_fixtures() -> Host { @@ -1923,7 +1955,10 @@ mod tests { crate::storage::Storage::write(&storage, Path::new(name), bytes) .expect("fixture writes"); } - let mut app = App::new(workbook(), None); + // A second view, so deleting an absent one is not refused as "the last". + let mut wb = workbook(); + wb.create_view("Scratch"); + let mut app = App::new(wb, None); app.storage = Box::new(storage); app.rebuild_layout(); Host::new(app) @@ -1963,6 +1998,15 @@ mod tests { ("ImportJsonHeadless of JSON that does not parse", import("bad.json", None)), ("ImportJsonHeadless at a path with no array", import("nested.json", Some("nope"))), ("ImportJsonHeadless that builds no model", import("measures.json", None)), + // improvise-40j4: `delete_view` answers Ok for a name it lacks. + ("DeleteView of a view the document lacks", ModelCommand::DeleteView("Ghost".into())), + ( + "AddFormula into a category the document lacks", + ModelCommand::AddFormula { + raw: "Total = 1".into(), + target_category: "Ghost".into(), + }, + ), ]; for (what, failing) in cases { let mut host = host_with_import_fixtures(); From c246dc1df6fe7def92c623d9e39b211cb08bd754 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:43:38 -0700 Subject: [PATCH 11/16] =?UTF-8?q?improvise-umi4:=20WIP=20failing=20tests?= =?UTF-8?q?=20=E2=80=94=20an=20untouched=20edit=20must=20store=20the=20ide?= =?UTF-8?q?ntical=20value?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .../src/command/cmd/commit.rs | 117 ++++++++++++++++++ .../improvise-wasm-client/tests/lockstep.rs | 69 +++++++++++ 2 files changed, 186 insertions(+) diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index 4815d166..e4be4be8 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -473,6 +473,123 @@ mod tests { "bulk entry lands at the start of the new row" ); } + + /// Values whose `Display` (four decimals) and value disagree, plus text. + fn values_display_rounds() -> Vec { + use crate::model::cell::CellValue::{Number, Text}; + vec![ + Number(1.23456789), + Number(0.1 + 0.2), + Number(98765.4321987), + Number(123456789012345.67), + Number(-0.00001234), + Text("0.1 + 0.2".into()), + ] + } + + /// Two regions, two measures, with `value` at (West, Revenue): never index 0. + fn grid_holding(value: crate::model::cell::CellValue, records: bool) -> crate::ui::app::App { + use crate::model::cell::{CellKey, CellValue}; + use crate::ui::app::App; + use crate::view::Axis; + + let mut wb = Workbook::new("Seed"); + wb.add_category("Region").unwrap(); + for region in ["North", "West"] { + wb.add_item("Region", region).unwrap(); + } + for measure in ["Cost", "Revenue"] { + wb.add_item("_Measure", measure).unwrap(); + } + let key = |r: &str, m: &str| { + CellKey::new(vec![ + ("Region".into(), r.into()), + ("_Measure".into(), m.into()), + ]) + }; + wb.model.set_cell(key("North", "Cost"), CellValue::Number(1.0)); + wb.model.set_cell(key("North", "Revenue"), CellValue::Number(2.0)); + wb.model.set_cell(key("West", "Cost"), CellValue::Number(3.0)); + wb.model.set_cell(key("West", "Revenue"), value); + { + let v = wb.active_view_mut(); + match records { + true => { + v.set_axis("_Index", Axis::Row); + v.set_axis("_Dim", Axis::Column); + v.set_axis("Region", Axis::None); + v.set_axis("_Measure", Axis::None); + } + false => { + v.set_axis("Region", Axis::Row); + v.set_axis("_Measure", Axis::Column); + } + } + } + let mut app = App::new(wb, None); + app.rebuild_layout(); + assert_eq!(app.layout.is_records_mode(), records, "wrong fixture mode"); + app + } + + /// Open the editor on (West, Revenue) and commit it untouched; the stored value. + fn no_op_commit(records: bool, value: crate::model::cell::CellValue) { + use crate::model::cell::CellKey; + use improvise_protocol::key::KeyCode; + + let mut app = grid_holding(value.clone(), records); + let target = CellKey::new(vec![ + ("Region".into(), "West".into()), + ("_Measure".into(), "Revenue".into()), + ]); + let at = match records { + true => (1, 3), + false => (1, 1), + }; + app.cursor_mut().selected = at; + if records { + app.view_state.mode = AppMode::RecordsNormal; + assert_eq!( + app.layout.record_col(at.1), + Some(&RecordCol::Measure("Revenue".into())), + "the cursor is not on the Revenue column" + ); + } + assert_eq!( + app.model_state.workbook.model.get_cell(&target), + Some(&value), + "the fixture does not hold the value under test" + ); + press(&mut app, KeyCode::Char('i')); + assert!(app.view_state.mode.is_editing(), "`i` did not open an editor"); + press(&mut app, KeyCode::Enter); + let stored = app.model_state.workbook.model.get_cell(&target).cloned(); + let same = match (&stored, &value) { + (Some(CellValue::Number(a)), CellValue::Number(b)) => a.to_bits() == b.to_bits(), + (stored, value) => stored.as_ref() == Some(value), + }; + assert!( + same, + "{} mode: i then Enter on {value:?} stored {stored:?}", + if records { "records" } else { "pivot" } + ); + } + + /// BUG (improvise-umi4) — see `context/test-evidence/command.md`. + #[test] + fn an_untouched_pivot_edit_stores_the_identical_value() { + for value in values_display_rounds() { + no_op_commit(false, value); + } + } + + /// BUG (improvise-umi4) — see `context/test-evidence/command.md`. + #[test] + fn an_untouched_records_edit_stores_the_identical_value() { + for value in values_display_rounds() { + no_op_commit(true, value); + } + } } // ── Commit commands (mode-specific buffer consumers) ──────────────────────── diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index a3e0115c..a9ee5b16 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1755,3 +1755,72 @@ fn yy_over_an_aggregate_copies_the_sum_the_host_copies() { "client and host yanked different values over an aggregate" ); } + +/// Two regions by two measures, `value` at (South, Revenue): row 1, column 1, +/// so "the cell under the cursor" is not "the first cell". +fn precise_host(value: improvise_protocol::CellValue) -> App { + let mut wb = Workbook::new("Test"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "North").unwrap(); + wb.add_item("Region", "South").unwrap(); + wb.add_item("_Measure", "Cost").unwrap(); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.active_view_mut().set_axis("Region", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + for (region, measure, v) in [ + ("North", "Cost", improvise_protocol::CellValue::Number(1.0)), + ("North", "Revenue", improvise_protocol::CellValue::Number(2.0)), + ("South", "Cost", improvise_protocol::CellValue::Number(3.0)), + ("South", "Revenue", value), + ] { + wb.model.set_cell(region_measure(region, measure), v); + } + let mut app = App::new(wb, None); + app.term_height = 2 + 8; + app.rebuild_layout(); + app.cursor_mut().selected = (1, 1); + app +} + +/// BUG (improvise-umi4) — see `context/test-evidence/clients.md`. +#[test] +fn an_untouched_browser_edit_stores_the_identical_value() { + use improvise_protocol::CellValue::{Number, Text}; + let window = (2, 2); + for value in [ + Number(1.23456789), + Number(0.1 + 0.2), + Number(98765.4321987), + Number(123456789012345.67), + Number(-0.00001234), + Text("0.1 + 0.2".into()), + ] { + let mut host = precise_host(value.clone()); + let mut client = client_ready_to_edit(&host, window); + for key in ["i", "Enter"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("{key}: the client must answer this key locally"); + }; + for cmd in &cmds { + client.fold(cmd); + } + apply_on(&mut host, &cmds); + } + assert!( + !client.view.mode.is_editing(), + "Enter did not commit, so nothing was written and this proves nothing" + ); + let stored = host + .model_state + .workbook + .model + .get_cell(®ion_measure("South", "Revenue")) + .cloned(); + let same = match (&stored, &value) { + (Some(Number(a)), Number(b)) => a.to_bits() == b.to_bits(), + (stored, value) => stored.as_ref() == Some(value), + }; + assert!(same, "i then Enter on {value:?} stored {stored:?}"); + } +} From 141e41ec4bd29cc8b4ad89ff187516b24ad020ab Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:49:20 -0700 Subject: [PATCH 12/16] improvise-umi4: an editor opens on the value, not on its rounded Display MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CellValue's Display writes a non-whole number {:.4}, and every editor seed was that string, so i then Enter on an untouched 1.23456789 stored 1.2346, in the TUI (pivot and records) and in the browser, which seeds from the projected edit_text. CellValue::edit_text is now the one home of the seed: the shortest decimal that reads back as the same f64 (non-finite values take the core.md §16 token). display_value_in's two arms and cell_display's records arm all go through it; the now-orphaned GridLayout::resolve_display is removed. Text that reads as a number (007) is exact in the seed but still reclassified by typed(); filed as improvise-ixxx. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 4 +- context/code/core.md | 32 +++++++++++ context/code/protocol.md | 15 ++++-- context/test-evidence/clients.md | 31 +++++++++++ context/test-evidence/command.md | 28 ++++++++++ .../src/command/cmd/commit.rs | 16 ++++-- .../improvise-command/src/command/cmd/mod.rs | 4 +- crates/improvise-command/src/ui/app.rs | 4 +- crates/improvise-command/src/ui/project.rs | 2 +- crates/improvise-core/src/model/cell.rs | 11 ++++ crates/improvise-core/src/view/layout.rs | 15 ++---- crates/improvise-protocol/src/render.rs | 2 +- .../improvise-wasm-client/tests/lockstep.rs | 53 ++++++++++++++++++- 13 files changed, 190 insertions(+), 27 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index 987df348..632dd0b2 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -1110,7 +1110,9 @@ emit `RebuildLayout`) first (gotcha 23). `display_value_of` is the value an editor opened on `key` starts with — unformatted, and so **not** `CellDisplay::text` (design-principles §4: display -rounding is view-only and must not feed back into the model). Crate-visible so +rounding is view-only and must not feed back into the model). It is also **not** +`CellValue`'s `Display`, which rounds to four places: both arms spell the value +with `CellValue::edit_text` (improvise-umi4, `core.md` §17). Crate-visible so `project::cell_display` can put it on the wire as `CellDisplay::edit_text`; a browser that un-formatted the drawn text instead would be a second derivation, and a lossy one. diff --git a/context/code/core.md b/context/code/core.md index e27a3c18..3e209485 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -1200,3 +1200,35 @@ off the `## Data` line and off `serde_json` and requires them equal. A second home that happens to agree is harmless; one that drifts fails there, and re-copying the tokens into `format.rs` with one letter changed was injected to prove it does. + +## 17. An editor opens on the value, not on its `Display` (improvise-umi4) + +`CellValue`'s `Display` is a **screen** spelling: a non-whole number is written +`{:.4}`, so `1.23456789` reads `1.2346`. Every editor seed used to be that +string (`display_value_in`'s pivot arm through `get_cell().to_string()`, its +records arm through `records_display`, and `project::cell_display`'s records +arm spelling it again), so pressing `i` and then `Enter` with nothing typed +stored `1.2346`. The screen looked the same before and after, which is what +made it silent data loss rather than a visible bug. The browser seeds from the +projected `CellDisplay::edit_text`, which came from the same call, so it had +the same bug. + +`CellValue::edit_text` is the one home of the seed. A number is written as the +**shortest decimal that parses back to the same `f64`**, which is what Rust's +`f64` `Display` already guarantees (no precision argument, no exponent). The +brief pointed at improvise-gp's `number_text` as the implementation to reuse. +It is not a second implementation of this one: it takes an `f32` and searches +fixed precisions for the shortest *f64* expansion of it, because a threshold +must route the same under f32 and f64 evaluation. For an `f64` the standard +library's spelling is the same answer with no search. A non-finite number takes +§16's token (`inf`, `-inf`, `nan`), which `typed()` also reads back, so the +editor and the `.improv` format spell it one way. + +Text and errors keep `Display`. Text is written unchanged, and an error cell has +no edit text on the wire anyway. Display itself is untouched: the grid still +rounds (gotcha 8), only the seed changed. + +What this does **not** fix: a text cell whose text reads as a number. `"007"` +seeds `"007"`, and `typed()` classifies that as `Number(7.0)`. The seed is +exact, but the classifier has no way to know the cell was text. That is +improvise-ixxx. diff --git a/context/code/protocol.md b/context/code/protocol.md index 85f47b8a..808208b6 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -622,6 +622,13 @@ a value. A browser found this: `tests/lockstep.rs` drove `i` through the host and the client over the same grid, and they disagreed by a comma. +**It is the value's exact spelling, not its `Display`** (improvise-umi4). +`CellValue`'s `Display` writes a non-whole number `{:.4}`, so this field +carried `1.2346` for a stored `1.23456789`, and `i` then `Enter` with nothing +typed stored the rounded number in both front ends. Every arm that seeds an +editor now goes through `CellValue::edit_text` (`core.md` §17). That is the +shortest decimal that parses back to the same `f64`. + `numeric` and `error` are decided by the server for the same reason: it is the side that knows which it was, and re-deriving client-side is how the two ends drift apart. `CellDisplay` is deliberately not *drawn from* a `CellValue` @@ -636,8 +643,9 @@ that `text` was formatted from: `evaluate_aggregated` over the view's because the browser's `yy` has to copy what the TUI's `YankCell` copies, and neither string on the cell can be turned back into it: -- `edit_text` is `CellValue::to_string()`, which writes a non-whole number - `{:.4}` — `1.23456789` becomes `1.2346`. +- `edit_text` was `CellValue::to_string()`, which writes a non-whole number + `{:.4}`, so `1.23456789` became `1.2346`. improvise-umi4 has since made it + exact, but the next bullet still holds. - Re-parsing any text with `typed()` turns number-like text into a number — `Text("007")` becomes `Number(7.0)`. - In a pivot grid `edit_text` is the *stored* cell (`get_cell`), so an @@ -647,7 +655,8 @@ So the value is **projected**, not re-derived (gotcha 47): a client that wants the value asks for the value. It is kept separate from `edit_text` rather than replacing it, because the two answer different questions — what an editor starts with is a *string*, deliberately the one the TUI's buffer -holds, and changing it would change what the TUI's own editor shows. +holds. (That string was itself lossy until improvise-umi4. The fix changed what +the TUI's own editor shows, which was the point.) It cost `Eq`: `CellValue` holds an `f64`, so `CellDisplay`, `GridCache` and `GridUpdate` are `PartialEq` only. Nothing hashed or `Eq`-bounded them. diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 85766267..221127b6 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4390,3 +4390,34 @@ Records mode is deliberately not covered. There the TUI yanks `None` and its `p` writes a junk `_Dim`/`_Index` cell, because it resolves the synthetic key (improvise-6p8q). The browser is right and the TUI is wrong, so a lockstep case there would assert the bug. + +### `an_untouched_browser_edit_stores_the_identical_value` + +**improvise-umi4**, the browser half. The client seeds `i` from the projected +`CellDisplay::edit_text` (`resolve.rs::edit_or_drill`), which was +`App::display_value_in` and so `CellValue`'s rounded `Display`. The client +resolves `i` and `Enter` itself, and what it composed is applied to the host +through `apply_on` (`Checked::against` plus `apply_command`). The stored value +is then compared by `f64::to_bits`. The fixture puts the value at row 1, +column 1 of a 2×2 pivot grid, with values that `Display` rounds (the list in +`context/test-evidence/command.md`). A records grid is not driven, because the +browser declines records editing (`edit_or_drill`). + +Failed before the fix on `Number(1.23456789)` stored as `Number(1.2346)`. +**Rival:** `CellValue::edit_text`'s number arm set back to `Display`. The test +failed on the same case. + +The test is in-process, so it cannot see the JSON wire. A float that does not +survive `serde_json` is improvise-6oy4, a separate fix. + +### `every_projected_edit_text_commits_back_to_its_value` + +The projection contract behind the test above, for **both** grid kinds. Every +`Record` cell's `edit_text`, fed through `typed()`, must give back the cell's +`value`. It runs over the whole projected grid in pivot mode and in records +mode (entered with `R`). A guard asserts that the precise value was projected +at all. This test exists because `project::cell_display`'s records arm spelled +`edit_text` itself, apart from `display_value_in`, and no key-path test reaches +that arm from a browser. **Rival:** that arm restored to `Some(text.clone())`, +the display text. It failed with `records=true: edit text "1.2346" does not +commit back to Some(Number(1.23456789))`. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 511a8461..61929749 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5021,3 +5021,31 @@ bracket-matched `#[…]` before reading the name. `an_attributed_or_wrapped_effect_is_still_an_effect` pins that on a fixture: a doc comment, two attributes (one with nested brackets), and a wrapped call. With the whitespace skip removed it fails with `[]`. + +### `an_untouched_pivot_edit_stores_the_identical_value` +### `an_untouched_records_edit_stores_the_identical_value` + +**improvise-umi4.** `App::display_value_in` seeded the edit buffer with +`CellValue`'s `Display`, which writes a non-whole number `{:.4}`. So `i` then +`Enter` with nothing typed stored `1.2346` over `1.23456789`, in pivot and in +records mode, and the screen looked the same before and after. + +Both tests go through the real key path (`App::handle_key`, `i`, `Enter`) on a +2×2 grid. The cursor sits on (West, Revenue), which is row 1, column 1 in pivot +mode and row 1, column 3 (`[Region, +cat, Cost, Revenue, +]`) in records mode, +so "the cell under the cursor" is not "the first cell". The values are chosen +so that `Display` and the value disagree: `1.23456789`, `0.1 + 0.2`, +`98765.4321987`, `123456789012345.67` (under the `1e15` integer cutoff, so it +takes the rounding arm), `-0.00001234` (rounds to `-0.0000`), `-0.0`, +`-inf`, plus a text value. The comparison is `f64::to_bits`, so `-0.0` against +`0.0` counts as a difference. + +Both failed before the fix, on `Number(1.23456789)` stored as `Number(1.2346)`. +**Rival injected:** `CellValue::edit_text`'s number arm set back to +`self.to_string()`, i.e. the display seed restored in its one home. Both tests +failed again on the same case. The fix was then restored and the suite re-run. + +A text cell that reads as a number (`Text("007")`) is deliberately **not** in +the list. Its seed is exact (`"007"`), but `typed()` classifies the committed +text as `Number(7.0)`. Probed here: both tests fail on it in both modes. That +is the classifier, not the seed, and it is improvise-ixxx. diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index e4be4be8..b3f551fc 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -483,6 +483,8 @@ mod tests { Number(98765.4321987), Number(123456789012345.67), Number(-0.00001234), + Number(-0.0), + Number(f64::NEG_INFINITY), Text("0.1 + 0.2".into()), ] } @@ -507,9 +509,12 @@ mod tests { ("_Measure".into(), m.into()), ]) }; - wb.model.set_cell(key("North", "Cost"), CellValue::Number(1.0)); - wb.model.set_cell(key("North", "Revenue"), CellValue::Number(2.0)); - wb.model.set_cell(key("West", "Cost"), CellValue::Number(3.0)); + wb.model + .set_cell(key("North", "Cost"), CellValue::Number(1.0)); + wb.model + .set_cell(key("North", "Revenue"), CellValue::Number(2.0)); + wb.model + .set_cell(key("West", "Cost"), CellValue::Number(3.0)); wb.model.set_cell(key("West", "Revenue"), value); { let v = wb.active_view_mut(); @@ -561,7 +566,10 @@ mod tests { "the fixture does not hold the value under test" ); press(&mut app, KeyCode::Char('i')); - assert!(app.view_state.mode.is_editing(), "`i` did not open an editor"); + assert!( + app.view_state.mode.is_editing(), + "`i` did not open an editor" + ); press(&mut app, KeyCode::Enter); let stored = app.model_state.workbook.model.get_cell(&target).cloned(); let same = match (&stored, &value) { diff --git a/crates/improvise-command/src/command/cmd/mod.rs b/crates/improvise-command/src/command/cmd/mod.rs index 0000d0fd..11615dec 100644 --- a/crates/improvise-command/src/command/cmd/mod.rs +++ b/crates/improvise-command/src/command/cmd/mod.rs @@ -77,8 +77,8 @@ pub(super) mod test_helpers { display_value: { let key = layout.cell_key(sr, sc); key.as_ref() - .and_then(|k| workbook.model.get_cell(k).cloned()) - .map(|v| v.to_string()) + .and_then(|k| workbook.model.get_cell(k)) + .map(crate::model::cell::CellValue::edit_text) .unwrap_or_default() }, visible_rows: 20, diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 56f6fb8c..fcfed55a 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -741,14 +741,14 @@ impl App { match crate::view::synthetic_record_info(key) { Some((idx, col)) => self .staged_drill_edit(layout, idx, col) - .or_else(|| layout.resolve_display(key)) + .or_else(|| layout.records_value(idx, col).map(|v| v.edit_text())) .unwrap_or_default(), None => self .model_state .workbook .model .get_cell(key) - .map(|v| v.to_string()) + .map(CellValue::edit_text) .unwrap_or_default(), } } diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 1fe8526e..4743efcf 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -277,7 +277,7 @@ fn cell_display( // with `parse::()`, which called a coordinate `2024` a number. let text = value.as_ref().map(|v| v.to_string()).unwrap_or_default(); return CellDisplay { - edit_text: Some(text.clone()), + edit_text: Some(value.as_ref().map(CellValue::edit_text).unwrap_or_default()), text, numeric: matches!(value, Some(CellValue::Number(_))), error: matches!(value, Some(CellValue::Error(_))), diff --git a/crates/improvise-core/src/model/cell.rs b/crates/improvise-core/src/model/cell.rs index b943a214..0b143831 100644 --- a/crates/improvise-core/src/model/cell.rs +++ b/crates/improvise-core/src/model/cell.rs @@ -134,6 +134,17 @@ impl CellValue { } } +impl CellValue { + /// The text an editor opens on, which commits back to this value unchanged. + /// `Display` rounds for the screen; this must not (`context/code/core.md` §17). + pub fn edit_text(&self) -> String { + match self { + CellValue::Number(n) => nonfinite_token(*n).map_or_else(|| n.to_string(), String::from), + CellValue::Text(_) | CellValue::Error(_) => self.to_string(), + } + } +} + impl std::fmt::Display for CellValue { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index 495c3967..1105655f 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -656,13 +656,6 @@ impl GridLayout { axis_label(&self.col_items, col) } - /// Display string for a synthetic records-mode key; `None` for a pivot - /// key. - pub fn resolve_display(&self, key: &CellKey) -> Option { - let (row, col) = synthetic_record_info(key)?; - self.records_display(row, col) - } - /// Display text for (row, col) in either mode. pub fn display_text( &self, @@ -1045,7 +1038,7 @@ mod tests { } #[test] - fn records_mode_resolve_display_returns_values() { + fn records_mode_records_display_returns_values() { let mut wb = records_workbook(); let v = wb.active_view_mut(); v.set_axis("_Index", Axis::Row); @@ -1057,13 +1050,11 @@ mod tests { // A measure column resolves to that measure's value let value_col = cols.iter().position(|c| c == "Revenue").unwrap(); - let key = layout.cell_key(0, value_col).unwrap(); - assert_eq!(layout.resolve_display(&key).as_deref(), Some("100")); + assert_eq!(layout.records_display(0, value_col).as_deref(), Some("100")); // Category column resolves to the coordinate value let region_col = cols.iter().position(|c| c == "Region").unwrap(); - let key = layout.cell_key(0, region_col).unwrap(); - let display = layout.resolve_display(&key).unwrap(); + let display = layout.records_display(0, region_col).unwrap(); assert!( !display.is_empty(), "Region column should resolve to a value" diff --git a/crates/improvise-protocol/src/render.rs b/crates/improvise-protocol/src/render.rs index 57f0d0c5..01573bab 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/crates/improvise-protocol/src/render.rs @@ -82,7 +82,7 @@ pub struct CellDisplay { /// **typed** it, not as it is drawn. The difference corrupts data (§3). pub edit_text: Option, /// The value the host evaluated here, unrounded and untyped-by-text: what - /// `yank` copies. Neither `text` nor `edit_text` can be re-parsed into it (§3). + /// `yank` copies. `text` cannot be re-parsed into it, nor can text like `007` (§3). pub value: Option, } diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index a9ee5b16..f45f0f2c 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1769,7 +1769,11 @@ fn precise_host(value: improvise_protocol::CellValue) -> App { wb.active_view_mut().set_axis("_Measure", Axis::Column); for (region, measure, v) in [ ("North", "Cost", improvise_protocol::CellValue::Number(1.0)), - ("North", "Revenue", improvise_protocol::CellValue::Number(2.0)), + ( + "North", + "Revenue", + improvise_protocol::CellValue::Number(2.0), + ), ("South", "Cost", improvise_protocol::CellValue::Number(3.0)), ("South", "Revenue", value), ] { @@ -1793,6 +1797,8 @@ fn an_untouched_browser_edit_stores_the_identical_value() { Number(98765.4321987), Number(123456789012345.67), Number(-0.00001234), + Number(-0.0), + Number(f64::NEG_INFINITY), Text("0.1 + 0.2".into()), ] { let mut host = precise_host(value.clone()); @@ -1824,3 +1830,48 @@ fn an_untouched_browser_edit_stores_the_identical_value() { assert!(same, "i then Enter on {value:?} stored {stored:?}"); } } + +/// BUG (improvise-umi4) — see `context/test-evidence/clients.md`. +#[test] +fn every_projected_edit_text_commits_back_to_its_value() { + use improvise_protocol::CellValue::Number; + let value = Number(1.23456789); + for records in [false, true] { + let mut app = precise_host(value.clone()); + if records { + reduce_key( + &mut app, + KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE), + ); + } + assert_eq!(app.layout.is_records_mode(), records, "wrong fixture mode"); + let grid = grid_of( + &app, + Viewport { + row_offset: 0, + col_offset: 0, + rows: 8, + cols: 8, + }, + ); + let cells: Vec<_> = grid + .cells + .iter() + .flatten() + .filter(|cell| matches!(cell.id, Some(improvise_protocol::CellId::Record(_)))) + .collect(); + assert!( + cells.iter().any(|cell| cell.value.as_ref() == Some(&value)), + "records={records}: the precise value was never projected, so this proves nothing" + ); + for cell in cells { + let text = cell.edit_text.clone().unwrap_or_default(); + assert_eq!( + improvise_protocol::typed(&text), + cell.value, + "records={records}: edit text {text:?} does not commit back to {:?}", + cell.value + ); + } + } +} From 54d9cc85de4f90d34ef6f302f9f74eff54ca7eef Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:52:01 -0700 Subject: [PATCH 13/16] WIP: 59q8 red tests Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-protocol/src/source_scan.rs | 98 ++++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/crates/improvise-protocol/src/source_scan.rs b/crates/improvise-protocol/src/source_scan.rs index 419705e8..55f1b41c 100644 --- a/crates/improvise-protocol/src/source_scan.rs +++ b/crates/improvise-protocol/src/source_scan.rs @@ -322,6 +322,104 @@ mod tests { )); } + /// improvise-59q8, the shapes that HID production code: a test-only item + /// whose end the line scan could not find. Evidence: test-evidence/protocol.md. + #[test] + fn a_test_only_item_hides_nothing_after_it() { + let cases = [ + ( + "a multi-line item ending in `];`", + concat!( + "impl X {\n", + " #[cfg(test)]\n", + " const NAMES: &[&str] = &[\n", + " \"a\",\n", + " ];\n", + " fn prod() { let c = Command::Hidden1(1); }\n", + "}\n", + "fn after() { Command::Visible(2) }\n", + ), + ), + ( + "a multi-line statement ending in `);`", + concat!( + "fn f() {\n", + " #[cfg(test)]\n", + " let x = foo(\n", + " 1,\n", + " );\n", + " let c = Command::Hidden1(1);\n", + "}\n", + "fn after() { Command::Visible(2) }\n", + ), + ), + ( + "a one-line `#[cfg(test)] use`", + concat!( + "#[cfg(test)] use foo::bar;\n", + "fn prod() { Command::Hidden1(1) }\n", + "fn after() { Command::Visible(2) }\n", + ), + ), + ]; + for (what, text) in cases { + let kept = production_text(text); + for name in ["Hidden1", "Visible"] { + assert!(kept.contains(name), "{what}: {name} was hidden: {kept:?}"); + } + } + } + + /// improvise-59q8, the shapes that COUNTED test code as production. + #[test] + fn a_test_only_cfg_is_recognised_whatever_its_spelling() { + let cases = [ + ( + "`all(test, not(..))`", + "#[cfg(all(test, not(target_arch = \"wasm32\")))]\nfn t() { Command::TestOnly(1) }\n", + ), + ( + "an inner `#![cfg(test)]` heading the file", + "#![cfg(test)]\nfn t() { Command::TestOnly(1) }\n", + ), + ( + "an attribute split across lines", + "#[cfg(\n test\n)]\nfn t() { Command::TestOnly(1) }\n", + ), + ]; + for (what, text) in cases { + let kept = production_text(text); + assert!(!kept.contains("TestOnly"), "{what} was read as production: {kept:?}"); + } + // The controls: a predicate a shipped build can satisfy is production. + for text in [ + "#[cfg(not(test))]\nfn p() { Command::Shipped(1) }\n", + "#[cfg(any(test, feature = \"serde\"))]\nfn p() { Command::Shipped(1) }\n", + "#[cfg(target_arch = \"wasm32\")]\nfn p() { Command::Shipped(1) }\n", + ] { + assert!(production_text(text).contains("Shipped"), "{text:?} was dropped"); + } + } + + /// improvise-59q8's minor case: a C string literal leaked its contents. + #[test] + fn a_c_string_literal_is_blanked() { + // The raw one quotes inside itself, so lexing it as ordinary strings + // leaves the text BETWEEN the inner quotes in the open. + let kept = code_only("let a = c\"Command::InC(1)\"; let b = cr#\"x\" Command::InRawC(2) \"y\"#;\n"); + for gone in ["InC", "InRawC"] { + assert!(!kept.contains(gone), "{gone} survived: {kept}"); + } + } + + /// A test-only item the scan cannot bound is REFUSED, never read to the + /// end of the file, which is what silently hid every shape above. + #[test] + #[should_panic(expected = "could not be bounded")] + fn an_unbounded_test_only_item_is_refused_not_swallowed() { + let _ = production_text("#[cfg(test)]\nfn broken() {\n Command::X(1)\n"); + } + #[test] fn comments_and_literals_are_blanked_and_code_is_kept() { let text = concat!( From 3bcd8774cb4a509feb94edc76a75440c8ceba852 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:53:01 -0700 Subject: [PATCH 14/16] WIP pr-watch: failing cases for the needs-gap BLOCKED and a head moving during UNKNOWN (improvise-zlj2, improvise-h3ni) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- scripts/pr-watch-test.sh | 50 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/scripts/pr-watch-test.sh b/scripts/pr-watch-test.sh index 1fdb22ec..172d5717 100755 --- a/scripts/pr-watch-test.sh +++ b/scripts/pr-watch-test.sh @@ -108,7 +108,7 @@ for m in MERGEABLE CONFLICTING UNKNOWN; do for s in BEHIND BLOCKED CLEAN DIRTY DRAFT HAS_HOOKS UNKNOWN UNSTABLE SOMETHING_NEW null; do if [ "$m" = CONFLICTING ]; then want=1 elif [ "$m" = UNKNOWN ] || [ "$s" = UNKNOWN ]; then want=3 - else case "$s" in CLEAN|UNSTABLE|HAS_HOOKS) want=0 ;; *) want=1 ;; esac + else case "$s" in CLEAN|UNSTABLE|HAS_HOOKS) want=0 ;; BLOCKED) want=3 ;; *) want=1 ;; esac fi sv="\"$s\""; [ "$s" = null ] && sv=null state "$work/table-$m-$s" last.json "\"$m\"" "$sv" "$green_rollup" @@ -116,5 +116,53 @@ for m in MERGEABLE CONFLICTING UNKNOWN; do done done +# improvise-zlj2. The required "Browser harness" check comes from a job that +# `needs: build`, so its check run does not exist until build finishes. In that +# gap every registered check is green and GitHub says MERGEABLE/BLOCKED. The +# rollup below is PR #189's, verbatim; the gap states drop or rewind entries. +cat > "$work/rollup.jsonl" <<'EOF' +{"__typename":"CheckRun","completedAt":"2026-09-23T04:41:53Z","conclusion":"SUCCESS","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261571/job/107047406461","name":"Build all bundles","startedAt":"2026-09-23T04:40:15Z","status":"COMPLETED","workflowName":"CI"} +{"__typename":"CheckRun","completedAt":"2026-09-23T04:40:32Z","conclusion":"SUCCESS","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261564/job/107047406233","name":"plan","startedAt":"2026-09-23T04:40:14Z","status":"COMPLETED","workflowName":"Release"} +{"__typename":"CheckRun","completedAt":"2026-09-23T04:40:32Z","conclusion":"SKIPPED","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261564/job/107047482378","name":"build-local-artifacts (${{ join(matrix.targets, ', ') }})","startedAt":"2026-09-23T04:40:32Z","status":"COMPLETED","workflowName":"Release"} +{"__typename":"CheckRun","completedAt":"2026-09-23T04:42:22Z","conclusion":"SUCCESS","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261571/job/107047406506","name":"Test and lint (host)","startedAt":"2026-09-23T04:40:16Z","status":"COMPLETED","workflowName":"CI"} +{"__typename":"CheckRun","completedAt":"2026-09-23T04:40:32Z","conclusion":"SKIPPED","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261564/job/107047482733","name":"build-global-artifacts","startedAt":"2026-09-23T04:40:32Z","status":"COMPLETED","workflowName":"Release"} +{"__typename":"CheckRun","completedAt":"2026-09-23T04:40:40Z","conclusion":"SUCCESS","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261571/job/107047406409","name":"wasm32 check (browser bundle crates)","startedAt":"2026-09-23T04:40:15Z","status":"COMPLETED","workflowName":"CI"} +{"__typename":"CheckRun","completedAt":"2026-09-23T04:40:32Z","conclusion":"SKIPPED","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261564/job/107047482801","name":"host","startedAt":"2026-09-23T04:40:33Z","status":"COMPLETED","workflowName":"Release"} +{"__typename":"CheckRun","completedAt":"2026-09-23T04:40:25Z","conclusion":"SUCCESS","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261571/job/107047406415","name":"Browser JS unit tests (node --test)","startedAt":"2026-09-23T04:40:14Z","status":"COMPLETED","workflowName":"CI"} +{"__typename":"CheckRun","completedAt":"2026-09-23T04:40:32Z","conclusion":"SKIPPED","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261564/job/107047483313","name":"announce","startedAt":"2026-09-23T04:40:33Z","status":"COMPLETED","workflowName":"Release"} +{"__typename":"CheckRun","completedAt":"2026-09-23T04:41:17Z","conclusion":"SUCCESS","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261571/job/107047406264","name":"Knowledge base consistency","startedAt":"2026-09-23T04:40:14Z","status":"COMPLETED","workflowName":"CI"} +{"__typename":"CheckRun","completedAt":"2026-09-23T04:40:39Z","conclusion":"SUCCESS","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261571/job/107047406373","name":"Lint the harness crate","startedAt":"2026-09-23T04:40:14Z","status":"COMPLETED","workflowName":"CI"} +{"__typename":"CheckRun","completedAt":"2026-09-23T04:44:17Z","conclusion":"SUCCESS","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261571/job/107047753280","name":"Browser harness (OPFS contract + shell)","startedAt":"2026-09-23T04:41:56Z","status":"COMPLETED","workflowName":"CI"} +{"__typename":"CheckRun","completedAt":"2026-09-23T04:44:18Z","conclusion":"SKIPPED","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261571/job/107048259684","name":"Deploy to GitHub Pages","startedAt":"2026-09-23T04:44:18Z","status":"COMPLETED","workflowName":"CI"} +EOF +harness='Browser harness (OPFS contract + shell)' +deploy='Deploy to GitHub Pages' +# $1 dir, $2 file, $3 mergeable, $4 state, $5 jq filter over the rollup array +gap_state() { + state "$1" "$2" "\"$3\"" "\"$4\"" "$(jq -cs "$5" "$work/rollup.jsonl")" +} +build_running='map(if .name == "Build all bundles" then .status = "IN_PROGRESS" | .conclusion = "" else . end)' +not_yet="map(select(.name != \"$harness\" and .name != \"$deploy\"))" +harness_running="map(if .name == \"$harness\" then .status = \"IN_PROGRESS\" | .conclusion = \"\" else . end) | map(select(.name != \"$deploy\"))" +gap_state "$work/gap" 1.json MERGEABLE BLOCKED "$not_yet | $build_running" +gap_state "$work/gap" 2.json MERGEABLE BLOCKED "$not_yet" +gap_state "$work/gap" 3.json MERGEABLE BLOCKED "$harness_running" +gap_state "$work/gap" last.json MERGEABLE CLEAN '.' +run_case "the needs-gap before the required check registers is not a failure" 0 "$work/gap" --timeout 60 + +gap_state "$work/gap-never" last.json MERGEABLE BLOCKED "$not_yet" +run_case "a required check that never registers is a timeout" 3 "$work/gap-never" + +# improvise-h3ni. The head must stay pinned across the UNKNOWN wait, or a +# verdict about b is reported as a verdict about a. +state "$work/unk-moved" 1.json '"UNKNOWN"' '"UNKNOWN"' "$green_rollup" +state "$work/unk-moved" 2.json '"UNKNOWN"' '"UNKNOWN"' "$green_rollup" +state "$work/unk-moved" last.json '"MERGEABLE"' '"CLEAN"' "$green_rollup" +for f in 2.json last.json; do + sed 's/aaaaaaaaaaaa/bbbbbbbbbbbb/' "$work/unk-moved/$f" > "$work/unk-moved/$f.new" + mv -f "$work/unk-moved/$f.new" "$work/unk-moved/$f" +done +run_case "a head that moves during the UNKNOWN wait is not a pass" 2 "$work/unk-moved" --timeout 60 + printf '\npr-watch-test: %d passed, %d failed\n' "$pass" "$fail" [ "$fail" -eq 0 ] From 43ba5e615174dd05df06fa48e567adb56cd11568 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:55:32 -0700 Subject: [PATCH 15/16] Bound test-only items by brackets and evaluate cfg predicates in the source scanner (improvise-59q8) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/protocol.md | 32 +- context/test-evidence/protocol.md | 48 +++ crates/improvise-protocol/src/source_scan.rs | 400 +++++++++++++++---- 3 files changed, 397 insertions(+), 83 deletions(-) diff --git a/context/code/protocol.md b/context/code/protocol.md index 85f47b8a..a07be6c0 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -230,11 +230,33 @@ guard below (improvise-c7mu). It removes three things a scan must not see. composers, so no verdict was wrong yet. It also had a mirror: a comment explaining that a `NOT_COMPOSED` variant is held back would have raised a false "stale entry" alarm. -- **Test items inside a file.** A line scan that relies on rustfmt putting an - item's closing brace at the attribute's own indentation. It treats - `cfg(any(test, …))` as test code and `cfg(not(test))` as production. - Over-stripping **fails** the test rather than passing it, which is the safe - direction. +- **Test items inside a file.** Each test-only attribute's item is bounded by + **balanced brackets on the blanked code** (improvise-59q8). It ends at a `;` + at depth zero, or at the `}` closing its body unless `;`, `.`, `?` or `else` + follows. The first version was a line scan that relied on rustfmt putting an + item's closing brace at the attribute's own indentation. The #172 review + found three shapes it never bounded: + - an item ending in `];` or `);`; + - a `#[cfg(test)]` statement spanning lines; + - a one-line `#[cfg(test)] use x;`. + + In each case the scan read on to the end of the file and **hid every line of + production code after it**, which is the silent direction for the forward + guard. An item the scan cannot bound is now a **panic**, never a scan to EOF. + + **Which `cfg` is test-only is evaluated, not substring-matched.** The + predicate is read off the file's *text*, because a `feature` value is a + literal and blanked in the code. It is evaluated in three-valued logic for a + build without `test`: `test` is false, and so is `feature = "test-support"`, + which this repo never ships (improvise-26dm). Everything else a scan cannot + know, such as a target or an ordinary feature, is unknown. An item is + test-only when its predicate is **false**. So + `cfg(all(test, not(target_arch = "wasm32")))` is test code, which the old + "any line containing `not(`" rule got wrong. `cfg(any(test, feature = + "serde"))` and `cfg(not(test))` are production. An inner `#![cfg(test)]` + heading a file makes the whole file test code. One appearing anywhere else + is a panic, because this scan cannot bound an inline module's inner + attribute. - **Test-only files.** A `#[cfg(test)] mod x;` makes `x.rs` and everything under `x/` test code, even though nothing inside the file says so. Before improvise-c7mu, this checker's own file and `improvise-io`'s diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index b00b88db..33c76148 100644 --- a/context/test-evidence/protocol.md +++ b/context/test-evidence/protocol.md @@ -1029,3 +1029,51 @@ Faults injected, one per half, each compiling only `improvise-protocol`: A field added to an existing variant is caught too, but not in isolation, because every constructor of that variant also fails. Reverted with `git restore`. + +### source_scan::tests::a_test_only_item_hides_nothing_after_it + +improvise-59q8, found by the PR #172 review. The scanner's line-based +`strip_test_items` looked for `}` at the attribute's indentation, or for an item +line ending in `;`. So three shapes were never bounded, and it dropped +**everything to the end of the file**: +- a test-only `const … = &[\n … \n];`; +- a `#[cfg(test)] let x = foo(\n 1,\n);` statement; +- a one-line `#[cfg(test)] use foo::bar;`, where the attribute's own line is + also the item line. + +In `vocabulary_is_complete` that silently hides every effect after such an +item. Each shape is a case here, asserting that the production code after it +(`Hidden1`, `Visible`) survives. Against the line scan all three failed; the +first printed `"impl X {\n"` and nothing else. They now pass because items are +bounded by balanced brackets on the blanked code. +`an_unbounded_test_only_item_is_refused_not_swallowed` pins the other half of +the acceptance criteria: an item that cannot be bounded panics instead of +consuming the rest of the file. Against the line scan it failed, because +nothing panicked. + +### source_scan::tests::a_test_only_cfg_is_recognised_whatever_its_spelling + +improvise-59q8, the other direction: test code counted as production, which +makes `vocabulary_is_composed` weaker. `cfg(all(test, not(target_arch = +"wasm32")))` was kept because the old rule rejected any line containing +`not(`. A leading `#![cfg(test)]` was kept because only `#[cfg(` was +recognised. An attribute split across lines was not seen at all. All three +failed against the old scanner. The predicate is now parsed and evaluated in +three-valued logic (`context/code/protocol.md` §2). The controls +(`cfg(not(test))`, `cfg(any(test, feature = "serde"))` and `cfg(target_arch = +…)`) must stay production, so a scanner that dropped every `cfg` item could +not pass. + +A consequence to know about: `cfg(any(test, feature = "x"))` for an ordinary +feature is now production. Only `feature = "test-support"`, the one test +feature this repo has, counts as a test build. +`test_only_items_and_modules_are_not_production` was updated to that spelling. + +### source_scan::tests::a_c_string_literal_is_blanked + +improvise-59q8's minor case. In `cr#"x" Command::InRawC(2) "y"#` the `r` has a +word character before it, so the literal was lexed as two ordinary strings, +and the text between the inner quotes leaked. It failed as +`InRawC survived`. `c"…"` and `cr#"…"#` are now literals. A first version of +this test used a C string with no inner quotes and passed against the bug, +because the ordinary lexing happened to blank it too. diff --git a/crates/improvise-protocol/src/source_scan.rs b/crates/improvise-protocol/src/source_scan.rs index 55f1b41c..f002c958 100644 --- a/crates/improvise-protocol/src/source_scan.rs +++ b/crates/improvise-protocol/src/source_scan.rs @@ -11,28 +11,28 @@ pub fn production_code(roots: &[PathBuf]) -> Vec<(PathBuf, String)> { for root in roots { collect_rust(root, &mut files); } - let code: Vec<(PathBuf, String)> = files + let read: Vec<(PathBuf, Source)> = files .into_iter() .map(|file| { let text = std::fs::read_to_string(&file).expect("a listed source reads"); - let code = code_only(&text); - (file, code) + let source = Source::of(&text, &file); + (file, source) }) .collect(); - let test_modules: BTreeSet = code + let test_modules: BTreeSet = read .iter() - .flat_map(|(file, code)| test_module_paths(file, code)) + .flat_map(|(file, source)| test_module_paths(file, source)) .collect(); - code.into_iter() + read.into_iter() .filter(|(file, _)| !test_modules.iter().any(|m| is_module_file(m, file))) - .map(|(file, code)| (file, strip_test_items(&code))) + .map(|(file, source)| (file, source.production())) .collect() } /// One file's text as [`production_code`] cleans it. Which *files* are test /// modules needs their declaring file too, so only `production_code` knows. pub fn production_text(text: &str) -> String { - strip_test_items(&code_only(text)) + Source::of(text, Path::new("")).production() } fn collect_rust(dir: &Path, into: &mut Vec) { @@ -55,83 +55,319 @@ fn is_module_file(m: &Path, file: &Path) -> bool { file == m.with_extension("rs") || file.starts_with(m) } +/// One file, read once. Its `cfg` predicates are read off the TEXT, whose +/// `feature` values are literals; every bracket off the code (improvise-59q8). +struct Source { + code: Vec, + /// Test-only items, as `(attribute start, item start, item end)`. + items: Vec<(usize, usize, usize)>, + /// A leading `#![cfg(test)]`: the whole file is test code. + whole_file: bool, +} + +impl Source { + fn of(text: &str, file: &Path) -> Self { + let code: Vec = code_only(text).chars().collect(); + let text: Vec = text.chars().collect(); + let (items, whole_file) = test_only_items(&text, &code, file); + Self { + code, + items, + whole_file, + } + } + + /// The code with every test-only item removed. + fn production(&self) -> String { + if self.whole_file { + return String::new(); + } + let mut kept = String::with_capacity(self.code.len()); + let mut at = 0; + for &(start, _, end) in &self.items { + kept.extend(&self.code[at..start]); + at = end; + } + kept.extend(&self.code[at..]); + kept + } + + fn item_code(&self, start: usize, end: usize) -> String { + self.code[start..end].iter().collect() + } +} + /// The out-of-line modules `file` declares under a test-only `cfg`, as paths /// without extension (`#[cfg(test)] mod x;` is `/x`). -fn test_module_paths(file: &Path, code: &str) -> Vec { +fn test_module_paths(file: &Path, source: &Source) -> Vec { let dir = match file.file_stem().and_then(|s| s.to_str()) { Some("lib" | "main" | "mod") => file.parent().map(Path::to_path_buf), Some(stem) => file.parent().map(|p| p.join(stem)), None => None, } .expect("a source file has a directory and a stem"); - let lines: Vec<&str> = code.lines().map(str::trim).collect(); - let mut found = Vec::new(); - for (i, line) in lines.iter().enumerate() { - if !is_test_cfg(line) { - continue; - } - let item = lines[i + 1..] - .iter() - .find(|l| !l.starts_with("#[") && !l.is_empty()); - if let Some(name) = item.and_then(|l| out_of_line_mod(l)) { - found.push(dir.join(name)); - } - } - found + source + .items + .iter() + .filter_map(|&(_, start, end)| out_of_line_mod(source.item_code(start, end).trim())) + .map(|name| dir.join(name)) + .collect() } /// `mod x;`, `pub mod x;`, `pub(crate) mod x;` → `x`. -fn out_of_line_mod(line: &str) -> Option<&str> { - let rest = line.split_once("mod ")?.1.strip_suffix(';')?; - let prefix = line.split_once("mod ")?.0.trim(); - (prefix.is_empty() || prefix.starts_with("pub")).then_some(rest.trim()) +fn out_of_line_mod(item: &str) -> Option { + let (prefix, rest) = item.split_once("mod ")?; + let name = rest.strip_suffix(';')?.trim(); + let prefix = prefix.trim(); + (prefix.is_empty() || prefix.starts_with("pub")).then(|| name.to_string()) } -/// A `cfg` attribute that compiles its item only under `test`. `cfg(not(test))` -/// is production, and `"test-support"` is a literal, already blanked. -fn is_test_cfg(line: &str) -> bool { - let line = line.trim(); - line.starts_with("#[cfg(") - && !line.contains("not(") - && line - .split(|c: char| !c.is_alphanumeric() && c != '_') - .any(|word| word == "test") +/// Every test-only item in a file, bounded by balanced brackets on the code +/// rather than by indentation. An item it cannot bound is a panic, never EOF. +fn test_only_items( + text: &[char], + code: &[char], + file: &Path, +) -> (Vec<(usize, usize, usize)>, bool) { + let mut items = Vec::new(); + let mut k = 0; + while k < code.len() { + let Some((inner, end)) = attribute_at(code, k) else { + k += 1; + continue; + }; + let predicate: String = text[k..end].iter().collect(); + if !is_test_only(&predicate) { + k = end; + continue; + } + if inner { + // An inner attribute heads the file or it heads an inline module; + // only the first is a shape this scan can honour. + let leading = code[..k].iter().all(|c| c.is_whitespace()); + assert!( + leading, + "{}: an inner test-only cfg could not be bounded", + file.display() + ); + return (Vec::new(), true); + } + let start = past_attributes(code, end); + let item_end = item_end(code, start).unwrap_or_else(|| { + panic!( + "{}: a test-only item at char {k} could not be bounded", + file.display() + ) + }); + items.push((k, start, item_end)); + k = item_end; + } + (items, false) } -/// Drop every test-only item. rustfmt puts an item's closing brace at the -/// attribute's own indentation, which is what makes this a line scan (§2). -fn strip_test_items(text: &str) -> String { - let lines: Vec<&str> = text.lines().collect(); - let mut kept = String::new(); - let mut i = 0; - while i < lines.len() { - let line = lines[i]; - match is_test_cfg(line) { - false => { - kept.push_str(line); - kept.push('\n'); - i += 1; - } - true => { - let indent = line.len() - line.trim_start().len(); - let close = format!("{}}}", " ".repeat(indent)); - i += 1; - while i < lines.len() && lines[i].trim_start().starts_with("#[") { - i += 1; - } - match lines.get(i).is_some_and(|l| l.trim_end().ends_with(';')) { - true => i += 1, - false => { - while i < lines.len() && lines[i].trim_end() != close { - i += 1; - } - i += 1; - } +/// An attribute starting at `k`: whether it is inner (`#!`) and where it ends. +fn attribute_at(code: &[char], k: usize) -> Option<(bool, usize)> { + if code[k] != '#' || (k > 0 && (code[k - 1].is_alphanumeric() || code[k - 1] == '_')) { + return None; + } + let inner = code.get(k + 1) == Some(&'!'); + let open = k + 1 + usize::from(inner); + (code.get(open) == Some(&'[')).then(|| matching(code, open).map(|close| (inner, close + 1)))? +} + +/// The index of the bracket closing the one at `open`, counting all three kinds. +fn matching(code: &[char], open: usize) -> Option { + let mut depth = 0usize; + for (k, &c) in code.iter().enumerate().skip(open) { + match c { + '(' | '[' | '{' => depth += 1, + ')' | ']' | '}' => { + depth -= 1; + if depth == 0 { + return Some(k); } } + _ => {} } } - kept + None +} + +/// Skip whitespace and any further outer attributes on the same item. +fn past_attributes(code: &[char], mut k: usize) -> usize { + loop { + while code.get(k).is_some_and(|c| c.is_whitespace()) { + k += 1; + } + let next = (k < code.len()).then(|| attribute_at(code, k)).flatten(); + match next { + Some((false, end)) => k = end, + _ => return k, + } + } +} + +/// Where the item starting at `start` ends: a `;` at depth zero, or the `}` +/// closing its body. `None` at the end of the file, which is not an end. +fn item_end(code: &[char], start: usize) -> Option { + let mut depth = 0usize; + let mut k = start; + while k < code.len() { + match code[k] { + '(' | '[' | '{' => depth += 1, + ';' if depth == 0 => return Some(k + 1), + // The enclosing block closed first: the item was its last expression. + ')' | ']' | '}' if depth == 0 => return Some(k), + ')' | ']' => depth -= 1, + '}' => { + depth -= 1; + if depth == 0 && !continues(code, k + 1) { + return Some(k + 1); + } + } + _ => {} + } + k += 1; + } + None +} + +/// Whether the item goes on after a `}` at depth zero: `S { .. };`, a method +/// call on a block, or an `if` with an `else`. +fn continues(code: &[char], after: usize) -> bool { + let rest: String = code[after..] + .iter() + .skip_while(|c| c.is_whitespace()) + .take(5) + .collect(); + rest.starts_with([';', '.', '?']) || rest.starts_with("else") +} + +/// A `cfg` attribute whose predicate no shipped build satisfies. Evaluated, +/// not substring-matched (improvise-59q8): `context/code/protocol.md` §2. +fn is_test_only(attribute: &str) -> bool { + let tokens = cfg_tokens(attribute); + match tokens.as_slice() { + [Token::Word(cfg), Token::Open, rest @ ..] if cfg == "cfg" => { + let mut at = 0; + predicate(rest, &mut at) == Truth::No + } + _ => false, + } +} + +/// What a `cfg` predicate is in a build without `test`. `Unknown` for what a +/// source scan cannot know, such as a target or an ordinary feature. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Truth { + Yes, + No, + Unknown, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +enum Token { + Word(String), + Str(String), + Open, + Close, + Comma, + Equals, +} + +/// The tokens of an attribute's text, from its first word: `#`, `!` and the +/// brackets around it are dropped. +fn cfg_tokens(attribute: &str) -> Vec { + let inner = attribute + .trim_start_matches('#') + .trim_start_matches('!') + .trim() + .trim_start_matches('[') + .trim_end() + .trim_end_matches(']'); + let chars: Vec = inner.chars().collect(); + let mut tokens = Vec::new(); + let mut k = 0; + while k < chars.len() { + let c = chars[k]; + match c { + '(' => tokens.push(Token::Open), + ')' => tokens.push(Token::Close), + ',' => tokens.push(Token::Comma), + '=' => tokens.push(Token::Equals), + '"' => { + let end = quoted_end(&chars, k + 1, '"'); + tokens.push(Token::Str(chars[k + 1..end - 1].iter().collect())); + k = end; + continue; + } + c if c.is_alphanumeric() || c == '_' => { + let word: String = chars[k..] + .iter() + .take_while(|c| c.is_alphanumeric() || **c == '_') + .collect(); + k += word.chars().count(); + tokens.push(Token::Word(word)); + continue; + } + _ => {} + } + k += 1; + } + tokens +} + +/// One predicate, advancing `at` past it. `test` is false; so is the +/// `test-support` feature, which this repo never ships (improvise-26dm). +fn predicate(tokens: &[Token], at: &mut usize) -> Truth { + let Some(Token::Word(word)) = tokens.get(*at) else { + return Truth::Unknown; + }; + *at += 1; + match (word.as_str(), tokens.get(*at)) { + (op @ ("all" | "any" | "not"), Some(Token::Open)) => { + *at += 1; + let mut args = Vec::new(); + while !matches!(tokens.get(*at), Some(Token::Close) | None) { + args.push(predicate(tokens, at)); + if tokens.get(*at) == Some(&Token::Comma) { + *at += 1; + } + } + *at += 1; + combine(op, &args) + } + (key, Some(Token::Equals)) => { + *at += 1; + let value = match tokens.get(*at) { + Some(Token::Str(v)) => v.clone(), + _ => String::new(), + }; + *at += 1; + match (key, value.as_str()) { + ("feature", "test-support") => Truth::No, + _ => Truth::Unknown, + } + } + ("test", _) => Truth::No, + _ => Truth::Unknown, + } +} + +/// Kleene's three-valued `all`, `any` and `not`. +fn combine(op: &str, args: &[Truth]) -> Truth { + match op { + "all" if args.contains(&Truth::No) => Truth::No, + "all" if args.iter().all(|a| *a == Truth::Yes) => Truth::Yes, + "any" if args.contains(&Truth::Yes) => Truth::Yes, + "any" if args.iter().all(|a| *a == Truth::No) => Truth::No, + "not" => match args.first() { + Some(Truth::Yes) => Truth::No, + Some(Truth::No) => Truth::Yes, + _ => Truth::Unknown, + }, + _ => Truth::Unknown, + } } /// `text` with comments, string, byte-string, raw-string and char literals @@ -166,8 +402,8 @@ fn literal_or_comment_end(c: &[char], i: usize) -> Option { (Some('/'), Some('/')) => Some(line_end(c, i)), (Some('/'), Some('*')) => Some(block_end(c, i)), (Some('"'), _) => Some(quoted_end(c, i + 1, '"')), - (Some('b'), Some('"')) if !word_before => Some(quoted_end(c, i + 2, '"')), - (Some('r' | 'b'), Some('r' | '#' | '"')) if !word_before => raw_end(c, i), + (Some('b' | 'c'), Some('"')) if !word_before => Some(quoted_end(c, i + 2, '"')), + (Some('r' | 'b' | 'c'), Some('r' | '#' | '"')) if !word_before => raw_end(c, i), (Some('\''), _) => char_end(c, i), _ => None, } @@ -211,9 +447,10 @@ fn quoted_end(c: &[char], mut k: usize, quote: char) -> usize { c.len() } -/// `r"…"`, `r#"…"#`, `br#"…"#`. `None` when the `r` is only an identifier. +/// `r"…"`, `r#"…"#`, `br#"…"#`, `cr#"…"#`. `None` when the `r` is only an +/// identifier. fn raw_end(c: &[char], i: usize) -> Option { - let mut k = i + usize::from(c[i] == 'b'); + let mut k = i + usize::from(matches!(c[i], 'b' | 'c')); if c.get(k) != Some(&'r') { return None; } @@ -283,21 +520,20 @@ mod tests { "}\n", "#[cfg(test)]\n", "mod out_of_line;\n", - "#[cfg(any(test, feature = \"x\"))]\n", + "#[cfg(any(test, feature = \"test-support\"))]\n", "pub mod gated;\n", "#[cfg(not(test))]\n", "fn production_only() {}\n", "fn also_kept() {}\n", ); - let code = code_only(text); - let stripped = strip_test_items(&code); + let stripped = production_text(text); for kept in ["fn kept", "fn production_only", "fn also_kept"] { assert!(stripped.contains(kept), "{kept} was dropped: {stripped}"); } assert!(!stripped.contains("gone_inline"), "{stripped}"); let lib = Path::new("/c/src/lib.rs"); assert_eq!( - test_module_paths(lib, &code), + test_module_paths(lib, &Source::of(text, lib)), vec![ PathBuf::from("/c/src/out_of_line"), PathBuf::from("/c/src/gated") @@ -305,7 +541,7 @@ mod tests { ); let nested = Path::new("/c/src/ui/reduce.rs"); assert_eq!( - test_module_paths(nested, &code)[0], + test_module_paths(nested, &Source::of(text, nested))[0], PathBuf::from("/c/src/ui/reduce/out_of_line") ); assert!(is_module_file( @@ -389,7 +625,10 @@ mod tests { ]; for (what, text) in cases { let kept = production_text(text); - assert!(!kept.contains("TestOnly"), "{what} was read as production: {kept:?}"); + assert!( + !kept.contains("TestOnly"), + "{what} was read as production: {kept:?}" + ); } // The controls: a predicate a shipped build can satisfy is production. for text in [ @@ -397,7 +636,10 @@ mod tests { "#[cfg(any(test, feature = \"serde\"))]\nfn p() { Command::Shipped(1) }\n", "#[cfg(target_arch = \"wasm32\")]\nfn p() { Command::Shipped(1) }\n", ] { - assert!(production_text(text).contains("Shipped"), "{text:?} was dropped"); + assert!( + production_text(text).contains("Shipped"), + "{text:?} was dropped" + ); } } @@ -406,7 +648,9 @@ mod tests { fn a_c_string_literal_is_blanked() { // The raw one quotes inside itself, so lexing it as ordinary strings // leaves the text BETWEEN the inner quotes in the open. - let kept = code_only("let a = c\"Command::InC(1)\"; let b = cr#\"x\" Command::InRawC(2) \"y\"#;\n"); + let kept = code_only( + "let a = c\"Command::InC(1)\"; let b = cr#\"x\" Command::InRawC(2) \"y\"#;\n", + ); for gone in ["InC", "InRawC"] { assert!(!kept.contains(gone), "{gone} survived: {kept}"); } From a4fa3ae17449e78724bc122f71b6cc51b804f012 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:56:15 -0700 Subject: [PATCH 16/16] pr-watch: wait on green-but-BLOCKED, the needs: gap before a required check registers (improvise-zlj2, improvise-h3ni) The required Browser harness check comes from a job that needs: build, so for a moment per push every registered check is green and GitHub says MERGEABLE/BLOCKED. #186 at 128a2a0 refused that and exited 1 on healthy PRs. BLOCKED with no failed check is now waited on (exit 3 if it never clears). Adds the gap sequence from PR #189's real rollup, and a head moving during the UNKNOWN wait. Corrects tooling.md's claim that nothing else produces BLOCKED, the evidence entry's rival-2 claims, and the fixture wording. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .agents/skills/pr-watch/SKILL.md | 10 ++++-- context/code/tooling.md | 37 ++++++++++++++++----- context/test-evidence/tooling.md | 55 ++++++++++++++++++++++++++------ scripts/pr-watch | 14 +++++--- 4 files changed, 91 insertions(+), 25 deletions(-) diff --git a/.agents/skills/pr-watch/SKILL.md b/.agents/skills/pr-watch/SKILL.md index a213ffe6..b397ce9a 100644 --- a/.agents/skills/pr-watch/SKILL.md +++ b/.agents/skills/pr-watch/SKILL.md @@ -15,13 +15,17 @@ exit 0 every check COMPLETED, none failed, and MERGEABLE with a state of exit 1 a check failed, or the PR is not mergeable (CONFLICTING, BLOCKED, DIRTY, BEHIND, DRAFT, or a state it does not know) exit 2 the head moved during the wait, or is not --expect -exit 3 timed out with checks still running or mergeability still UNKNOWN +exit 3 timed out with checks still running, mergeability still UNKNOWN, + or still BLOCKED with every registered check green exit 4 usage or environment error ``` **Exit 0 now means mergeable, not only green.** GitHub computes mergeability -lazily and reports `UNKNOWN` for a while after the base moves. The watch waits -that out, so you do not need to re-check `mergeable` by hand after it. Why each +lazily and reports `UNKNOWN` for a while after the base moves. It also reports +`BLOCKED` for the moment before a required check that `needs:` another job has +registered. The watch waits out both, so you do not need to re-check +`mergeable` by hand after it. **Exit 3 is not a pass.** It may be a required +check that never ran. Why each state counts the way it does: `context/code/tooling.md`, "pr-watch and mergeability" (improvise-edpl). diff --git a/context/code/tooling.md b/context/code/tooling.md index 0667f408..6b09fc0f 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -259,7 +259,7 @@ the deploy's and pass. `README.md` therefore states the rule and deliberately **does not list the jobs**: a second copy with nothing holding it to the first is what went stale, twice. -### pr-watch and mergeability (improvise-edpl) +### pr-watch and mergeability (improvise-edpl, improvise-zlj2) `scripts/pr-watch` is what the orchestrator's merge procedure stands on, and until improvise-edpl its verdict failed only on `mergeable == CONFLICTING`. @@ -273,6 +273,9 @@ The rule now: - **`UNKNOWN` in either field is not an answer, so it is waited on.** On timeout it exits 3, the same code as a check still running. It never exits 0. +- **`MERGEABLE/BLOCKED` with no failed check is waited on too** (improvise-zlj2). + A required check that never runs therefore times out at exit 3, and does not + exit 1. - **A known answer ends the wait early**: a failed check (exit 1 whatever mergeability says, since waiting would turn a known red into a timeout), or `mergeable == CONFLICTING`. @@ -286,24 +289,42 @@ repos/fiddlerwoaroof/improvise/rules/branches/main`, read 2026-09-22) requires three status checks. It requires no approvals, and its required checks are not strict. -- `BLOCKED` with a `MERGEABLE` flag means a required check has not passed, - either unrun or failing. With zero required approvals nothing else produces it - here. It is refused, not waited on. A required check that never runs would - otherwise hold the watch until the timeout, and exit 1 is the "not mergeable" - answer. +- `BLOCKED` with a `MERGEABLE` flag means a required check has not passed. A + failing one is already in the rollup, so the script has exited 1 before it + reads the state. What is left is a required check that **has not registered + yet**, and every healthy push passes through that state. The required + `Browser harness (OPFS contract + shell)` check comes from the `browser` job, + which `needs: build`. GitHub creates its check run only after `build` + completes. For a second or three per push, then, every registered check is + `COMPLETED/SUCCESS`, the required one does not exist, and GitHub reports + `MERGEABLE/BLOCKED`. The first version of this rule refused `BLOCKED`, and so + exited 1 on a healthy PR whenever a poll landed in that gap. The reviewer put + that at 5–15% of watches at the 20s interval. An "unrun" required check and a + "not yet registered" one look the same from the rollup, so the state is + waited on. A check that genuinely never runs costs a timeout, exit 3, not a + false exit 1. An earlier version of this section said that nothing else + produces `BLOCKED` here. That was wrong: the `needs:` gap produces it. - `DIRTY` can wear `MERGEABLE` for a moment while the two fields settle. It is refused. - `BEHIND` needs strict required checks, which this repo does not set. `DRAFT` cannot be merged. Both are refused. - `UNSTABLE` means a non-required check is unhappy. It stays a pass, as it was - before. Any failed check in the rollup has already exited 1, so in practice it - only means a non-required status still settling. + before. It is safe because the loop only reads the state once every + registered check is `COMPLETED`, and any failed one has already exited 1. So + whatever `UNSTABLE` is reporting, it is not a check in the rollup that is + still running or red. - `HAS_HOOKS` is GitHub Enterprise's "mergeable, with pre-receive hooks". It is harmless to allow. The suite enumerates the whole `mergeable × mergeStateStatus` table, plus an unshipped value and `null`, rather than hand-picking cases. Why: improvise-90ea. +The suite also plays the `needs:` gap through, poll by poll: build running, then +build done with no harness check, then the harness running, then `CLEAN`. That +sequence must exit 0. The rollup it starts from is PR #189's, copied from +`gh pr view --json`. Each earlier poll is derived from it with `jq`, by dropping +entries or rewinding them to `IN_PROGRESS`. + ### Why the Pages deploy fails on "Found 0 artifact(s)" (improvise-864a) `actions/deploy-pages@v5` intermittently fails a push to `main` with *No diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 439ca4aa..0bdaca76 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -622,10 +622,11 @@ mergeability". `ae96b8c`, 19 of the 34 new cases failed and all 19 exited **0**. `UNKNOWN that resolves to CONFLICTING fails` is PR #172's shape exactly: every check green, `UNKNOWN/UNKNOWN` on the first poll, `CONFLICTING/DIRTY` on the -second. The old script broke out of the loop on the first poll and passed. Each -fixture is the real `gh pr view --json` payload with only the two mergeability -fields changed. A stub that errors instead would have exited 4, a wider shape -than the fault. +second. The old script broke out of the loop on the first poll and passed. The +fixtures are hand-built. They are the subset of the `gh pr view --json` shape +that the script reads: `headRefOid`, the two mergeability fields, and the +rollup's `status` and `conclusion`. They are not a captured payload. A stub that +errors instead would have exited 4, a wider shape than the fault. **Rival 1: treat `UNKNOWN` as an answer.** The wait condition was replaced with `if true`. 13 cases fail: every `UNKNOWN` row of the table, @@ -633,14 +634,50 @@ than the fault. `UNKNOWN that resolves to MERGEABLE/CLEAN passes`. That last one exits 1 under this rival, so the fix cannot pass by refusing everything uncertain. It has to wait. `UNKNOWN that resolves to CONFLICTING fails` still passes under this -rival, because the allowlist refuses `UNKNOWN` as well. The next rival is what -separates it. +rival, because the allowlist refuses `UNKNOWN` as well. Rival 2 does not +separate it either: its first poll is `UNKNOWN`, which rival 2 still waits on. +What separates it is the original fault, which combined both rivals. An earlier +version of this entry said rival 2 separated it. That was wrong. **Rival 2: the old verdict.** The allowlist was replaced with `exit 0` after the -`MERGEABLE` test. Six cases fail: `MERGEABLE/BEHIND`, `BLOCKED`, `DIRTY`, -`DRAFT`, `SOMETHING_NEW` and `null`. These are the states that look mergeable -by the flag and are not. +`MERGEABLE` test. Five cases fail: `MERGEABLE/BEHIND`, `DIRTY`, `DRAFT`, +`SOMETHING_NEW` and `null`. These are the states that look mergeable by the flag +and are not. At `128a2a0` it was six, with `BLOCKED` among them. Since +improvise-zlj2, `BLOCKED` is waited on, so this rival never reaches the verdict +for it. The entry below covers that. `a failed check fails without waiting on mergeability` runs at `--timeout 0`. A script that waited on `UNKNOWN` before reading the rollup would report that known red as exit 3. + +### The pr-watch needs-gap and pinned-head cases — `scripts/pr-watch-test.sh` + +improvise-zlj2 and improvise-h3ni. These were found by review of PR #186 at +`128a2a0`, which was ruled do-not-merge. + +**zlj2: a healthy PR exited 1.** The required `Browser harness` check run +exists only after the `build` job completes (`needs: build`). In that gap +GitHub reports `MERGEABLE/BLOCKED` while every registered check is green, and +the #186 rule refused `BLOCKED`. The rollup in the new cases is PR #189's, +captured verbatim with `gh pr view --json`. It has thirteen entries with every +field GitHub returns. Each gap state drops or rewinds entries of that rollup +with `jq`. Against `128a2a0` three cases failed, all with exit 1: + +- `the needs-gap before the required check registers is not a failure`: the + healthy sequence should exit 0. +- `a required check that never registers is a timeout`: should exit 3. +- `table: MERGEABLE/BLOCKED`: should exit 3, not 1. + +**Rival for zlj2:** remove `MERGEABLE/BLOCKED` from the wait. The same three +cases fail with exit 1. That rival is `128a2a0`'s code. + +**h3ni: nothing pinned the head across the UNKNOWN wait.** The case is +`a head that moves during the UNKNOWN wait is not a pass`. Its polls are +UNKNOWN@a, then UNKNOWN@b, then CLEAN@b, and it must exit 2. It passed on the +first run, so it is a guard for behaviour the script already has, not a +reproduction. + +**Rival for h3ni:** re-pin the head whenever `mergeable` is UNKNOWN. That case +alone fails, exiting 0. All 44 others pass. That is the review's point: the +older `a head that moves mid-wait is not a pass` starts from MERGEABLE, so it +cannot see this rival. diff --git a/scripts/pr-watch b/scripts/pr-watch index e90dc799..0c96ddaf 100755 --- a/scripts/pr-watch +++ b/scripts/pr-watch @@ -15,7 +15,8 @@ exit 0 every check COMPLETED, none failed, and GitHub says MERGEABLE with a state of CLEAN, UNSTABLE or HAS_HOOKS exit 1 a check failed, or the PR is not mergeable (any other known state) exit 2 the head moved during the wait, or is not --expect -exit 3 timed out with checks still running or mergeability still UNKNOWN +exit 3 timed out with checks still running, or mergeability still UNKNOWN, + or still BLOCKED with every registered check green exit 4 usage or environment error EOF exit 4 @@ -92,11 +93,14 @@ while :; do mergeable="$(printf '%s' "$json" | jqf '.mergeable')" state="$(printf '%s' "$json" | jqf '.mergeStateStatus')" - # UNKNOWN is GitHub not having computed mergeability yet, so it is waited on; - # a red check or CONFLICTING is already an answer (improvise-edpl). + # UNKNOWN and green-but-BLOCKED are waited on: see context/code/tooling.md, + # "pr-watch and mergeability" (improvise-edpl, improvise-zlj2). if [ "$total" -gt 0 ] && [ "$running" -eq 0 ]; then if [ "$failed" -gt 0 ] || [ "$mergeable" = CONFLICTING ]; then break; fi - if [ "$mergeable" != UNKNOWN ] && [ "$state" != UNKNOWN ]; then break; fi + case "$mergeable/$state" in + UNKNOWN/*|*/UNKNOWN|MERGEABLE/BLOCKED) ;; + *) break ;; + esac fi now=$(date +%s) @@ -116,7 +120,7 @@ printf '%s' "$json" | jqf '.statusCheckRollup[]? | select(.conclusion != "SKIPPE echo "pr-watch: head=$head $mergeable/$state $total checks, $failed failed" [ "$failed" -eq 0 ] || exit 1 -# An allowlist, so a state GitHub adds later fails closed. BLOCKED and DIRTY +# An allowlist, so a state GitHub adds later fails closed. DIRTY and BLOCKED # can wear a MERGEABLE flag: context/code/tooling.md, "pr-watch and mergeability". [ "$mergeable" = MERGEABLE ] || exit 1 case "$state" in CLEAN|UNSTABLE|HAS_HOOKS) exit 0 ;; *) exit 1 ;; esac