diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl index 6093dda8..f130b34c 100644 --- a/.beads/issues.jsonl +++ b/.beads/issues.jsonl @@ -40,7 +40,8 @@ {"_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-mcq4","title":"The frame fingerprint is a 4x regression on the held-key path improvise-748f just optimized","description":"MEASURED, not suspected. Found while merging PR #78 (improvise-ni6k) with PR #76 (improvise-748f), which landed first.\n\nimprovise-748f split the geometry fingerprint so that a batch which writes NO cell -- the held-key path, [/] and axis cycle and hide/show -- stays O(config) instead of walking 18,000 cells. improvise-ni6k's App::view_frame builds a GridLayout per view via with_frozen_records, and with_frozen_records applies prune_empty, and prune_empty WALKS CELLS. So for a view with prune_empty on, ni6k puts the O(cells) walk straight back onto the path 748f had just taken it off.\n\nMEASURED on the merged tree, benches/reshape_observation.rs, one CycleAxis batch, prune_empty ON, frame fingerprint disabled vs enabled:\n\n 400 cells 93.6us -\u003e 283.2us 3.0x\n 9,600 cells 1.97ms -\u003e 7.34ms 3.7x\n 18,000 cells 3.04ms -\u003e 12.41ms 4.1x\n 90,000 cells 28.5ms -\u003e 91.7ms 3.2x\n\nThe 16.7ms frame budget is crossed at 18,000 cells, where the pre-fingerprint tree sat comfortably inside it at 3.0ms.\n\nWHY 3-4x AND NOT 2x: view_frames builds every view's layout, before AND after each model effect. The bench fixture has two views, so that is four pruned layout builds against the one rebuild_layout already does.\n\nWHY THE COST WAS NOT SEEN EARLIER: ni6k's own benchmark measures a SetCell batch, where 748f's document_content walk dominates and the fingerprint reads as +2 to +5 percent. And the geometry case, when first added, used the dense_app fixture as-is -- prune_empty OFF -- where with_frozen_records does not walk cells at all and a CycleAxis batch reads 17-176us. THAT FIXTURE MEASURES THE CHEAP HALF AND CALLS IT THE COST. It is the same non-discriminating-fixture failure this repo names everywhere; the numbers above come from turning prune_empty on, and the benchmark now does.\n\nOPTIONS, none taken:\n- Fingerprint only the ACTIVE view for the cursor and fall back to the cheaper config hash (last_reshaped_views) for the client audience. Over-resets bystanders on other views, which is the safe direction per gotcha 35, and removes the per-view multiplier. Probably the right answer.\n- Memoise the pruned layout across the before/after pair.\n- Accept, on the argument that prune_empty plus a large document plus a held key is rare. That argument needs the user, not an agent.\n\nDO NOT close this by re-running the benchmark with prune_empty off.","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T04:20:08Z","created_by":"claude-agent-protocol","updated_at":"2026-09-21T04:20:08Z","dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"improvise-ijql","title":"check-comment-runs.py --check-evidence silently skips the comment-run cap it is named for","description":"Found while landing improvise-kc7n (PR #84). The local gate in that brief ran scripts/check-comment-runs.py --check-evidence; it printed '572 entries, all name a test that exists' and exited 0. CI runs the SAME script with NO flags, which is the spelling that enforces the two-line comment-run cap, and it failed on a three-line clap doc comment in crates/improvise-ws-server/src/main.rs.\n\nSo one script answers two different questions and a FLAG selects which, with no indication in the output of the flagged run that the other check did not run. That is a gate reporting success on a question nobody asked -- the same shape as a check that cannot fail. Both spellings were green individually; only running both catches both faults.\n\nOptions, cheapest first: (a) --check-evidence ADDS the evidence audit rather than replacing the run-cap check, so one invocation answers both and no caller can pick the weaker one; (b) the flagged run prints an explicit 'comment-run cap NOT checked' line; (c) the flag is removed and the script always does both.\n\n(a) is the fix that makes the failure mode unrepresentable rather than documented. Note scripts/ was another wave's ground during kc7n, which is why this is filed rather than fixed there.","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T04:31:48Z","created_by":"Edward Langley","updated_at":"2026-09-21T04:31:48Z","dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"improvise-mcq4","title":"The frame fingerprint is a 4x regression on the held-key path improvise-748f just optimized","description":"MEASURED, not suspected. Found while merging PR #78 (improvise-ni6k) with PR #76 (improvise-748f), which landed first.\n\nimprovise-748f split the geometry fingerprint so that a batch which writes NO cell -- the held-key path, [/] and axis cycle and hide/show -- stays O(config) instead of walking 18,000 cells. improvise-ni6k's App::view_frame builds a GridLayout per view via with_frozen_records, and with_frozen_records applies prune_empty, and prune_empty WALKS CELLS. So for a view with prune_empty on, ni6k puts the O(cells) walk straight back onto the path 748f had just taken it off.\n\nMEASURED on the merged tree, benches/reshape_observation.rs, one CycleAxis batch, prune_empty ON, frame fingerprint disabled vs enabled:\n\n 400 cells 93.6us -\u003e 283.2us 3.0x\n 9,600 cells 1.97ms -\u003e 7.34ms 3.7x\n 18,000 cells 3.04ms -\u003e 12.41ms 4.1x\n 90,000 cells 28.5ms -\u003e 91.7ms 3.2x\n\nThe 16.7ms frame budget is crossed at 18,000 cells, where the pre-fingerprint tree sat comfortably inside it at 3.0ms.\n\nWHY 3-4x AND NOT 2x: view_frames builds every view's layout, before AND after each model effect. The bench fixture has two views, so that is four pruned layout builds against the one rebuild_layout already does.\n\nWHY THE COST WAS NOT SEEN EARLIER: ni6k's own benchmark measures a SetCell batch, where 748f's document_content walk dominates and the fingerprint reads as +2 to +5 percent. And the geometry case, when first added, used the dense_app fixture as-is -- prune_empty OFF -- where with_frozen_records does not walk cells at all and a CycleAxis batch reads 17-176us. THAT FIXTURE MEASURES THE CHEAP HALF AND CALLS IT THE COST. It is the same non-discriminating-fixture failure this repo names everywhere; the numbers above come from turning prune_empty on, and the benchmark now does.\n\nOPTIONS, none taken:\n- Fingerprint only the ACTIVE view for the cursor and fall back to the cheaper config hash (last_reshaped_views) for the client audience. Over-resets bystanders on other views, which is the safe direction per gotcha 35, and removes the per-view multiplier. Probably the right answer.\n- Memoise the pruned layout across the before/after pair.\n- Accept, on the argument that prune_empty plus a large document plus a held key is rare. That argument needs the user, not an agent.\n\nDO NOT close this by re-running the benchmark with prune_empty off.","notes":"FIXED to 1.9x, not closed: one pruned build per model effect remains.\n\nThe coordinator ruled this must be fixed before PR #78 lands, and was right -- a cost measured to four significant figures is a decision to ship, not a thing to file.\n\nWHAT WORKED, AND IT IS NOT WHAT THE TICKET PROPOSED. The ticket's first option (and my own suggestion) was to fingerprint only the ACTIVE view and fall back to the config hash for the client audience. Implemented and measured: it bought NOTHING -- 12.41ms -\u003e 12.56ms at 18,000 cells. The bench's second view is one created fresh with everything on Axis::None, so building it costs nothing. THE PER-VIEW MULTIPLIER WAS NEVER THE COST; two EXPENSIVE builds were. That is only known because it was measured after implementing rather than reasoned about before.\n\nWHAT ACTUALLY WORKED: the opening basis is free. At batch start App::layout already IS the active view's pruned layout, so opening_view_frame() hashes it instead of building a second one. frame_of() is the shared hash and view_frame() builds then calls it.\n\n one CycleAxis batch, prune_empty ON:\n before after fingerprint off\n 400 cells 283.2us 167.0us 93.6us\n 9,600 cells 7.34ms 3.879ms 1.967ms\n 18,000 cells 12.41ms 5.752ms 3.043ms\n 90,000 cells 91.69ms 56.54ms 28.52ms\n\n4.1x -\u003e 1.9x at the canonical size, back inside the 16.7ms frame budget.\n\nTwo guards on the free basis: it is gated on drill_state.is_none(), because a drill freezes the record list and the held layout is then not what view_frame would build; and it is opening-use only, because App::layout is stale mid-batch (gotcha 23).\n\nTHE NARROWING WAS KEPT anyway, because it is the honest shape -- the cursor's question is about one view. The audience half is now the config diff observe_reshape already computes free. It over-reports (safe direction) and is read only behind the epoch gate in adopt_cursor_epoch, so its imprecision cannot reach a batch that reframed nothing. a_reshape_of_one_view_leaves_a_client_on_another_view_alone still passes, so it has not widened into improvise-nynk's negative arm.\n\nBOTH SUBSUMPTION PROOFS RE-RUN AFTER THE NARROWING, since narrowing is exactly what could quietly stop covering something: #76's own two arms still pass with no per-effect reset anywhere; AddFormula/RemoveFormula and SortData are still covered; disabling the narrowed observation fails ten tests.\n\nWHAT REMAINS, AND WHY THIS STAYS OPEN: one pruned layout build per model effect. The LAST one duplicates work rebuild_layout does immediately afterwards, and eliding it means moving the check after the loop -- which is the position that clobbers a deliberate cursor placement in the same batch (the three records-mode tests that caught it the first time). A restructure that knows which effect is last, including on abort, is possible and was not attempted at this hour.\n\nThe benchmark now carries the held-key case with prune_empty ON and a comment saying not to turn it off, because that fixture is the only reason any of this was visible.","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T04:20:08Z","created_by":"claude-agent-protocol","updated_at":"2026-09-21T04:33:07Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-5ubs","title":"A cyclic-successor all() can silently omit a variant while its doc says it cannot","description":"Found by the adversarial review of PR #78, which probed it rather than reading the doc comment.\n\nAppMode::all() and Rejection::all() walk a total, cyclic after(). The claim in both doc comments is that a new variant cannot be omitted, because after() has no None arm so the only way to answer for a new variant is to NAME ANOTHER ONE.\n\nThat is half true and the half it gets wrong is the half that matters. ANSWERING after() FOR a new variant is forced by the compiler. POINTING AN EXISTING ARM AT IT IS NOT. A probe variant whose after() arm returns some existing variant, with nothing in the cycle pointing back to it, is simply never walked: all().len() came back 17, not 18, with the suite green.\n\nSo the consolidation replaced five grep-visible lists with one derivation whose totality is asserted in prose and checked by nothing. THE REVIEWER'S META-POINT IS THE REAL TICKET: five disagreeing AppMode lists were visible to grep; all() under-enumerating is visible to nothing. A consolidation owes a test that the surviving derivation is TOTAL, not merely that it agrees with what it replaced.\n\nWHAT IS STILL BETTER THAN BEFORE, so this is not a revert: the compiler now forces a human to look, and two of the five lists it replaced were ALREADY WRONG (both missing SavePrompt). This is a gap in a guarantee, not a regression.\n\nOPTIONS, roughly in order of cost:\n- A test that walks the cycle and asserts every variant is visited, driven by something independent of after() -- which needs an independent enumeration, and that is the problem restated.\n- A derive macro or a dependency (strum). Rejected during PR #78 only because adding a workspace dep mid-wave collides in Cargo.toml; on its own it is the honest answer and cheap.\n- Accept and document the residual precisely, replacing both doc comments, which currently overclaim.\n\nWHATEVER IS CHOSEN, FIX THE DOC COMMENTS FIRST: they presently say the omission is impossible, and a reader checking this property will believe them. An overclaiming comment is worse than none, because it stops the next person looking.","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-21T04:01:27Z","created_by":"claude-agent-protocol","updated_at":"2026-09-21T04:01:27Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-349m","title":"format_md/parse_md is not idempotent when a .improv file has two Category sections with the same name","description":"format -\u003e parse -\u003e format differs, the two sides differing only in the order of '\u003e' group lines. DETERMINISTIC -- 8 failures in 8 runs on unmodified main (00f7678) -- so it is NOT gotcha 18's HashSet shape.\n\nCause: the file carries two '## Category: ||' sections with the SAME name. The second merges into the first and its group lines are appended in a position the second parse does not reproduce.\n\nReproducer, from the proptest's own minimal input:\n\n let file = \"v2025-04-09\\n# |\\\\a|\\n## View: a\\n_-: row\\ncollapsed: ||/A--\\n\\\n ## Category: ||\\n- |\\\\a\\\\a\\\\a|,|\\\\a\\\\a|,|\\\\a|,|\\\\a|\\n\u003e _---[|\\\\a|]\\n\u003e _[|\\\\a|]\\n\\\n ## Category: ||\\n- ||[||]\\n\u003e ||[||]\\n\";\n let m1 = parse_md(file).unwrap();\n let printed = format_md(\u0026m1);\n assert_eq!(printed, format_md(\u0026parse_md(\u0026printed).unwrap())); // fails\n\nWHY IT LOOKS FLAKY AND IS NOT: persistence::parse::grammar_prop_tests::generated_file_roundtrips goes red only when the grammar-walking generator happens to emit a duplicate section header, which is rare. It was observed once during PR #70's CI, the rerun passed, and the honest move was to take proptest's minimal input and try it deterministically rather than rerun and move on. That is what turned a 'flake' into a filed bug.\n\nFound by the improvise-pdvm/75ld agent on PR #70; not caused by that PR. bd search for idempotent, roundtrip, duplicate category, format_md and group order returned nothing, so it was not already filed.\n\nWorth deciding as part of the fix: is a duplicate '## Category:' header legal input at all? If it is not, the parser should refuse it and the generator should stop emitting it; if it is, the merge must produce a canonical group order. The round-trip property cannot hold without one of those answers.","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-20T23:42:59Z","created_by":"orchestrator","updated_at":"2026-09-20T23:42:59Z","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"improvise-v5pc","title":"restore the browser assertion that ? opens help","description":"36ade35 moved the shell suite to node and deleted harness/tests/widgets.spec.js, taking the Playwright test '? opens help, and its rows are the shared ones' with it. Nothing replaced it: no node test and no spec asserts that the help overlay is drawn, that its active tab is the shared module's page title, or that l pages it. The evidence entry in context/test-evidence/clients.md survived the deletion and went on promising the coverage for a whole release -- the improvise-5ray cost, now caught by --check-evidence, and the heading is demoted to prose rather than deleted so its two injections (dropping the paintHelp call; removing the mode != Help guard in help_rows) survive for whoever restores it. improvise-ktv1's acceptance -- a browser shows help -- is currently unasserted end to end.","status":"closed","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-20T23:25:28Z","created_by":"claude-agent-scripts","updated_at":"2026-09-21T00:06:02Z","closed_at":"2026-09-21T00:06:02Z","close_reason":"Duplicate of improvise-78v6, already fixed and merged in PR #72. harness/tests/help.spec.js on main carries test(\"? opens help, and its rows carry the keys and the words, not empty frames\") plus a second test that reads an advertised key out of the DOM, presses it, and asserts the selection moved.\n\nWorth noting HOW this duplicate happened, because the filing agent did nothing wrong: it searched bd before filing, as AGENTS.md requires, and found nothing -- because the fix was in a sibling agent's UNMERGED PR at the time. A parallel wave makes in-flight work invisible to bd search. The cheap addition to the wave brief is to check open PRs, not only the tracker, before filing something a sibling unit might own.","dependency_count":0,"dependent_count":0,"comment_count":0} @@ -72,7 +73,7 @@ {"_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-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":"open","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-20T07:57:18Z","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-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} {"_type":"issue","id":"improvise-p1ym","title":"Project the page-axis item lists so [ and ] work in a browser","description":"The one key pair an independent smoke test found inert that the shared keymap did NOT fix, and it is a genuine projection gap rather than a resolution gap.\n\nGridCache::page_coords carries (category, SELECTED ITEM). PageNext/PagePrev need THE ITEM LIST TO CYCLE THROUGH, which nothing projects. So [ and ] resolve to a command name the client holds but cannot compose an argument for, and they reach Host::handle empty vec -- doctrine(an_upstream_key_is_a_black_hole), so they do nothing at all.\n\nCONFIRMED BY SMOKE TEST 2026-09-20 against the live page: \"[ / ] did not cycle Quarter (never saw Q2 ~17,100)\". The demo model puts Quarter on the page axis specifically so paging is a thing a visitor would try.\n\nTHE AGENT WAS TOLD NOT TO FORCE IT and did not -- a truthful \"these four keys work, [ / ] needs X\" is worth more than five half-working keys.\n\nSCOPE: extend page_coords (or add a sibling) to carry the item list per page-axis category. Note gotcha 27 -- \"what data is visible\" is View::filtering_cats() (Page UNION Filter) while \"what the user can page\" is categories_on(Axis::Page). This needs the SECOND one; projecting the first would let a client try to cycle a Filter axis, which by design it must not.\n\nWhile you are there: check whether the item list should be the EFFECTIVE items (hidden items excluded) rather than all of them, since [ and ] in the TUI cycle what the user can actually select.","acceptance_criteria":"GridCache carries the item list per page-axis category, so PageNext/PagePrev compose client-side and [ / ] cycle Quarter on the live page. It projects categories_on(Axis::Page), NOT filtering_cats() — projecting the latter would let a client try to cycle a Filter axis, which by design it must not (gotcha 27). Decide and state whether the list is the EFFECTIVE items (hidden excluded), since that is what the TUI cycles. Verified against the demo, which puts Quarter on the page axis precisely because paging is a thing a visitor tries.","status":"closed","priority":2,"issue_type":"task","assignee":"wave2-b-projection","owner":"el-github@elangley.org","created_at":"2026-09-20T04:38:45Z","created_by":"Edward Langley","updated_at":"2026-09-20T18:05:59Z","started_at":"2026-09-20T18:00:04Z","closed_at":"2026-09-20T18:05:59Z","close_reason":"Closed","labels":["area:projection"],"dependency_count":0,"dependent_count":0,"comment_count":0} diff --git a/.claude/skills/code-review/SKILL.md b/.claude/skills/code-review/SKILL.md index de9fd46f..46dbceb2 100644 --- a/.claude/skills/code-review/SKILL.md +++ b/.claude/skills/code-review/SKILL.md @@ -103,8 +103,18 @@ that cannot fail in the direction the defect lies. cargo test --workspace --no-fail-fast # fail-fast hides the blast radius cargo clippy --workspace --all-targets -- -D warnings cargo fmt --all --check +python3 scripts/check-comment-runs.py # the two-line comment cap +python3 scripts/check-comment-runs.py --check-evidence # a DIFFERENT question ``` +**Run both spellings of `check-comment-runs.py`.** It is one script answering +two questions and the flag *selects* rather than adds, so `--check-evidence` +never runs the comment cap and says nothing about having skipped it. Three +separate agents hit this in one campaign: a local gate using only the flagged +form went green while CI, which runs both, went red on a three-line doc comment +(improvise-ijql). The flagged form is also the expensive one — it needs a +compiled workspace — which makes it the one people run alone. + The knowledge base is checked in CI as code: ```bash diff --git a/context/repo-map.md b/context/repo-map.md index 79d67a7b..9d9d2db4 100644 --- a/context/repo-map.md +++ b/context/repo-map.md @@ -165,7 +165,7 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text 4. **Commands never mutate.** `&CmdContext` is read-only; return `Vec>`. If you want to touch `&mut App`, add or extend an Effect. 5. **`CellKey` is always sorted.** Use the constructors; equality and hashing rely on canonical form. 6. **No `Model::add_item`.** With a `Workbook` in hand use the facade: `wb.add_item("Region", "East")?` / `add_item_in_group` / `remove_item` (improvise-tvt). With only a `Model`, go through the category: `m.category_mut("Region").unwrap().add_item("East")`. -7. **Virtual categories.** `_Measure` items = explicit items ∪ formula targets. Use `measure_item_names()` / `effective_item_names("_Measure")`. `add_formula` does *not* add items to `_Measure`. Formula-target lookup also falls back into `_Measure` via `Model::find_item_category`. Cell aggregation goes through `apply_agg`, formula `SUM(…)`/`AVG(…)` through `eval_agg` — they differ on empty input (absent vs. A third rule joins them for **non-numeric** values: `aggregate_cell_values` reduces text by **unanimity** — one matching value, or several copies of the same value, is that value; differing values, or text among numbers, is `CellValue::Error("mixed")` (improvise-l4qx). The **all-numeric branch is checked first**, so `Count` of a lone number is still `1` and not the number — that ordering is the whole difference between this rule and "a single value passes through". What the rule forbids is *dropping*: `filter_map(as_f64)` used to, and since `_Index`/`_Dim` are on `Axis::None` in every default view that made **every text cell in every document render blank**, including the pipe-quoted text `.improv` carries as a first-class value. `evaluate_aggregated` and `aggregate_raw` share the helper, so a formula ref over mixed values now errors `"mixed"` rather than silently summing the numeric ones. `0` for Sum/Count, error for Avg/Min/Max). +7. **Virtual categories.** `_Measure` items = explicit items ∪ formula targets. Use `measure_item_names()` / `effective_item_names("_Measure")`. `add_formula` does *not* add items to `_Measure`. Formula-target lookup also falls back into `_Measure` via `Model::find_item_category`. Cell aggregation goes through `apply_agg`, formula `SUM(…)`/`AVG(…)` through `eval_agg` — they differ on empty input (absent vs. A third rule joins them for **non-numeric** values: `aggregate_cell_values` reduces text by **unanimity** — one matching value, or several copies of the same value, is that value; differing values, or text among numbers, is `CellValue::Error("mixed")` (improvise-l4qx). The **all-numeric branch is checked first**, so `Count` of a lone number is still `1` and not the number — that ordering is the whole difference between this rule and "a single value passes through". What the rule forbids is *dropping*: `filter_map(as_f64)` used to, and since `_Index`/`_Dim` are on `Axis::None` in every default view that made **every text cell in every document render blank**, including the pipe-quoted text `.improv` carries as a first-class value. `evaluate_aggregated` and `aggregate_raw` share the helper, so a formula ref over mixed values now errors `"mixed"` rather than silently summing the numeric ones. `0` for Sum/Count, error for Avg/Min/Max). `Model::evaluate_aggregated` takes `Aggregation::{Exact, OverHidden}` rather than `none_cats: &[String]` (improvise-5jm5) — the list was only ever tested for emptiness, and the guard against that shape returning is the **signature**. `recompute_formulas`/`aggregate_raw` keep the list because they walk it. 8. **Display rounding is view-only.** `format_f64` (half-away-from-zero) is only called in rendering. Formula eval uses full `f64`. Never feed a rounded value back into eval. 9. **Formula eval is fixed-point.** `recompute_formulas(none_cats)` iterates until stable, bounded by `MAX_EVAL_DEPTH`; circular refs converge to `CellValue::Error("circular")`. `App::new` runs it before the first frame so formula cells render on startup. 10. **Drill into formula cells** strips the `_Measure` coordinate when it names a formula target, so `matching_cells` returns the raw records that feed the formula instead of returning empty. @@ -203,7 +203,7 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text 33. **The grid's labels frame its cells, and `apply` enforces it — the fold is not commutative.** Three consequences in `improvise-protocol/src/render.rs`. (a) A `GridUpdate::Cells` that overtakes the `RowFrame`/`ColFrame` naming its positions is **dropped**, because `fit_to_labels` truncates to the labels it has; a client must not reorder the stream, and `a_cell_update_that_overtakes_its_labels_is_lost` pins it. (b) A label change that *renames* a position blanks the cell under it: `fit_to_labels` preserves by POSITION, which is only right where the label did not change, so `apply` calls `invalidate_relabelled` itself rather than trusting a producer to send `GridUpdate::Invalidated` first. `Invalidated` therefore now means only "contents stale, frame unchanged". (c) **every arm that touches labels *or* widths re-fits** — `col_widths` is re-fitted with the columns — a new column is drawn at its label's own character count until `ColWidths` catches up — because a renderer zips widths with labels and a length mismatch indexes off the end. Fitting on the label arms alone is what let a late `ColWidths` put the mismatch straight back — a short update keeps each unheard-of column at its label's width, and a long one is dropped as having overtaken the frame that names it, exactly as (a) holds `Cells`. `GridCache` also carries `totals` and `page_coords` (improvise-n10). `totals` is framed by `col_frame` on the same terms as the cells and widths and re-fits with them, with one asymmetry: an **empty** strip means "this grid has no totals" (records mode, or an empty grid) and is therefore *not* padded, where a non-empty one is — padding it would draw a blank total row under a records grid that has none. `Invalidated` blanks the totals too: they are contents, and a stale sum under fresh cells is the one number a user takes at face value. `page_coords` is framed by nothing (it names categories, not positions) and survives a reshape. Note the fold tests: asserting `fold(prefix)+fold(rest) == fold(whole)` is vacuous (it is the definition of a left fold), so the tests that earn their place are the order-sensitivity one and the compaction-equivalence one — see `doctrine(fold_tests_must_test_order_not_the_axiom)`. -34. **A formula target is unquoted, and pipe quoting has one home.** `parse_formula` strips the pipes off a target exactly as it does off an `Expr::Ref` — the target names a `_Measure` item, so a target that kept its delimiters made `add_formula` create an item literally named `|Total Revenue|`: a ghost column the user's real item never matched, and `Expr::Ref("Total Revenue")` never resolved to. Both the TUI formula minibuffer (`AddFormula`) and `.improv` load go through that one call, so both were affected. The writer emits `Formula::raw`, not `target`, so file text is unaffected either way — which is exactly why the round-trip tests never saw it, and why the fix repairs existing files on next load with no migration. Pipe quoting has exactly **one home**: `improvise_formula::quote`, owning both halves — `quote_name`/`is_bare_name`, `pipe_quote` (unconditional, for a `.improv` text cell that must not read back as a number) and `unescape_pipe_body`. `improvise-gp` re-exports it, `improvise-io` calls it from `persistence/{format,parse}.rs`, `parser.rs::unquote_pipe` is a shim over it, and io's own copy is **deleted** (improvise-i2m5). **The keyword guard is canonical — user-confirmed, so do not revert it as over-caution**: an item literally named `SUM` is written `|SUM|`. Bare keywords do parse, but only because `agg_call` needs a `(` and PEG backtracking falls through to `ref_expr` — rule ordering, not a stated rule — and a `## Formulas` line carries formula text verbatim, so a name `format_md` writes is read back by `parse_formula`: one namespace, one spelling rule. One thing the guard does not cover, and one that used to be an exception and no longer is. **A section header takes its name as the whole line, totally** (improvise-pczv): `header_name = { pipe_quoted ~ &NEWLINE | line_name }` — the pipe-quoted arm needs the lookahead, or `|A|B` would parse as `A` and silently drop `B`, and `line_name` catches everything else, so a header name can no longer fail to parse. Four rules share it (`model_name`, `initial_view`, `category_section`, `view_section`), which is why it is a rule and not four copies. Note a header is still *written* raw where a reference is quoted — the grammar accepts both arms, so the quoting asymmetry between a header and a reference is a writer convention, not a format one. And the two former *unquoting* copies differed only on a trailing lone backslash, unreachable from the grammar, where io returned it and `parser.rs` panicked — the surviving `unescape_pipe_body` is total. +34. **A formula target is unquoted, and pipe quoting has one home.** `parse_formula` strips the pipes off a target exactly as it does off an `Expr::Ref` — the target names a `_Measure` item, so a target that kept its delimiters made `add_formula` create an item literally named `|Total Revenue|`: a ghost column the user's real item never matched, and `Expr::Ref("Total Revenue")` never resolved to. Both the TUI formula minibuffer (`AddFormula`) and `.improv` load go through that one call, so both were affected. The writer emits `Formula::raw`, not `target`, so file text is unaffected either way — which is exactly why the round-trip tests never saw it, and why the fix repairs existing files on next load with no migration. Pipe quoting has exactly **one home**: `improvise_formula::quote`, owning both halves — `quote_name`/`is_bare_name`, `pipe_quote` (unconditional, for a `.improv` text cell that must not read back as a number) and `unescape_pipe_body`. `improvise-gp` re-exports it, `improvise-io` calls it from `persistence/{format,parse}.rs`, `parser.rs::unquote_pipe` is a shim over it, and io's own copy is **deleted** (improvise-i2m5). **The keyword guard is canonical — user-confirmed, so do not revert it as over-caution**: an item literally named `SUM` is written `|SUM|`. Bare keywords do parse, but only because `agg_call` needs a `(` and PEG backtracking falls through to `ref_expr` — rule ordering, not a stated rule — and a `## Formulas` line carries formula text verbatim, so a name `format_md` writes is read back by `parse_formula`: one namespace, one spelling rule. One thing the guard does not cover, and one that used to be an exception and no longer is. **A section header takes its name as the whole line, totally** (improvise-pczv): `header_name = { pipe_quoted ~ &NEWLINE | line_name }` — the pipe-quoted arm needs the lookahead, or `|A|B` would parse as `A` and silently drop `B`, and `line_name` catches everything else, so a header name can no longer fail to parse. Four rules share it (`model_name`, `initial_view`, `category_section`, `view_section`), which is why it is a rule and not four copies. Note a header is still *written* raw where a reference is quoted — the grammar accepts both arms, so the quoting asymmetry between a header and a reference is a writer convention, not a format one. And the two former *unquoting* copies differed only on a trailing lone backslash, unreachable from the grammar, where io returned it and `parser.rs` panicked — the surviving `unescape_pipe_body` is total. Two `## Category:` sections naming one category are **one** category, concatenated by `ParsedFile::push_category` in **pass 1** (improvise-349m) — merging in pass 2 was not idempotent, because `build_category` creates its own section's item-groups before its own `>` lines while `format_md` writes every item before every `>` line. No grammar change was needed: this is the tree-walker, not the format, and duplicate headers stay legal. And **`WHERE` now carries `IS`'s word boundary** — both are compound-atomic with `sp+` (improvise-r08u), so `SUM(Rev WHEREAS x = y)` is a syntax error rather than a `WHERE` on a category `AS`. The guard is `sp+` and **not** the `!bare_char` lookahead the ticket proposed, because the grammar-walking generator reads the grammar through `pest_meta` and **skips predicates**, emitting `WHEREa=""` — a rule whose only guard is a lookahead is a rule the generator can violate. It costs one spelling: a category may no longer abut the keyword. 35. **`Dirty` over-reports on purpose, and the asymmetry is the reason.** `Dirty::of` maps an `EffectKind` to the widgets that kind can reach, and the two directions do not cost the same: over-reporting pays one redundant `Projection`, under-reporting leaves a client's cache wrong **forever**, with nothing in the stream to correct it. `LayoutGeometry` therefore includes `formulas`, because `RemoveCategory` wears it and `Model::remove_category` ends by dropping every formula targeting the category — a client's formula panel otherwise kept showing a formula the document no longer had. It now coincides with `Dirty::EVERYTHING` and is still spelled out, because the two are derived from different effects and need not stay equal. The derivation now lives in `context/code/command.md`, and the doc comment on `Dirty::of` points there rather than restating it; the reason `LayoutGeometry` is spelled out rather than written as `EVERYTHING` is a two-line comment inside the match body. It went stale once already — claiming `AddCategory` was `CellData` after it was reclassified — so reclassifying an effect means re-reading the prose, not just the comment. @@ -256,11 +256,13 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text **A third thing no test here can see, which is why the docs must be read as code.** Three comments in one range said "not built yet" about work that had landed *in that same range* — `README.md`'s browser section, `app.js`'s keyboard note, and `Host::handle`'s `Key` arm — each because a follow-up commit updated **one copy of an argument and not its siblings**. When a fact is stated in more than one place, changing it is a grep, not an edit. +49. **A websocket handshake is exempt from the same-origin policy, and that is a server-side problem.** There is no CORS preflight, so any page a user visits can open a socket to `improvise-ws-server` *from that user's own allowed address* and read and rewrite every document it holds — cross-site websocket hijacking (improvise-kc7n). **No network control can see it**: an IP allow-list, a private network and loopback binding all answer "may this **client** reach the endpoint", and the honest answer is yes; only `Origin` says what is **driving** the client. **Loopback is the least protected case, not the safest** — no proxy in front of it, and loopback is not a boundary a browser enforces. `src/origin.rs` is the whole decision and `transport::admitted` its one caller; it refuses the **upgrade** with a 403 rather than accepting a socket and ignoring it, and `serve` **takes** the policy so a host cannot forget to apply it. Three arms are decided rather than defaulted: **loopback allowed** (the shipped local recipe serves its page from `http://localhost:8080`, so the one case worth optimising for must need no flag), **absent allowed** (no browser omits `Origin`, so absent is a non-browser client an address check already answers — and refusing it breaks `tests/socket.rs`, the repo's own instrument), and **two refused** (`HeaderMap::get` is first-wins, so a proxy that *appends* rather than sets leaves two, and answering from the first lets the second be anything). The deployed Pages origin is deliberately **not** hard-coded: it can only reach a plain-`ws://` server through a TLS proxy anyway, so one flag is an honest cost on an already multi-step path where a baked-in default would be a permanent hole for every other deployment. The residual hole is **stated rather than guarded** — a proxy that *strips* `Origin` turns the check off, which is why the proxy's own allow-list stays in `README.md` as defence in depth. **None of this is authentication**: it answers what page is driving a client, never who is connecting, and improvise-kpnr is still open. It correctly does **not** move to `improvise-command` beside `Host` (gotcha 45) — a `MessagePort` has no handshake and no headers, and a worker and its page are the same origin by construction, so this is genuinely transport-only and not a mirror waiting to drift. + --- ## File Inventory -Line counts are static; test counts are informational — run `cargo test --workspace` for live numbers. Files under 100 lines and render-only widgets omitted. +Line counts are a **dated snapshot**, not a maintained fact: measured 2026-09-21, and **26 of 61 had drifted past 10%** before that refresh — several by a third. Treat them as "is this file big", never as a figure to cite; re-derive with `wc -l`. Note the `ws-server/` and `improvise-worker-server/` entries use a shorthand prefix that no path-walking refresh resolves, so they drift silently and must be checked by hand. Test counts are informational — run `cargo test --workspace` for live numbers. Files under 100 lines and render-only widgets omitted. ### `improvise-core` (`crates/improvise-core/src/`) ``` @@ -285,7 +287,7 @@ quote.rs 192 / 7t quote_name / is_bare_name / pipe_quote / une the one home, both halves (improvise-i2m5). improvise-gp re-exports it; improvise-io calls it from persistence/{format,parse}.rs -formula.pest 92 PEG grammar for formula text +formula.pest 103 PEG grammar for formula text ast.rs 77 Expr, BinOp, AggFunc, Formula, Filter ``` @@ -369,27 +371,27 @@ improvise-gp/src/fit.rs 1426 / 23t one fit end to end: a View's records in, ### Command layer (`crates/improvise-command/src/command/`) ``` -cmd/core.rs 327 / 2t Cmd trait, CmdContext, CmdRegistry, parse helpers -cmd/registry.rs 749 / 3t default_registry() — all registrations -cmd/navigation.rs 534 / 12t Move, EnterAdvance, Page* -cmd/cell.rs 192 / 6t ClearCell, YankCell, PasteCell, TransposeAxes, SaveCmd -cmd/commit.rs 723 / 11t CommitAndAdvance, CommitBuffer(CommitKind), CommitExport, +cmd/core.rs 333 / 2t Cmd trait, CmdContext, CmdRegistry, parse helpers +cmd/registry.rs 822 / 3t default_registry() — all registrations +cmd/navigation.rs 502 / 12t Move, EnterAdvance, Page* +cmd/cell.rs 194 / 6t ClearCell, YankCell, PasteCell, TransposeAxes, SaveCmd +cmd/commit.rs 739 / 11t CommitAndAdvance, CommitBuffer(CommitKind), CommitExport, CommitSave -cmd/effect_cmds.rs 460 / 5t effect_cmd! macro, 25+ simple wrappers -cmd/fit.rs 382 / 7t fit-measure: evolve a tree over the active view and store it +cmd/effect_cmds.rs 459 / 5t effect_cmd! macro, 25+ simple wrappers +cmd/fit.rs 280 / 7t fit-measure: evolve a tree over the active view and store it as an ordinary `## Formulas` line — nothing GP-specific is left in the document afterwards -cmd/grid.rs 512 / 9t ToggleGroup, ViewNavigate, DrillIntoCell, TogglePruneEmpty -cmd/mode.rs 427 / 11t EnterMode, Quit, EditOrDrill, EnterTileSelect -cmd/panel.rs 713 / 17t Panel toggle/cycle/cursor, formula/category/view panels -cmd/search.rs 202 / 4t SearchNavigate, SearchOrCategoryAdd, ExitSearchMode -cmd/text_buffer.rs 434 / 14t AppendChar, PopChar, CommandModeBackspace, ExecuteCommand -cmd/tile.rs 139 / 5t MoveTileCursor, TileAxisOp -cmd/wizard.rs 253 / 6t HandleWizardKey — per-step import-wizard key tables -keymap.rs 1117 / 35t KeymapSet, Keymap::dispatch/dispatch_transient, effects_of, +cmd/grid.rs 540 / 9t ToggleGroup, ViewNavigate, DrillIntoCell, TogglePruneEmpty +cmd/mode.rs 414 / 11t EnterMode, Quit, EditOrDrill, EnterTileSelect +cmd/panel.rs 688 / 17t Panel toggle/cycle/cursor, formula/category/view panels +cmd/search.rs 206 / 4t SearchNavigate, SearchOrCategoryAdd, ExitSearchMode +cmd/text_buffer.rs 382 / 14t AppendChar, PopChar, CommandModeBackspace, ExecuteCommand +cmd/tile.rs 138 / 5t MoveTileCursor, TileAxisOp +cmd/wizard.rs 244 / 6t HandleWizardKey — per-step import-wizard key tables +keymap.rs 917 / 35t KeymapSet, Keymap::dispatch/dispatch_transient, effects_of, SetTransientPrefix — what a thin client cannot have. The table itself is improvise-protocol/src/keymap.rs -parse.rs 236 / 19t Script/command-line parser (prefix syntax) +parse.rs 232 / 19t Script/command-line parser (prefix syntax) ``` ### Semantic UI state (`crates/improvise-command/src/ui/`) @@ -422,19 +424,23 @@ main.rs 393 CLI entry (clap): open, import, cmd, script tests/grid_frame_agreement.rs 422 / 4t the two front ends draw the same grid frame (improvise-anr9) tests/records_wide_smoke.rs 67 / 1t -tests/shipped_documents_load.rs 59 / 1t -tests/web_demo_model.rs 87 / 2t every visible cell in web/demo.improv is a Record +tests/shipped_documents_load.rs 42 / 1t +tests/web_demo_model.rs 80 / 2t every visible cell in web/demo.improv is a Record ``` ### Browser host and client (`crates/improvise-ws-server/src/`, `crates/improvise-wasm-client/src/`, `web/`) ``` -ws-server/registry.rs 705 / 14t Registry of Hosts keyed by DocumentId; ClientId {document, +ws-server/registry.rs 595 / 14t Registry of Hosts keyed by DocumentId; ClientId {document, session} — a SessionId is unique only within its document. A document is never evicted while the server runs (ADR 001) ws-server/host.rs 23 / 0t a re-export shim only. The session loop is improvise-command/src/ui/host.rs (gotcha 45) -ws-server/transport.rs 286 / 1t tokio + tungstenite plumbing -ws-server/main.rs 171 / 4t CLI entry: serve a file on a port +ws-server/origin.rs 132 / 6t OriginPolicy: which origins may open a socket. Loopback by + default, --allow-origin widens, absent allowed, two refused + (improvise-kc7n). The only decision transport::admitted makes +ws-server/transport.rs 324 / 4t tokio + tungstenite plumbing, plus the Origin check at the + handshake — a refused upgrade, not an ignored socket +ws-server/main.rs 190 / 5t CLI entry: serve a file on a port; --bind, --allow-origin ws-server/tests/socket.rs 284 / 2t the one test that uses a real socket: handshake, JSON framing, SessionId routing, which document a URL asked for wasm-client/lib.rs 406 / 7t Client: the wasm-bindgen surface (init, on_message, on_frame, @@ -446,16 +452,16 @@ wasm-client/socket.rs 444 / 7t the fold (fold_frame/handle_frame), the two wasm-client/state.rs 310 / 5t ClientState: ViewState + one cache per widget, the fold wasm-client/tests/browser_round_trip.rs 442 / 5t improvise-1ey's scope against a real Host wasm-client/tests/lockstep.rs 606 / 13t host and client agree on a keypress -web/app.js 519 string calls only, no protocol knowledge, no framework. +web/app.js 496 string calls only, no protocol knowledge, no framework. Run under Chromium by harness/tests/shell.spec.js — see gotcha 48 for what that does and does not reach -web/worker.js 100 three string calls, no protocol knowledge. RUN by +web/worker.js 64 three string calls, no protocol knowledge. RUN by shell.spec.js's no-?ws path, with bindings.rs (gotcha 45) web/index.html, style.css the page; legibility only web/build.sh 38 wasm-pack build → web/pkg (gitignored). Needs wasm-opt = ["-O", "--enable-bulk-memory"] or the bundled wasm-opt rejects LLVM's memory.copy -web/assemble.sh 72 assembles the deployable site (copies web/*.improv) +web/assemble.sh 120 assembles the deployable site (copies web/*.improv) web/demo.improv 57 the document the site ships and seeds from. Every cell a Record — asserted (gotcha 46) ``` @@ -477,17 +483,17 @@ improvise-worker-server/tests/post_message.rs 381 / 7t main-thread traffic, ### Browser harness (`harness/`) — not a workspace member ``` -harness/playwright.config.js 77 shell.spec.js runs on CHROMIUM ONLY (engine-independent +harness/playwright.config.js 50 shell.spec.js runs on CHROMIUM ONLY (engine-independent logic; routeWebSocket is slower and flakier across three). opfs.spec.js runs chromium + firefox + webkit, and webkit provides NO OPFS at all, so it skips on a runtime probe -harness/tests/shell.spec.js 199 the first execution of web/app.js. A change to web/*.js +harness/tests/shell.spec.js 165 the first execution of web/app.js. A change to web/*.js belongs here (gotcha 48) -harness/tests/opfs.spec.js 102 drives the OPFS contract page and reads back its result -harness/opfs-contract/src/lib.rs 417 runs improvise_io::storage::contract::storage_contract +harness/tests/opfs.spec.js 79 drives the OPFS contract page and reads back its result +harness/opfs-contract/src/lib.rs 361 runs improvise_io::storage::contract::storage_contract against a real OpfsFs inside a real Web Worker harness/www/opfs/{index.html,worker.js} the page and worker that host it -harness/build.sh 27 wasm-pack build for the contract crate +harness/build.sh 35 wasm-pack build for the contract crate # gated by the `browser` job in .github/workflows/ci.yml, which can fail the build ``` diff --git a/prolog-sessions/kb.pl b/prolog-sessions/kb.pl index bd02ba48..43782468 100644 --- a/prolog-sessions/kb.pl +++ b/prolog-sessions/kb.pl @@ -117,6 +117,9 @@ invariant(no_input_line_is_dropped,'The Markdown renderer covers exactly the sub invariant(a_table_cell_boundary_is_an_unescaped_pipe_outside_a_code_span,'splitRow walks the row rather than split(\"|\"), because this corpus documents a file format whose own quoting syntax is |Time Period| and a cell containing one would be shredded into extra columns -- a row that still parses, so no column count could detect it. Latent when fixed, which is the point: the trigger is one ordinary sentence away in a document whose subject is pipe quoting.'). invariant(code_spans_stand_aside_for_emphasis,'Inline rendering replaces code spans with placeholders before matching emphasis, because this corpus routinely opens ** on one side of a code span and closes it on the other; matching per segment silently drops that bold with no visible symptom. Strong is matched before emphasis with a body that may contain a single * pair, because [^*]+ cannot match **a *b* c** and the leftover * then pairs across the bold -- which turned one sentence of gotcha 33 into three italic paragraphs.'). invariant(key_matching_is_variant_equality_never_unification,'scripts/kb-merge.pl compares clause keys with same_key/2 (copy_term + numbervars), never by unifying them. Unification matched too much -- a non-ground key probe(X) matched probe(a) and deleted it -- and MUTATED the incoming clause, binding X to a, so the clause written was not the clause supplied; the round-trip verify cannot see that, since it compares the file against an intended list already corrupted by the same unification (improvise-dj1l). A RULE keys on the whole clause, because one head shape with different bodies is ordinary Prolog. check_total/2 asserts the arithmetic the tool already PRINTED beside the delta size.'). +invariant(duplicate_category_sections_merge_in_pass_one,'Two ## Category: sections naming one category are ONE category, concatenated by ParsedFile::push_category in pass 1 (improvise-349m). Merging in pass 2 was not idempotent: build_category creates its own section item-groups before its own > lines, while format_md writes every item before every > line, so the reparse built the other order. push_category destructures PCategory so a fourth field cannot be silently dropped on the duplicate path. No grammar change was needed -- this is the tree-walker, not the format, and duplicate headers stay legal.'). +invariant(where_and_is_share_a_word_boundary,'where_clause, inline_where and is_expr are all compound-atomic with sp+ after the keyword (improvise-r08u). Non-atomic, pest auto-inserts WHITESPACE and SUM(Rev WHEREAS x = y) reads as a WHERE on a category AS. Note compound-atomicity ALONE refuses that input, since AS x cannot be one identifier, so the residue sp+ actually closes is WHEREAS=y -- a negative test needs both shapes.'). +invariant(evaluate_aggregated_takes_a_named_state,'Model::evaluate_aggregated takes Aggregation::{Exact, OverHidden}, not none_cats: &[String] (improvise-5jm5). The list was consulted once, for emptiness, in every revision since Axis::None was introduced -- the aggregation runs over key.0 via matching_values. Aggregation::of reduces at the boundary and the regression guard is the SIGNATURE. recompute_formulas and aggregate_raw keep &[String] because they walk the elements.'). doctrine(bug_first,'write a failing test demonstrating the bug before fixing it'). doctrine(unit_over_e2e,'prefer small unit tests over integration/e2e'). doctrine(suite_speed,'Two numbers, because the ambiguity between them is what let this drift: NO SINGLE TEST BINARY may exceed 2 seconds, and the WORKSPACE WALL CLOCK is the sum of them. Measured at 22d43b2: 1309 tests over 33 binaries, ~2.2s wall, longest single binary 1.42s -- so the workspace is already past 2s and the rule that bites is the per-binary one. Re-MEASURE rather than re-assert: cargo test --workspace prints a \"finished in\" per binary. Excludes compilation, and excludes harness/ (Playwright, minutes, its own CI job).'). @@ -240,6 +243,13 @@ doctrine(a_careful_comment_is_weak_evidence_that_anything_checks_it,'The compani doctrine(a_prepare_step_is_not_evidence_until_deleted,'A test-fixture prepare step is claimed load-bearing far more often than it is. Measured 2026-09-20 on every_layout_geometry_effect_is_visible_to_the_narrow_fingerprint: of four prepare steps documented as \"the reason the test is not vacuous\", only DeleteView and ShowItem are. ToggleGroup passes without one because ToggleGroup::apply calls toggle_group_collapse UNCONDITIONALLY -- the membership check gates only reset_cursor -- and TransposeAxes step is literally a no-op, because View::on_category_added already assigns the first regular category Row and the second Column. The check is cheap and nobody runs it: delete each prepare in turn and require the case to fail. A prepare documenting realistic input is fine; a prepare DESCRIBED as carrying the test and not carrying it is an entry that will be cited. Note the orchestrator brief asserting the replacement claim was itself wrong on two of the four, so this is not a failure of the original author alone.'). doctrine(a_precondition_asserted_after_the_act_asserts_nothing,'An assertion naming a fixture precondition must run BEFORE the code under test, or the code under test establishes it. Proven 2026-09-20 on selecting_the_page_already_selected_invalidates_no_cursor: \"the fixture was not on Q1\" ran after a SetPageSelection to Q1, so it could never see a fixture that started elsewhere. The property was still guarded -- a Q2 fixture failed -- but at a DIFFERENT assertion, under a message about renumbering. That split is the failure mode: the guard survives, the explanation drifts, and the next reader trusts the sentence rather than the assertion.'). doctrine(an_evidence_entry_that_measurement_contradicts_is_worse_than_none,'context/test-evidence records WHY a test can fail, so a mechanism stated there is load-bearing prose and will be cited. Proven 2026-09-20: the entry for every_layout_geometry_effect_is_visible_to_the_narrow_fingerprint claimed a dirty=false clear stopped the preparation satisfying the assertion. Removing the clear AND blinding document_config to 0 left all THIRTEEN cases reading false -- dirty is raised only inside App::apply_effects and no prepare runs an effect, so no preparation can set it. Nothing checks an evidence claim against the code: check-comment-runs.py --check-evidence verifies only that each heading names a test that exists. Re-run an evidence entry injection when you touch the test it describes. Probing this particular test needs the assert replaced by a print, because the loop aborts at case 1 under any injection blinding document_config -- which is probably how the wrong claim survived.'). +doctrine(a_grammar_guard_must_be_generatable,'A pest rule whose only guard is a negative lookahead is a rule the grammar-walking generator can violate: the generator reads the grammar through pest_meta and SKIPS PREDICATES, so a caret-WHERE followed by !bare_char emitted WHEREa=\"\" and generated_formula_parses went red. An explicit separator (sp+) is generatable where a lookahead is not. This is why WHERE took IS\'s sp+ rather than the !bare_char improvise-r08u proposed, at the cost of one accepted spelling: a category may no longer abut the keyword. Applies to improv.pest equally, and constrains every future change to either grammar in this repo.'). +doctrine(a_roundtrip_equality_is_blind_to_a_loss_both_sides_share,'fmt(parse(x)) == fmt(parse(fmt(parse(x)))) tests STABILITY of the printed form and says nothing about PRESERVATION. Demonstrated on improvise-349m: injecting a merge that DROPS a duplicate section left the whole improvise-io suite green at 2000 proptest cases while an item and its group line vanished from the document, because both operands lose the same thing. Main was unstable-but-lossless and the test could not tell that from stable-and-lossy. A preservation assertion must run BEFORE the equality. The sharpest form of check_the_checker yet seen here: the fault that survives is the one that changes BOTH OPERANDS.'). +doctrine(could_not_reproduce_and_does_not_reproduce_are_different_claims,'Filing the second having earned only the first is how a real defect gets closed. On improvise-gfvj an agent concluded SortData does not reorder; the mechanism was real, and the cause was that ToggleRecordsMode pushes effect::SortData as its FIRST effect, so any records-mode fixture built by pressing R has a pre-sorted store BY CONSTRUCTION. The failure to reproduce was a property of the fixture, not of the code. Before writing \"does not reproduce\", name what would have to be true for the bug to exist and check THAT -- a fixture that cannot reach the condition reports the condition absent. Sibling of a_benchmark_fixture_that_cannot_reach_the_slow_branch_measures_the_fast_one.'). +doctrine(a_script_with_two_modes_has_two_gates,'scripts/check-comment-runs.py runs the comment-run cap bare and the evidence cross-check under --check-evidence, and NEITHER MODE RUNS THE OTHER. Every wave brief this campaign, and the gate list in the code-review skill, named only --check-evidence -- so the cap was effectively unrun locally and caught only by CI. It caught one: an enum inserted between Model doc comment and its derive, leaving the enum carrying the comment and Model with none. When a gate grows a flag, the unflagged invocation stops being run by everyone who copied the command, and a gate nobody invokes is indistinguishable from a gate with nothing to catch.'). +doctrine(an_injecting_reviewer_needs_its_own_tree,'An adversarial reviewer given leave to inject faults RACES THE AUTHOR if they share a worktree: on wave F the reviewer subagent injections landed in the author tree mid-gate and the author own run reported a failure for a fault it had not written. Everything was restored cleanly, but the diagnosis cost real time and could have been read as a genuine regression. A reviewer that injects gets its own worktree or a scratch copy, always. Related and separately costly: the session SCRATCHPAD IS SHARED across wave agents, and one agent overwrote another pr-body.md mid-task -- unique filenames, or a per-agent subdirectory.'). +doctrine(who_may_connect_is_two_questions,'Ask of any access control whether it constrains the CLIENT or what may DRIVE the client. A websocket handshake is exempt from the same-origin policy, so an address allow-list, a private network and loopback binding all answer \"may this client reach the endpoint\" and answer YES to a cross-site hijack: the request genuinely comes from an allowed client, driven by a page the user never intended to trust. Only Origin separates them. Loopback is the LEAST protected case, having no proxy in front of it at all -- the intuition that it is the safe default is exactly backwards (improvise-kc7n).'). +doctrine(a_line_count_in_a_document_is_a_dated_snapshot,'repo-map File Inventory line counts were audited 2026-09-21: 26 of 61 resolvable entries had drifted past 10 percent, several by a third. They are useful as \"is this file big\" and useless as a figure to cite, which is how false precision enters a document that is otherwise carefully checked. Refreshed and labelled with the date and the method. Two traps found while doing it: a naive path-walking refresh resolves a bare name like lib.rs or render.rs against the FIRST matching crate and silently rewrites the wrong number (it reported lib.rs 561 -> 8), and the ws-server/ and improvise-worker-server/ entries use a shorthand prefix no such walk resolves at all, so they drift silently and must be checked by hand.'). 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.'). @@ -339,6 +349,10 @@ decision('improvise-864a',skip_a_superseded_pages_deploy,'Three merges within 20 decision('improvise-9c8c',gate_the_comment_cap_last,'The CI step enforcing the two-line comment cap and the test-evidence keys was held back until every unit of the comment wave had merged, so that the gate\'s first green run was evidence rather than vacuous.'). decision('improvise-kh8',landing_page_at_the_root,'USER RULING 2026-09-20: the landing page should be served at the root of the repository and link to the in-browser demo. Supersedes the agent\'s proposed landing_page_beside_the_app, which argued for the app at the root with the landing page at /landing/ on the grounds that the live app is the better front door; that argument is recorded as the reason the alternative was considered, and it was overruled. web/assemble.sh puts docs/index.html at the site root and moves the app bundle (index.html, app.js, style.css, worker.js, demo.improv, pkg/, worker-pkg/) under /app/, moving the two bundle dirs WITH the app so every relative path in the generated glue stays correct. The non-obvious cost: harness/tests/shell.spec.js navigates to /index.html six times, so a path move at the root silently retargets the whole shell spec at a different page. The deployed arrangement IS what the harness serves (improvise-vvl3), which is the property that makes the harness trustworthy and also the property that makes a layout change a test change.'). decision('pr82-fingerprint-guards-2026-09-20',a_test_reads_the_view_its_effect_writes,'App::view() resolves ViewState::active_view (this client) and Workbook::active_view() is the DOCUMENT DEFAULT; gotcha 29 makes them deliberately free to differ since improvise-jytm. A test asserting what a geometry effect did must read App::view(), the view the effect writes through view_mut(). They agree in a single-client fixture, so the mismatch is latent and survives review easily -- found only because a sibling guard added beside it used the other accessor, one test reading one view through two accessors.'). +decision('improvise-l8rh',ambiguous_date_is_a_known_limit,'Ruled by the user 2026-09-20: leave detect_date_format first-match. A day-first default only moves which half of the world imports silently wrong; the honest fix surfaces the ambiguity and changes FieldProposal shape plus the wizard step, which is deliberate work rather than a patch. Tolerable because improvise-7yx made it deterministic. Pinned by an_ambiguous_date_is_read_american_first with a_day_first_only_value_settles_the_ambiguity as the discriminating arm.'). +decision('improvise-kc7n',origin_default_is_loopback_not_the_projects_own_page,'improvise-ws-server refuses a handshake whose Origin is neither loopback nor named by --allow-origin. Loopback is the default because the shipped local recipe serves the page from http://localhost:8080 while the socket is on another loopback port, so the one case worth optimising for must need no flag. The deployed Pages origin is NOT hard-coded: it can only reach a plain-ws server through a TLS proxy, already a multi-step setup carrying an Origin allow-list, so one flag is an honest cost there where a hard-coded default would be a permanent hole for every other deployment.'). +decision('improvise-kc7n',an_absent_origin_is_allowed_and_two_are_not,'Absent is DECIDED, not defaulted: every browser sends Origin on a handshake unconditionally, so absent is a non-browser client (curl, a native client, the crate own tests) that an address check already answers, and refusing it would break the repo own instrument for nothing. TWO Origin headers are refused, because HeaderMap::get is first-wins and a proxy that APPENDS rather than sets leaves two -- answering from the first lets the second be anything. An Origin that is not valid text is refused on the same ground. The residual cost is stated rather than guarded: a proxy that STRIPS Origin turns the check off, so the proxy own allow-list stays in README as defence in depth.'). +decision('improvise-kc7n',an_origin_check_is_not_authentication,'It answers what page is DRIVING a client, never WHO is connecting, and anything that is not a browser page sends no Origin and is unaffected. improvise-kpnr is untouched by it and still open. Agent recommendation on the PR thread: a token in the URL, ws-server only, because new WebSocket(url) takes no headers so the URL is the one place a page can put a credential DELIBERATELY rather than ambiently; basic auth is ruled out on evidence, since a page cannot answer a challenge on a socket handshake.'). wire_carries(command,both_directions,'user intent up, projection commands down'). wire_carries(snapshot,on_connect_only,'improvise-q08: full ViewState + render cache filled from current viewport'). wire_carries(command,both_directions,'user intent client->server, projections server->client').