diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl index f83081cc..78781ac6 100644 --- a/.beads/issues.jsonl +++ b/.beads/issues.jsonl @@ -40,10 +40,11 @@ {"_type":"issue","id":"improvise-km8","title":"1.2 Fix Cargo.toml metadata","description":"Add missing package fields: description, repository, homepage, documentation, readme, keywords, categories, license. Description: 'Terminal pivot-table modeling in the spirit of Lotus Improv'. Keywords: tui, pivot, spreadsheet, data, improv. Categories: command-line-utilities, visualization.","status":"closed","priority":1,"issue_type":"task","assignee":"Edward Langley","owner":"el-github@elangley.org","created_at":"2026-04-09T04:06:48Z","created_by":"Edward Langley","updated_at":"2026-07-30T23:06:31Z","closed_at":"2026-04-09T07:20:17Z","close_reason":"Metadata already present: description, license, repository, homepage, readme, keywords, categories all populated.","dependency_count":0,"dependent_count":2,"comment_count":0} {"_type":"issue","id":"improvise-902","title":"1.1 Audit and remove or update context/SPEC.md","description":"Compare context/SPEC.md against actual code. If spec contradicts code significantly, delete it. Optionally move salvageable conceptual content to docs/design-notes.md with a staleness disclaimer. Do not try to bring spec into sync.","status":"closed","priority":1,"issue_type":"task","assignee":"Edward Langley","owner":"el-github@elangley.org","created_at":"2026-04-09T04:06:46Z","created_by":"Edward Langley","updated_at":"2026-07-30T23:06:31Z","closed_at":"2026-04-09T07:10:05Z","close_reason":"Audited SPEC.md against code. Found minor inaccuracies (storage internals, wizard step count, search mode representation) but no major architectural contradictions. Deleted SPEC.md since it was largely redundant with repo-map.md and design-principles.md. Salvaged product vision and non-goals into docs/design-notes.md with staleness disclaimer.","dependency_count":0,"dependent_count":1,"comment_count":0} {"_type":"issue","id":"improvise-tik","title":"Phase 1: Repository hygiene","description":"Raise the quality floor of what a stranger sees when they land on the repo. Subtasks cover spec audit, Cargo.toml metadata, publish-readiness, CSV audit, and example files.","status":"closed","priority":1,"issue_type":"feature","owner":"el-github@elangley.org","created_at":"2026-04-09T04:05:45Z","created_by":"Edward Langley","updated_at":"2026-04-09T07:22:34Z","closed_at":"2026-04-09T07:22:34Z","close_reason":"All 5 subtasks complete: SPEC.md audited, Cargo.toml metadata fixed, publish dry-run clean, CSV audit done, example files created.","dependencies":[{"issue_id":"improvise-tik","depends_on_id":"improvise-2fr","type":"blocks","created_at":"2026-04-08T23:37:31Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-tik","depends_on_id":"improvise-902","type":"blocks","created_at":"2026-04-08T23:37:30Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-tik","depends_on_id":"improvise-bv1","type":"blocks","created_at":"2026-04-08T23:37:32Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-tik","depends_on_id":"improvise-km8","type":"blocks","created_at":"2026-04-08T23:37:30Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-tik","depends_on_id":"improvise-n1h","type":"blocks","created_at":"2026-04-08T23:37:32Z","created_by":"Edward Langley","metadata":"{}"}],"dependency_count":5,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"improvise-x1ns","title":"encoded_width undercounts a forced category that has a missing coordinate","description":"Found by ChatGPT Codex reviewing PR #90 at 99cfb7f. THE REVIEW WAS NOT READ BEFORE THE PR MERGED — orchestrator process failure, see improvise-\u003cthis\u003e's sibling note. Re-verified on main at ce895ff: STILL LIVE.\n\nencoded_width (crates/improvise-gp/src/fit.rs:18) computes has_missing at line 32 and uses it ONLY in the numeric branch:\n\n true =\u003e 1 + usize::from(has_missing), // numeric: feature + .missing flag\n false =\u003e present.iter().collect::\u003cHashSet\u003c_\u003e\u003e().len().max(1), // categorical: present only\n\nbuild_categorical (crates/gp-tree/src/data.rs:359) maps every missing cell to MISSING_CAT and includes it in distinct(\u0026mapped), emitting an ADDITIONAL one-hot.\n\nSo a forced categorical column with N distinct present values plus one record lacking that coordinate passes the preflight as N and materializes N+1 columns. It is caught only by the LATER EXACT CHECK — which is precisely the allocation the preflight exists to prevent. At default max_records that is a large one-hot built before the error.\n\nThis is doctrine(guard_the_cost_not_the_result) defeated, and it is the SECOND time on this code path: improvise-zv8t's own adversarial round found forcing-by-name-while-budgeting-by-index, repaired as a data dependency. This one is the same class one level in — the two branches of one match disagree about what a column costs.\n\nNote the Wave K agent's own review had spotted a related shape (recorded as F5, \"build_categorical counts MISSING_CAT, the preflight does not — under-counts by one per category with an absent coordinate\") and recorded it as READ, NOT INJECTED and bounded by the exact check. Codex is right that \"bounded by the exact check\" is not a defence: the exact check runs after the allocation.\n\n## Acceptance Criteria\n- Count the missing level in the categorical branch.\n- A boundary regression with a FORCED NUMERIC CATEGORY containing a missing coordinate — the interesting case, because forcing is what makes a numeric column take the categorical branch.\n- Prove it by injection: without the fix, the preflight must under-report by exactly one and the test must fail naming that.\n- While there: gp.md §14 documents where the preflight and the exact check \"still differ\". Re-read it — it is the prose that made this look bounded.\n","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T17:13:05Z","created_by":"Edward Langley","updated_at":"2026-09-21T17:13:05Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-ac2z","title":"A host's none_cats leak into another view's grid: formula columns vanish","description":"A client on a view whose none_cats differ from the HOST's is projected a grid\nmissing that view's formula columns entirely. Not a wrong number -- absent\ncolumns.\n\nFOUND BY CORRECTING A FIXTURE, which is the part worth keeping. The wave-I\nhand-back for improvise-t6db reported this hazard as \"argued and NOT observed\",\non the strength of a fixture that was said to put Product on the None axis in\none view and on Column in another. It did not: add_category defaults a new\ncategory to Column and nothing moved it, so all three views had none_cats\n{_Index, _Dim} and the stated input to the hazard was absent. An unprobed path\nreported as a negative result. Setting Product to Axis::None explicitly made the\nproperty test fail on the first run.\n\nMECHANISM. App::rebuild_layout calls recompute_formulas(\u0026none_cats) for the\nHOST's view, populating Model::formula_cache. project::grid_in on the elsewhere\npath builds the other view's GridLayout against that same, stale cache. The\ncache is keyed by the coordinates the recompute used, so with Product on None in\nthe host's view the Total cells are keyed WITHOUT a Product coordinate; the other\nview, which has Product on Column, asks for (Region, Product, _Measure=Total),\nfinds nothing, and -- because prune_empty is on -- drops both Total columns.\n\nMEASURED, two-view fixture, formula Total = SUM(Revenue):\n\n host on Second, projecting Second 6 columns incl. Total x Widget/Gadget\n host on Default, projecting Second 4 columns NO Total columns at all\n\nTHE FIX IS NOT LOCAL. grid_in takes \u0026App and recompute_formulas needs \u0026mut Model,\nso grid_in cannot recompute for the view it is projecting. Candidates, none\nchosen:\n\n - Key formula_cache by view (or by none_cats), so one Model can hold more than\n one view's derived values.\n - Recompute on the session's behalf at a \u0026mut App boundary (Sessions::project\n is one) and cache per session.\n - Declare it out of scope: formulas are evaluated for the host's view and a\n client on another view sees the host's derivation. That is a decision, not\n the current state, which is an accident.\n\nNote this is the SAME SHAPE as improvise-t6db: the elsewhere path of grid_in is\nonly as correct as the inputs it can rebuild, and prune_empty was the first such\ninput. none_cats is the second, and it is worse, because prune_empty was one\nline and this one cannot be fixed where it is observed.\n\nGUARDED MEANWHILE by a_hosts_none_cats_leak_into_another_views_formula_columns\nin ui/project.rs, a characterization test that asserts the bug. Its message says\nso: if it fails, the bug is fixed, and the none_cats guard should come out of\na_clients_grid_does_not_depend_on_which_view_its_host_is_on at the same time.\n\n## Steps to Reproduce\n\n1. Two views over one document, both with prune_empty on, with a formula.\n2. Put a category on Axis::None in view A and on Axis::Column in view B.\n3. Host on B: grid_in(app, \"B\") draws the formula's columns.\n4. Host on A: grid_in(app, \"B\") does not.\n","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T11:31:45Z","created_by":"Edward Langley","updated_at":"2026-09-21T11:31:45Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-711n","title":"parse_number_format silently yields 0 decimals for the ,.2f spelling our own docs show","description":"Found by the adversarial review of PR #92, as a side finding while printing a test fixture's rendered cells and noticing they said \"1\" where the fixture had set format \",.2f\".\n\nparse_number_format (crates/improvise-core/src/format.rs:14) reads the decimal count as:\n\n fmt.rfind('.').and_then(|i| fmt[i + 1..].parse::\u003cu8\u003e().ok()).unwrap_or(0)\n\nFor \",.2f\" that takes everything after the dot — \"2f\" — and parses it as u8. It FAILS, so unwrap_or(0) yields ZERO DECIMALS. Silently.\n\n## Why this is not hypothetical\n- context/repo-map.md's own .improv format example documents `format: ,.2f`.\n- persistence/format.rs:109 writes `format: {}` with view.number_format verbatim, so whatever a user types round-trips into the file unchanged and is re-parsed the same wrong way on every load.\n- The trailing-f spelling is the Python/printf convention a user would most naturally reach for.\n\nSo a document whose author wrote the spelling our own documentation shows renders every number with zero decimals, and nothing reports it.\n\n## Steps to Reproduce\nSet a view's number_format to \",.2f\" and render any non-integer value. Expect 1.00; observe 1.\n\n## Acceptance Criteria\n- Decide which spellings are legal and say so in ONE place. The grammar is the source of truth for .improv, so if \",.2f\" is legal the parser must honour it; if it is not, loading must refuse it rather than silently reinterpreting it.\n- A test with the \",.2f\" spelling asserting the decimals actually applied. It must fail on today's code.\n- Whichever way it goes, the repo-map example and the format doc must agree with the parser — this bug is partly a documentation bug, and a fact stated twice is changed by grep.\n- Consider the neighbouring spellings while you are there: \".2\", \",.2\", \"0.00\", \"%.2f\".\n\nNOT verified: whether any shipped document or fixture uses the trailing-f spelling, and whether the import wizard can produce it.\n","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T11:29:53Z","created_by":"Edward Langley","updated_at":"2026-09-21T11:29:53Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-e5pf","title":"Command::ChangeMode(Normal) is on the wire and reduce_view folds it without the records resolution","description":"WIDENED 2026-09-21 after a sweep of resolve.rs. The fault is ONE SUBSTITUTION AT THREE DECISION POINTS, not one command on the wire, and the first of the three composes a malformed write rather than declining.\n\n wherever the host asks layout.is_records_mode(),\n the client asks state.view.mode.is_records()\n\nCommand::ChangeMode(Normal) is HOW the client reaches the bad state; the substitution is WHY the bad state does damage. The two want fixing together, and the second decides whether enriching the projection is enough.\n\n## How the client reaches the bad state\nimprovise-sihr made the TUI settle AppMode::Normal against the layout the batch ends on (App::settle_normal_mode). The wire carries Command::ChangeMode(AppMode::Normal); improvise-protocol/src/reduce.rs:195 folds it as `view.mode = mode.clone()` with no resolution. improvise_protocol::resolve — re-exported as improvise_wasm_client::resolve, and the LIVE browser resolver; keys.rs holds only BrowserKey — emits it at resolve.rs lines 369, 692, 972, 1461, 1613 and 2045.\n\nDemonstrated with a lockstep-shaped probe (resolve + ClientState::fold against reduce_key; enter records with R, open a panel, Esc):\n left: Normal (client)\n right: RecordsNormal (host)\n\n## What the bad state then breaks\nresolve.rs:685 commit() early return — host counterpart CmdContext::is_records_mode() via CommitAndAdvance. The client is meant to DECLINE a records commit it cannot compose. With the mode wrong it does not decline: it composes typed_cell_command(id, \u0026text) and a Basis claim against a records grid, whose columns are [coords.., +cat, measures.., +] (gotcha 1). That is A WRONG WRITE, strictly worse than the black hole it was written to produce.\n\nresolve.rs:708 create_column() exit mode — picks Normal where it should pick RecordsNormal, re-entering the same wrong state after every column creation. Self-sustaining.\n\nresolve.rs:744 advanced() — host counterpart commit.rs:547, `at_bottom_right \u0026\u0026 ctx.is_records_mode()` -\u003e AddRecordRow. The client returns None to decline what it cannot name; with the mode wrong it clamps and re-enters, so TAB AT THE BOTTOM-RIGHT OF A RECORDS GRID SILENTLY STOPS APPENDING ROWS. That is the records-mode Tab contract failing silently.\n\n## Why the suite cannot see any of it\nlockstep.rs is blind to the class BY CONSTRUCTION. Its records coverage (typing_at, records_pair) sets host.view_state.mode = AppMode::records_editing() by ASSIGNMENT and seeds the client with client.view = app.view_state.clone() — it starts from a state where mode and layout ALREADY AGREE, and every divergence above requires them to disagree. A guard seeded on agreement cannot detect drift: the same vacuity shape as gotcha 36's bystander already sitting on the relayed value.\n\n## Acceptance Criteria\n- lockstep DRIVES THE KEYS into records mode and back out of a minibuffer rather than hand-placing the mode. Without this, any fix is unguarded.\n- The projection carries what the client needs to settle the mode (gotcha 47: where a client cannot compose the answer, ENRICH THE PROJECTION rather than moving resolution back).\n- All three decision points answer the same question the host answers.\n- Or: a written argument that ChangeMode(Normal) should not be on the wire, plus its removal — which still leaves the substitution at the three sites.\n\nDo NOT close this by having the client re-derive records-ness from the axes: that is a second derivation, improvise-edp's disease.\n","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T11:04:35Z","created_by":"Edward Langley","updated_at":"2026-09-21T11:29:10Z","dependency_count":0,"dependent_count":0,"comment_count":0} -{"_type":"issue","id":"improvise-9c9o","title":"A geometry effect right after a mid-batch SwitchView skips the cursor check entirely","description":"Introduced by PR #78 round 2. PASSES on pre-PR main (0947fd0) and on round-1 head (bcccfde); FAILS at d9cb16c.\n\nRound 1 compared frame hashes across a view change, which threw a cursor away spuriously. Round 2 added a 'was == self.view_name()' guard, which fixes that — but it also exempts the FIRST geometry effect after a mid-batch SwitchView from the check altogether, because there is no basis for the new view yet, so the code re-bases and skips.\n\n## Steps to Reproduce\n app.apply_effects(vec![SwitchView(\"Second\"), TransposeAxes]);\nBoth preconditions hold: the switch lands, and row_count() really changes. Yet cursor_epoch does not move and the cursor stays at (2,0) in a transposed grid.\n\n## Acceptance Criteria\n- The probe above lands as a guard.\n- The basis re-bases to the new view AT THE SWITCH rather than at the next geometry effect — i.e. the basis follows active_view changes, which SwitchView being ViewOnly currently prevents. Not a one-liner.\n\nWHY THE DIRECTION MATTERS: round 1 threw away a cursor that should have been kept; round 2 keeps one that should have been thrown away. Gotcha 35's asymmetry says under-reporting is the expensive side — a stale cursor has nothing in the stream to correct it, where an extra invalidation costs one redundant projection.\n\nReachability is the same narrow class as the bug it replaced: a . -chained : line, or any future Binding::Sequence composing the two. DrillIntoCell and EnterRecordsMode hit the pattern constantly but their target view is freshly created, so it is benign there. Merged on that basis rather than blocked.","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T05:15:07Z","created_by":"Edward Langley","updated_at":"2026-09-21T05:15:07Z","dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"improvise-9c9o","title":"A geometry effect right after a mid-batch SwitchView skips the cursor check entirely","description":"Introduced by PR #78 round 2. PASSES on pre-PR main (0947fd0) and on round-1 head (bcccfde); FAILS at d9cb16c.\n\nRound 1 compared frame hashes across a view change, which threw a cursor away spuriously. Round 2 added a 'was == self.view_name()' guard, which fixes that — but it also exempts the FIRST geometry effect after a mid-batch SwitchView from the check altogether, because there is no basis for the new view yet, so the code re-bases and skips.\n\n## Steps to Reproduce\n app.apply_effects(vec![SwitchView(\"Second\"), TransposeAxes]);\nBoth preconditions hold: the switch lands, and row_count() really changes. Yet cursor_epoch does not move and the cursor stays at (2,0) in a transposed grid.\n\n## Acceptance Criteria\n- The probe above lands as a guard.\n- The basis re-bases to the new view AT THE SWITCH rather than at the next geometry effect — i.e. the basis follows active_view changes, which SwitchView being ViewOnly currently prevents. Not a one-liner.\n\nWHY THE DIRECTION MATTERS: round 1 threw away a cursor that should have been kept; round 2 keeps one that should have been thrown away. Gotcha 35's asymmetry says under-reporting is the expensive side — a stale cursor has nothing in the stream to correct it, where an extra invalidation costs one redundant projection.\n\nReachability is the same narrow class as the bug it replaced: a . -chained : line, or any future Binding::Sequence composing the two. DrillIntoCell and EnterRecordsMode hit the pattern constantly but their target view is freshly created, so it is benign there. Merged on that basis rather than blocked.","status":"closed","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T05:15:07Z","created_by":"Edward Langley","updated_at":"2026-09-21T17:09:50Z","closed_at":"2026-09-21T17:09:50Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-iot6","title":"render_csv builds GridLayout::new, so CSV export carries rows the view prunes","description":"FOURTH instance of the GridLayout::new versus with_frozen_records trap, and the first one in production that a user can see.\n\ncrates/improvise-io/src/persistence/export.rs:33 builds GridLayout::new(\u0026workbook.model, view), which does NOT apply prune_empty. So :export in the TUI and the CLI export path both write rows the grid does not draw.\n\n## Steps to Reproduce\nFixture: Region {North, Service, South}, Revenue set for North and South only, prune_empty = true. The view draws 2 rows (assert this as the precondition).\n\nrender_csv output:\n Region,Revenue\n North,1\n Service, \u003c- pruned on screen, exported anyway\n South,1\n\n## Acceptance Criteria\n- render_csv honours the view's prune_empty, via the same path grid_in now uses.\n- A test whose precondition asserts the view really prunes (a row that is actually empty, pruning left ON in the view). A fixture that turns pruning off, or has nothing to prune, is vacuous — that exact vacuity was demonstrated while writing the improvise-t6db property.\n- None of export.rs's three existing tests mentions pruning; that is why this shipped.\n\nPre-existing, not a regression from PR #78. Found by the adversarial review of #78 while enumerating every GridLayout::new call site: three are outside test modules, two of those are test helpers, and this is the third.","status":"closed","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T05:14:59Z","created_by":"Edward Langley","updated_at":"2026-09-21T11:09:27Z","closed_at":"2026-09-21T11:09:27Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-zv8t","title":"fit_measure never tells preprocess which columns are categories, so a numeric- or date-valued category is unspellable","description":"fit_measure never tells preprocess which columns are categories, so a numeric- or date-valued category is unspellable\n\nFound reviewing improvise-wd6n's branch, and it was very nearly foreclosed in\nprose: the first draft of context/code/gp.md §11 called Artefact::NumericCategory\na permanent refusal that \"no vocabulary closes\". That is wrong, and this is the\nticket the prose would have prevented.\n\ncrates/improvise-gp/src/fit.rs passes `\u0026Overrides::default()` to `preprocess`,\nso it never tells preprocess which header columns are document categories --\nalthough the very next line does tell `Features::of`. `preprocess` then infers\nNumeric -\u003e Date -\u003e Categorical from the cell values, so:\n\n * a regular category whose items all parse as numbers (a `Year` category with\n items 2024/2025) becomes ColumnKind::Numeric, and its feature is refused as\n Artefact::NumericCategory\n * a regular category whose items are all ISO dates becomes ColumnKind::Date,\n and its features are refused as Artefact::DatePart\n\nIn both cases `Year IS 2024` is EXACTLY spellable, and the fit is refused\nanyway. Unlike the two families improvise-wd6n declines, this needs no\nformula-language change at all: `Overrides::force[col] = Forced::Categorical`\nalready exists (gp-tree/src/data.rs, consumed in preprocess's `match ov.force`)\nand is the knob. fit.rs knows which columns are categories -- it computes\n`category_columns` on the line above.\n\nThe refusal text for NumericCategory argues about `Expr::Ref` resolving to a\n_Measure item of the same name, which was never the right spelling for a\ncategory column. It also sits awkwardly beside this crate's own rule (gotcha\n37(d), gp.md §11: the check asks the document, never the name's shape) and\nbeside a_coordinate_is_spelled_as_one_even_when_an_item_shares_the_one_hots_name,\nwhich exists precisely to show the two spellings cannot collide.\n\nNeither NumericCategory nor AbsentCoordinate has a test today; they were\ninvented by the wd6n branch and only three of the five families are reached by\nits fixture.\n\n## Acceptance\n\nA fit over a view with a numeric-valued or date-valued regular category emits a\n`Cat IS Item` coordinate for it rather than being refused, demonstrated by a\ntest that fails before the fix. Whatever forcing is applied must not change how\na MEASURE column is inferred -- a numeric measure is still Numeric. If forcing\nturns out to be wrong for some reason not visible from here, say so in\ncontext/code/gp.md and correct the \"permanent refusal\" paragraph either way.\n","status":"closed","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T05:05:31Z","created_by":"Edward Langley","updated_at":"2026-09-21T11:09:28Z","closed_at":"2026-09-21T11:09:28Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-t6db","title":"grid_in ignored prune_empty for a non-active view, hiding a staleness bug","description":"FIXED in PR #78; filed because the FINDING is worth more than the fix and because the shape has now recurred three times.\n\nproject::grid_in built a layout for a view the host is NOT on with GridLayout::new, which does not apply prune_empty. So a client on another view was projected rows its own view config prunes. Measured on a two-view fixture where both views prune and one row is empty:\n\n Second via grid_in (GridLayout::new) 3 rows: North, Service, South\n Second via with_frozen_records (pruned) 2 rows: North, South\n\nOne line. It broke no other test, which is itself evidence nothing depended on the unpruned behaviour.\n\nWHY IT MATTERS MORE THAN ITS SIZE -- IT WAS A BUG HIDING A BUG. The adversarial review of #78 found a P1: after improvise-mcq4 narrowed the cursor fingerprint to the active view, a CellData write that prunes a row out of ANOTHER client's view stopped invalidating that client. Writing the regression test for it FAILED AT THE PRECONDITION -- the bystander's projected grid never lost a row, because grid_in never pruned it. The staleness was real and logically reachable, and completely unobservable through the projection. Fixing grid_in is what made the P1 demonstrable, and the P1 test is what found grid_in.\n\nTHIRD INSTANCE OF ONE TRAP: GridLayout::new does not prune; GridLayout::with_frozen_records does. Previously bitten by App::view_frame (improvise-ni6k, caught by a failing test) and by the reshape benchmark's geometry fixture (improvise-mcq4, caught only by measuring). A constructor whose name suggests it is the general one, and which silently omits a view setting, is worth a look on its own terms -- either new should prune, or the names should say which is which.\n\nSTILL OPEN AROUND IT, not fixed here: nothing asserts that a client on a non-active view is projected a grid matching ITS OWN view config in general. prune_empty is one setting; the rest is a guarantee by inspection rather than by test.\n","status":"closed","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T05:00:08Z","created_by":"claude-agent-protocol","updated_at":"2026-09-21T12:39:35Z","closed_at":"2026-09-21T12:39:35Z","dependency_count":0,"dependent_count":0,"comment_count":0} @@ -79,7 +80,7 @@ {"_type":"issue","id":"improvise-yqpd","title":"Transactional rollback: a frame of mutations applies whole or not at all (ADR 001 clause 4)","description":"Clause 4 of the transaction contract the user specified 2026-09-20. Clauses 1-3 landed in PR #26; this is the half that was deliberately deferred with a costed design note.\n\nTHE CONTRACT, in the user's own words: \"a sequence of commands sent to the server should assume that it's played in order from start to end and that each command sees the state after the previous command runs\", as \"a single transaction from a given base\". Four clauses:\n\n 1. A frame states ONE basis, describing the base the client composed against. [DONE, PR #26]\n 2. Checked ONCE, before anything is applied. [DONE, PR #26]\n 3. Applied IN ORDER, each command seeing what its predecessor left. [DONE, PR #26]\n 4. ALL OR NOTHING -- a failure leaves the document exactly as the base described. [THIS BEAD]\n\nWHERE THE MECHANISM BELONGS: App::apply_effects, NOT Host::commands. A boundary drawn around a sequence of COMMANDS gets redrawn the day one command becomes a sequence -- and the user has said a command could eventually be \"run script\", which makes an unbounded number of mutations internally. So arity is not a property of a variant (doctrine(arity_is_not_a_property_of_a_variant)): a classifier can honestly say WHETHER a command changes the document, never HOW MANY TIMES. Design at the one mutation site and let both the frame case and the future script case use it.\n\nTHE CORE PROBLEM IN ONE LINE: abort_effects stops a batch and UNDOES NOTHING. That is an abort, not a rollback (gotcha 21), and clause 4 needs the latter. Two live examples sit in main today: a failed Save, and AddCategory refusing on MAX_CATEGORIES (PR #27).\n\nTHE DESIGN NOTE FROM PR #26, whose arguments should not be re-derived:\n * SNAPSHOT AND RESTORE, not speculative validation and not inverse effects. Speculation fails a concrete divergence argument: AcceptWizardModel calls take_built(), which CONSUMES the operand a real run would need. Inverse effects degenerate -- the general inverse of a document-replacing effect IS a snapshot -- so they do not avoid the clone, they multiply the surface by ~50.\n * THE FINGERPRINTS CANNOT BE AN UNDO LOG. The per-record XOR (gotcha 40) is a change DETECTOR, not invertible, and covers CellData only.\n * THE BOUNDARY IS THE DOCUMENT AND ITS TIERS, NOT ALL App STATE. AddCategory writes its refusal into view_state.status_msg and THEN aborts, so rolling that back would undo the explanation along with the damage.\n * WHY ROLLBACK IS SOUND AT ALL, structurally rather than by a flag: the three storage-WRITING effects (Save, SaveAs, ExportCsv) are all ViewOnly and unreachable from ModelCommand; LoadModel and ImportJsonHeadless only read. That is testable, and it becomes an acceptance criterion for any future RunScript -- a script language that can run :w breaks it.\n\nTWO ACCEPTANCE CRITERIA THAT CAME OUT OF THE P1 THE REVIEWER FOUND, and they are not obvious:\n * THE CHECKS MUST STAY BEFORE ANY APPLICATION. A frame that writes ONE RECORD TWICE is legitimate and likely -- two measures on one record IS one record -- and it works only because the second write is compared to the record's PRE-FRAME counter rather than the one its own sibling just bumped. Proved by injection: moving the record check into the apply loop fails that arm. `a_later_write_in_a_chain_still_answers_for_its_own_record` arm 3 is the guard.\n * CLAUSE 4'S ROLLBACK IS NOT THE RECORD TIER. They fail on DISJOINT frames, which is exactly why the P1 could not be deferred into this bead: rollback never fires on a frame where every command SUCCEEDS, and the lost update was a wrong answer fully applied.\n\nTHE TRADE NOBODY HAD NAMED, and the reason arm 3 exists: INTERLEAVING THE RECORD CHECK WITH THE APPLIES WOULD CLOSE THIS BEAD'S SILENCE FOR FREE. [RemoveItem(North), SetCell(North)] under interleaving is refused RecordGone instead of silently half-applying. So check-before-apply is not a free win -- it BUYS the legitimate double-write and SELLS the loud failure on a broken chain. Clause 4 must pay that back some other way, and arm 3 is what stops someone taking the trade without noticing they made it. Confirmed by two independent injections (the author's and the reviewer's, which differ in detail and agree on the direction).\n\nTHE BIGGEST AND LEAST OBVIOUS COST: ROLLING BACK THE App IS NOT ENOUGH. Host::command projects PER COMMAND, and apply_switch/apply_model adopt mirrors inline, so by the time a later command fails every session's mirror and believed grid cache have already moved. Deferring projection and relay to commit is the right fix and is also a bandwidth win -- but it changes Applied and restructures project.rs, so it is a merge-order problem as well as a size one.\n\nPROTOCOL MOVE THAT BELONGS HERE: clause 1 says the FRAME states one basis, but the wire still carries a Basis inside each Command::Model -- right when one command was one transaction. Later commands' Basis fields are currently IGNORED. Moving it to ClientFrame::Commands makes \"one basis per frame\" unrepresentable-otherwise instead of a convention.\n\nALSO FOLD IN: the `answered`/`frame_basis` classifier is a matches! whose failure direction is safe (a missed variant over-checks, never under-checks) -- make it exhaustive when the classifier returns, for the reason mutates_the_document was made exhaustive before it was deleted.\n\nESTIMATED COST from the design note: ~350-450 lines of production and as much again in tests, across app.rs, reduce.rs, project.rs, host.rs, improvise-protocol and an ADR section.\n\nINVERT `a_frame_whose_later_mutation_fails_is_not_yet_undone` when this lands. It currently characterizes the hole; all three of its assertions flip.\n","acceptance_criteria":"A ClientFrame::Commands whose later command fails leaves the document exactly as its stated basis described. The mechanism sits at or near App::apply_effects, not Host::commands, so a future multi-mutation command (RunScript) uses the same machinery. The checks stay BEFORE any application — a frame writing one record twice is legitimate and refuses itself otherwise, which a_later_write_in_a_chain_still_answers_for_its_own_record arm 3 guards. Rolling back the App is not enough: Host::command projects per command, so mirrors and believed grid caches must be deferred to commit. a_frame_whose_later_mutation_fails_is_not_yet_undone inverts. And the Basis moves from Command::Model to ClientFrame::Commands, making one-basis-per-frame unrepresentable-otherwise.","status":"open","priority":2,"issue_type":"feature","owner":"el-github@elangley.org","created_at":"2026-09-20T07:29:16Z","created_by":"Edward Langley","updated_at":"2026-09-20T07:45:26Z","labels":["area:protocol"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-0o6x","title":"A group header first on its axis is never drawn, so a leading collapsed group has no way back","description":"Found by the independent adversarial review of PR #23, which calls it \"the most valuable thing I found that nobody named\". PRE-EXISTING and correctly characterized, but improvise-anr9 changes what it costs.\n\nTHE BEHAVIOUR: body_from(idx) starts at the first ITEM, so at row_offset == 0 a leading FrameEntry::Group is skipped. GROUPED_FRAME shows it plainly -- the group header \"Coastal\" above \"East\" is simply not drawn -- and that constant is BYTE-IDENTICAL to main, so the TUI has always behaved this way and the characterization is honest.\n\nWHY IT MATTERS MORE NOW: a COLLAPSED group that is first on its axis has NO HEADER ANYWHERE ON SCREEN, in either front end, and therefore NO WAY BACK. Its rows are hidden and the only affordance for unhiding them is the header that is not drawn.\n\nAND IT SITS EXACTLY WHERE THE WINDOWING RULE SAYS IT MUST NOT. improvise-anr9's axis_frame carries ALL headers between two in-window items specifically because \"a collapsed group's header is the only thing on screen the user can click to get its rows back\". That clause is correct and the rule is right. Position zero is the one place the rule does not reach -- not because the frame omits the header (it does not) but because body_from drops it on the way to the screen.\n\nTHE AUTHOR KNEW, WHICH IS WHY THIS IS FILED RATHER THAN REDISCOVERED: tests/grid_frame_agreement.rs deliberately orders its fixture with \"Carol\" before \"Family\" to dodge the case, and says so in a comment. It was not in the PR body and no ticket was cited, so the knowledge lived in one comment in one test.\n\nFILE THIS BEFORE ANYONE BUILDS GROUP INTERACTION IN THE BROWSER. web/app.js's group \u003cth\u003e has no click listener today and `toggle-group-under-cursor` is Upstream -- a black hole, doctrine(an_upstream_key_is_a_black_hole) -- so the affordance does not exist in the browser at all yet. The moment it does, a leading collapsed group becomes a trap: rows hidden, nothing to click.\n\nWHAT THE FIX TOUCHES, so nobody starts casually: body_from/visual_of decide what a renderer paints from a window offset. Drawing a leading header changes the row count of the painted body, which moves the exact-frame characterization buffers in src/ui/grid.rs -- so this is a deliberate re-pinning with the reason in the commit message, the same acceptance clause improvise-q0vb had. Check the interaction with the windowing rule's first clause (the nearest header ABOVE the window is carried for group_of and discarded by body_from): at offset zero there is no \"above\", so the leading header is an in-window entry that body_from is discarding, which may make the fix a one-line boundary condition rather than a redesign. Establish which before estimating.\n\nRELATED: improvise-plcx (the harness needs a grouped, multi-level fixture) is what would put this in front of a browser at all.\n\n## Steps to Reproduce\n\n1. Build a view whose row axis has a group as its FIRST entry — e.g. a category whose first item belongs to group `Coastal`.\n2. Open it in the TUI at `row_offset == 0`.\n3. The `▼ Coastal` header is not drawn at all: `body_from(0)` starts at the first ITEM and skips the leading `FrameEntry::Group`. `GROUPED_FRAME` in `src/ui/grid.rs` pins this, byte-identical to pre-anr9.\n4. Now collapse that group. Its rows disappear and its header is still not drawn — so there is nothing on screen to act on, in either front end.\n","acceptance_criteria":"A group header that is first on its axis is drawn, in the TUI and in the browser, so a collapsed group at position zero has an affordance. The exact-frame characterization buffers in src/ui/grid.rs are re-pinned DELIBERATELY, with the commit message naming which moved and why — the improvise-q0vb acceptance shape. Establish first whether this is a boundary condition in body_from rather than a redesign: at offset zero there is no 'above', so the leading header is an in-window entry body_from is discarding.","status":"closed","priority":2,"issue_type":"bug","assignee":"agent-render-frame","owner":"el-github@elangley.org","created_at":"2026-09-20T07:26:33Z","created_by":"Edward Langley","updated_at":"2026-09-21T03:46:31Z","started_at":"2026-09-21T03:39:55Z","closed_at":"2026-09-21T03:46:31Z","close_reason":"Fixed on PR #77 (fix/render-frame-wave, head 8f3a976). Each guard proved by injecting the fault it names; GROUPED_FRAME and harness/tests/grouped.spec.js re-pinned deliberately.","labels":["area:browser"],"dependencies":[{"issue_id":"improvise-0o6x","depends_on_id":"improvise-plcx","type":"relates-to","created_at":"2026-09-20T07:26:35Z","created_by":"Edward Langley","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-plcx","title":"The browser harness needs a grouped, multi-level fixture — the demo has neither","description":"Named by improvise-anr9 (PR #23) as \"the highest-value follow-up in this hand-back\", and it is a gap in the NEW harness rather than in the old blind spot.\n\nmain now carries harness/tests/shell.spec.js (improvise-vvl3), which renders web/app.js in Chromium, Firefox and WebKit and asserts the grid draws, a header reads \"Revenue\", an editable cell exists, and click-to-edit commits and shows the host's value. It PASSED on PR #23's merged head, which means the DOM rewrite that PR shipped is genuinely browser-verified.\n\nBUT ONLY ON THE SIMPLE PATH. web/demo.improv has NO GROUPS AND ONE CATEGORY PER VISIBLE AXIS -- deliberately, per gotcha 46, because before improvise-anr9 a second category on an axis put the slash-join gap on the front page. That is no longer a reason, and it is now the harness's blind spot: the multi-level header, group-span, collapse-indicator and inline-group-row paths of gridTable are EXACTLY the code improvise-anr9 added, and the harness never exercises any of them.\n\nWHAT IT NEEDS: a second .improv served by the harness -- not a change to web/demo.improv, which is the shipped front page and is correct as it is -- carrying at least two categories on one axis, a group with a couple of members, a collapsed group, and enough rows to scroll. Then assertions that the DOM has real header levels rather than slash-joined labels, that a group header spans its members, that the collapse indicator renders, and that an inline group row carries no cells (RenderRow::Group exists so it cannot, which is worth asserting from the outside).\n\nWHY IT IS WORTH DOING SOON: it would be the FIRST TEST IN THIS REPOSITORY able to see that code in a browser, and improvise-anr9's own hand-back says so. It also unblocks the sibling scrollIntoView bead, which needs a document big enough to scroll.\n\nNOTE THE STALE-DOCUMENTATION LESSON THAT CAME WITH IT, recorded because it cost real caution: improvise-anr9 wrote \"web/app.js is never executed here\" into three comments and a PR body on the strength of gotcha 42 -- while a harness that renders it in three browsers had ALREADY LANDED on main and was passing on its own code. Gotcha 48 says a fact stated in several places is a grep. This is the version where the stale copy is the one telling you what you CANNOT test, and it makes you under-claim your own evidence.\n","acceptance_criteria":"The harness serves a SECOND .improv (not web/demo.improv, which is the shipped front page and correct as it is) carrying at least two categories on one axis, a group with several members, a collapsed group, and enough rows to scroll. Specs assert: real header levels rather than slash-joined labels; a group header spanning its members; the collapse indicator rendering; and an inline group row carrying no cells (RenderRow::Group makes that unconstructible, which is worth asserting from outside). Each assertion is proved discriminating before it is kept.","notes":"PR #72. harness/fixtures/grouped.improv + harness/tests/grouped.spec.js, five injections. Note: the leading group header (Coastal) is never drawn -- improvise-0o6x observed in the wild; the spec asserts 2 group rows and says so.","status":"closed","priority":2,"issue_type":"task","assignee":"wave4-harness","owner":"el-github@elangley.org","created_at":"2026-09-20T07:16:28Z","created_by":"Edward Langley","updated_at":"2026-09-21T00:24:05Z","started_at":"2026-09-20T23:21:56Z","closed_at":"2026-09-21T00:24:05Z","close_reason":"Fixed and merged: rmvx/fdt6/gcvb in PR #71, the harness four in PR #72. Both adversarially reviewed, with each finding's injection re-run against the fix. Closed by the orchestrator with --force because the claiming agent has stood down; the claim guard refusing an unforced close is the mechanism working, not a problem.","labels":["area:browser"],"dependencies":[{"issue_id":"improvise-plcx","depends_on_id":"improvise-0o6x","type":"relates-to","created_at":"2026-09-20T07:26:35Z","created_by":"Edward Langley","metadata":"{}"}],"dependency_count":0,"dependent_count":2,"comment_count":0} -{"_type":"issue","id":"improvise-sihr","title":"Every minibuffer mode entered from records mode exits to plain Normal while the layout is still records","description":"Found by Copilot on PR #27 against the new Ctrl-W save prompt, and DECLINED there with evidence because fixing the one arm the finding named would have been one arm of a three-arm rule.\n\nTHE BEHAVIOUR: every minibuffer mode entered from RecordsNormal exits to plain Normal while the LAYOUT IS STILL RECORDS. It affects `:` (CommandMode), Ctrl-E (ExportPrompt) and Ctrl-W (SavePrompt, new in PR #27). Only the last is new; the class predates it.\n\nTHE MECHANISM: the exit mode is a CONSTANT, baked into TextEntry::new(.., \"normal\") in the keymap builder and into each commit command's ChangeMode. Nothing carries where the user came FROM, so nothing can put them back.\n\nWHAT IS NOT THE MECHANISM, checked rather than reasoned (doctrine(binding_dump_diff)): the binding TABLE is fine. RecordsNormal goes 155 -\u003e 156 dumped lines across PR #27, the single addition being Char('w')+CONTROL -\u003e enter-mode [\"save-prompt\"], with nothing removed. with_parent inheritance (gotcha 13) is intact. What is lost is the RUNTIME MODE, not a binding.\n\nCHARACTERIZED, NOT FIXED: every_minibuffer_mode_entered_from_records_returns_to_plain_normal drives all three and asserts both halves -- the mode is Normal, and the layout is still records. So the current behaviour is pinned and a fix will have to update it deliberately.\n\nTHE FIX, and it is why this is its own bead: text-entry modes must carry their entry mode. That is a change to TextEntry, to AppMode (the exit is part of a mode's configuration, so it likely belongs beside MinibufferConfig -- note gotcha 11, MinibufferConfig::buffer_key is a String and not \u0026'static str precisely because ViewState must stay Deserialize-able, and the same constraint applies to anything added there), and to three commit commands. Wider than the PR that found it.\n\nWATCH TWO THINGS:\n * The exit is currently spelled in TWO places per mode -- the TextEntry spine's Esc binding and the commit command's ChangeMode. If they are made to carry an entry mode, make them read ONE source or the two will disagree on cancel versus commit.\n * SearchMode is hand-written on purpose and has no commit key (gotcha 11). It is not part of this class; do not sweep it in.\n\nRELATED: improvise-mai9 (a status message written while a minibuffer is open is never drawn) is the other half of why these modes are awkward -- both are consequences of the minibuffer owning the bottom row and nothing tracking what it displaced.\n\n## Steps to Reproduce\n\n1. Enter records mode (`R`).\n2. Press `:`, or `Ctrl-E`, or `Ctrl-W` — any minibuffer mode.\n3. Commit or cancel.\n4. The app is now in plain `Normal` while the LAYOUT is still records: the exit mode is a constant baked into `TextEntry::new(.., \"normal\")` and into each commit command's `ChangeMode`, and nothing carries where the user came from.\n\n`every_minibuffer_mode_entered_from_records_returns_to_plain_normal` drives all three and asserts both halves.\n","acceptance_criteria":"A minibuffer mode entered from RecordsNormal returns to RecordsNormal. Text-entry modes carry their entry mode rather than a constant 'normal'. The exit is currently spelled TWICE per mode — the TextEntry spine's Esc binding and the commit command's ChangeMode — so both read ONE source or they will disagree on cancel versus commit. SearchMode is hand-written on purpose and is not part of this class. every_minibuffer_mode_entered_from_records_returns_to_plain_normal is updated deliberately, not deleted.","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-20T06:12:20Z","created_by":"Edward Langley","updated_at":"2026-09-20T07:57:17Z","labels":["area:tui"],"dependencies":[{"issue_id":"improvise-sihr","depends_on_id":"improvise-mai9","type":"relates-to","created_at":"2026-09-20T06:12:22Z","created_by":"Edward Langley","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"improvise-sihr","title":"Every minibuffer mode entered from records mode exits to plain Normal while the layout is still records","description":"Found by Copilot on PR #27 against the new Ctrl-W save prompt, and DECLINED there with evidence because fixing the one arm the finding named would have been one arm of a three-arm rule.\n\nTHE BEHAVIOUR: every minibuffer mode entered from RecordsNormal exits to plain Normal while the LAYOUT IS STILL RECORDS. It affects `:` (CommandMode), Ctrl-E (ExportPrompt) and Ctrl-W (SavePrompt, new in PR #27). Only the last is new; the class predates it.\n\nTHE MECHANISM: the exit mode is a CONSTANT, baked into TextEntry::new(.., \"normal\") in the keymap builder and into each commit command's ChangeMode. Nothing carries where the user came FROM, so nothing can put them back.\n\nWHAT IS NOT THE MECHANISM, checked rather than reasoned (doctrine(binding_dump_diff)): the binding TABLE is fine. RecordsNormal goes 155 -\u003e 156 dumped lines across PR #27, the single addition being Char('w')+CONTROL -\u003e enter-mode [\"save-prompt\"], with nothing removed. with_parent inheritance (gotcha 13) is intact. What is lost is the RUNTIME MODE, not a binding.\n\nCHARACTERIZED, NOT FIXED: every_minibuffer_mode_entered_from_records_returns_to_plain_normal drives all three and asserts both halves -- the mode is Normal, and the layout is still records. So the current behaviour is pinned and a fix will have to update it deliberately.\n\nTHE FIX, and it is why this is its own bead: text-entry modes must carry their entry mode. That is a change to TextEntry, to AppMode (the exit is part of a mode's configuration, so it likely belongs beside MinibufferConfig -- note gotcha 11, MinibufferConfig::buffer_key is a String and not \u0026'static str precisely because ViewState must stay Deserialize-able, and the same constraint applies to anything added there), and to three commit commands. Wider than the PR that found it.\n\nWATCH TWO THINGS:\n * The exit is currently spelled in TWO places per mode -- the TextEntry spine's Esc binding and the commit command's ChangeMode. If they are made to carry an entry mode, make them read ONE source or the two will disagree on cancel versus commit.\n * SearchMode is hand-written on purpose and has no commit key (gotcha 11). It is not part of this class; do not sweep it in.\n\nRELATED: improvise-mai9 (a status message written while a minibuffer is open is never drawn) is the other half of why these modes are awkward -- both are consequences of the minibuffer owning the bottom row and nothing tracking what it displaced.\n\n## Steps to Reproduce\n\n1. Enter records mode (`R`).\n2. Press `:`, or `Ctrl-E`, or `Ctrl-W` — any minibuffer mode.\n3. Commit or cancel.\n4. The app is now in plain `Normal` while the LAYOUT is still records: the exit mode is a constant baked into `TextEntry::new(.., \"normal\")` and into each commit command's `ChangeMode`, and nothing carries where the user came from.\n\n`every_minibuffer_mode_entered_from_records_returns_to_plain_normal` drives all three and asserts both halves.\n","acceptance_criteria":"A minibuffer mode entered from RecordsNormal returns to RecordsNormal. Text-entry modes carry their entry mode rather than a constant 'normal'. The exit is currently spelled TWICE per mode — the TextEntry spine's Esc binding and the commit command's ChangeMode — so both read ONE source or they will disagree on cancel versus commit. SearchMode is hand-written on purpose and is not part of this class. every_minibuffer_mode_entered_from_records_returns_to_plain_normal is updated deliberately, not deleted.","status":"closed","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-20T06:12:20Z","created_by":"Edward Langley","updated_at":"2026-09-21T17:09:48Z","closed_at":"2026-09-21T17:09:48Z","labels":["area:tui"],"dependencies":[{"issue_id":"improvise-sihr","depends_on_id":"improvise-mai9","type":"relates-to","created_at":"2026-09-20T06:12:22Z","created_by":"Edward Langley","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-kc7n","title":"improvise-ws-server never checks Origin, so any page a user visits can drive their session","description":"Found by Copilot on PR #21 round 5, against a reverse-proxy recipe whose IP allow-list had itself been added two rounds earlier to close a DIFFERENT hole. Verified independently by the orchestrator: `grep -rni origin crates/improvise-ws-server/src/` returns NOTHING. The server never inspects the Origin header and accepts every handshake.\n\nTHE HOLE: a WebSocket is EXEMPT FROM THE SAME-ORIGIN POLICY -- there is no CORS preflight on a handshake. So any hostile page that an allowed user visits can open a socket to the endpoint FROM THAT USER'S OWN ALLOWED ADDRESS, and read and edit every document the server holds. That is cross-site WebSocket hijacking.\n\nWHY NETWORK CONTROLS CANNOT SEE IT: an IP allow-list, a private network, loopback binding -- all answer \"may this CLIENT reach the endpoint\", and in this attack the answer is genuinely yes. The request really does come from an allowed client; it is simply being DRIVEN by a page the user did not intend to trust. Only Origin separates the two.\n\nLOOPBACK IS NOT EXEMPT, and this is the half a documentation-only fix cannot reach. In the local pairing case there is no proxy at all, so there is nothing in front to check Origin -- and loopback is not a boundary a browser enforces. While the server runs, ANY page open in that browser can reach ws://127.0.0.1:8787. So the local development path, which is the one the project actually recommends, is the least protected.\n\nDISTINCT FROM improvise-kpnr, and both are needed. kpnr is AUTHENTICATION: who is allowed to talk to this server at all. This is CSWSH: given that someone is allowed, what may put words in their mouth. An answer to either leaves the other open.\n\nTHE FIX IS CHEAP AND SERVER-SIDE: an allow-list of permitted origins checked at the handshake, defaulting to something safe for the local case (the page's own origin, and whatever --bind implies), with a flag to widen it. Refuse the upgrade rather than accepting and ignoring.\n\nTWO THINGS TO GET RIGHT:\n * A MISSING Origin header is not the same as a disallowed one. Non-browser clients (the repo's own tests, curl, a native client) send none, and the browser always does. Decide explicitly what absent means rather than letting the match arm decide by accident.\n * PR #21 has already documented Origin filtering in the PROXY as a required companion to the network control, phrased as \"in addition to, never instead of\". When this lands, the README should say the server does it too, and the proxy line becomes defence in depth rather than the only defence.\n\nTHE GENERAL LESSON, now doctrine(who_may_connect_is_two_questions): ask of any access control whether it constrains the CLIENT or what may DRIVE the client. Three consecutive rounds of that PR reasoned about the page's capabilities and none about the attacker's.\n\n## Steps to Reproduce\n\n1. Run `improvise-ws-server` on any document.\n2. From a browser that is allowed to reach it — the user's own machine, or any address an IP allow-list permits — visit an unrelated page that opens `new WebSocket(\"ws://127.0.0.1:8787\")`.\n3. The handshake succeeds. A WebSocket is exempt from the same-origin policy, so there is no CORS preflight and `grep -rni origin crates/improvise-ws-server/src/` returns nothing — the server never looks.\n4. That page can now read and edit every document the server holds, from the user's own allowed address.\n\nLoopback is not exempt and has no proxy in front of it, so the local development path is the least protected.\n","acceptance_criteria":"improvise-ws-server refuses a handshake whose Origin is not allow-listed, defaulting to something safe for the local case with a flag to widen it. A MISSING Origin is decided explicitly rather than by accident — non-browser clients (the repo's own tests, curl, a native client) send none and browsers always do. The README's proxy-side Origin filtering becomes defence in depth rather than the only defence.","status":"closed","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-20T06:09:42Z","created_by":"Edward Langley","updated_at":"2026-09-21T04:33:43Z","closed_at":"2026-09-21T04:33:43Z","close_reason":"Closed","labels":["area:browser"],"dependencies":[{"issue_id":"improvise-kc7n","depends_on_id":"improvise-kpnr","type":"relates-to","created_at":"2026-09-20T06:09:49Z","created_by":"Edward Langley","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-mai9","title":"A status message written while a minibuffer is open is never drawn","description":"Found by WU-G while building improvise-lae (PR #27), and CONFIRMED THE HARD WAY in the same PR: Copilot then found a regression that this mechanism causes, which the agent had itself written down an hour earlier and not checked its own change against.\n\nTHE MECHANISM: src/draw.rs::draw_bottom_bar renders the minibuffer INSTEAD OF the status line. So a status_msg written while a minibuffer is open is never drawn at all -- not truncated, not overwritten, simply never rendered.\n\nWHO IS AFFECTED TODAY:\n * CategoryAdd / ItemAdd: a failed `commit-buffer category` is INVISIBLE. Worse, improvise-6t3 deliberately ordered that command's effects so the add effect is emitted LAST, specifically so its failure message would WIN over the command's optimistic status. It wins a race to a surface nobody can see.\n * A failed `:w \u003cpath\u003e` from CommandMode, which is the regression Copilot found on PR #27: the abort stops the batch before ReturnToNormalUnlessMoved and clear-buffer run, so the app stays in CommandMode with the stale command in the minibuffer and the new error hidden behind it.\n\nTHE WORKAROUND ALREADY SHIPPED, and it is a workaround: CommitSave emits [ChangeMode(Normal), SaveAs] so the error lands when the bottom row is the status bar again. That works for one command and does not generalise -- every other text-entry commit has the same hole.\n\nTWO CANDIDATE FIXES, and the choice is the ticket:\n * Draw both: give the minibuffer and the status line separate rows, or render the status into the minibuffer row when a message is pending. Cheap to describe, and it changes frame geometry, which is pinned by exact-frame characterization tests (see improvise-q0vb for what that costs).\n * Make it unrepresentable: a status message set while a minibuffer is open is a bug in the CALLER, so make the caller unable to do it -- e.g. commit paths always leave the mode before reporting. That is what CommitSave now does by hand, promoted to a rule.\n\nPREFER WHICHEVER MAKES THE FAILURE LOUD. The current state is the quiet kind: the code sets a message, the test asserts the message was set, and the user never sees it. A test asserting `app.status_msg == \"...\"` passes in every one of these cases, which is why this survived improvise-6t3's careful effect ordering.\n\n## Steps to Reproduce\n\n1. Enter a text-entry mode that opens a minibuffer — `CategoryAdd` (`N`) is the clearest.\n2. Commit something that fails, so the command sets a status message.\n3. Nothing appears: `src/draw.rs::draw_bottom_bar` renders the minibuffer INSTEAD OF the status line, so the message is never drawn at all — not truncated, not overwritten.\n4. A test asserting `app.status_msg == \"...\"` passes, which is why this survived improvise-6t3's careful effect ordering.\n\nSecond path, found on PR #27: a failed `:w /bad/path` aborts before `ReturnToNormalUnlessMoved`, leaving the app in `CommandMode` with the stale command drawn and the error hidden behind it.\n","acceptance_criteria":"A status message set while a minibuffer is open reaches the user. Either the two share the bottom row, or a commit path is structurally unable to report while its minibuffer is open (which is what CommitSave now does by hand, promoted to a rule). Whichever is chosen, the failure becomes LOUD: a test asserting app.status_msg was set must no longer pass while the user sees nothing.","status":"closed","priority":2,"issue_type":"bug","assignee":"agent-render-frame","owner":"el-github@elangley.org","created_at":"2026-09-20T06:04:20Z","created_by":"Edward Langley","updated_at":"2026-09-21T03:46:31Z","started_at":"2026-09-21T03:39:56Z","closed_at":"2026-09-21T03:46:31Z","close_reason":"Fixed on PR #77 (fix/render-frame-wave, head 8f3a976). Each guard proved by injecting the fault it names; GROUPED_FRAME and harness/tests/grouped.spec.js re-pinned deliberately.","labels":["area:tui"],"dependencies":[{"issue_id":"improvise-mai9","depends_on_id":"improvise-sihr","type":"relates-to","created_at":"2026-09-20T06:12:22Z","created_by":"Edward Langley","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-kpnr","title":"improvise-ws-server has no authentication, and the documented sharing path now exposes it","description":"Found by Copilot on PR #21 (improvise-o8fa, the reverse-proxy documentation), and it is a finding ABOUT the documentation that PR introduced rather than about code.\n\nimprovise-ws-server has NO AUTHENTICATION BY DESIGN. crates/improvise-ws-server/src/transport.rs says it outright: anyone who can open a socket to the port can rewrite every document it serves. The only control today is that it binds loopback.\n\nTHE README NOW TELLS USERS TO PUT A PUBLIC TLS PROXY IN FRONT OF IT (improvise-o8fa, per the user's ruling that the reverse proxy is the supported path to reaching a TUI-hosted document from an https origin). That instruction REMOVES THE ONLY CONTROL. TLS is confidentiality, not access control, and in a copy-pasteable recipe the two read alike.\n\nSo proxy-side auth is now LOAD-BEARING DOCUMENTATION rather than a server feature. PR #21 mitigated it -- the warning leads the section, the Caddyfile carries basic_auth marked not-optional, the --bind note now separates what loopback buys from what it does not, and the tunnel line says cloudflared/ngrok print public URLs and authenticate nothing -- but mitigation in prose is not the same as a property of the system.\n\nTHE DECISION TO MAKE, and it is a real fork rather than an oversight to correct:\n * The server grows a shared secret or token, checked at the websocket handshake. Small, and it makes the documented path safe by default rather than safe if followed.\n * Or \"auth is the proxy's job\" is the PERMANENT answer, stated in the server's own docs as a design position rather than left as an absence. Defensible -- it is what most single-binary dev servers do -- but then the README recipe is the security boundary and should say so in those words.\n\nDO NOT SPLIT THE DIFFERENCE. A token that is optional and off by default is the worst of both: it reads as a feature and defends nothing.\n\nNOTE THE GENERALISABLE PART, worth keeping whichever way this goes: DOCUMENTATION THAT INSTRUCTS AN ACTION CARRIES THAT ACTION'S CONSEQUENCES. The PR #21 section was scrupulous about one axis (build complexity, the cost of a second binary to a one-binary tool) and blind on another (exposure), and the --bind paragraph actively REASSURED the reader -- \"not reachable from anywhere else\", true of the port and false of the deployment. Neither the ticket, nor gotcha 48, nor the agent's own /code-review pass caught it; a reviewer reading the server's source did.\n\n## Rationale\n\nimprovise-ws-server has no authentication by design and `transport.rs` says so outright: anyone who can open a socket to the port can rewrite every document it serves. The only control was loopback binding — and improvise-o8fa's README now instructs users to put a public TLS proxy in front of it, which REMOVES exactly that control. TLS is confidentiality, not access control, and in a copy-pasteable recipe the two read alike.\n\nSo proxy-side auth is now load-bearing documentation rather than a property of the system. PR #21 mitigated it in prose; mitigation in prose is not the same as a mechanism.\n\n## Alternatives Considered\n\n**A shared secret or token checked at the handshake.** Small, and it makes the documented path safe by default rather than safe-if-followed. Note a URL query parameter is the one credential `new WebSocket(url)` can carry — the API sets no headers.\n\n**\"Auth is the proxy's job\" as a stated permanent position**, written into the server's own docs as a design decision rather than left as an absence. Defensible — it is what most single-binary dev servers do — but then the README recipe IS the security boundary and should say so in those words.\n\n**Rejected: a token that is optional and off by default.** The worst of both: it reads as a feature and defends nothing.\n","status":"open","priority":2,"issue_type":"decision","owner":"el-github@elangley.org","created_at":"2026-09-20T05:45:28Z","created_by":"Edward Langley","updated_at":"2026-09-20T07:45:40Z","labels":["area:browser"],"dependencies":[{"issue_id":"improvise-kpnr","depends_on_id":"improvise-kc7n","type":"relates-to","created_at":"2026-09-20T06:09:49Z","created_by":"Edward Langley","metadata":"{}"}],"dependency_count":0,"dependent_count":0,"comment_count":0} @@ -199,12 +200,12 @@ {"_type":"issue","id":"improvise-yk4","title":"Phase 2: README and demo artifacts","description":"The main launch work. README is 80% of the launch, demo artifacts are 20%. Covers Nix tooling for asciinema/VHS, writing the README, creating demo GIF and asciinema casts, and verifying all artifacts.","status":"closed","priority":2,"issue_type":"feature","assignee":"spot","owner":"el-github@elangley.org","created_at":"2026-04-09T04:05:49Z","created_by":"Edward Langley","updated_at":"2026-04-14T07:47:43Z","closed_at":"2026-04-14T07:47:43Z","close_reason":"All 5 subtasks closed; artifacts verified present and tracked in git (README.md, docs/demo.gif, docs/demo.tape, docs/casts/*.cast).","dependencies":[{"issue_id":"improvise-yk4","depends_on_id":"improvise-abz","type":"blocks","created_at":"2026-04-08T23:37:37Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-yk4","depends_on_id":"improvise-d4w","type":"blocks","created_at":"2026-04-08T23:37:36Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-yk4","depends_on_id":"improvise-ihv","type":"blocks","created_at":"2026-04-08T23:37:34Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-yk4","depends_on_id":"improvise-odx","type":"blocks","created_at":"2026-04-08T23:37:35Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-yk4","depends_on_id":"improvise-pby","type":"blocks","created_at":"2026-04-08T23:37:36Z","created_by":"Edward Langley","metadata":"{}"}],"dependency_count":5,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-hsoe","title":"check-evidence cannot see a duplicate heading, and three exist","description":"Found by the Wave I agent while merging PR #92, which had accumulated TWO entries headed `### a_window_narrows_the_cells_and_neither_the_totals_nor_the_widths` — the original, describing a fixture that no longer existed and repeating a claim the adversarial review had measured false, plus the superseding one. A reader hitting the first would have taken it as current.\n\n--check-evidence CANNOT SEE THIS. It verifies that a heading names a test that runs, and a duplicate heading names one just as well. Both copies pass.\n\n## Three already on main, verified 2026-09-21\n ### a_layout_change_moves_the_layout_version_and_no_record (test-evidence/command.md)\n ### the_two_front_ends_draw_the_same_column_header (test-evidence/tui.md)\n ### the_whole_frame_is_unchanged (test-evidence/tui.md)\n\nReproduce:\n grep -rhoE '^### [A-Za-z_:][A-Za-z_0-9:]*$' context/test-evidence/*.md | sort | uniq -d\n\n## Why it matters beyond tidiness\ncontext/test-evidence records WHY a test can fail. Two entries under one heading means one of them is stale by construction — they cannot both describe the current test — and nothing indicates which. That is the same class as an evidence entry whose mechanism measurement contradicts (doctrine an_evidence_entry_that_measurement_contradicts_is_worse_than_none): a reader gets a confident wrong answer rather than no answer.\n\n## Acceptance Criteria\n- --check-evidence refuses a duplicate heading, naming the file and both line numbers. It should refuse rather than warn, per the precedent set for ambiguous leaf names in improvise-rm4s.\n- Prove it by injection: add a second copy of an existing heading and watch the checker fail by name.\n- Resolve the three above. Each needs a read to decide which copy is current, or whether they describe genuinely different aspects and want distinct names.\n- Consider whether the same check should apply ACROSS files, not only within one: a heading is keyed to a test name, and test names are unique workspace-wide once module-qualified (improvise-rm4s), so two files claiming the same test is also a defect.\n","status":"open","priority":3,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T12:39:28Z","created_by":"Edward Langley","updated_at":"2026-09-21T12:39:28Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-swre","title":"grid_of recomputes an unchanged grid every frame; memoise on the tiers that already exist","description":"Split out of improvise-jmjz on the adversarial review of PR #92, which closed jmjz on its own acceptance criterion (fuse the passes) and argued this is the separate, larger win.\n\ngrid_of recomputes the whole grid from scratch EVERY FRAME, even when nothing moved. After jmjz that is exactly one pass rather than three — 95ms at 18,000 cells, 1.32s at 90,000 — but the common case during scrolling, panel toggling and cursor movement is that the grid has not changed at all.\n\n## Why this is cheap to do here\nThe invalidation signal ALREADY EXISTS and is maintained for other reasons:\n- document_generation, layout_version, record_versions (gotcha 40, ADR 001's three tiers)\n- improvise-mcq4's view_shapes / view_frame observation\n\nA GridBody memo keyed on those takes an unchanged grid to roughly zero.\n\n## Why the remaining pass cannot be narrowed instead\nRuled out twice over, and the review measured it: totals sum EVERY row including ones a viewport scrolled past (gotcha 44), and widths are content-adaptive over every row. The benchmark's windowing ratio stayed 1.2-1.4x precisely because the surviving pass is the one a viewport cannot narrow. improvise-i5tk's decline stands.\n\n## Acceptance Criteria\n- A memo on the projected grid body keyed on the existing invalidation tiers; no NEW tier invented, because a second description of \"has the document changed\" is the disease (see the reasoning behind gotcha 39 and 45, where the trigger is the counter moving rather than a table of command variants).\n- Benchmarked: an unchanged grid across frames should approach zero, and the first frame must not regress.\n- The negative arm matters as much as the positive: a test that a grid which DID change is recomputed. A memo that never invalidates is fast and wrong, and it is the failure mode nothing will notice.\n\n## Second-order, much bigger, not this ticket\n~5us per cell at 18k is dominated by evaluate_aggregated doing a matching_values set intersection per cell (gotcha 18). An incremental aggregate index is the deeper fix. 90,000 cells at 1.32s remains unusable; that case is labelled pathological and memoisation helps it most.\n","status":"open","priority":3,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-21T11:30:15Z","created_by":"Edward Langley","updated_at":"2026-09-21T11:30:15Z","dependency_count":0,"dependent_count":0,"comment_count":0} -{"_type":"issue","id":"improvise-4dpj","title":"The RecordsNormal-to-Normal arm of settle_normal_mode has zero coverage, and it is live","description":"Found by the round-2 review of PR #91, by injection.\n\nReplacing settle_normal_mode's guard with a one-way version — matching only AppMode::Normal, dropping the RecordsNormal arm — leaves THE ENTIRE WORKSPACE SUITE PASSING.\n\n## Why the crossing test does not catch it\na_batch_that_leaves_records_and_then_sets_normal_lands_in_normal drives the LAYOUT both ways, but both branches end the batch with enter-mode normal, so the mode handed to the settle is AppMode::Normal in BOTH directions. The test discriminates 'settles at all' and 'settles against the ending layout', but not 'settles from either mode'. Its intent is honest; it is half-blind.\n\n## The arm is live, not dead\nProved reachable in production:\n\n records mode -\u003e apply_effects([SetAxis { \"_Index\", Axis::None }])\n layout.is_records_mode() == false, mode was RecordsNormal\n -\u003e settle yields AppMode::Normal (passes on shipped code)\n\nSo a geometry effect that takes _Index off Row is the live path through the arm nothing tests.\n\n## Acceptance Criteria\n- One extra case in the existing loop: seed the mode as RecordsNormal and cross OUT. Two lines.\n- The injection above (one-way guard) must then fail by name.\n\nSmall irony worth keeping: the paragraph that ends 'a careful comment standing in for a test is what let the per-effect version ship' names this very test as the remedy.","status":"open","priority":3,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T11:29:18Z","created_by":"Edward Langley","updated_at":"2026-09-21T11:29:18Z","dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"improvise-4dpj","title":"The RecordsNormal-to-Normal arm of settle_normal_mode has zero coverage, and it is live","description":"Two gaps in settle_normal_mode's coverage, both live code with zero tests. The second was raised by ChatGPT Codex on PR #91 and is recorded here because the behaviour turned out CORRECT and only the guard is missing.\n\n## Gap 1 — the RecordsNormal-to-Normal arm (found by injection)\nReplacing settle_normal_mode's guard with a one-way version — matching only AppMode::Normal, dropping the RecordsNormal arm — leaves THE ENTIRE WORKSPACE SUITE PASSING.\n\nThe existing crossing test drives the LAYOUT both ways, but both branches end with `enter-mode normal`, so the mode handed to the settle is AppMode::Normal in BOTH directions. It discriminates \"settles at all\" and \"settles against the ending layout\", but not \"settles from either mode\".\n\nThe arm is live, proved reachable in production:\n\n records mode -\u003e apply_effects([SetAxis { \"_Index\", Axis::None }])\n layout.is_records_mode() == false, mode was RecordsNormal\n -\u003e settle yields AppMode::Normal\n\nOne extra case in the existing loop — seed the mode as RecordsNormal, cross OUT — closes it, and the one-way injection must then fail by name.\n\n## Gap 2 — the SwitchView path, raised by Codex and VERIFIED CORRECT\nCodex reviewed PR #91 at c186909 (round 1, the per-effect resolution) and pointed out that SwitchViewAtCursor emits [SwitchView(target), ChangeMode(Normal)]: open the view panel over a records view, select a pivot view, and round 1 would leave RecordsNormal over the pivot grid, with the inverse leaving Normal over records.\n\nRound 2 (a10e95e) moved the settle to ONE call after rebuild_layout at the end of apply_effects, so it resolves against the POST-batch layout. Verified on main at ce895ff with a throwaway probe driving a real `:` line that crosses records-ness within one batch:\n\n start_records=true -\u003e ended_records=false mode=Normal\n start_records=false -\u003e ended_records=true mode=RecordsNormal\n\nCorrect in both directions. So the behaviour Codex named is fixed.\n\nTHE GUARD IS STILL MISSING. Codex asked specifically for \"a regression that switches between records and pivot views through the view panel\", and no such test exists. The path it names is different from the one the crossing test drives (toggle-records-mode, not SwitchView), and records-ness is auto-detected from the axes (gotcha 1), so a view switch can change it.\n\n## Acceptance Criteria\n- The gap-1 case added to the existing loop; the one-way-guard injection fails by name.\n- A regression driving SwitchViewAtCursor over the view panel, records view to pivot view AND back, asserting the mode matches the grid the batch ends on.\n- Both must fail if the settle is moved back before rebuild_layout.\n","status":"open","priority":3,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T11:29:18Z","created_by":"Edward Langley","updated_at":"2026-09-21T17:13:17Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-26dm","title":"The fourth length-not-completeness list: cases.len() == 13 in ui/effect.rs","description":"improvise-5ubs is closed for the two CYCLIC all() functions, by reading serde's\nderive-generated variant list out of its \"unknown variant ... expected one of\"\nmessage and diffing it against the names the cycle reached. That closes the\nAppMode and Rejection halves and the mechanism generalises to any serde enum.\n\nIt does NOT reach the FOURTH instance 5ubs records in its notes:\nlayout_geometry_effects_actually_change_document_config in\ncrates/improvise-command/src/ui/effect.rs, whose assert_eq!(cases.len(), 13) is\nover a hand-built list of EFFECTS, not of enum variants. serde has nothing to\nsay about it: an effect is a struct implementing a trait, so there is no derived\nenumeration to diff against.\n\nThe general statement stands and is the reason this is worth its own ticket:\ndoctrine(a_list_checked_for_length_reads_like_a_list_checked_for_completeness).\nA count catches a case being DELETED from the list and never one being ABSENT\nfrom it, and the two are written identically as an assert_eq over a number. The\nadversarial review of PR #78 added a fourteenth pub struct declaring\nEffectKind::Model(ModelChange::LayoutGeometry), pointed at by nothing, and the\nwhole suite stayed green.\n\nWhat would close it: an enumeration of the effects the compiler maintains. The\ncandidates are a proc-macro over the impl blocks, or inventory-style\nregistration, or reading the Debug names off a derived list the way\nevery_model_command_names_the_effect_of_the_same_name already does for the\nModelCommand mapping -- that last one is the closest existing precedent in this\nrepo and is worth looking at first.\n\nNot fixed in wave I because ui/effect.rs was another wave's ground that round,\nand because a fourth hand-rolled attempt in a fourth place is the disease 5ubs\nnames.\n","status":"open","priority":3,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T11:09:56Z","created_by":"Edward Langley","updated_at":"2026-09-21T11:09:56Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-e8no","title":"A wide numeric category now refuses a fit that used to ignore it","description":"improvise-zv8t forces every document category to `Forced::Categorical`, which is\nright (a `Year` category with items 2024/2025 is a coordinate, not a number).\nThe cost: a WIDE numeric category one-hots past `MAX_FEATURES` and refuses the\nwhole fit, where before it encoded as one numeric feature and a fit that never\nsplit on it succeeded.\n\nMeasured on 100 records with a 100-item numeric `Year` beside a `Cost` that\npredicts `Score` on its own:\n- pre-zv8t: fit lands on `IF(Cost \u003c 48.015...)`; `Year` never appears.\n- post-zv8t: preflight refuses at 101 features.\n\nPinned by `a_wide_numeric_category_now_refuses_a_fit_that_used_to_ignore_it`\nin crates/improvise-gp/src/fit.rs, and argued in context/code/gp.md section 11.\n\nThe workaround the refusal names works: mark the category a Label and the\npre-zv8t feature set is restored exactly. What is lost is the case where the\nuser wants neither -- a wide numeric category left alone while the fit uses the\nmeasures.\n\n## Steps to Reproduce\n\nBuild a model with a Regular category `Year` holding 100 numeric items\n(1900..1999), a Label category for record identity, and two measures `Cost` and\n`Score` where Score = 4000 if Cost \u003e 50 else 1000. Put `Year` on Axis::Row and\n_Measure on Axis::Page. Run `fit_measure(.., \"Score\", ..)`.\n\nPre-zv8t it lands on `IF(Cost \u003c 48.015..)`. Post-zv8t it is refused with\n\"would encode 101 features from 100 records\". The fixture is\n`wide_category_beside_a_good_measure()` in crates/improvise-gp/src/fit.rs.\n\n## Acceptance Criteria\n\nThe honest fix is a budget that can decline A COLUMN rather than the whole\ntable: `encoded_width` (and the exact check beside it) would drop the widest\ncategory and report it as a `Dataset::warnings` notice -- gotcha 37(a), it comes\nback as DATA and nothing prints. That keeps the zv8t decision intact (a category\nis still never inferred numeric) and changes only what the budget does when it\ncannot afford one.\n\nDo NOT close this by making the forcing conditional on cardinality: that is\ninference by another name, and makes the ENCODING depend on a budget rather than\non what the document says.\n","status":"open","priority":3,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T10:55:32Z","created_by":"Edward Langley","updated_at":"2026-09-21T10:55:39Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-sxsf","title":"cargo test -- --list cannot name the binary, so two crates could share a full test path","description":"MEASURED 2026-09-21 (PR #88). Re-verified latent: 1444 names, zero duplicate full paths, zero duplicate (crate, path) pairs.\n\nTHE PROPOSED FIX IS DECLINED ON TWO INDEPENDENT GROUNDS, and the second is the interesting one:\n\n1. COST: 45.2s against 2.4s for the one workspace call (11 members, warm) — 18x. And --check-evidence needs the --ignored twin, so the honest figure is 22 cargo invocations.\n\n2. IT DOES NOT CLOSE THE CLASS IT NAMES. A package is not a binary. The improvise crate alone has FIVE integration test binaries beside its lib, and an integration test's top-level fn lists with NO MODULE PATH AT ALL — 60 of 1444 names are bare. So '-p improvise' prefixes all six identically and tests/a.rs::foo still cannot be told from tests/b.rs::foo.\n\nAny real fix is per-TARGET (--lib, --test \u003cname\u003e), not per-package. Whoever takes this should start there.\n\nWorth noting how this was found: the agent set out to time the per-package fix and discovered it does not work while reading the target list. The cost measurement was the excuse for reading it; the correctness finding was the payoff — an argument for measuring declines rather than asserting them.","status":"open","priority":3,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T04:49:16Z","created_by":"Edward Langley","updated_at":"2026-09-21T05:23:48Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-8shu","title":"Constrain the .improv grammar generator's strategy; improvise-io's lib binary is the run's outlier","description":"Split out of improvise-8ec3, whose second half (stop pinning one machine's number) is done. This is the first half, left open.\n\nimprovise-io's lib binary is an order of magnitude above every other binary in the run on every machine it has been measured on: 1.40-1.45s over three runs here (vs a ~0.06s median), 3.70-5.24s on the machine that filed 8ec3. The ratio is the portable signal; the seconds are not.\n\nIts slow half is the grammar-walking generator in persistence/parse.rs plus the .improv roundtrip proptests at the default 256 cases. A grammar walk with no size bound generates deeply nested documents whose cost is not uniform across cases, so a few cases dominate.\n\ndoctrine(proptest_cases) rules out the cheap fix: constrain the strategy, do NOT lower the count below 256.\n\nAcceptance: improvise-io's lib binary is no longer an order of magnitude above the run's median, with the case count still 256 and the properties still covering groups, formulas, views, hidden items and pipe-quoting edges. Report the before/after ratio, not seconds.","status":"closed","priority":3,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-21T04:14:01Z","created_by":"Edward Langley","updated_at":"2026-09-21T05:15:42Z","closed_at":"2026-09-21T05:15:42Z","dependency_count":0,"dependent_count":0,"comment_count":0} -{"_type":"issue","id":"improvise-9v0j","title":"view_frame and view_shape are separated by example, not by property","description":"Found by the adversarial review of PR #78.\n\nimprovise-ni6k introduced view_frame (the drawn frame, drives the cursor) beside view_shape (the configuration, drives layout_version). They answer different questions -- AddCategory of an empty category moves view_shape and not view_frame -- and context/code/command.md section 'Two observations, not one' now records the argument.\n\nTHE GUARD IS ONE-DIRECTIONAL. Tests assert specific commands move specific counters. Nothing asserts the general relation, in either direction:\n\n- Is every reframe also a reshape? (Probably yes: a drawn-frame change should always be a config change. If so it is a property worth pinning, and its failure would mean layout_version under-reports, which per gotcha 35's asymmetry leaves a client wrong FOREVER.)\n- Is every reshape a reframe? (Definitely not -- that is the AddCategory case, and it is the whole reason there are two.)\n\nSo the first is a candidate property test and the second is a documented asymmetry. Today a change that accidentally made view_frame a superset of view_shape, or vice versa, would pass the suite.\n\nWorth doing as a proptest over a generated workbook plus a generated model command, comparing the two differing-key sets. Note the fixture trap improvise-gfvj's investigation turned up: pressing R to enter records mode fires effect::SortData first, so a records-mode fixture built through the key path has an already-sorted store and cannot demonstrate order-sensitive behaviour.","status":"open","priority":3,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-21T04:01:34Z","created_by":"claude-agent-protocol","updated_at":"2026-09-21T04:01:34Z","dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"improvise-9v0j","title":"view_frame and view_shape are separated by example, not by property","description":"RE-SCOPED 2026-09-21. The ticket proposed a property test asserting that view_frame and view_shape agree — roughly \"every reframe is also a reshape\". THAT PROPERTY IS FALSE and the test would have failed. Measured by the Wave I agent and confirmed by the adversarial review of PR #92, with a prune_empty view of two rows and a ClearCell on North's only cell, driven through sessions.apply_caught_up:\n\n rows 2 -\u003e 1, layout_version 0 -\u003e 0, cursor_epoch 0 -\u003e 1\n\nA genuine reframe with no reshape. That is improvise-mcq4 working as designed: a cell write changes DATA, not the view's CONFIGURATION, and view_shape hashes configuration. Writing the proposed test would have been wrong.\n\nSo the original acceptance criterion is withdrawn. What remains is the real question the ticket was reaching for:\n\n## Acceptance Criteria (replaced)\n- State, in context/code/command.md, the PROPERTY that actually separates the two: view_shape answers \"has this view's configuration changed\" and view_frame answers \"does this view draw differently\". A data write moves the second and not the first, by design.\n- If a property test is wanted, it must be the true one — e.g. a configuration change moves BOTH, and a pure data write moves only the frame — with the direction asserted rather than the conjunction.\n- The consequence for ADR 001's middle tier is filed separately as improvise-6nsd (P4), with the severity note: a client names a cell by RecordHash, a coordinate surrogate, so a renumbered row does not misaddress a write, and cursor_epoch did move so every client is told.\n\nNote the file ownership that blocked the original attempt: view_frame, view_shape, view_frames and view_shapes are all private in crates/improvise-command/src/ui/app.rs.\n","status":"open","priority":3,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-21T04:01:34Z","created_by":"claude-agent-protocol","updated_at":"2026-09-21T17:09:51Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-h0iv","title":"group_runs coalesces by name alone, so two runs under different headers merge","description":"Found by the adversarial review of PR #77, which landed `AxisFrame::group_runs`\nas the one home of column-group run coalescing (improvise-8qlr).\n\n`group_runs` coalesces adjacent positions by group **name**:\n\n Some((prev, span)) if prev.map(|p| p.name.as_str()) == name =\u003e *span += 1,\n\nSo two runs under two DIFFERENT headers that happen to share a name read as one\nrun, and the browser draws one `\u003cth colspan\u003e` across both. `FrameGroup` carries\n`cat` as well as `name` — precisely so a client never has to work out which\ncategory a label came from — and comparing the pair would fix it.\n\nWHY PR #77 WAS RIGHT NOT TO FIX IT, so this is a decision and not a\nrediscovery: that PR's job was to make one rule out of two. Both prior copies\n— `src/ui/grid.rs`'s `group_name != prev_group` and\n`improvise-wasm-client/src/render.rs::col_groups`' `run.label == label` —\ncoalesced by name alone, so name-only is what the consolidation had to preserve\nto stay a consolidation. Changing the rule in the same commit that moves it\nwould have meant the agreement test could no longer tell \"the two ends agree\"\nfrom \"the two ends changed together\", which is the whole evidence of the move.\n\nTHE RISK OF LEAVING IT IS SPECIFIC AND NEW: a single home reads as decided.\nThe next person to look at `group_runs` finds one rule, one call site per front\nend, and a test pinning both — and has no reason to suspect the comparison is\nnarrower than the data it is given. That is the cost of a good consolidation\nand the reason to file rather than to note.\n\nReachability today: two categories on one axis, where the inner category has\ngroups of the same name under two different outer items — e.g. `Month` grouped\ninto `Q1` nested under `Region`, giving `North/Q1` and `South/Q1` adjacent. The\nexisting fixture in `tests/grid_frame_agreement.rs` has exactly this shape but\nis saved by the ungrouped `Sep` sitting between the two `Q1` runs, which is\nalso what makes improvise-gd83's spans come out as\n`[(Q1,2), (None,1), (Q1,2), (None,1)]` rather than merged. Remove `Sep` and the\ntwo runs merge.\n\nNot obviously a bug in every case: two adjacent runs of the same group name may\nwell be one visual span a user WANTS spanned. That is the question to answer\nfirst, and it is a question about what a group header means across a nesting\nboundary — which is why this is filed rather than fixed.\n\n## Steps to Reproduce\n\n1. Build a two-level column axis whose inner category is grouped and whose\n groups repeat under each outer item, with NO ungrouped item between them.\n2. `AxisFrame::group_runs` returns one run spanning both.\n3. Both front ends draw one group header across the boundary, because both now\n ask the same (name-only) rule.\n\n## Acceptance Criteria\n\nDecide whether two adjacent same-named runs under different headers are one\nspan or two, and say why in the commit. If two, `group_runs` compares\n`(cat, name)` and the agreement-test fixture gains a case with no separating\nungrouped item — the current one cannot see this.\n","status":"closed","priority":3,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T04:00:18Z","created_by":"agent-render-frame","updated_at":"2026-09-21T12:39:37Z","closed_at":"2026-09-21T12:39:37Z","labels":["area:projection"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-1wl0","title":"AxisFrame::first is serde(default), so a frame with no origin claims it leads the axis","description":"Found by the adversarial review of PR #77, which landed `AxisFrame::first`.\n\n`#[serde(default)]` on `first` forges the PERMISSIVE claim. `first: 0` is not\nan absence — it is the assertion \"this frame LEADS the axis\", and that is\nexactly what `body_from` acts on:\n\n match self.first + idx == 0 { true =\u003e \u0026self.entries[..], ... }\n\nSo an older peer's frame, or any frame whose `first` is dropped in transit,\ndeserializes as leading the axis. A scrolled browser then draws the carried\nheader ABOVE its first row — the header `body_from` exists to discard there,\nand the mirror image of the improvise-0o6x bug the field was added to fix.\n\nThis is the same argument that gave `Basis` no `Default` and no public\nconstructor (gotcha 43): a zero-valued claim that happens to be TRUE in the\ncommon case is a forgeable bypass rather than an absence. `first` is worse in\none respect — a `Basis` at least has to be constructed deliberately, while a\nserde default is applied silently by a missing field.\n\nNO SKEW SHIPS TODAY. Both ends are built from one workspace at one commit, so\nthere is no older peer. This is about the day there is one, and about the\nattribute reading as a considered compatibility decision when it was not.\n\nOptions, not yet chosen:\n\n * Drop the attribute, so a frame missing `first` fails to deserialize. Loud,\n and correct if a frame is never partially constructed. Note gotcha 37's\n observation that `#[serde(default)]` on an Option is documentation and not\n mechanism — here it is the opposite, it IS the mechanism.\n * Make the leading-ness unrepresentable-otherwise: `Option\u003cusize\u003e`, or a\n two-variant enum (`LeadsAxis` / `StartsAt(usize)`), so \"unknown\" cannot\n silently mean \"leads\".\n\nWhichever, say which in the commit, because the reason to prefer one is about\nwire compatibility and not about the type.\n\n## Steps to Reproduce\n\n1. Serialize an `AxisFrame` built for a scrolled window (`first \u003e 0`) and\n remove the `first` key from the JSON — the shape an older peer sends.\n2. Deserialize it. `first` is 0.\n3. `body_from(0)` now takes the `first + idx == 0` arm and returns the whole\n entry list, including the carried group header.\n4. The browser draws a group header above its first row, at every scroll\n position. Nothing reports anything.\n","status":"closed","priority":3,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T04:00:14Z","created_by":"agent-render-frame","updated_at":"2026-09-21T12:39:36Z","closed_at":"2026-09-21T12:39:36Z","labels":["area:projection"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-gfvj","title":"A pure record reorder invalidates no cursor, and I could not build a fixture that reorders","description":"Left open by improvise-ni6k, which closed the rest of its class by observing the grid frame.\n\nni6k names SortData as its SHARPEST case: it changes neither the row count nor any label, and every position still names a different record. The frame fingerprint ni6k landed (App::view_frame, hashing GridLayout row_items/col_items) CANNOT see it, because records-mode row_items are positional (gotcha 1) -- they are 0,1,2... whatever order the store is in.\n\nI wrote the mechanism -- hashing layout.records[].coords alongside the labels -- and then REMOVED it before committing, because I could not demonstrate it catches anything. Every fixture I built came back in key order whatever order the cells were inserted in:\n\n wb.add_item(Region, North); wb.add_item(Region, East);\n set_cell(North/Revenue); set_cell(East/Revenue);\n -\u003e records mode row order is [East, North] BEFORE any SortData\n\nSo either SortData is a no-op for that shape, or something between DataStore::live_records / matching_cells / RecordRow::group is ordering records that gotcha 18 says arrive in insertion order. I did not chase which, and I am not asserting a bug in gotcha 18 on this evidence -- matching_positions sorts POSITIONS (which preserves insertion order), so the explanation is probably elsewhere and probably boring.\n\nTWO THINGS TO DO, in order:\n1. Find out what actually orders records-mode rows, and whether SortData can reorder them at all. If it cannot, this ticket closes as 'not a case' and ni6k's note should be corrected -- its sharpest example would be hypothetical.\n2. Only if it can: put the coords back into view_frame and pin it with the fixture that reproduces.\n\nWHY IT WAS NOT SHIPPED UNVERIFIED: a confident comment over a mechanism nobody has seen fire is this repo's named defect. See doctrine(check_the_checker) and the wave brief's 'a green suite is not evidence'.","notes":"REPRODUCED AND FIXED. My original conclusion was WRONG, and the way it was wrong is the useful part.\n\nI claimed I could not build a fixture where SortData reorders records. The reviewer of PR #78 reproduced it in thirty lines. I re-derived it: insert West/North/East, call sort_by_key, and both live_records() and the records-mode GridLayout reorder to East/North/West.\n\nWHY MY FIXTURE HID IT -- not the reviewer's guess (insertion already sorted), and better than that: MY FIXTURE ENTERED RECORDS MODE BY PRESSING R, and ToggleRecordsMode (command/cmd/grid.rs:476) pushes effect::SortData AS ITS FIRST EFFECT. So the store was sorted by the act of entering records mode, and the SortData I then applied was a genuine no-op. My two-record store did reorder at the DataStore level -- I checked after the review -- so the fixture, not the store, was the problem.\n\nTHE GENERAL TRAP, worth carrying: a records-mode fixture built through the key path has an already-sorted store by construction, so it cannot demonstrate anything order-sensitive. Build one by setting _Index/_Dim axes directly. That is now written into context/test-evidence/command.md under sorting_the_store_in_records_mode_invalidates_the_cursor.\n\nTHE FIX: the layout.records coords hash I removed is restored in App::view_frame, and sorting_the_store_in_records_mode_invalidates_the_cursor pins it -- asserting the row COUNT and the LABELS are both unchanged, so it is the sharp case and not a shape change wearing its name. Proved by removing the coords hash again: that test fails and NOTHING ELSE IN THE CRATE does.\n\nMY PROCESS ERROR, for the record: I concluded 'cannot reproduce' from ONE fixture without varying it, and then filed that conclusion as a finding. Removing the unverified mechanism was right; concluding the hole was not real was not. 'I could not reproduce it' and 'it does not reproduce' are different claims and I filed the second having only earned the first.\n\nClosing.","status":"closed","priority":3,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T03:47:15Z","created_by":"claude-agent-protocol","updated_at":"2026-09-21T04:01:44Z","closed_at":"2026-09-21T04:01:44Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0} @@ -350,7 +351,7 @@ {"_type":"issue","id":"improvise-9d5k","title":"Two small wrong claims in kb-merge.pl's own comments","description":"Both found by review on PR #69, both one-line fixes, both are claims ABOUT A CHECK sitting where nothing verifies them -- which is the subject of the PR they arrived in.\n\n1. kb-test.sh case 10's comment says 'under unification conflicts sees the non-ground key match both facts and refuses the add outright, so the reverted fix fails HERE.' DEMONSTRATED false: reverting conflicts to unification leaves case 10 PASSING and fails case 17 instead. A rule now keys on its whole clause, so the ':-' term cannot unify with probe_fact(a) and conflicts never fires there. The site is guarded, by a different case, so this is a wrong explanation rather than a hole.\n\n2. replace's new refusal message says '...so it would ADD them beside the ones already there' even when the predicate has no existing clauses. Behaviour is right and fail-safe; only that clause of the sentence is wrong in that arm.","status":"closed","priority":4,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-21T00:11:27Z","created_by":"orchestrator","updated_at":"2026-09-21T04:49:29Z","closed_at":"2026-09-21T04:49:29Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-r08u","title":"IS and WHERE differ on trailing word boundary in formula.pest","description":"improvise-4zl7 gave `IS` a trailing word-boundary lookahead: `is_expr` is compound-atomic so `Region ISLAND` no longer parses as `Is(Region, LAND)`. `^\"WHERE\"` in `where_clause` and `inline_where` still has the identical hole — `SUM(Rev WHEREAS x = y)` would read as a WHERE on a category `AS`. It is far less exposed than IS was (WHERE sits in one fixed position after an expression, where IS follows any identifier in expression position), which is why it was left alone rather than fixed in the same pass: it is pre-existing and changing it is a grammar change with its own round-trip risk.\n\n## Steps to Reproduce\n\n1. `parse_formula(\"X = SUM(Rev WHEREAS x = y)\", \"Cat\")`\n2. It parses, reading `AS` as the filter category, where it should be a syntax error.","acceptance_criteria":"Either `^\"WHERE\"` gets the same `!bare_char` lookahead `IS` has and a test asserting `WHEREAS` does not parse as a WHERE clause, or the asymmetry is declined in writing in context/code/formula-and-io.md with the reason. The grammar-walking generator proptests must still pass either way.","status":"closed","priority":4,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-20T19:16:46Z","created_by":"wave3-4zl7","updated_at":"2026-09-21T04:42:55Z","closed_at":"2026-09-21T04:42:55Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-1l9o","title":"furnished_client hand-builds a basis instead of taking the server's","description":"A nit from the adversarial review of PR #50, explicitly not worth a push at\nthe time.\n\nlockstep.rs's furnished_client builds its BasisCache by walking the client's\nown grid for Record cells and calling basis::published(0, 0, records) with\nevery version hard-coded to 0. That is a small re-implementation of\nproject.rs::basis_update's record selection, in a test whose entire purpose is\nto rule out re-implementations.\n\nIt is harmless today -- a fresh App really is at generation 0, layout 0, and\nevery record at version 0 -- which is exactly why it will stop being harmless\nsilently if any of those three stops being true.\n\nThe reviewer demonstrated the alternative while checking something else: seed\nthe ClientState from a real Sessions::connect snapshot and let the server's own\nProjection::Basis populate the cache. That removes the fabrication and makes\nthe fixture a client the server actually produced.\n\nNote the same fabrication exists in client_ready_to_edit, which predates this\nPR, so a fix should take both.","acceptance_criteria":"The lockstep fixtures get their basis from a real Sessions::connect\nsnapshot rather than constructing one. Both furnished_client and\nclient_ready_to_edit, since they share the fabrication. The tests that use\nthem must still pass unchanged -- if seeding from the server changes an\noutcome, that is a finding about the fixture and worth stopping for.","status":"closed","priority":4,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-20T18:58:19Z","created_by":"wave2-h-frontend","updated_at":"2026-09-21T12:39:37Z","closed_at":"2026-09-21T12:39:37Z","labels":["area:shared-frontend"],"dependency_count":0,"dependent_count":0,"comment_count":0} -{"_type":"issue","id":"improvise-thpe","title":"help::compact discriminates a chord by parsing a string it formatted","description":"Finding 3 of the adversarial review of PR #50. Not live-broken and\ndeliberately not fixed in that PR.\n\nimprovise_protocol::help::compact drops chorded keys from a status-bar hint by\ntesting whether the formatted label starts with \"Ctrl-\" or \"Alt-\". The\nreviewer enumerated every format_key_label output and none begins with either,\nso there is no false positive today.\n\nThe defect is the shape, not the behaviour. keymap.rs::labels_for HAS the\nstructured fact -- it matches KeyPattern::Key(code, mods) and then throws mods\naway into a string -- and compact then reconstructs a worse version of it by\nparsing. A future modifier spelled any other way (Super-, Cmd-, a bare glyph)\nis silently not a chord, lands in the status bar, and nothing fails.\n\nThe fix is to carry the fact rather than re-derive it: either Hint keeps the\nKeyModifiers beside each label, or labels_for returns (KeyCode, KeyModifiers)\npairs and help.rs formats at the end. The second is tidier and is what\nwhich_key already does with key_label(code, *mods).\n\n## Steps to Reproduce\n1. Add a modifier to keymap.rs::key_label spelled with any prefix other than\n \"Ctrl-\" or \"Alt-\" -- \"Super-x\", say.\n2. Bind a command advertised by a mode's hint to that chord only.\n3. hint_line puts it in the status bar. Nothing fails.","acceptance_criteria":"compact stops parsing a string to learn something the keymap already\nknew: either Hint carries the KeyModifiers beside each label, or labels_for\nreturns (KeyCode, KeyModifiers) and help.rs formats last. Proved by adding a\nmodifier spelled differently and watching a test fail -- the point is that\ntoday it would not.","status":"open","priority":4,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-20T18:45:36Z","created_by":"wave2-h-frontend","updated_at":"2026-09-20T18:45:36Z","labels":["area:shared-frontend"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"improvise-thpe","title":"help::compact discriminates a chord by parsing a string it formatted","description":"Finding 3 of the adversarial review of PR #50. Not live-broken and\ndeliberately not fixed in that PR.\n\nimprovise_protocol::help::compact drops chorded keys from a status-bar hint by\ntesting whether the formatted label starts with \"Ctrl-\" or \"Alt-\". The\nreviewer enumerated every format_key_label output and none begins with either,\nso there is no false positive today.\n\nThe defect is the shape, not the behaviour. keymap.rs::labels_for HAS the\nstructured fact -- it matches KeyPattern::Key(code, mods) and then throws mods\naway into a string -- and compact then reconstructs a worse version of it by\nparsing. A future modifier spelled any other way (Super-, Cmd-, a bare glyph)\nis silently not a chord, lands in the status bar, and nothing fails.\n\nThe fix is to carry the fact rather than re-derive it: either Hint keeps the\nKeyModifiers beside each label, or labels_for returns (KeyCode, KeyModifiers)\npairs and help.rs formats at the end. The second is tidier and is what\nwhich_key already does with key_label(code, *mods).\n\n## Steps to Reproduce\n1. Add a modifier to keymap.rs::key_label spelled with any prefix other than\n \"Ctrl-\" or \"Alt-\" -- \"Super-x\", say.\n2. Bind a command advertised by a mode's hint to that chord only.\n3. hint_line puts it in the status bar. Nothing fails.","acceptance_criteria":"compact stops parsing a string to learn something the keymap already\nknew: either Hint carries the KeyModifiers beside each label, or labels_for\nreturns (KeyCode, KeyModifiers) and help.rs formats last. Proved by adding a\nmodifier spelled differently and watching a test fail -- the point is that\ntoday it would not.","status":"closed","priority":4,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-20T18:45:36Z","created_by":"wave2-h-frontend","updated_at":"2026-09-21T17:09:50Z","closed_at":"2026-09-21T17:09:50Z","labels":["area:shared-frontend"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-h074","title":"Two edges of App::resolve_view: a worse panic message, and a host-side DeleteView bypass","description":"Found by the adversarial review of PR #48 (improvise-jytm). Two small things, both\nabout what happens at the edges of the fallback improvise-jytm introduced.\n\n**The panic message got worse.** `App::resolve_view` falls back to\n`workbook.active_view` for a name the document has not got, then indexes\n`workbook.views[default]`. `Workbook::active_view()` — which several of these call\nsites used before — carries `.expect(\"active_view always names an existing view\")`.\n`IndexMap`'s `Index` impl panics with no such message. So a `Workbook` whose\n`active_view` names nothing (which only a hand-built or deserialized-from-elsewhere\nworkbook can be, since `create_view` and `delete_view` both maintain it) now fails\nwith a bare index panic where it used to name the invariant that was broken. Cheap\nfix: keep the `.expect` string.\n\n**A host-initiated `DeleteView` bypasses `adopt_deleted_views`.** That function runs\nin `Sessions::apply_model`, which is the door a *client's* command comes through. A\nhost driving `App::apply_effects` directly — through `Host::app_mut()` — deletes a\nview without any session mirror being moved off it.\n\n**Read the unreachability argument carefully, because it is weaker than it looks.**\nA first version of this ticket said `app_mut()` had one caller, in\n`improvise-ws-server/src/main.rs`, at startup, before any client had connected. That\nwas wrong, and review caught it. The one caller is\n`crates/improvise-worker-server/src/worker.rs:64`, inside `persist()` — which runs\n**after every accepted model command, with a client connected**. The bypass is still\nunreachable, but only because `persist()` happens to pass `effect::Save`, which\ndeletes no view. \"The one existing caller passes a harmless effect\" is a much thinner\nguarantee than \"nobody is connected yet\", and it is the exact shape of premise this\nrepo has been burned by: gotcha 47 records that the lost update `CheckedBatch` later\nfixed had shipped under an unreachability argument too.\n\nThe second is the interesting one to decide rather than to patch: it is the general\nshape of \"session bookkeeping that only the session door performs\". `Host::project`\nis already where every outbound path funnels, so whether the adoption belongs there\ninstead is the design question.\n\n## Steps to Reproduce\n\nThe panic message:\n\n1. Build a `Workbook` whose `active_view` names a view not in `views` (only reachable\n by constructing the struct directly — the constructors maintain the invariant).\n2. Call `App::view()`. It panics from `IndexMap`'s `Index` impl with no message,\n where `Workbook::active_view()` would have said which invariant broke.\n\nThe bypass:\n\n1. A client connects and switches to \"Second\".\n2. The host calls `host.app_mut().apply_effects(vec![Box::new(DeleteView(\"Second\"))])`\n — the same door `improvise-worker-server`'s `persist()` uses for `effect::Save`\n after every accepted model command.\n3. The session's mirror still names \"Second\". Nothing moved it and nothing told the\n client, because `Sessions::adopt_deleted_views` runs only in `apply_model`.\n","acceptance_criteria":"App::resolve_view's fallback index carries the invariant message Workbook::active_view() had. And a decision is recorded on whether session adoption belongs at the Sessions door or at Host::project — with a test for a host-initiated DeleteView if the answer moves it.","notes":"PR #76 (branch wave/command-effect-gaps) does HALF of this: App::resolve_view's fallback carries back the .expect(\"active_view always names an existing view\") message, guarded by a_default_that_names_nothing_says_which_invariant_broke in ui/app.rs. The SECOND half is untouched — whether a host-initiated DeleteView's session adoption belongs at the Sessions door or at Host::project is a decision about ui/project.rs and ui/host.rs, which were another agent's ground in this wave. Reported rather than guessed.","status":"in_progress","priority":4,"issue_type":"task","assignee":"claude-agent-cmd","owner":"el-github@elangley.org","created_at":"2026-09-20T18:33:43Z","created_by":"wave2-e-active-view","updated_at":"2026-09-21T03:42:18Z","started_at":"2026-09-21T03:27:56Z","lease_expires_at":"2026-09-21T03:32:56Z","heartbeat_at":"2026-09-21T03:27:56Z","labels":["area:active-view"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-x4wv","title":"assemble.sh's stamped guard under-reports multiple broken substitutions, and its comment reads as though it does not","description":"Found by injection during PR #47 (the landing-page inversion), by the WU-P agent.\n\nweb/assemble.sh's four 'stamped' checks exist because one grep over four files succeeds when ANY one of them matches, so three failed substitutions could still produce a successful assembly under set -e (found by Copilot on PR #24). The comment says each stamped reference 'is verified on its own and names itself when it is the one missing'.\n\nInjection result: breaking ONE substitution (a wrong sedi pattern for worker.js) produces exactly the claimed behaviour —\n\n assemble: .../app/worker.js is missing the cache-bust stamp: worker-pkg/improvise_worker_server.js?v=inj\n\nwith the app.js guard ahead of it passing and printing its match. Correct and isolated.\n\nBut 'stamped' calls exit 1 on the FIRST failure, so breaking BOTH app.js and worker.js reports app.js alone. The phrase 'when it is the one missing' is accurate for a single failure and misleading for a multiple one: a reader takes it as 'each missing stamp names itself'.\n\nThis FAILS SAFE — the build stops and a real problem is reported — so it is P4, not a correctness bug. It is filed because the prose is what a future agent will act on, and because the original guard was itself the product of a review finding about a check that passed when it should not have.\n\nDeliberately NOT fixed in PR #47: pre-existing, out of that unit, and reshaping stamped would rewrite a guard whose current form came out of the PR #24 finding. The cheap fix is probably the comment, not the code.\n\n## Steps to Reproduce\n\n1. In web/assemble.sh, corrupt the sedi patterns for BOTH app.js and worker.js.\n2. Run web/assemble.sh into a scratch dir.\n3. Only the app.js failure is reported; the worker.js one is never reached.","acceptance_criteria":"Either stamped reports every missing stamp before exiting, or its comment says plainly that it reports the FIRST failure only. Demonstrated by breaking two substitutions and seeing both named, or by the comment matching the single-report behaviour.","status":"closed","priority":4,"issue_type":"bug","assignee":"claude-agent-scripts","owner":"el-github@elangley.org","created_at":"2026-09-20T18:14:57Z","created_by":"orchestrator","updated_at":"2026-09-20T23:35:12Z","started_at":"2026-09-20T23:22:06Z","closed_at":"2026-09-20T23:35:12Z","close_reason":"fixed in PR #69, each proved by injection","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-gcvb","title":"The browser cannot commit a records-mode creating column, though it is now nameable","description":"`keys.rs::commit` returns early for EVERY records-mode commit:\n\n if state.view.mode.is_records() { return None; }\n\nThe comment gives the real reason — \"a category column renames every measure\ncell in the row, which needs `record_col`\" (gotcha 1) — and that reason is\nsound for the coordinate columns and for the measure columns' advance, which\n`CommitAndAdvance` handles with `AddRecordRow` at the bottom-right and a\n`RebuildLayout` when `+cat` inserts a column.\n\nBut the guard is broader than its reason. improvise-poia made the two CREATING\ncolumns nameable: a cell at the cursor now projects `CellId::Create(Measure)`\nor `CellId::Create(Category)`, and committing one is a single existing\n`ModelCommand` over the typed NAME — `AddItem { category: \"_Measure\", item }`\nor `AddCategory(name)`. Neither reads `record_col`, neither touches the record\nunder the cursor, and neither writes a cell. So this is the one records-mode\ncommit a thin client could already compose, and the early return is what stops\nit.\n\nWhat has to be answered before it lands, and why it is not a one-liner:\n\n- The ADVANCE differs. `CommitAndAdvance` returns straight after a `+cat`\n commit with a `RebuildLayout` and no cursor move, because an inserted column\n shifts every index to its right and the advance was computed from the\n pre-commit layout (gotcha 23). A client that advanced anyway would land one\n column off, silently.\n- `AddCategory` can be REFUSED (the 12-category limit), and it sets\n `abort_effects` (gotcha 21). A client composing `[AddCategory, ChangeMode]`\n is composing a batch whose first step may abort — which is exactly the hole\n clause 4 of gotcha 47 leaves open.\n\nBoth are arguments for doing this with a lockstep test against\n`CommitAndAdvance`, not by reading the resolver and hoping.\n","acceptance_criteria":"keys.rs::commit composes AddItem/AddCategory when the cell at the cursor is CellId::Create, instead of returning None for the whole of records mode — and a tests/lockstep.rs case proves the client and CommitAndAdvance land the document AND the cursor in the same place, including the +cat case where an inserted column means the TUI does not advance at all. The coordinate and measure columns stay the host's; the early return narrows rather than disappears.","status":"closed","priority":4,"issue_type":"task","assignee":"claude-wasm-keys-agent","owner":"el-github@elangley.org","created_at":"2026-09-20T18:13:31Z","created_by":"wave2-b-projection","updated_at":"2026-09-21T00:24:03Z","started_at":"2026-09-20T23:21:16Z","closed_at":"2026-09-21T00:24:03Z","close_reason":"Fixed and merged: rmvx/fdt6/gcvb in PR #71, the harness four in PR #72. Both adversarially reviewed, with each finding's injection re-run against the fix. Closed by the orchestrator with --force because the claiming agent has stood down; the claim guard refusing an unforced close is the mechanism working, not a problem.","labels":["area:projection"],"dependency_count":0,"dependent_count":0,"comment_count":0} diff --git a/AGENTS.md b/AGENTS.md index 4c479d23..fd8bf5bc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -153,6 +153,28 @@ worth it on a repo with other collaborators. One PR per wave, built by a **dedicated PR agent in its own worktree**, so waves pipeline against it: the next wave runs while the PR absorbs review fixes. +**Read the PR's reviews before merging it.** This is separate from CI and from +the adversarial agent, and it has been missed: two ChatGPT Codex reviews landed +on merged PRs and were not read until afterwards, one of them a live P2 +(improvise-x1ns). Both were correct. + +```bash +gh pr view --json reviews -q '.reviews[]|"[\(.state)] \(.submittedAt) \(.commit.oid[0:8])\n\(.body)"' +``` + +Two things make a Codex review easy to miss. It arrives as a **review**, not a +comment, so a glance at the comment thread does not show it. And it posts under +the repository owner's account — the same account our own agents post under — +so **authorship does not distinguish it**. The tell is the trailing line +`_Automated review by ChatGPT Codex (…)_`; ours open with +`_Posted by Claude (…)_`. Check the `commit.oid` too: a review may target an +earlier head, and the finding may already be fixed — verify rather than assume +in either direction. + +Reviews arrive **as the owner's quota allows**, so absence is not evidence. +Wait where a review is plausible, and when one is found, answer it on the +thread even if the PR has already merged: that is where the next reader looks. + **Copilot is not reviewing this repository, and the request is a silent no-op.** Measured 2026-09-20: PRs #12, #21 and #27 carry 2, 8 and 4 Copilot reviews; #61, #66, #68 and #70 carry none. Both request paths report success and produce zero diff --git a/prolog-sessions/kb.pl b/prolog-sessions/kb.pl index f5eea1ec..86af2b92 100644 --- a/prolog-sessions/kb.pl +++ b/prolog-sessions/kb.pl @@ -287,6 +287,8 @@ doctrine(the_elsewhere_path_is_only_as_correct_as_the_inputs_it_can_rebuild,'pro doctrine(a_conflicting_pr_runs_no_ci_because_it_has_no_merge_ref,'A pull_request workflow runs against refs/pull/N/merge, and GitHub cannot build that ref for a CONFLICTING pull request -- so NO RUN IS EVER CREATED and the PR simply shows zero checks. That is indistinguishable from a dropped webhook if you only look at the runs list. PR #92 sat with zero checks across two pushes and a close/reopen; the cause was that PR #91 had merged and both had appended to the same four context/ files. DIAGNOSIS ORDER: before theorising about event delivery, ask \`gh pr view N --json mergeable\` -- one call, and it answers first. The orchestrator ran a genuinely good control (push to another PR, watch a run appear in five seconds) which correctly proved delivery works repo-wide, and then drew the WRONG conclusion from a sound experiment, because the hypothesis space had two entries and the truth was a third. A control discriminates only between the hypotheses you listed.'). doctrine(no_checks_reported_is_a_merge_state_symptom,'A pull_request-triggered workflow builds refs/pull/N/merge, and a CONFLICTING pull request HAS NO MERGE REF, so no run is ever created -- \"no checks reported\" then looks IDENTICAL to a dropped webhook. Measured on PR #92, 2026-09-21: 34 minutes of polling actions/runs?head_sha=, a close/reopen and an empty commit all produced zero runs; one git merge main produced a run in seconds. ASK gh pr view --json mergeable,mergeStateStatus FIRST -- one call answers what no amount of polling the runs API can distinguish. Both the agent and the orchestrator hypothesised a dropped event and a stalled delivery; NEITHER LISTED THE THIRD CASE, and the orchestrator control -- pushing to another PR and watching a run appear in five seconds -- is consistent with all three. A control discriminates only between the hypotheses you listed.'). doctrine(grep_over_git_diff_is_silently_empty_under_an_external_differ,'This repo configures diff.external = difftastic, which prints a SIDE-BY-SIDE format carrying no unified-diff markers. So \`git diff | grep \"^-\"\` matches nothing REGARDLESS OF CONTENT, and an agent verifying \"nothing was dropped in the merge\" read three consecutive empty outputs as three clean bills. Verified by the orchestrator: on a file with one real deletion, git diff | grep -c \"^-\" gives 0 and git diff --no-ext-diff | grep -c \"^-\" gives 1. Use --no-ext-diff in any programmatic read of a diff, or --numstat / --name-only, which are unaffected. The general shape, and it is the sharpest form of check_the_checker: A CHECK THAT RETURNS EMPTY FOR THE WRONG REASON IS INDISTINGUISHABLE FROM ONE THAT PASSES -- and an empty result is the one outcome nobody investigates.'). +doctrine(read_the_reviews_before_merging_and_authorship_will_not_identify_them,'Twenty PRs were merged in one campaign without reading their REVIEWS -- CI green plus a dispatched adversarial agent felt like the whole gate, and it was not. Two ChatGPT Codex reviews were found afterwards on merged PRs; both were correct and one was a live P2 (improvise-x1ns). Three things make a Codex review easy to miss. It arrives as a REVIEW, not a comment, so scanning the comment thread does not show it. It posts under the repository OWNER account -- the same account our own agents post under -- so AUTHORSHIP DOES NOT DISTINGUISH IT; the tell is the trailing line \"Automated review by ChatGPT Codex\", where ours OPEN with \"Posted by Claude\". And it may target an EARLIER head, so a finding can be already fixed or still live and you must verify which rather than assume from the date. Reviews arrive as the owner quota allows, so absence is not evidence of approval. Answer a review on the thread even after the PR has merged: that is where the next reader looks.'). +doctrine(bounded_by_a_later_check_does_not_bound_the_cost,'improvise-x1ns: encoded_width is a PREFLIGHT whose whole purpose is to refuse an expansion BEFORE allocating it, and its categorical branch under-counted by the missing level that build_categorical actually emits. The wave own review had seen the shape and reasoned it away as \"bounded by the exact check\". Codex rebuttal is the keeper: THE EXACT CHECK RUNS AFTER THE ALLOCATION, so it bounds the error MESSAGE, not the cost. When a guard exists to prevent work rather than to report a wrong answer, a downstream correctness check is not a backstop for it -- the thing you were protecting has already happened. Second instance on this path: improvise-zv8t own round found forcing by NAME while budgeting by INDEX, repaired as a data dependency so the ordering became a compile error. Both are the same shape -- two halves of one decision disagreeing about what a column costs.'). work_done('improvise-8tk',bug_fix,'ZZ/:wq quit even when the save failed, silently discarding the model. Save now aborts the effect batch on failure; save_to_current_path/1 returns Result; three tests in ui/effect.rs.'). work_done('improvise-907',bug_fix,'Import wizard step machine redesign: validation_error() gate at ReviewProposals (no-category and over-limit), history-stack previous() bound to Left on every step, and Done made a real confirmation step holding the built workbook. Superseded improvise-gfq, improvise-o1c, improvise-c6j.'). work_done('improvise-6sq',wont_do,'Merging EditOrDrill and EnterEditAtCursorCmd into one EnterEdit{mode} is not safe: they differ in WHEN the cell value is read, not in a mode parameter. Added ui/app.rs::tests::i_and_o_diverge_on_an_empty_records_grid as executable evidence.').