diff --git a/.agents/skills/kb/SKILL.md b/.agents/skills/kb/SKILL.md index 85c0ffc7..ea57a3f6 100644 --- a/.agents/skills/kb/SKILL.md +++ b/.agents/skills/kb/SKILL.md @@ -78,9 +78,41 @@ examples and history belong; prose docs should stay operational. - **Reading through the MCP is fine.** `loadSession` at session start is still how a session gets the model in hand; only writing moved. -Every write verifies the result reads back as intended and runs `kb_consistent` -on the candidate, so a bad delta leaves the file untouched. `count` before and -after is still the cheapest evidence a write did what you meant. +Every write checks three things before it replaces the file: the result reads +back as intended, it still holds every clause **HEAD** has, and `kb_consistent` +passes. So a bad delta leaves the file untouched. `count` before and after is +still the cheapest evidence a write did what you meant. + +## When a write is refused + +Exit codes: `0` done, `1` refused, `2` usage, `3` HEAD's `kb.pl` could not be +read, so nothing was compared, `127` no `tpl`. Only `1` is a verdict. + +**"clauses HEAD has are gone, with no kb rm recorded"** means the working-tree +`kb.pl` is short of the committed one. Your delta did not cause it. The file +was already damaged, by a killed write, a bad merge resolution or an edit. The +refusal lists the clauses that are missing. The check counts clauses **per +key**, so losing one of two clauses that share a key is caught too. + +- **Recover:** `git show HEAD:prolog-sessions/kb.pl > prolog-sessions/kb.pl`, + then re-run your `add`/`replace`/`rm` deltas. +- **Do not commit the short file to get past the refusal.** HEAD is what the + check compares against, so a committed loss becomes the new floor and is + never reported again. +- **To remove a clause on purpose, use `kb rm`.** It records the removal in a + per-worktree ledger (`git rev-parse --git-path kb-removed.pl`, never + committed), and that record is what lets later writes accept the missing + clause. A hand deletion has no record, so it is refused. + +The ledger vouches only for the exact `kb.pl` file the tool last wrote. Any +rewrite outside the tool voids it, including a restore, `git checkout`, `cp` +or an editor. After an `rm`, a restore therefore also undoes the `rm`, and you +must redo it. That is deliberate. Otherwise the abandoned `rm` would go on +excusing a later, accidental loss of the same clause. + +A killed write can leave `kb.pl.cand.*` files and a `kb.pl.lock` directory in +`prolog-sessions/`. Git ignores them. The lock makes the next write wait 30 +seconds and then fail, so delete it if nothing else is writing. `context/code/kb-tool.md` has the argument. `scripts/kb-test.sh` proves each refusal by injecting the fault it names. diff --git a/.claude/skills/kb/SKILL.md b/.claude/skills/kb/SKILL.md index 85c0ffc7..ea57a3f6 100644 --- a/.claude/skills/kb/SKILL.md +++ b/.claude/skills/kb/SKILL.md @@ -78,9 +78,41 @@ examples and history belong; prose docs should stay operational. - **Reading through the MCP is fine.** `loadSession` at session start is still how a session gets the model in hand; only writing moved. -Every write verifies the result reads back as intended and runs `kb_consistent` -on the candidate, so a bad delta leaves the file untouched. `count` before and -after is still the cheapest evidence a write did what you meant. +Every write checks three things before it replaces the file: the result reads +back as intended, it still holds every clause **HEAD** has, and `kb_consistent` +passes. So a bad delta leaves the file untouched. `count` before and after is +still the cheapest evidence a write did what you meant. + +## When a write is refused + +Exit codes: `0` done, `1` refused, `2` usage, `3` HEAD's `kb.pl` could not be +read, so nothing was compared, `127` no `tpl`. Only `1` is a verdict. + +**"clauses HEAD has are gone, with no kb rm recorded"** means the working-tree +`kb.pl` is short of the committed one. Your delta did not cause it. The file +was already damaged, by a killed write, a bad merge resolution or an edit. The +refusal lists the clauses that are missing. The check counts clauses **per +key**, so losing one of two clauses that share a key is caught too. + +- **Recover:** `git show HEAD:prolog-sessions/kb.pl > prolog-sessions/kb.pl`, + then re-run your `add`/`replace`/`rm` deltas. +- **Do not commit the short file to get past the refusal.** HEAD is what the + check compares against, so a committed loss becomes the new floor and is + never reported again. +- **To remove a clause on purpose, use `kb rm`.** It records the removal in a + per-worktree ledger (`git rev-parse --git-path kb-removed.pl`, never + committed), and that record is what lets later writes accept the missing + clause. A hand deletion has no record, so it is refused. + +The ledger vouches only for the exact `kb.pl` file the tool last wrote. Any +rewrite outside the tool voids it, including a restore, `git checkout`, `cp` +or an editor. After an `rm`, a restore therefore also undoes the `rm`, and you +must redo it. That is deliberate. Otherwise the abandoned `rm` would go on +excusing a later, accidental loss of the same clause. + +A killed write can leave `kb.pl.cand.*` files and a `kb.pl.lock` directory in +`prolog-sessions/`. Git ignores them. The lock makes the next write wait 30 +seconds and then fail, so delete it if nothing else is writing. `context/code/kb-tool.md` has the argument. `scripts/kb-test.sh` proves each refusal by injecting the fault it names. diff --git a/context/code/clients.md b/context/code/clients.md index b535916a..edd147d8 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -124,6 +124,47 @@ So this is the browser's own answer, and `ClientFrame::Save` is the other one for a socket client editing a TUI-hosted document, where there *is* a file somebody owns. +### A read-only session refuses edits; it does not accept them and fail to save (improvise-j4qt) + +A second tab on the same document cannot take the exclusive OPFS handle, so it +boots **read-only** (improvise-i0v9). It reads the bytes, paints the grid, and +navigates. Until improvise-j4qt, "read-only" described only the *storage*. The +session was writable: an edit was applied to the tab's own in-memory `App` and +projected, so the grid **painted a value the document did not have** +(`18,400.00` became `184,004,242.00`). Only then did the automatic persist +fail, with `NOT SAVED` replacing the read-only reason in the status. The +first tab never saw the edit. + +The fix sets the session's access at boot: `Worker::read_only` versus +`Worker::new`, as a private `Access`. A read-only worker answers any +`ClientFrame::Commands` that contains a `Command::Model` with +`Rejected(Rejection::ReadOnly)` **before the host sees it**. So nothing is +applied, nothing is projected, and nothing is persisted. Three things make +this the right place: + +- **It is decided up front, not observed afterwards.** The persist failure is + an observation, and it arrives *after* the paint. By the time a failed save + can be seen, the user has already been shown the number. +- **The client's rollback is already correct for it** (improvise-nxah). + `Rejection::ReadOnly` answers `refused_an_edit() == true`, so the client + pops that frame's entry and replays `ViewState` from its settled slice. No + cache needs rolling back: `Command::Model` is a no-op in `reduce_view`, so + the optimistic fold never painted the value. The paint came from the host's + projection, and that projection is exactly what the refusal prevents. +- **Only the document is refused.** A frame of view commands, such as a cursor + move, a panel or a help page, is answered as before. Read-only is a claim + about the document, not about this client's own slice. + +It is a property of the *boot*, not the backend. The backend in both cases is +a `vfs::ReadOnly`, and `worker.rs`'s `a_persist_that_fails_raises_a_notice…` +still uses one as a disk-full injection over a *writable* session. Reading +access off the storage would have made those two indistinguishable. +`ClientFrame::Save` is unchanged: a read-only tab that asks to save is still +answered `SaveFailed` by the backend. + +What this does **not** do is i0v9's gap 2. There is still no upgrade path when +the first tab closes, and the tab stays read-only until it is reloaded. + ### Ordering `Worker::on_frame` returns the frames for one inbound frame, in order, and a @@ -1491,6 +1532,19 @@ what says it is closed**. That is the shape `col_groups` already uses for "this grid has no column groups" — an absent list rather than a flag the page has to interpret alongside a mode it would then have to understand. +**That shape is only sound where "open" implies "non-empty"**, and help is the +one place it does — its content is static. `views` and `formulas` borrowed it +and were wrong: a document with no formulas opened a formula panel that built +the same `RenderModel` as a closed one, so the page hid it under a mode reading +FORMULAS (improvise-df2w). Both are now `Option>` — `None` closed, +`Some(vec![])` open and empty — one field, so the two facts cannot disagree, +where a `formula_panel_open` flag beside the rows (the `category_panel_open` +shape) would have been two. The same mistake as improvise-xg1z: an absence +spent on encoding a second fact. What the page does **not** yet draw is the +TUI's empty-state line (`(no formulas — press 'n' to add)`); that string lives +in `src/ui/formula_panel.rs`, and copying it into `render.rs` would be a second +home for it. + The rows themselves are `improvise_protocol::help::pages()`, the same ones `src/ui/help.rs` draws. `RenderHelpRow` carries a `kind` string (`heading`, `keys`, `accent`, …) which is a *semantic* name the page maps to a diff --git a/context/code/core.md b/context/code/core.md index 27c6dfc3..e27a3c18 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -832,6 +832,108 @@ that item. Under route-then-aggregate that arm is reached only where a record genuinely lacks the coordinate, because the fold hands the expression a record key. Evaluated bare, outside a `SUM`, it degrades exactly as `Formula::filter` does — which is why a fitted tree is always emitted wrapped. +## 14. A collapsed group is one row, and its cell folds the group (improvise-6qet) + +**The ruling.** "When collapsing a group, I'd generally expect the cursor to move +to whichever place the group collapsed into" (user, 2026-09-23). The shape chosen +from a presented fork: **a collapsed group contributes one data row, whose cell is +an aggregate over the group.** Before this, `expand_category` emitted the group's +`GroupHeader` and then `continue`d past every item, so a collapsed group had no +data row at all. `row_group_for` is indexed by data row, so nothing on the grid +could name the group once it was collapsed: `z` pressed twice collapsed the +*neighbour*, and a collapsed group could be reopened only by `:toggle-group`. + +The fix is to emit **one** `DataItem` where the `continue` was, and every +cursor-addressed question then answers itself by arithmetic. With +`Q1={Jan,Feb}, Q2={Apr,May}` and the cursor on `Apr` (row 2), collapsing Q2 gives +rows `[Jan, Feb, Q2]`, so row 2 *is* Q2 and a second `z` names Q2 again. `z z` is +an involution without anything being taught about involution, and neither +`ViewCursor` nor `row_group_for` changed. + +### The row + +- **Its tuple is the group name** (`Month=Q1`). `AxisEntry::DataItem` became a + struct variant `{ tuple, collapsed }` rather than growing a second variant, + because a new variant would be silently missed by `matches!(e, DataItem(_))` + sites; changing the shape made the compiler list all six external matches. +- **`collapsed` cannot be derived from the tuple**: a group and an item may + legally share a name. A parallel `Vec` was rejected for the reason gotcha + 1 gives `prune_axis_items` a predicate — pruning shifts indices. +- **The flag is inherited** down `cross_product`, so a category *nested under* a + collapsed group (`Q1/North`) is also a fold: no record carries `Month=Q1` + whichever inner coordinate sits beside it. +- **A group's items need not be contiguous** (`A[G1], B[G2], C[G1]` is legal), so + `expand_category` keeps a `folded` list: a collapsed non-contiguous group folds + **once**, and its header is not re-emitted on the repeat — that header would + open a run with no row in it. + +### The cell + +`Model::evaluate_aggregated` expands, at its head, any coordinate that names a +group rather than an item into one key per member (a cross product where several +coordinates do). Doing it in the **model** is what makes `display_text`, prune, +column totals, `grid_of`, search, yank and CSV export all follow with no change. +Three decisions: + +- **Membership is read off each item's own `group` field, never off + `Category::groups`.** `add_item_in_group` registers no `Group`; that list is + filled only by `.improv` `>` lines. Asking it answers "not a group" for every + document built in code, and the symptom is the fold's row pruned as empty. +- **An item with the same name as the group wins.** `items.contains_key` + short-circuits the expansion, so an existing document with an item `Q1` reads + exactly as before. +- **Stored values fold once over every record the group covers; a derived value + folds its members' results.** Folding the members' *aggregates* is right for + `Sum` and wrong for `Avg`/`Count` under `Axis::None` — `avg(avg(2,4), 9) = 6` + where the group's average is `5`. A formula target has no stored records to + pool, so its members' evaluated results are folded with the measure's + aggregation; for a ratio formula that is the usual pivot-table limitation, not + a new one. Hidden items are included in the fold, as on `Axis::None`: hiding + decides what is drawn, not what an aggregate covers. + +### Settability and drill + +The row is not a record, so `project.rs::cell_id` answers +`CellId::Aggregate` when `GridLayout::folds_a_group(row, col)` — the one home of +that question — whatever is on `Axis::None`. Without it, a document with nothing +regular on None projected the fold as `CellId::Record`, offered it for editing, +and a `SetCell` would have written the coordinate `Month=Q1`, which is not an +item. `render.rs::editable` is untouched (ADR 002: the variant decides). +`EditOrDrill` asks the same predicate and declines out loud: drilling would filter +on `Month=Q1`, which no record carries, and show an empty list. + +### The cursor, and why the order is the fix + +`ToggleGroup` is `Model(LayoutGeometry)`, and `apply_effects` resets the cursor +when it observes the frame move *inside* the loop (gotcha 24). So +`ToggleGroupAtCursor` emits `ToggleGroup` **first** and the cursor placement +**after**; the swap is the rival, and it lands every toggle at `(0, 0)`. The +placement also restores the scroll offsets the reset zeroed. + +Where the cursor lands is asked of the layout the toggle **will** produce +(`GridLayout::row_run_start(cat, group, prefix)` on a view with the toggle +applied), not of the current one. The same answer serves collapse and expand — +the group's first row — and it matters when the group is at an inner level: +collapsing `Month` under `Region` shrinks every earlier `Region`'s run too, so +`South/Apr` at row 6 lands on `South/Q2` at row 5, not row 6. That is the only +test the "ask the current layout" rival fails. + +### What this does not do + +- **Part 2 of the ruling** (a toggle carrying the group it acted on, so a later + command in the same batch can be invalidated) is not added: nothing binds `z` + as a `Binding::Sequence`, and if something did, both toggles would resolve + against one pre-batch context and name the same group, so the pair is a no-op — + which is what `z z` means. A mechanism with no reachable case is not built. +- **CSV export writes the fold as a row**, because an export writes what the grid + draws (improvise-iot6); a collapsed group is its own total row there too. +- Under `prune_empty` an **empty** collapsed group now disappears header and all, + where improvise-lhi4 kept its header. It is a narrowing, and deliberate: the + header was kept because it was the only way back to the rows, and the fold row + is now that way back — an empty fold has nothing to show. +- The browser still cannot press `z` (`toggle-group-under-cursor` resolves + `Upstream`), so the cursor half takes effect in the TUI only. + ## `PageAxis` — what constrains, and what the user may turn (improvise-p1ym) `GridLayout::page_coords` answers two questions that gotcha 27 says must not be diff --git a/context/code/kb-tool.md b/context/code/kb-tool.md index 92ddafe6..bce04912 100644 --- a/context/code/kb-tool.md +++ b/context/code/kb-tool.md @@ -99,34 +99,98 @@ refuses unless every clause HEAD has is still present **by key**. It runs first so that the refusal names the loss rather than whatever consistency error the loss happens to cause. -It compares keys rather than counts, although the ticket asked for a count, -because a count can be masked: uncommitted adds in one predicate offset a +It compares keys rather than totals, although the ticket asked for a count, +because a total can be masked: uncommitted adds in one predicate offset a truncation in another, and the count comes out even. Keys cost the same (0.26s for a write, most of it trealla start-up) and the message still names both totals and the number missing. +**Keys are counted, not collected** (improvise-ih3k). The guard first compared +the *set* of keys, and a set cannot see one of two clauses sharing a key. +`work_done/3`, `decision/3` and `work_followup/3` key on two arguments and are +not `functional/1`, so `kb_consistent` accepts two clauses under one key. `kb +add` refuses to *write* such a pair, but a git merge is not a `kb add`: two +branches that each record `work_done(T, bug_fix, …)` conflict at the end of the +block, and keep-both is the natural resolution. Once HEAD holds a pair, losing +either clause left the key present and the write was accepted, with both totals +printed equal. So the guard counts clauses per key, and a key is short when the +result holds fewer than HEAD does. The refusal names the HEAD clauses the result +holds no copy of, which is the one that went. + +Two alternatives were rejected. Comparing whole clauses would refuse every +`replace`, whose purpose is to change the text under a key. Making the three +predicates functional would refuse the pair itself at `kb_consistent`. That +would force a merge to resolve it, and may be worth doing, but it is a change to +the gate in `kb.pl`, not to this tool. `functional/1` also keys an arity-3 +predicate on its first argument, which is wrong for `decision/3`, so it would +need a new two-argument form. And the tool must not lose a clause whatever the +gate allows, so counting is needed either way. + +A pair also had no way out through the tool. `rm` checked its result against +`before - clauses in the delta`, which assumes one clause per key, so removing a +paired key was refused as "a clause was dropped". The expected total is now +`before - clauses the delta's keys match`, which is still counted separately +from the fold that removes them. `replace` of a paired key is still refused by +its total. That is the right answer, because it would silently collapse two +clauses into one. The route for a pair is `kb rm` the key, then `kb add` the one +clause to keep. + **How the count legitimately falls.** `add` never lowers it and `replace` overwrites in place, so neither can trip the guard. `kb rm` is the only intended removal, and it records the keys it removed in a **ledger** that later -writes read: a clause missing from the working tree is excused only if its key -is in the ledger. The ledger lives in the git dir (`git rev-parse --git-path +writes read: a key the result is short of is excused only if the ledger names +it. The ledger lives in the git dir (`git rev-parse --git-path kb-removed.pl`), so it is per-worktree and can never be committed; with -`KB_FILE` set it sits beside that file. It prunes itself on every write, keeping -only keys HEAD still has and the candidate does not, so an entry drops once the -removal is committed, and once the key is re-added. That second case matters: +`KB_FILE` set it sits beside that file. Every write rewrites it, keeping only +the keys the result is still short of, so an entry drops once the removal is +committed and once the key is back to HEAD's count. That second case matters: without it, a re-added clause that was lost again would still be excused. There -is deliberately no override flag. The way out of a refusal is to restore -`kb.pl` from HEAD and re-apply, or to commit the removal so HEAD moves. +is deliberately no override flag. + +The way out of a refusal is to restore `kb.pl` from HEAD and re-apply your +deltas. Do **not** commit the short `kb.pl` to get past it. The refusal +message used to offer that, and taken on a loss nobody intended it makes the +loss permanent: the committed file becomes the floor this guard compares +against, and nothing ever reports it again. A removal you intend goes through +`kb rm`, onto a restored file. + +**A ledger speaks only for the file its write published** (improvise-yea8). +Pruning happens only when the tool writes. A commit, a `git checkout -- +prolog-sessions/kb.pl`, a `cp`, and a branch switch are not writes. So an +entry from an `rm` that was later abandoned would excuse a later loss of that +same clause by any path. That is easy to reach, because the recommended +recovery is exactly a restore. Pruning against the kb read at the next write +cannot close it, because that kb has already lost the clause. So the ledger's +first line is a `% kb-stamp` comment naming the kb file the write published, +and a later write reads the entries only if the kb it finds is still that file. + +The stamp is the content hash **and** the file's inode and nanosecond mtime. +Content alone is not enough, and the test that showed it was the ticket's own +reproduction. `kb.pl` is kept in canonical form, so `grep -v` of one clause +from HEAD produces output **byte-identical** to what `kb rm` of that clause +writes. After `rm X`, a restore, and then the same loss by hand, a content-only +stamp still matched. `rename(2)` carries the candidate's inode and mtime onto +`kb.pl`. Anything else that rewrites the file, whether a `cp` onto it, a git +checkout or an editor, gives a new mtime and usually a new inode. `stat -c` is +tried before `stat -f` because GNU `stat -f` means *filesystem* status and +would silently stamp the wrong thing. It is not keyed by HEAD commit, because a +commit that does not touch `kb.pl` does not make the ledger wrong. And an +uncommitted `rm` carried across a branch switch, with `kb.pl` untouched, is +still exactly the kb it describes. + +The cost is a false refusal after an intended `rm` followed by anything that +rewrites the file without the tool. That refusal is loud, and the recovery is +the usual one: restore and redo the `rm`. **The write is a rename.** The candidate used to come from `mktemp` in `$TMPDIR`, and `mv` from there onto the repository crosses a device wherever `/tmp` is its own filesystem. That makes `mv` a copy and an unlink, so a kill partway through leaves a truncated `kb.pl`. The candidate is now a sibling of `kb.pl` (`kb.pl.cand.XXXXXX`), so publishing it is `rename(2)`. The ledger is -renamed first. A kill between the two renames then leaves a ledger naming keys -the kb still has, and the next write prunes those entries. Until it does, they -would excuse the loss of exactly those clauses, which is a narrower hole than a -kb.pl that is already short. +renamed first. A kill between the two renames leaves a ledger stamped for a +candidate that was never published. It therefore matches nothing, and the next +write ignores it. That can refuse a write an earlier `rm` should have excused. +It cannot excuse a loss, which was the hole before the stamp. **Exit codes.** 0 done, 1 refused, 2 usage, 3 HEAD's `kb.pl` could not be read, 127 no `tpl`. Only 1 is a verdict. 3 is separate because "could not compare" @@ -139,10 +203,22 @@ The completeness guard trusts HEAD. A loss that has been **committed** is invisible to it by construction. That was the right trade for this incident, where HEAD held all 801, but a bad commit of `kb.pl` becomes the new floor. -A `SIGKILL` leaves its `kb.pl.cand.*` and `kb.pl.lock` behind, because no trap -runs. The lock already had that property. The stray candidate is new: it is -never read, but it shows as untracked in `prolog-sessions/`, where a `git add --A` would pick it up. +A `SIGKILL` leaves six files behind, because no trap runs: the candidate +`kb.pl.cand.XXXXXX`, its `.base`, `.ledger`, `.removed` and `.stamped` +siblings, and the `kb.pl.lock` directory. None of them is ever read again, +and **git does not see them**. `.gitignore` ignores `prolog-sessions/*`, so +neither `git status --untracked-files=all` nor `git add -A` lists them. That +was measured, and an earlier version of this paragraph warned about `git add +-A` staging them, which does not happen. The one that matters is the lock: the +next write waits 30 seconds for it and then exits 1 with "remove it if stale". +Delete it and the candidates by hand. + +The ledger's stamp names a file by inode and nanosecond mtime. A rewrite that +preserves both, such as a `touch -r`, a filesystem with coarse timestamps +rewriting within one tick, or an inode reused at the same mtime, with the bytes +also identical to what the tool published, is indistinguishable from the file +the tool wrote. That takes deliberate effort or a filesystem this repository +does not run on. The key rule is looser than `functional/1` for `crate/3`, which keys on the first argument alone; `kb_consistent` is the backstop for that case rather than diff --git a/context/code/protocol.md b/context/code/protocol.md index fdbf9e8e..85f47b8a 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -218,14 +218,86 @@ its verdict. The list is checked in **both** directions, so an entry that starts being composed fails just as loudly as one that stops (`doctrine(a_stale_list_of_what_is_missing_is_worse_than_no_list)`). -Two limits are worth stating, because the check is a source scan. -`#[cfg(test)]` items are dropped by a line scan that relies on rustfmt putting -an item's closing brace at the attribute's own indentation; over-stripping would -**fail** the test rather than pass it, which is the safe direction. And a -`Command::X` inside `matches!` or `if let` is read as a construction rather than -a pattern, which can only make the check *weaker* for that variant — never raise -a false alarm. A variant nothing composes at all is immune to both, since its -only pattern occurrence is its own fold arm, which `=>` identifies. +The check is a source scan, and **what counts as source has one home**: +`improvise_protocol::source_scan::production_code`, shared with the forward +guard below (improvise-c7mu). It removes three things a scan must not see. + +- **Comments and literals.** Both are blanked, and newlines are kept. Before + this, a `// TODO: Command::SetYanked(...)` left where the real composer had + been deleted still passed all four tests. That is the *weaker* direction, the + one a scan weakness was supposed to be unable to go. It was live in a small + way: a `///` in `project.rs` named `Command::SwitchView`, which has real + composers, so no verdict was wrong yet. It also had a mirror: a comment + explaining that a `NOT_COMPOSED` variant is held back would have raised a + false "stale entry" alarm. +- **Test items inside a file.** A line scan that relies on rustfmt putting an + item's closing brace at the attribute's own indentation. It treats + `cfg(any(test, …))` as test code and `cfg(not(test))` as production. + Over-stripping **fails** the test rather than passing it, which is the safe + direction. +- **Test-only files.** A `#[cfg(test)] mod x;` makes `x.rs` and everything under + `x/` test code, even though nothing inside the file says so. Before + improvise-c7mu, this checker's own file and `improvise-io`'s + `roundtrip_tests.rs` were scanned as production. + +One limit remains. A `Command::X` inside `matches!` or `if let` is read as a +construction rather than a pattern. That can only make the check *weaker* for +that variant; it never raises a false alarm. A variant nothing composes at all +is immune, since its only pattern occurrence is its own fold arm, which `=>` +identifies. + +### And forwards: every effect is on the wire or has a verdict (improvise-h3y7) + +`vocabulary_is_composed.rs` checks that the vocabulary does not name more than +production composes. The other direction is *does the vocabulary name +everything it should?* That was the direction `ViewBack` and `ViewForward` +failed: they were missing, and no client had a back history. Until improvise-h3y7 +nothing checked it, and the audit that finally ran it by hand under-reported. +Brace-matching `impl Effect for` blocks misses the eight `simple_effect!` +expansions and `SetTransientPrefix` in `command/keymap.rs`. All nine are +`ViewOnly`, which is the half the `ViewBack` bug was in. + +`improvise-command/src/ui/reduce/vocabulary_is_complete.rs` is that direction. +It scans the crate's `src/` for both ways an effect is declared, reading +each name past any attributes and doc comments before it. The macro accepts +those, and missing them hid effects (improvise-94x2). It requires +each name to be a `Command` or `ModelCommand` variant (read off serde's derive) +or to appear in `NOT_ON_THE_WIRE` with a verdict. A verdict is one of four +kinds, and the check verifies each kind: + +- `Command(v)`: on the wire under another name (`HelpPageNext`/`Prev`/`Set` → + `Command::HelpPage`). `v` must be a real variant. +- `Frame(v)`: answered by a `ClientFrame` that is not a `Command` (`Save` → + `ClientFrame::Save`, gotcha 45). `v` must be a real frame. +- `Off(why)`: excluded on purpose, with the argument: session-local residue + (gotcha 30), host-local caches, or batch-relative control flow. +- `Gap(ticket)`: missing, and a ticket owns closing it (`SaveAs`, `ExportCsv` → + improvise-2mqz). + +The list is also checked in reverse. An entry that is now on the wire fails, +and so does an entry that names no effect. The scan is `production_code`, so +the comments, strings and test fakes that name an effect do not count as +effects. If they did, a stale verdict could outlive the effect it names. + +What it does **not** check is whether a same-named pair *agrees*: whether +`reduce_view`'s arm does what the effect does. The name match is the +completeness half. The semantic half stays a per-pair read (improvise-h3y7's +notes put it at about 21 pairs). The resolver lists in `resolve.rs` +(`declined()` and the host-only names) are also not checked against the +vocabulary yet. The audit found them stale in three places. + +### No `ModelCommand` field is a position (improvise-hkbc) + +improvise-6nsd closed "a stale layout tier misdirects a mutation" on one +condition: no `ModelCommand` carries a row or column index. `position_free` +in `reduce.rs`'s tests enforces that at compile time. It destructures every +variant with no `..`, down into `RecordCell`, and hands each field to a +function that accepts only a name, a path, a surrogate, a value or an axis. So +a new variant, a new field, or a field retyped to `usize` fails to build, and +the build failure forces the ADR conversation. It **cannot** see a position +spelled as a `String`. It also does not make the client position-free: +improvise-2wu3 resolves a painted `(row, col)` against the current cache before +it ever builds a command. ### Why two model effects are key-path only diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 1d1898c5..3ce396f8 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -1130,8 +1130,8 @@ Normal and never closes the panel, exactly as `src/draw.rs` keeps drawing it off `view_panel_open` alone. Asserting it hidden would have pinned the browser to behaviour the TUI does not have. -The gating rule itself — an empty list is what says a panel is closed, the same -rule `help` follows — is asserted in Rust as well, by +The gating rule itself — `None` is what says a panel is closed (it was an empty +list, the rule `help` follows, until improvise-df2w) — is asserted in Rust as well, by `a_closed_panel_projects_no_rows_and_an_open_one_does` and `the_tile_bar_is_always_present_and_only_its_cursor_is_modal`, because a browser test cannot distinguish "closed" from "the cache is empty" and those two tests @@ -4231,6 +4231,70 @@ and converged. **Fault injected:** the same `Binding::Prefix` → `Upstream` reversion. Fails on the second press. +### `a_read_only_session_refuses_an_edit_before_anything_is_painted` + +improvise-j4qt. The finder measured this in Chromium and Firefox: two tabs, one +document. The second tab boots read-only. Typing `4242` into a cell applied it +to that tab's in-memory `App` and projected it, so the grid showed +`184,004,242.00`. Only then did the persist fail and `NOT SAVED` replace the +read-only reason. The first tab never saw the edit. + +The test builds the read-only boot the way `bindings::read_only` does: the +document's bytes over a `vfs::ReadOnly` backend. It sends a caught-up `SetCell` +and asserts four things: +- the whole reply is `[Rejected(ReadOnly)]`; +- the client's folded grid still shows the old text; +- no notice was raised (no persist ran); +- the document is not dirty. + +**Red first**: the same test over `Worker::new`, which was the boot's +constructor before this change, failed. Its reply was a `Commands` frame of +projections carrying the new value. + +The control is `a_read_only_session_still_answers_a_view_command`: a +`SetSelected` frame is not refused. Without it, a worker that refused *every* +frame would pass the test above. + +End to end: `harness/tests/shell.spec.js`, "a read-only tab refuses an edit and +never paints it". It opens two real tabs on one document in Chromium, edits in +the second, and asserts four things: +- the toast says `read-only`; +- the cell keeps its text; +- the grid contains no `4,242`; +- `#status` still says `read-only` and not `NOT SAVED`. + +Fault injected: `bindings.rs` reverted to `Worker::new(app)` for the read-only +boot, then `harness/build.sh` rebuilt. The spec then failed on the toast, which +received `""` because no refusal arrived. Reverted and rebuilt, and the whole +of `shell.spec.js` passed (18). + +### `an_open_empty_panel_is_not_a_closed_one` + +improvise-df2w. `views` and `formulas` said "closed" by being empty, so an open +formula panel over a document with no formulas built a `RenderModel` identical +to a closed one, and the page hid it while the mode read FORMULAS. The test +compares the two models whole rather than asserting a field, so it does not +depend on how the difference is spelled. + +**Reproduced:** fails at HEAD before the fix with `left == right` on the formula +half — the two models were byte-identical. The existing +`a_closed_panel_projects_no_rows_and_an_open_one_does` could not see it, because +it fills both caches before opening the panels. + +### the formula panel opens over a document with no formulas + +The browser half of improvise-df2w, in `harness/tests/contract.spec.js`, over +`harness/fixtures/tall.improv`, which has no `## Formulas` section. It asserts +**both** directions, because each rival fails a different one. + +**Reproduced:** against a bundle built from the unfixed `render.rs` and +`app.js`, fails at `#formulas` `toBeVisible()`, immediately after `#mode` reads +FORMULAS — the exact disagreement the ticket describes. + +**Fault injected:** `paintPanel` writing `host.hidden = false` (a page that +stops inferring closed and simply never hides). Fails at the launch-time +`toBeHidden()` before the panel is opened. + ## improvise-ktlk — the overlay and the commit read one buffer PR #162 (improvise-xg1z) changed what the overlay **draws** for an `Editing` diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 04ade789..a00efc3a 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -1257,9 +1257,11 @@ substring. ### a_window_reaching_the_last_row_carries_the_trailing_group_header -A collapsed group at the bottom of the grid has no items to hang off, and is still -the only row that says those rows exist. A window that reaches the last row -therefore carries it. +A collapsed group at the bottom of the grid folds into one row (improvise-6qet, +`context/code/core.md` §14), and the header comes above it. A window that reaches +the last row therefore carries both: the frame is `[North, South, [Late], Late]`. +Before 6qet a collapsed group had no row, and this test asserted the header was the +*last* entry — the premise 6qet changed, so the length moved from 2 to 3. It must be *seen*; it is not yet actionable in a browser. `web/app.js` registers no listener on a group ``, and `toggle-group-under-cursor` is still in @@ -1289,23 +1291,30 @@ say which group a column or row belongs to — while `body_from` declines to pai header that sits above the window. **And only the nearest one comes.** The fixture puts **two** headers above the -window (`Hidden` is collapsed, so it emits a header and no items, and `Late`'s -follows it immediately) because that is the only way to tell "the nearest header -comes" from "every header above comes". With one group above, deleting the +window (`Hidden`, whose collapsed group is now one row, and `Late`) because that is +the only way to tell "the nearest header comes" from "every header above comes". +Since improvise-6qet the fold is itself a row, so the window opens at row **2** +(`East`), past both `Hidden`'s header and its folded row; at row 1 the window would +start on the fold and bring `Hidden`'s header with it. With one group above, deleting the compaction leaves this green — it is a *cost* property, O(the groups a deep scroll crossed), and correctness alone cannot see it: `group_of` still finds the nearest and `body_from` still starts at the first item either way. -#### Helper — `app_with_consecutive_headers` +#### Helper — `app_with_a_collapsed_group_between_rows` -Not a test — the fixture for that test and for `consecutive_headers_between_two_rows_all_come`. +Not a test — the fixture for that test and for `every_header_between_two_in_window_rows_comes`. -Two consecutive group headers between two rows: `Hidden` is collapsed, so it emits -a header and no items, and `Late`'s header follows it immediately. +Two group headers between `North` and `East`: `Hidden`, collapsed, and `Late`, open. +It was `app_with_consecutive_headers` until improvise-6qet: a collapsed group now +contributes one row, so a collapse no longer produces two *adjacent* headers and +the old name described a shape the layout cannot emit. -### consecutive_headers_between_two_rows_all_come +### every_header_between_two_in_window_rows_comes -Headers between two in-window rows all come, not just the nearest. +Headers between two in-window rows all come, not just the nearest — the frame is +`[North, [Hidden], Hidden, [Late], East]`, each header above the row it opens. +Renamed from `consecutive_headers_between_two_rows_all_come` for the reason the +fixture was. Only the nearest wins `group_of`, so a compaction that kept one would still answer every question about *membership* correctly — and would silently lose the `Hidden` @@ -4803,40 +4812,80 @@ cursor at data row 0: 3. The row axis is now empty, so `row_group_for(0)` resolves nothing and `z` returns no effects at all. -So the cursor-addressed toggle is not an involution, and a collapsed group cannot -be reopened from the grid — only by `:toggle-group`, which names the group. That -behaviour is unchanged by this work and is filed separately as improvise-6qet; the -three tests below pin what the command *does*, which is what the old test claimed -to and could not. +So the cursor-addressed toggle was not an involution, and a collapsed group could +not be reopened from the grid. **improvise-6qet changed that**, and the tests below +are its guards; the two that pinned the old behaviour +(`a_second_toggle_at_the_same_cursor_names_the_next_group_not_the_collapsed_one`, +`a_fully_collapsed_row_axis_leaves_the_command_nothing_to_address`) were replaced +rather than kept, since the behaviour they pinned was the bug. The old law stays +deleted; a real determinism law over two independently built contexts is +improvise-hzjo. + +improvise-6eqi found the survivors of ub4o all seated the cursor at data row 0, so a +toggle that ignored the cursor (`row_group_for(0)`) passed the whole workspace. Every +toggle test below starts off row 0; that rival was injected and fails five of them. ### toggling_at_the_cursor_names_the_group_the_cursor_sits_in_and_collapses_it -Asserts the effect names `category: "Month", group: "Q1"` — a category and a group -by name, which no assertion in the tile/grid suite had ever done — and that the -view's collapsed set actually moved afterwards. The fixture is asserted expanded -first, so the post-condition is not supplied by the default. +Asserts the effect names `category: "Month", group: "Q2"` — a category and a group +by name — and that the view's collapsed set actually moved, and that the neighbour +`Q1` did not. The cursor sits on `Apr` (row 2): at row 0 the neighbour and the +subject are the same answer. Fixture asserted expanded first, so the post-condition +is not supplied by the default. -Fault injected: `execute` returning `vec![]` after resolving the lookup. The old -law passed (both sides empty, as they already were); this test fails on the -missing effect, and the two below fail with it. +Faults injected: `execute` returning `vec![]` after resolving the lookup, and +`row_group_for(0)` in place of the cursor row (improvise-6eqi). Both fail it. -### a_second_toggle_at_the_same_cursor_names_the_next_group_not_the_collapsed_one +### toggling_a_group_collapses_that_group_and_the_cursor_follows_it -The test the old one could not be: it requires the second `execute` to see a -layout the first one changed. +improvise-6qet, driven through the real path (`App::cmd_context` → `execute` → +`apply_effects`). Cursor at row 2 (`Apr`); `z` gives rows `[Jan, Feb, Q2]` with the +cursor on `Q2` at row 2, and `z` again gives `[Jan, Feb, Apr, May]` with the cursor +back on row 2 — the involution the old law claimed. -Fault injected: a stateless `execute` that always returns -`ToggleGroup { category: "Month", group: "Q1" }` — i.e. one that ignores the -applied state, which is exactly what "does not apply effects between calls" hides. -Run against it, the old law **passes** (`first == second` by construction) and this -test fails, as does the one below. That rival is one-directional: it can only ever -name `Q1`, so a test that merely asserts "some group is named" is satisfied by it. +Faults injected: **the swap** (cursor placement emitted before `ToggleGroup`) fails +it `left: (0, 0) right: (2, 0)` — the toggle's observed reframe resets the cursor +after the placement. `row_group_for(0)` fails it too. -### a_fully_collapsed_row_axis_leaves_the_command_nothing_to_address +### a_collapse_from_the_tail_of_a_run_lands_on_the_runs_first_row + +Cursor on `May` (row 3). Collapsing Q2 lands on row 2, the fold. The rival "leave +the cursor where it was" is right for a cursor already on the run's first row and +wrong here (row 3 is past the end of `[Jan, Feb, Q2]`); injected, it fails this and +the next test only. + +### collapsing_an_inner_group_lands_where_every_earlier_run_shrank + +`Region × Month` on Row, `Month` the inner level. Cursor on `South/Apr` (row 6). +Collapsing Q2 shrinks `North`'s run as well, so `South/Q2` is at row **5**. The only +test that separates "ask the layout the toggle will produce" from "ask the current +layout": that rival returns 6 (`left: (6, 0) right: (5, 0)`). + +### a_collapsed_group_can_be_re_expanded_from_the_grid + +Both groups collapsed (`[Q1, Q2]`), cursor on the `Q2` row: `z` names Q2 and +reopens it, landing on `Apr` at row 1. Before 6qet a collapsed group had no row and +this was impossible from the grid. The swap fails it at `(0, 0)`. + +### edit_or_drill_declines_a_collapsed_groups_row + +A fold is neither editable nor drillable: no record carries `Month=Q2`. The same +context at `Feb` (row 1) still opens the editor, so the decline is about the row, +not the fixture. Rival: the `folds_a_group` guard removed — the command opens an +editor on the fold, and the test fails. + +## `ui/project.rs` — a collapsed group's row is an aggregate (improvise-6qet) + +### ui::project::cell_identity::a_collapsed_groups_row_is_projected_as_an_aggregate + +Nothing regular is on `Axis::None`, so the none-axis arm of `cell_id` cannot supply +`Aggregate` — and `North` in the same grid is asserted `Record` to prove it. The +`Coastal` fold must be `Aggregate`, and its text `7.00` (East 3 + West 4). Rival: +`folds_a_group` dropped from `cell_id` — the fold is projected `Record`, which +`render.rs::editable` would offer for a write to the non-item `Region=Coastal`. It +also fails when group membership is read off `Category::groups` instead of each +item's `group`, because a document built with `add_item_in_group` registers none. -Pins step 3 — `row_count() == 0` is asserted before the call, so the empty effect -vector is the command declining to name a group rather than the fixture never -having had one. Both rivals above fail it. ## `ui/project.rs` — a formula lost with its target ITEM (improvise-en4s) `Dirty::of(ModelChange::CellData)` reports `formulas: true`. That was written as @@ -4934,6 +4983,45 @@ in so that a change making the fixture's load stop rebasing fails *here*, where the message says what actually broke, rather than turning the rollback test green by emptying it. +### ui::reduce::vocabulary_is_complete::every_effect_is_on_the_wire_or_has_a_verdict + +improvise-h3y7, the forward direction of the vocabulary derivation. It is the +direction `ViewBack`/`ViewForward` failed: they were missing, so no client ever +had a back history. The test scans this crate's production `src/` for +`impl Effect for X` and `simple_effect!(X`. That covers both spellings, and the +h3y7 audit's brace-matcher missed nine effects by seeing only the first. It +then requires each name to be a `Command` or `ModelCommand` variant (read off +serde's derive) or to carry a `NOT_ON_THE_WIRE` verdict. + +Faults injected, together: `HelpPageNext`'s verdict replaced by one for +`SetSelected`. This test failed naming `HelpPageNext`, and +`the_verdict_list_names_nothing_that_is_on_the_wire` failed naming +`SetSelected`. Separately, `HelpPageSet`'s verdict was pointed at +`Command::HelpPag`, and +`the_verdict_list_names_only_real_effects_and_real_variants` failed. + +`the_scan_reaches_every_way_an_effect_is_declared` pins one effect of each +spelling plus `SetTransientPrefix` (which lives in `command/keymap.rs`). A +broken walk therefore fails that test instead of shrinking the set. +`an_effect_named_only_in_a_comment_or_string_is_not_an_effect` shares +improvise-c7mu's fault. With `code_only` made the identity, it failed with +`["SaveAs", "ExportCsv", "Real", "SetYanked", "AlsoReal"]`. + +**improvise-94x2, found by the PR #172 review.** The reader took the +identifier *immediately* after `simple_effect!(`. The macro's own matcher is +`$(#[$meta:meta])* $name:ident(…)`, so a doc comment before the name (blanked +to whitespace by `code_only`), an attribute, or a line break after `(` left +nothing to read, and the effect was invisible. The reviewer injected a +documented `simple_effect!(\n /// Rival.\n SetBogusRival(bool) …)` into +`effect.rs`, and all five tests passed. Re-injected here, it also passed all +five before the fix. After the fix +`every_effect_is_on_the_wire_or_has_a_verdict` fails naming `SetBogusRival`. +The rival was removed afterwards. `name_after` now skips whitespace and every +bracket-matched `#[…]` before reading the name. +`an_attributed_or_wrapped_effect_is_still_an_effect` pins that on a fixture: +a doc comment, two attributes (one with nested brackets), and a wrapped call. +With the whitespace skip removed it fails with `[]`. + ### yy_then_p_in_records_mode_copies_between_real_records improvise-6p8q. At `gh/main` `9e78051` it failed at the yank: `left: None, diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index 299a152c..d27feb93 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -1293,3 +1293,71 @@ at different assertions — the first at the East count, the second at the total The fixture is deliberately asymmetric: one matching cell under the scoped key and three under the unscoped one, so no single constant can satisfy both. + +## `view/layout.rs`, `model/types.rs` — a collapsed group is one aggregate row (improvise-6qet) + +Reasoning in `context/code/core.md` §14. The bug is one statement: `expand_category` +`continue`d past every item of a collapsed group, so the group had no data row and +the cursor-addressed `z` could never name it. Restoring that `continue` against the +fixed tree fails fifteen tests across `improvise-core`, `improvise-io` and +`improvise-command` — every test below that touches the layout. + +### a_collapsed_group_contributes_one_aggregate_row + +The inversion of `collapsed_group_has_header_but_no_data_items`, which asserted the +premise 6qet changed and was rewritten rather than deleted. **Two** groups, one +collapsed and one not, so `folds_a_group` is asserted `true` for the fold and +`false` for `Apr` under the open group — a predicate that answered `true` for +everything under a header would pass a one-group fixture. Also asserts the fold +sits under its own header (`row_group_for(0) == Q1`), which is what lets `z` name +it. + +### a_non_contiguous_collapsed_group_folds_once + +`A[G1], B[G2], C[G1]`: a group's items need not be contiguous. Rival: dedup only +against the *previous* group (right for the contiguous case, which every other +fixture is) — `G1` then folds twice, under two headers, and only this test fails. + +### a_category_nested_under_a_collapsed_group_is_folded_too + +`Month × Region` on Row, `Q1` collapsed: `Q1/North` is a fold too, because no +record carries `Month=Q1` whatever sits beside it. Rival: the `collapsed` flag not +inherited down `cross_product` — only this test fails, and without it the TUI would +offer `Q1/North` for editing. + +### model::types::model_tests::a_collapsed_groups_cell_aggregates_its_items + +`Jan=3, Feb=4` under `Q1`, `Apr=100` under `Q2`: the fold is `7`. That value +separates the real fold from absent (`None`), from pass-through of the first member +(`3`), from a count (`2`) and from a fold that leaks another group (`107`). + +Also the witness for the trap the first implementation of this ticket fell into: +reading membership off `Category::groups` rather than each item's `group` field. +`add_item_in_group` registers no `Group`, so that rival answers "not a group" for +this fixture and the value is absent — this test, the next, and +`a_collapsed_groups_header_survives_pruning` fail (the fold's row prunes as empty). + +### model::types::model_tests::a_groups_stored_values_fold_once_not_per_member + +Under `Axis::None` (`Store`), `Avg` and `Count` over `Jan/A=2, Jan/B=4, Feb/A=9`. +The group's average is `5` and its count `3`; folding the members' aggregates gives +`avg(3, 9) = 6` and `count(2, 1) = 2`. Rival: always fold members' results (the +formula-target branch taken for stored values) — this test fails. `Sum` cannot see +the difference, which is why the fixture sets `measure_agg`. + +### model::types::model_tests::an_item_named_like_a_group_is_read_as_the_item + +An item literally named `Q1` beside the group `Q1`: the item wins and reads `50`, so +an existing document with such an item is unchanged. Rival: drop the +`items.contains_key` short-circuit — the key expands to `Jan + Feb = 7` and this +test fails. + +#### `a_collapsed_groups_header_survives_pruning`, re-aimed + +Re-aimed by improvise-6qet (the entry above, under improvise-lhi4, still holds for +why the fixture carries an unfunded `Mar`). The expected sequence now includes the +fold, `DataItem { ["Q1"], collapsed: true }`, and `row_group_for(0)` is `Q1` — the +fold is the row `z` names the group from. Note the narrowing this implies: an +**empty** collapsed group now has an empty fold row, which `prune_empty` drops, and +its header goes with it. lhi4 kept that header because it was the only way back to +the rows; the fold is now that way back. diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index 91930f47..daf7a527 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -1424,3 +1424,13 @@ This is the second time in this ticket that the interesting question was *which rows the count is over*. The first was one warning per derived column rather than one overall; this is one warning per **landed** row rather than per attempted one. + +### export_writes_a_collapsed_group_as_its_own_total_row + +improvise-6qet (`context/code/core.md` §14). An export writes what the grid draws +(improvise-iot6), and the grid now draws a collapsed group as one row whose cell +folds the group, so the CSV body is exactly `["Q1,7.00"]` (Jan 3 + Feb 4). Decided +rather than defaulted: the alternative — expanding the group in the export — would +reintroduce the iot6 divergence between what the user sees and what they export. +On the pre-6qet tree the body is empty (the group had no row, and pruning left +nothing), so the test fails there. diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index e0c49021..b00b88db 100644 --- a/context/test-evidence/protocol.md +++ b/context/test-evidence/protocol.md @@ -969,3 +969,63 @@ That assertion is the discriminating one: a fold arm that removed the key would give `None` and fail. Asserting only that `"command"` survived would pass under either behaviour, which is what the old spelling amounted to for the cleared key. + +### vocabulary_is_composed::a_variant_named_only_in_a_comment_or_string_is_not_composed + +improvise-c7mu, found by the post-merge review of PR #160. `constructed_in` ran +over raw source text, so any `Command::X` counted as a composer, including one +inside a comment or a string. The review deleted the real `Command::SetYanked` +composer from `resolve.rs`, left `// TODO: Command::SetYanked(value)` in its +place, and all four tests passed. The test feeds the whole per-file pipeline a +line comment, a block comment and a string that each name a variant, and +asserts that none of them counts. A positive control asserts that a real +construction still does. + +Fault injected: `code_only` made the identity function. This test failed with +`["SetYanked", "SetSearchMode", "ChangeMode"]`, and so did +`source_scan::tests::comments_and_literals_are_blanked_and_code_is_kept`. The +four whole-tree tests stayed green under that fault. This confirms the review's +finding that no live comment gives a wrong verdict *today*, and it is the +reason the property has to be pinned on a fixture rather than on the tree. + +The fault on the real tree (deleting `resolve.rs`'s composer) was not rerun +here. `resolve.rs` is outside this lane, and the edit was refused as another +agent's ground. + +### source_scan::tests::a_test_only_module_file_is_not_production_code + +improvise-c7mu, the second half. `#[cfg(test)] mod x;` makes a whole file test +code, and nothing inside the file says so. The old scan saw it as production, +and that included this checker's own file. The test builds a fixture tree: +`lib.rs` declares `mod p;` and `#[cfg(test)] mod t;`, and `t.rs` declares a +nested `mod deeper;`. It asserts that `production_code` returns exactly +`lib.rs` and `p.rs`. + +Fault injected: the file filter in `production_code` made always-true. This +test failed with `["lib.rs", "p.rs", "t.rs", "t/deeper.rs"]`. Before this +fixture existed, the `improvise-protocol` and `improvise-command` lib suites +**stayed green** under the same fault, and so did +`test_only_items_and_modules_are_not_production`, which checks the helpers one +by one. The wiring between them was untested until this fixture existed. + +### reduce::tests::every_model_command_field_is_position_free + +improvise-hkbc. improvise-6nsd's "a stale layout tier cannot misdirect a +mutation" holds only while no `ModelCommand` carries a row or column. The guard +is the signature of `position_free`. It destructures every variant with no `..`, +down into `RecordCell`, and hands each field to a function that accepts only +`&str`, a path, a `RecordHash`, a `CellValue`, an `Axis` or an optional name. +The `#[test]` only keeps that signature compiled and reached. + +Faults injected, one per half, each compiling only `improvise-protocol`: + +- A new variant `ScrollTo { row: usize }`, also added to `records_named`'s + or-pattern the way an author would. The lib built. The test build failed + with `non-exhaustive patterns: ScrollTo` **at `position_free` and nowhere + else**. +- The same variant classified as `ScrollTo { row } => name(row)`. The test + build failed with `expected &str, found &usize`. + +A field added to an existing variant is caught too, but not in isolation, +because every constructor of that variant also fails. Reverted with +`git restore`. diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 2d350c15..341a0a32 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -551,3 +551,62 @@ forward. **Checked by hand, not in the suite:** the ledger prunes an entry once the key is re-added and once HEAD stops having it (a committed removal). + +### The kb completeness guard's follow-ups — `scripts/kb-test.sh` cases 19b, 20b, 23–25 + +improvise-ih3k, improvise-yea8 and improvise-za97, filed by the review of PR +#166. The argument is in `context/code/kb-tool.md`. This entry covers what each +case was watched to catch. + +**Reproduced at `f909cad` before any fix.** The new cases 23, 23c and 24 gave +40 passed, 3 failed. 23 failed because an add onto a kb that had lost one of +two `work_done(,bug_fix,…)` clauses exited 0. 23c failed +because `rm` of that paired key was refused as "a clause was dropped": its +expected total assumed one clause per key. The tickets did not predict that +one, and it meant a pair could not be resolved through the tool at all. 24 +failed because the stale ledger entry excused the loss. + +**The ticket's own reproduction defeats a content stamp.** The first fix +stamped the ledger with the kb's sha256, and case 24 still failed. `kb.pl` is +canonical, so `grep -v` of one clause from HEAD is byte-identical to what `kb +rm` of that clause writes. The stamp therefore also carries the inode and the +nanosecond mtime. + +**Case 20b had to change, and why that is not a weakening.** It made its +second loss with `sed -i` on the kb. Under the fix, any edit outside the tool +voids the ledger, so 20b then refused two clauses instead of one and failed on +its "1 of" assertion. It was testing the new voiding rather than what its +comment names. The second loss now comes from giving the *baseline* an extra +clause (`KB_BASELINE`). The kb file stays exactly as the tool wrote it, the +ledger stays valid, and the case again isolates "excuses only what it names". +It also now asserts the unexcused clause is named. + +**Rivals injected, one at a time. Each failed only the cases listed:** + +- keys compared as a set (`min(1, N)` per key) in `complete`: 23 only. +- `rm`'s expected total back to `before - |delta|`: 23c's rm step only. +- ledger pruned by key presence rather than count: 23c's final write only. +- ledger honoured whenever it exists: 24 and 25. +- ledger stamped by content alone: 24 only. 25 cannot see it, because its + intervening `add` makes the restored-then-lost file differ in content + anyway. 24 is the case that carries identity. +- ledger re-stamped only by `rm`, keeping the old stamp on other writes: 24b + (as named), 20b and 23c. All three write again after an add. +- ledger stamped with the kb as read rather than the candidate: 20, 20b, 23c, + 24b and 25. That is wider than any one case, as expected, since no later + write can ever match. +- the `gh/main` `scripts/kb` (old refusal text, no stamp): 19b, 24 and 25. + +**Case 25 is the first to run the production paths.** Every other case sets +`KB_FILE`, so `ledger_path`'s `git rev-parse --git-path` branch and +`baseline_to`'s `git show HEAD` were unexercised. It copies the tool into a +throwaway `git init` repo and checks four things. `rm` records to the git-path +ledger. `git status` shows only `kb.pl` modified, so the ledger is invisible +to git. A later `add` is excused. After `git checkout --` and a hand loss, the +`add` is refused. + +**Checked by hand, not in the suite:** a `SIGKILL` at the publish point (the +`KB_TEST_KILL` seam) on a copy inside `prolog-sessions/` leaves the candidate, +its `.base`, `.ledger`, `.removed` and `.stamped` siblings and `.lock`. `git +status --untracked-files=all` and `git add -A --dry-run` both show none of +them, and `git check-ignore -v` names `.gitignore:40` (`prolog-sessions/*`). diff --git a/crates/improvise-command/src/command/cmd/grid.rs b/crates/improvise-command/src/command/cmd/grid.rs index ef471310..b06b8570 100644 --- a/crates/improvise-command/src/command/cmd/grid.rs +++ b/crates/improvise-command/src/command/cmd/grid.rs @@ -1,9 +1,10 @@ use crate::model::cell::CellValue; use crate::ui::app::AppMode; use crate::ui::effect::{self, Effect, ViewTarget}; -use crate::view::AxisEntry; +use crate::view::{AxisEntry, GridLayout}; use super::core::{Cmd, CmdContext}; +use super::navigation::viewport_effects; #[cfg(test)] mod tests { @@ -33,10 +34,12 @@ mod tests { wb } + /// The real path: the App's own context, the command, then `apply_effects`, + /// so a placement the toggle's reframe throws away is seen. fn toggle_group_at_cursor(app: &mut crate::ui::app::App) -> String { + use crate::command::key::{KeyCode, KeyModifiers}; let effects = { - let reg = make_registry(); - let ctx = make_ctx(&app.model_state.workbook, &app.layout, ®); + let ctx = app.cmd_context(KeyCode::Char('z'), KeyModifiers::NONE); ToggleGroupAtCursor { is_row: true }.execute(&ctx) }; let dbg = effects_debug(&effects); @@ -44,6 +47,12 @@ mod tests { dbg } + fn row_labels(app: &crate::ui::app::App) -> Vec { + (0..app.layout.row_count()) + .map(|r| app.layout.row_label(r)) + .collect() + } + /// Evidence: `context/test-evidence/command.md`. #[test] fn toggling_at_the_cursor_names_the_group_the_cursor_sits_in_and_collapses_it() { @@ -51,68 +60,157 @@ mod tests { let mut app = App::new(two_groups_on_the_row_axis(), None); app.rebuild_layout(); + // Off row 0, or a toggle that ignores the cursor names Q1 and passes. + app.cursor_mut().selected = (2, 0); assert!( !app.model_state .workbook .active_view() - .is_group_collapsed("Month", "Q1"), + .is_group_collapsed("Month", "Q2"), "the fixture starts expanded, or the assertion below proves nothing" ); let dbg = toggle_group_at_cursor(&mut app); assert!( - dbg.contains(r#"ToggleGroup { view: Active, category: "Month", group: "Q1" }"#), - "the cursor sits on Jan, which is in Q1, got: {dbg}" + dbg.contains(r#"ToggleGroup { view: Active, category: "Month", group: "Q2" }"#), + "the cursor sits on Apr, which is in Q2, got: {dbg}" + ); + let view = app.model_state.workbook.active_view(); + assert!( + view.is_group_collapsed("Month", "Q2"), + "the effect was returned but the view did not move" ); assert!( - app.model_state - .workbook - .active_view() - .is_group_collapsed("Month", "Q1"), - "the effect was returned but the view did not move" + !view.is_group_collapsed("Month", "Q1"), + "the neighbour was left alone" ); } /// Evidence: `context/test-evidence/command.md`. #[test] - fn a_second_toggle_at_the_same_cursor_names_the_next_group_not_the_collapsed_one() { + fn toggling_a_group_collapses_that_group_and_the_cursor_follows_it() { use crate::ui::app::App; let mut app = App::new(two_groups_on_the_row_axis(), None); app.rebuild_layout(); - toggle_group_at_cursor(&mut app); + app.cursor_mut().selected = (2, 0); + assert_eq!( + app.layout.row_label(2), + "Apr", + "the cursor starts inside Q2" + ); + + let dbg = toggle_group_at_cursor(&mut app); + + assert!(dbg.contains(r#"group: "Q2""#), "Apr is in Q2, got: {dbg}"); + assert_eq!(row_labels(&app), ["Jan", "Feb", "Q2"]); + assert_eq!(app.cursor().selected, (2, 0), "the cursor is on the Q2 row"); let dbg = toggle_group_at_cursor(&mut app); assert!( dbg.contains(r#"group: "Q2""#), - "collapsing Q1 removes its rows, so data row 0 is now Apr in Q2, got: {dbg}" + "z again names Q2, got: {dbg}" ); - let view = app.model_state.workbook.active_view(); - assert!( - view.is_group_collapsed("Month", "Q1") && view.is_group_collapsed("Month", "Q2"), - "pressing twice collapses both groups; it is not an involution" + assert_eq!( + row_labels(&app), + ["Jan", "Feb", "Apr", "May"], + "z z is a no-op" + ); + assert_eq!(app.cursor().selected, (2, 0), "back on Q2's first item"); + } + + /// Evidence: `context/test-evidence/command.md`. + #[test] + fn a_collapse_from_the_tail_of_a_run_lands_on_the_runs_first_row() { + use crate::ui::app::App; + + let mut app = App::new(two_groups_on_the_row_axis(), None); + app.rebuild_layout(); + app.cursor_mut().selected = (3, 0); + assert_eq!(app.layout.row_label(3), "May"); + + toggle_group_at_cursor(&mut app); + + assert_eq!(row_labels(&app), ["Jan", "Feb", "Q2"]); + assert_eq!(app.cursor().selected, (2, 0), "May folded into the Q2 row"); + } + + /// Evidence: `context/test-evidence/command.md`. + #[test] + fn collapsing_an_inner_group_lands_where_every_earlier_run_shrank() { + use crate::ui::app::App; + use crate::view::Axis; + + let mut wb = crate::workbook::Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_category("Month").unwrap(); + wb.add_category("Type").unwrap(); + wb.add_item("Region", "North").unwrap(); + wb.add_item("Region", "South").unwrap(); + for (month, quarter) in [("Jan", "Q1"), ("Feb", "Q1"), ("Apr", "Q2"), ("May", "Q2")] { + wb.add_item_in_group("Month", month, quarter).unwrap(); + } + wb.add_item("Type", "Food").unwrap(); + wb.active_view_mut().set_axis("Region", Axis::Row); + wb.active_view_mut().set_axis("Month", Axis::Row); + wb.active_view_mut().set_axis("Type", Axis::Column); + let mut app = App::new(wb, None); + app.rebuild_layout(); + assert_eq!( + app.layout.row_cats, + ["Region", "Month"], + "Month is the INNER level" + ); + app.cursor_mut().selected = (6, 0); + assert_eq!( + app.layout.row_tuple(6), + Some(&["South".into(), "Apr".into()][..]) + ); + + toggle_group_at_cursor(&mut app); + + assert_eq!( + row_labels(&app), + [ + "North/Jan", + "North/Feb", + "North/Q2", + "South/Jan", + "South/Feb", + "South/Q2" + ] + ); + assert_eq!( + app.cursor().selected, + (5, 0), + "South's Q2, not the old index 6" ); } /// Evidence: `context/test-evidence/command.md`. #[test] - fn a_fully_collapsed_row_axis_leaves_the_command_nothing_to_address() { + fn a_collapsed_group_can_be_re_expanded_from_the_grid() { use crate::ui::app::App; let mut app = App::new(two_groups_on_the_row_axis(), None); app.rebuild_layout(); + app.cursor_mut().selected = (2, 0); toggle_group_at_cursor(&mut app); + app.cursor_mut().selected = (1, 0); toggle_group_at_cursor(&mut app); - assert_eq!(app.layout.row_count(), 0); + assert_eq!(row_labels(&app), ["Q1", "Q2"], "both groups collapsed"); + app.cursor_mut().selected = (1, 0); let dbg = toggle_group_at_cursor(&mut app); - assert_eq!( - dbg, "[]", - "there is no row under the cursor to name a group" + assert!( + dbg.contains(r#"group: "Q2""#), + "the Q2 row names Q2, got: {dbg}" ); + assert_eq!(row_labels(&app), ["Q1", "Apr", "May"]); + assert_eq!(app.cursor().selected, (1, 0), "on Q2's first item"); } #[test] @@ -338,11 +436,61 @@ impl Cmd for ToggleGroupAtCursor { let Some((cat, group)) = lookup else { return vec![]; }; - vec![Box::new(effect::ToggleGroup { + let landing = self.landing(ctx, &cat, &group); + // The toggle reframes and so resets the cursor; the placement must come + // AFTER it or it is thrown away (gotcha 24, core.md §14). + let mut effects: Vec> = vec![Box::new(effect::ToggleGroup { view: ViewTarget::Active, category: cat, group, - })] + })]; + effects.extend( + landing + .map(|at| self.place_cursor(ctx, at)) + .unwrap_or_default(), + ); + effects + } +} + +impl ToggleGroupAtCursor { + /// Where the toggled group sits once toggled, asked of the layout the toggle + /// will produce — collapsing an inner level shifts every earlier run (§14). + fn landing(&self, ctx: &CmdContext, cat: &str, group: &str) -> Option { + let (cats, tuple) = match self.is_row { + true => (&ctx.layout.row_cats, ctx.layout.row_tuple(ctx.selected.0)?), + false => (&ctx.layout.col_cats, ctx.layout.col_tuple(ctx.selected.1)?), + }; + let prefix = tuple.get(..cats.iter().position(|c| c == cat)?)?; + let mut view = ctx.view.clone(); + view.toggle_group_collapse(cat, group); + let after = GridLayout::with_frozen_records(ctx.model, &view, None); + match self.is_row { + true => after.row_run_start(cat, group, prefix), + false => after.col_run_start(cat, group, prefix), + } + } + + /// The cursor on `at`, the scroll restored where the toggle zeroed it and + /// moved only as far as `at` needs. + fn place_cursor(&self, ctx: &CmdContext, at: usize) -> Vec> { + let (row, col) = match self.is_row { + true => (at, ctx.selected.1), + false => (ctx.selected.0, at), + }; + let mut effects: Vec> = vec![ + Box::new(effect::SetRowOffset(ctx.row_offset)), + Box::new(effect::SetColOffset(ctx.col_offset)), + ]; + effects.extend(viewport_effects( + row, + col, + ctx.row_offset, + ctx.col_offset, + ctx.visible_rows, + ctx.visible_cols, + )); + effects } } @@ -363,8 +511,8 @@ impl Cmd for HideSelectedRowItem { .row_items .iter() .filter_map(|e| { - if let AxisEntry::DataItem(v) = e { - Some(v) + if let AxisEntry::DataItem { tuple, .. } = e { + Some(tuple) } else { None } diff --git a/crates/improvise-command/src/command/cmd/mode.rs b/crates/improvise-command/src/command/cmd/mode.rs index 043e4b73..6d5d31b7 100644 --- a/crates/improvise-command/src/command/cmd/mode.rs +++ b/crates/improvise-command/src/command/cmd/mode.rs @@ -188,6 +188,39 @@ mod tests { ); } + /// Evidence: `context/test-evidence/command.md`. + #[test] + fn edit_or_drill_declines_a_collapsed_groups_row() { + let mut wb = crate::workbook::Workbook::new("T"); + wb.add_category("Month").unwrap(); + wb.add_category("Type").unwrap(); + for (month, quarter) in [("Jan", "Q1"), ("Feb", "Q1"), ("Apr", "Q2"), ("May", "Q2")] { + wb.add_item_in_group("Month", month, quarter).unwrap(); + } + wb.add_item("Type", "Food").unwrap(); + wb.active_view_mut().toggle_group_collapse("Month", "Q2"); + let layout = make_layout(&wb); + let reg = make_registry(); + let mut ctx = make_ctx(&wb, &layout, ®); + let edit = EditOrDrill { + edit_mode: AppMode::editing(), + }; + ctx.selected = (1, 0); + assert!( + effects_debug(&edit.execute(&ctx)).contains("Editing"), + "Feb, beside the fold, still edits" + ); + + ctx.selected = (2, 0); + let dbg = effects_debug(&edit.execute(&ctx)); + + assert_eq!(layout.row_label(2), "Q2"); + assert!( + dbg.contains("Collapsed group") && !dbg.contains("Editing") && !dbg.contains("Drill"), + "the fold names no record to edit or drill into, got: {dbg}" + ); + } + /// improvise-ir7l: the cell under the cursor is a formula target, so there is /// nothing to type into. `context/test-evidence/command.md`. #[test] @@ -320,6 +353,12 @@ impl Cmd for EditOrDrill { "edit-or-drill" } fn execute(&self, ctx: &CmdContext) -> Vec> { + // Neither edit nor drill: no record carries a group's name (core.md §14). + if ctx.layout.folds_a_group(ctx.selected.0, ctx.selected.1) { + return vec![effect::set_status( + "Collapsed group — press z to expand it, then edit an item", + )]; + } // Only **regular** categories on None are true aggregation: virtuals like // `_Index`/`_Dim` are always None in pivot mode and imply nothing. let regular_none = ctx.none_cats().iter().any(|c| { diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index b026516f..56f6fb8c 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -101,9 +101,10 @@ fn frame_of(layout: &crate::view::layout::GridLayout, view: &View) -> u64 { group_name.hash(&mut h); view.is_group_collapsed(cat_name, group_name).hash(&mut h); } - crate::view::layout::AxisEntry::DataItem(tuple) => { + crate::view::layout::AxisEntry::DataItem { tuple, collapsed } => { 1u8.hash(&mut h); tuple.hash(&mut h); + collapsed.hash(&mut h); } // Drawn as nothing, but it ends a group's run (improvise-gd83), // so where it sits is what a row's group attribution depends on. diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 441af17e..1fe8526e 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -205,7 +205,7 @@ fn axis_frame( ) -> AxisFrame { let total = items .iter() - .filter(|e| matches!(e, AxisEntry::DataItem(_))) + .filter(|e| matches!(e, AxisEntry::DataItem { .. })) .count(); let mut data_idx = 0usize; let mut entries: Vec = Vec::new(); @@ -230,7 +230,7 @@ fn axis_frame( // Queued like a header, never clearing one: a COLLAPSED group has // no item to carry it and reaches the frame only through `pending`. AxisEntry::GroupEnd => pending.push(FrameEntry::GroupEnd), - AxisEntry::DataItem(tuple) => { + AxisEntry::DataItem { tuple, .. } => { let idx = data_idx; data_idx += 1; if window.contains(&idx) { @@ -332,11 +332,14 @@ fn cell_id(app: &App, layout: &GridLayout, row: usize, col: usize) -> Option App { + /// Two group headers between `North` and `East`: `Hidden`, collapsed into one + /// row, and `Late`, open. + fn app_with_a_collapsed_group_between_rows() -> App { let mut wb = Workbook::new("Consecutive"); wb.add_category("Region").unwrap(); wb.add_item("Region", "North").unwrap(); @@ -1420,11 +1423,11 @@ mod tests { app } - /// Headers between two in-window rows all come, not just the nearest: the far - /// one is the only row saying those items exist. Above the window, the opposite. + /// Headers between two in-window rows all come, each above the row it opens. + /// Evidence: context/test-evidence/command.md. #[test] - fn consecutive_headers_between_two_rows_all_come() { - let app = app_with_consecutive_headers(); + fn every_header_between_two_in_window_rows_comes() { + let app = app_with_a_collapsed_group_between_rows(); let grid = grid_of( &app, Viewport { @@ -1436,25 +1439,17 @@ mod tests { ); assert_eq!( frame_shape(&grid.row_frame), - vec!["North", "[Hidden]", "[Late]", "East"], - "both headers sit between the two rows and both are drawn" + vec!["North", "[Hidden]", "Hidden", "[Late]", "East"], + "both headers are drawn, the collapsed one above its folded row" ); - let group = grid.row_frame.group_of(1).expect("East is under Late"); - assert_eq!(group.name, "Late", "the NEAREST header wins membership"); let hidden = grid .row_frame - .entries - .iter() - .find_map(|e| match e { - FrameEntry::Group(g) if g.name == "Hidden" => Some(g), - _ => None, - }) - .expect("kept"); - assert!( - hidden.collapsed, - "and it arrives collapsed, which is what makes it the row that \ - says those rows still exist" - ); + .group_of(1) + .expect("the fold is under Hidden"); + assert_eq!(hidden.name, "Hidden"); + assert!(hidden.collapsed, "and it arrives collapsed"); + let late = grid.row_frame.group_of(2).expect("East is under Late"); + assert_eq!(late.name, "Late", "the NEAREST header wins membership"); } fn app_with_data() -> App { @@ -4302,6 +4297,48 @@ mod cell_identity { .unwrap_or_else(|| panic!("cell ({row}, {col}) was projected with no identity")) } + /// Nothing regular on None, so only the fold can make the row an aggregate. + /// Evidence: context/test-evidence/command.md. + #[test] + fn a_collapsed_groups_row_is_projected_as_an_aggregate() { + let mut wb = Workbook::new("Folded"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "North").unwrap(); + wb.add_item_in_group("Region", "East", "Coastal").unwrap(); + wb.add_item_in_group("Region", "West", "Coastal").unwrap(); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.active_view_mut().set_axis("Region", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + for (region, value) in [("North", 1.0), ("East", 3.0), ("West", 4.0)] { + wb.model.set_cell( + CellKey::new(vec![ + ("Region".to_string(), region.to_string()), + ("_Measure".to_string(), "Revenue".to_string()), + ]), + CellValue::Number(value), + ); + } + wb.active_view_mut() + .toggle_group_collapse("Region", "Coastal"); + let mut app = App::new(wb, None); + app.rebuild_layout(); + assert_eq!(app.layout.row_label(1), "Coastal"); + + assert!( + matches!(id_at(&app, 0, 0), CellId::Record(_)), + "North is stored, so the none-axis arm is not what answers below" + ); + assert!( + matches!(id_at(&app, 1, 0), CellId::Aggregate(_)), + "a fold has no record under it and must not be offered as settable" + ); + let grid = grid_of(&app, WHOLE); + assert_eq!( + grid.cell(1, 0).expect("the fold's cell").text.trim(), + "7.00" + ); + } + fn record_of_id(id: CellId) -> RecordCell { match id { CellId::Record(cell) => cell, diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index 55d78cd7..e59e54cf 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -10,6 +10,9 @@ use crate::ui::app::App; use crate::ui::effect::{self, Effect, ViewTarget}; use crate::ui::project::{Dirty, document_projections}; +#[cfg(test)] +mod vocabulary_is_complete; + /// A wire [`Command`] whose basis the authoritative state has answered for /// (improvise-ondj/7v62, ADR 001). See `context/code/command.md`. pub struct Checked<'a>(&'a Command); diff --git a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs new file mode 100644 index 00000000..b91ed80d --- /dev/null +++ b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs @@ -0,0 +1,247 @@ +//! Every effect is on the wire under its own name, or named below with its +//! verdict: the forward twin of `vocabulary_is_composed.rs`. `context/code/protocol.md` §2. + +use std::collections::BTreeSet; +use std::path::Path; + +use improvise_protocol::source_scan::{production_code, production_text}; +use improvise_protocol::variants::declared; +use improvise_protocol::{ClientFrame, Command, ModelCommand}; + +/// Why an effect has no same-named `Command`/`ModelCommand` variant. +enum Verdict { + /// On the wire as this `Command` variant, under another name. + Command(&'static str), + /// Answered by this `ClientFrame` variant, which is not a `Command`. + Frame(&'static str), + /// Deliberately off the wire, and the argument. + Off(&'static str), + /// Missing, and the ticket that owns closing it. + Gap(&'static str), +} + +/// Checked in all directions: an entry must name a real effect, must not be on +/// the wire by name, and a `Command`/`Frame` verdict must name a real variant. +const NOT_ON_THE_WIRE: &[(&str, Verdict)] = &[ + ("HelpPageNext", Verdict::Command("HelpPage")), + ("HelpPagePrev", Verdict::Command("HelpPage")), + ("HelpPageSet", Verdict::Command("HelpPage")), + ("Save", Verdict::Frame("Save")), + ( + "SaveAs", + Verdict::Gap("improvise-2mqz: writes the host's Storage and has no frame"), + ), + ( + "ExportCsv", + Verdict::Gap("improvise-2mqz: writes the host's Storage and has no frame"), + ), + ( + "ApplyAndClearDrill", + Verdict::Off("operand is one session's staged drill edits (gotcha 38, improvise-3x3u)"), + ), + ( + "AcceptWizardModel", + Verdict::Off("operand is one session's built wizard workbook (gotcha 38, improvise-3x3u)"), + ), + ( + "StartDrill", + Verdict::Off("writes drill_state, session-local residue (gotcha 30)"), + ), + ( + "SetDrillPendingEdit", + Verdict::Off("writes drill_state.pending_edits, session-local residue (gotcha 30)"), + ), + ( + "StartImportWizard", + Verdict::Off("writes App::wizard, session-local residue (gotcha 30, improvise-764)"), + ), + ( + "WizardApply", + Verdict::Off("writes App::wizard, session-local residue (gotcha 30, improvise-764)"), + ), + ( + "CloseWizard", + Verdict::Off("writes App::wizard, session-local residue (gotcha 30, improvise-764)"), + ), + ( + "RebuildLayout", + Verdict::Off( + "refreshes the host's layout cache mid-batch; a client holds none (gotcha 23)", + ), + ), + ( + "AbortChain", + Verdict::Off("control flow inside one apply_effects batch; it writes no state (gotcha 21)"), + ), + ( + "ReturnToNormalUnlessMoved", + Verdict::Off("relative to the batch it ends (gotcha 31); a client composes ChangeMode"), + ), + ( + "EnterEditAtCursor", + Verdict::Off( + "reads the host's layout; a client composes SetBuffer + ChangeMode off its own cache", + ), + ), +]; + +#[test] +fn every_effect_is_on_the_wire_or_has_a_verdict() { + let on_the_wire = wire_names(); + let missing: Vec = effects_in_production() + .into_iter() + .filter(|name| !on_the_wire.contains(name)) + .filter(|name| !NOT_ON_THE_WIRE.iter().any(|(n, _)| n == name)) + .collect(); + assert!( + missing.is_empty(), + "effect::{missing:?} has no Command or ModelCommand of its name and no \ + verdict. Put it on the wire, or name it in NOT_ON_THE_WIRE with the \ + argument (improvise-h3y7) — gotcha 38's ViewBack was exactly this" + ); +} + +#[test] +fn the_verdict_list_names_nothing_that_is_on_the_wire() { + let on_the_wire = wire_names(); + let stale: Vec<&str> = NOT_ON_THE_WIRE + .iter() + .map(|(name, _)| *name) + .filter(|name| on_the_wire.contains(*name)) + .collect(); + assert!( + stale.is_empty(), + "effect::{stale:?} has a verdict and IS on the wire. \ + doctrine(a_stale_list_of_what_is_missing_is_worse_than_no_list)" + ); +} + +#[test] +fn the_verdict_list_names_only_real_effects_and_real_variants() { + let effects = effects_in_production(); + let commands = declared::(); + let frames = declared::(); + for (name, verdict) in NOT_ON_THE_WIRE { + assert!(effects.contains(*name), "no effect is named {name}"); + match verdict { + Verdict::Command(variant) => assert!( + commands.iter().any(|c| c == variant), + "{name}'s verdict names Command::{variant}, which does not exist" + ), + Verdict::Frame(variant) => assert!( + frames.iter().any(|f| f == variant), + "{name}'s verdict names ClientFrame::{variant}, which does not exist" + ), + Verdict::Off(why) | Verdict::Gap(why) => { + assert!(!why.is_empty(), "{name}'s verdict gives no argument") + } + } + } +} + +/// The scan must reach both spellings of an effect and every file that holds +/// one — the h3y7 audit's brace-matcher missed nine that way (check_the_checker). +#[test] +fn the_scan_reaches_every_way_an_effect_is_declared() { + let effects = effects_in_production(); + for name in ["TransposeAxes", "SetRowOffset", "SetTransientPrefix"] { + assert!( + effects.contains(name), + "the scan missed {name}, so it under-reports: {effects:?}" + ); + } +} + +fn wire_names() -> BTreeSet { + declared::() + .into_iter() + .chain(declared::()) + .collect() +} + +/// Every `impl Effect for X` and `simple_effect!(X` in this crate's production +/// `src/`: the one scanner both vocabulary guards share (improvise-c7mu). +fn effects_in_production() -> BTreeSet { + production_code(&[Path::new(env!("CARGO_MANIFEST_DIR")).join("src")]) + .iter() + .flat_map(|(_, code)| effects_named(code)) + .collect() +} + +/// One file's effects. A comment, a string or a test fake naming one is not +/// one, or a stale verdict could outlive the effect it names. +fn effects_in(text: &str) -> Vec { + effects_named(&production_text(text)) +} + +fn effects_named(code: &str) -> Vec { + let mut found = Vec::new(); + for marker in ["impl Effect for ", "simple_effect!("] { + for (at, _) in code.match_indices(marker) { + let name = name_after(&code[at + marker.len()..]); + if name.starts_with(|c: char| c.is_ascii_uppercase()) { + found.push(name); + } + } + } + found +} + +/// The identifier a marker introduces. `simple_effect!` takes `$(#[$meta])*` +/// before the name, and a doc comment is one: skip them (improvise-94x2). +fn name_after(rest: &str) -> String { + let mut rest = rest.trim_start(); + while let Some(attr) = rest.strip_prefix("#[") { + rest = attr[closing_bracket(attr) + 1..].trim_start(); + } + rest.chars() + .take_while(|c| c.is_ascii_alphanumeric() || *c == '_') + .collect() +} + +/// Where the `]` that closes an attribute's `#[` falls in `inside`, the text +/// after it. Nested brackets count; an unclosed one runs to the end. +fn closing_bracket(inside: &str) -> usize { + let mut depth = 1usize; + for (i, c) in inside.char_indices() { + match c { + '[' => depth += 1, + ']' if depth == 1 => return i, + ']' => depth -= 1, + _ => {} + } + } + inside.len().saturating_sub(1) +} + +/// The spelling the macro itself documents, a doc comment and an attribute +/// before the name, and a line break after `(`. Each hid an effect. +#[test] +fn an_attributed_or_wrapped_effect_is_still_an_effect() { + let text = concat!( + "simple_effect!(\n", + " /// Documented, as the macro's own matcher allows.\n", + " #[allow(dead_code)]\n", + " #[cfg_attr(all(), doc = \"x\")]\n", + " Documented(bool) => |app| x, K\n", + ");\n", + "simple_effect!(\n Wrapped(bool) => |app| y, K);\n", + ); + assert_eq!(effects_in(text), vec!["Documented", "Wrapped"]); +} + +#[test] +fn an_effect_named_only_in_a_comment_or_string_is_not_an_effect() { + let text = concat!( + "// impl Effect for SaveAs {}\n", + "/// simple_effect!(SetYanked(Option) => |app| x, K);\n", + "let s = \"impl Effect for ExportCsv\";\n", + "impl Effect for Real {}\n", + "simple_effect!(AlsoReal(bool) => |app| y, K);\n", + "#[cfg(test)]\n", + "mod tests {\n", + " impl Effect for TestFake {}\n", + "}\n", + ); + assert_eq!(effects_in(text), vec!["Real", "AlsoReal"]); +} diff --git a/crates/improvise-command/src/ui/viewport.rs b/crates/improvise-command/src/ui/viewport.rs index 50bb2522..3edc7db9 100644 --- a/crates/improvise-command/src/ui/viewport.rs +++ b/crates/improvise-command/src/ui/viewport.rs @@ -123,7 +123,7 @@ fn cell_value( fn header_level_seed(layout: &GridLayout, n: usize) -> Vec { let mut widths = vec![0u16; n]; let items = layout.col_items.iter().filter_map(|e| match e { - AxisEntry::DataItem(v) => Some(v), + AxisEntry::DataItem { tuple, .. } => Some(tuple), AxisEntry::GroupHeader { .. } | AxisEntry::GroupEnd => None, }); for (ci, levels) in items.enumerate().take(n) { @@ -152,7 +152,7 @@ pub fn compute_col_widths(model: &Model, layout: &GridLayout, plan: &FormatPlan) /// keypress-path caller that borrows tuples — see `context/code/command.md`. pub fn compute_row_header_width(layout: &GridLayout) -> u16 { let items = layout.row_items.iter().filter_map(|e| match e { - AxisEntry::DataItem(v) => Some(v), + AxisEntry::DataItem { tuple, .. } => Some(tuple), AxisEntry::GroupHeader { .. } | AxisEntry::GroupEnd => None, }); header_level_widths(items, layout.row_cats.len().max(1)) diff --git a/crates/improvise-core/src/model/category.rs b/crates/improvise-core/src/model/category.rs index a2748df6..500ad680 100644 --- a/crates/improvise-core/src/model/category.rs +++ b/crates/improvise-core/src/model/category.rs @@ -199,6 +199,16 @@ impl Category { pub fn ordered_item_names(&self) -> Vec<&str> { self.items.keys().map(|s| s.as_str()).collect() } + + /// The items whose own `group` names `group`, in insertion order. Read off the + /// items, never `groups`: `add_item_in_group` registers no `Group` (core.md §14). + pub fn items_in_group(&self, group: &str) -> Vec<&str> { + self.items + .values() + .filter(|i| i.group.as_deref() == Some(group)) + .map(|i| i.name.as_str()) + .collect() + } } #[cfg(test)] diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index 522f7da6..ea394dbe 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -523,6 +523,10 @@ impl Model { } pub fn evaluate_aggregated(&self, key: &CellKey, agg: Aggregation) -> Option { + let members = self.group_expansion(key); + if !members.is_empty() { + return self.evaluate_group(key, &members, agg); + } if agg == Aggregation::Exact { return self.evaluate(key); } @@ -536,6 +540,55 @@ impl Model { aggregate_cell_values(&self.data.matching_values(&key.0), self.agg_for(key)) } + /// One key per member where `key` names a collapsed GROUP rather than an item — + /// the cross product over every such coordinate. Empty when none does (§14). + fn group_expansion(&self, key: &CellKey) -> Vec { + let mut expanded = vec![key.clone()]; + for (cat_name, name) in &key.0 { + let members = self + .category(cat_name) + .filter(|cat| !cat.items.contains_key(name.as_str())) + .map(|cat| cat.items_in_group(name)) + .unwrap_or_default(); + if members.is_empty() { + continue; + } + expanded = expanded + .iter() + .flat_map(|k| members.iter().map(|m| k.clone().with(cat_name, *m))) + .collect(); + } + match expanded.as_slice() { + [only] if only == key => Vec::new(), + _ => expanded, + } + } + + /// A group's cell: stored values fold ONCE over every record the group covers, + /// a derived one folds its members' results — why the two differ: core.md §14. + fn evaluate_group( + &self, + key: &CellKey, + members: &[CellKey], + agg: Aggregation, + ) -> Option { + let values: Vec = match self.formula_for(key) { + Some(_) => members + .iter() + .filter_map(|k| self.evaluate_aggregated(k, agg)) + .collect(), + None => members + .iter() + .flat_map(|k| match agg { + Aggregation::Exact => self.data.get(k).into_iter().collect(), + Aggregation::OverHidden => self.data.matching_values(&k.0), + }) + .cloned() + .collect(), + }; + aggregate_cell_values(&values.iter().collect::>(), self.agg_for(key)) + } + /// [`Self::evaluate_aggregated`] as `f64`, with `0.0` for an empty cell. pub fn evaluate_aggregated_f64(&self, key: &CellKey, agg: Aggregation) -> f64 { self.evaluate_aggregated(key, agg) @@ -1403,6 +1456,80 @@ mod model_tests { let result = m.evaluate_aggregated(&key, Aggregation::of(&none_cats)); assert_eq!(result, Some(CellValue::Number(20.0))); // avg(10, 30) = 20 } + + /// `Month` with `Q1 = {Jan, Feb}` built through `add_item_in_group`, which + /// registers no `Group` — the shape a document built in code has. + fn quarter_model(cells: &[(&str, &str, f64)]) -> Model { + let mut m = Model::new("T"); + m.add_category("Month").unwrap(); + m.add_category("Store").unwrap(); + for (month, quarter) in [("Jan", "Q1"), ("Feb", "Q1"), ("Apr", "Q2")] { + m.category_mut("Month") + .unwrap() + .add_item_in_group(month, quarter); + } + for store in ["A", "B"] { + m.category_mut("Store").unwrap().add_item(store); + } + m.category_mut("_Measure").unwrap().add_item("Revenue"); + for (month, store, value) in cells { + m.set_cell( + coord(&[("Month", month), ("Store", store), ("_Measure", "Revenue")]), + CellValue::Number(*value), + ); + } + m + } + + /// Evidence: context/test-evidence/core.md. + #[test] + fn a_collapsed_groups_cell_aggregates_its_items() { + let m = quarter_model(&[("Jan", "A", 3.0), ("Feb", "A", 4.0), ("Apr", "A", 100.0)]); + let key = coord(&[("Month", "Q1"), ("Store", "A"), ("_Measure", "Revenue")]); + assert_eq!( + m.evaluate_aggregated(&key, Aggregation::Exact), + Some(CellValue::Number(7.0)), + "Jan + Feb; not absent, not Jan alone, not a count, not Q2's Apr" + ); + } + + /// Evidence: context/test-evidence/core.md. + #[test] + fn a_groups_stored_values_fold_once_not_per_member() { + use crate::formula::AggFunc; + let mut m = quarter_model(&[("Jan", "A", 2.0), ("Jan", "B", 4.0), ("Feb", "A", 9.0)]); + let key = coord(&[("Month", "Q1"), ("_Measure", "Revenue")]); + let over_store = Aggregation::of(&["Store".to_string()]); + m.measure_agg.insert("Revenue".to_string(), AggFunc::Avg); + assert_eq!( + m.evaluate_aggregated(&key, over_store), + Some(CellValue::Number(5.0)), + "avg(2, 4, 9); folding the members' averages gives avg(3, 9) = 6" + ); + m.measure_agg.insert("Revenue".to_string(), AggFunc::Count); + assert_eq!( + m.evaluate_aggregated(&key, over_store), + Some(CellValue::Number(3.0)), + "three records; counting the members' counts gives 2" + ); + } + + /// Evidence: context/test-evidence/core.md. + #[test] + fn an_item_named_like_a_group_is_read_as_the_item() { + let mut m = quarter_model(&[("Jan", "A", 3.0), ("Feb", "A", 4.0)]); + m.category_mut("Month").unwrap().add_item("Q1"); + m.set_cell( + coord(&[("Month", "Q1"), ("Store", "A"), ("_Measure", "Revenue")]), + CellValue::Number(50.0), + ); + let key = coord(&[("Month", "Q1"), ("Store", "A"), ("_Measure", "Revenue")]); + assert_eq!( + m.evaluate_aggregated(&key, Aggregation::Exact), + Some(CellValue::Number(50.0)), + "the stored item, not Jan + Feb" + ); + } } #[cfg(test)] diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index d6ab4c00..495c3967 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -187,7 +187,7 @@ pub enum RecordCol { } /// One entry on a grid axis. A `GroupHeader` is always visible so its label -/// and collapse state show; a `DataItem` is absent when its group collapses. +/// and collapse state show; a collapsed group is ONE `DataItem` (core.md §14). #[derive(Debug, Clone, PartialEq)] pub enum AxisEntry { GroupHeader { @@ -197,18 +197,73 @@ pub enum AxisEntry { /// The run opened by the last `GroupHeader` ends here (improvise-gd83). /// Nothing is drawn for it; it exists so membership is not inherited. GroupEnd, - DataItem(Vec), + /// `collapsed` is not derivable from `tuple`: a group and an item may share a + /// name. Set for a folded group and for everything expanded beneath one. + DataItem { tuple: Vec, collapsed: bool }, +} + +impl AxisEntry { + /// An ordinary, uncollapsed data item. + pub fn item(tuple: Vec) -> Self { + AxisEntry::DataItem { + tuple, + collapsed: false, + } + } } /// The data-item tuples on an axis, skipping group headers. Every public /// `row`/`col` argument indexes this sequence, not the visual entry list. fn data_items(items: &[AxisEntry]) -> impl Iterator> { items.iter().filter_map(|e| match e { - AxisEntry::DataItem(v) => Some(v), + AxisEntry::DataItem { tuple, .. } => Some(tuple), AxisEntry::GroupHeader { .. } | AxisEntry::GroupEnd => None, }) } +/// Whether the data item at `idx` folds a collapsed group. +fn folds(items: &[AxisEntry], idx: usize) -> bool { + items + .iter() + .filter_map(|e| match e { + AxisEntry::DataItem { collapsed, .. } => Some(*collapsed), + AxisEntry::GroupHeader { .. } | AxisEntry::GroupEnd => None, + }) + .nth(idx) + .unwrap_or(false) +} + +/// The data index of the first item in the run `cat`/`group` opens under +/// `prefix` — where a toggle's cursor lands, collapsing or expanding (§14). +fn run_start( + cats: &[String], + items: &[AxisEntry], + cat: &str, + group: &str, + prefix: &[String], +) -> Option { + let level = cats.iter().position(|c| c == cat)?; + let mut data_idx = 0usize; + let mut armed = false; + for entry in items { + match entry { + AxisEntry::GroupHeader { + cat_name, + group_name, + } => armed |= cat_name == cat && group_name == group, + AxisEntry::GroupEnd => {} + AxisEntry::DataItem { tuple, .. } => { + if armed && tuple.get(..level) == Some(prefix) { + return Some(data_idx); + } + armed = false; + data_idx += 1; + } + } + } + None +} + /// The display label of the data item at `idx`, or empty when out of bounds. fn axis_label(items: &[AxisEntry], idx: usize) -> String { data_items(items) @@ -222,7 +277,7 @@ fn data_index_to_visual(items: &[AxisEntry], data_idx: usize) -> Option { items .iter() .enumerate() - .filter(|(_, e)| matches!(e, AxisEntry::DataItem(_))) + .filter(|(_, e)| matches!(e, AxisEntry::DataItem { .. })) .map(|(vi, _)| vi) .nth(data_idx) } @@ -267,7 +322,7 @@ fn prune_axis_items(items: &mut Vec, keep: impl Fn(usize) -> bool) { run_open = false; } } - AxisEntry::DataItem(_) => { + AxisEntry::DataItem { .. } => { run_had_item = true; if keep(data_idx) { if let Some(h) = pending_header.take() { @@ -344,7 +399,7 @@ impl GridLayout { && let Some(records) = frozen_records { let row_items: Vec = (0..records.len()) - .map(|i| AxisEntry::DataItem(vec![i.to_string()])) + .map(|i| AxisEntry::item(vec![i.to_string()])) .collect(); layout.row_items = row_items; layout.records = Some(records); @@ -427,13 +482,13 @@ impl GridLayout { let records = Self::group_records(model, &page_constraints(&page_coords)); let row_items: Vec = (0..records.len()) - .map(|i| AxisEntry::DataItem(vec![i.to_string()])) + .map(|i| AxisEntry::item(vec![i.to_string()])) .collect(); let record_cols = Self::records_columns(model); let col_items: Vec = record_cols .iter() - .map(|c| AxisEntry::DataItem(vec![Self::record_col_label(c)])) + .map(|c| AxisEntry::item(vec![Self::record_col_label(c)])) .collect(); Self { @@ -687,11 +742,38 @@ impl GridLayout { group_name, } => Some(Some((cat_name.clone(), group_name.clone()))), AxisEntry::GroupEnd => Some(None), - AxisEntry::DataItem(_) => None, + AxisEntry::DataItem { .. } => None, }) .flatten() } + /// Whether the cell at `(row, col)` folds a collapsed group: an aggregate + /// with no record under it. The one home of that answer (§14). + pub fn folds_a_group(&self, row: usize, col: usize) -> bool { + folds(&self.row_items, row) || folds(&self.col_items, col) + } + + /// The first data row of `cat`/`group`'s run under `prefix` — where a toggle + /// of that group puts the cursor, asked of the layout AFTER the toggle (§14). + pub fn row_run_start(&self, cat: &str, group: &str, prefix: &[String]) -> Option { + run_start(&self.row_cats, &self.row_items, cat, group, prefix) + } + + /// [`Self::row_run_start`] for a column group. + pub fn col_run_start(&self, cat: &str, group: &str, prefix: &[String]) -> Option { + run_start(&self.col_cats, &self.col_items, cat, group, prefix) + } + + /// The coordinate tuple of the Nth data row. + pub fn row_tuple(&self, row: usize) -> Option<&[String]> { + data_items(&self.row_items).nth(row).map(Vec::as_slice) + } + + /// The coordinate tuple of the Nth data column. + pub fn col_tuple(&self, col: usize) -> Option<&[String]> { + data_items(&self.col_items).nth(col).map(Vec::as_slice) + } + /// The `(cat_name, group_name)` of the nearest `GroupHeader` preceding /// the Nth data column. pub fn col_group_for(&self, data_col: usize) -> Option<(String, String)> { @@ -705,25 +787,28 @@ impl GridLayout { group_name, } => Some(Some((cat_name.clone(), group_name.clone()))), AxisEntry::GroupEnd => Some(None), - AxisEntry::DataItem(_) => None, + AxisEntry::DataItem { .. } => None, }) .flatten() } } /// Expand one category into `AxisEntry` values under a coordinate prefix: a -/// `GroupHeader` per boundary, then the visible, non-collapsed items. +/// `GroupHeader` per boundary, then each visible item or collapsed group (§14). fn expand_category( model: &Model, view: &View, cat_name: &str, prefix: Vec, + prefix_collapsed: bool, ) -> Vec { let Some(cat) = model.category(cat_name) else { return vec![]; }; let mut result = Vec::new(); let mut last_group: Option<&str> = None; + // A group's items need not be contiguous, so a collapsed one folds ONCE. + let mut folded: Vec<&str> = Vec::new(); // effective_item_names, so _Measure picks up formula targets. let effective_names = model.effective_item_names(cat_name); @@ -735,6 +820,10 @@ fn expand_category( .items .get(item_name.as_str()) .and_then(|i| i.group.as_deref()); + let collapsed = item_group.filter(|g| view.is_group_collapsed(cat_name, g)); + if collapsed.is_some_and(|g| folded.contains(&g)) { + continue; + } // A header on entering a group; a `GroupEnd` only on leaving one for // NO group — another group's header already ends the run. @@ -751,14 +840,18 @@ fn expand_category( last_group = item_group; } - // Skip the data item if its group is collapsed. - if item_group.is_some_and(|g| view.is_group_collapsed(cat_name, g)) { - continue; + let mut tuple = prefix.clone(); + match collapsed { + Some(g) => { + folded.push(g); + tuple.push(g.to_string()); + } + None => tuple.push(item_name.to_string()), } - - let mut row = prefix.clone(); - row.push(item_name.to_string()); - result.push(AxisEntry::DataItem(row)); + result.push(AxisEntry::DataItem { + tuple, + collapsed: prefix_collapsed || collapsed.is_some(), + }); } // The expansion is the end of the run too: the next prefix's items follow // immediately and must not inherit it (improvise-gd83). @@ -772,14 +865,16 @@ fn expand_category( /// `vec![DataItem(vec![])]` when `cats` is empty. fn cross_product(model: &Model, view: &View, cats: &[String]) -> Vec { if cats.is_empty() { - return vec![AxisEntry::DataItem(vec![])]; + return vec![AxisEntry::item(vec![])]; } - let mut result: Vec = vec![AxisEntry::DataItem(vec![])]; + let mut result: Vec = vec![AxisEntry::item(vec![])]; for cat_name in cats { result = result .into_iter() .flat_map(|entry| match entry { - AxisEntry::DataItem(prefix) => expand_category(model, view, cat_name, prefix), + AxisEntry::DataItem { tuple, collapsed } => { + expand_category(model, view, cat_name, tuple, collapsed) + } marker @ (AxisEntry::GroupHeader { .. } | AxisEntry::GroupEnd) => vec![marker], }) .collect(); @@ -832,13 +927,13 @@ mod tests { #[test] fn prune_axis_items_indexes_data_positions_not_visual_ones() { let mut items = vec![ - AxisEntry::DataItem(vec!["A".into()]), + AxisEntry::item(vec!["A".into()]), AxisEntry::GroupHeader { cat_name: "C".into(), group_name: "G".into(), }, - AxisEntry::DataItem(vec!["B".into()]), - AxisEntry::DataItem(vec!["C".into()]), + AxisEntry::item(vec!["B".into()]), + AxisEntry::item(vec!["C".into()]), ]; // Data index 1 is "B": the header does not consume an index. If the // predicate saw visual indices, "A" would be the one dropped. @@ -861,8 +956,8 @@ mod tests { cat_name: "C".into(), group_name: "G".into(), }, - AxisEntry::DataItem(vec!["A".into()]), - AxisEntry::DataItem(vec!["B".into()]), + AxisEntry::item(vec!["A".into()]), + AxisEntry::item(vec!["B".into()]), ]; prune_axis_items(&mut items, |_| false); assert!(items.is_empty()); @@ -1256,29 +1351,92 @@ mod tests { ); } + /// The inversion of `collapsed_group_has_header_but_no_data_items`: a + /// collapsed group is ONE row. Evidence: context/test-evidence/core.md. #[test] - fn collapsed_group_has_header_but_no_data_items() { + fn a_collapsed_group_contributes_one_aggregate_row() { let mut wb = Workbook::new("T"); wb.add_category("Month").unwrap(); wb.add_category("Type").unwrap(); wb.add_item_in_group("Month", "Jan", "Q1").unwrap(); wb.add_item_in_group("Month", "Feb", "Q1").unwrap(); wb.add_item_in_group("Month", "Apr", "Q2").unwrap(); + wb.add_item_in_group("Month", "May", "Q2").unwrap(); wb.add_item("Type", "Food").unwrap(); wb.active_view_mut().toggle_group_collapse("Month", "Q1"); let layout = GridLayout::new(&wb.model, wb.active_view()); - // Q1 collapsed: header present, Jan and Feb absent; Q2 intact - assert_eq!(layout.row_count(), 1); // only Apr - let q1_header = layout + let labels: Vec = (0..layout.row_count()) + .map(|r| layout.row_label(r)) + .collect(); + assert_eq!( + labels, + ["Q1", "Apr", "May"], + "Q1 folds into one row; Q2 stays open" + ); + assert!(layout.folds_a_group(0, 0), "the Q1 row is a fold"); + assert!( + !layout.folds_a_group(1, 0), + "Apr, under the OPEN group, is not" + ); + assert_eq!( + layout.row_group_for(0), + Some(("Month".into(), "Q1".into())), + "the fold sits under its own header, so z can name it" + ); + } + + /// Evidence: context/test-evidence/core.md. + #[test] + fn a_non_contiguous_collapsed_group_folds_once() { + let mut wb = Workbook::new("T"); + wb.add_category("Month").unwrap(); + wb.add_category("Type").unwrap(); + wb.add_item_in_group("Month", "A", "G1").unwrap(); + wb.add_item_in_group("Month", "B", "G2").unwrap(); + wb.add_item_in_group("Month", "C", "G1").unwrap(); + wb.add_item("Type", "Food").unwrap(); + wb.active_view_mut().toggle_group_collapse("Month", "G1"); + let layout = GridLayout::new(&wb.model, wb.active_view()); + let labels: Vec = (0..layout.row_count()) + .map(|r| layout.row_label(r)) + .collect(); + assert_eq!(labels, ["G1", "B"], "one G1 row, not one per run"); + let g1_headers = layout .row_items .iter() - .find(|e| matches!(e, AxisEntry::GroupHeader { group_name, .. } if group_name == "Q1")); - assert!(q1_header.is_some()); - let jan = layout - .row_items - .iter() - .find(|e| matches!(e, AxisEntry::DataItem(v) if v.contains(&"Jan".to_string()))); - assert!(jan.is_none()); + .filter( + |e| matches!(e, AxisEntry::GroupHeader { group_name, .. } if group_name == "G1"), + ) + .count(); + assert_eq!( + g1_headers, 1, + "a second G1 header would open a run with no row" + ); + } + + /// Evidence: context/test-evidence/core.md. + #[test] + fn a_category_nested_under_a_collapsed_group_is_folded_too() { + let mut wb = Workbook::new("T"); + wb.add_category("Month").unwrap(); + wb.add_category("Region").unwrap(); + wb.add_category("Type").unwrap(); + wb.add_item_in_group("Month", "Jan", "Q1").unwrap(); + wb.add_item_in_group("Month", "Apr", "Q2").unwrap(); + wb.add_item("Region", "North").unwrap(); + wb.add_item("Type", "Food").unwrap(); + wb.active_view_mut().set_axis("Region", Axis::Row); + wb.active_view_mut().toggle_group_collapse("Month", "Q1"); + let layout = GridLayout::new(&wb.model, wb.active_view()); + assert_eq!( + layout.row_tuple(0), + Some(&["Q1".to_string(), "North".to_string()][..]) + ); + assert!( + layout.folds_a_group(0, 0), + "Q1/North has no record under it either" + ); + assert!(!layout.folds_a_group(1, 0), "Apr/North does"); } /// improvise-gd83: two arms — an ungrouped item AFTER a grouped run, and @@ -1513,13 +1671,22 @@ mod tests { cat_name: "Month".into(), group_name: "Q1".into() }, + AxisEntry::DataItem { + tuple: vec!["Q1".into()], + collapsed: true + }, AxisEntry::GroupEnd, - AxisEntry::DataItem(vec!["Sep".into()]), + AxisEntry::item(vec!["Sep".into()]), ], - "Q1 is collapsed, not empty — its header is the only way back, and Mar pruned" + "Q1 folds to one funded row under its header, and Mar pruned" ); assert_eq!( layout.row_group_for(0), + Some(("Month".into(), "Q1".into())), + "the fold is where z names the collapsed group from" + ); + assert_eq!( + layout.row_group_for(1), None, "Sep must not inherit the collapsed run" ); diff --git a/crates/improvise-io/src/persistence/export.rs b/crates/improvise-io/src/persistence/export.rs index 993de670..30a50c13 100644 --- a/crates/improvise-io/src/persistence/export.rs +++ b/crates/improvise-io/src/persistence/export.rs @@ -204,6 +204,35 @@ mod tests { assert_eq!(header, ["Jan"], "CSV wrote a column the grid does not draw"); } + /// An export writes what the grid draws (improvise-iot6), so a collapsed group + /// is one total row. Evidence: context/test-evidence/formula-and-io.md. + #[test] + fn export_writes_a_collapsed_group_as_its_own_total_row() { + use crate::view::Axis; + let mut wb = Workbook::new("T"); + wb.add_category("Month").unwrap(); + wb.add_category("Type").unwrap(); + wb.add_item_in_group("Month", "Jan", "Q1").unwrap(); + wb.add_item_in_group("Month", "Feb", "Q1").unwrap(); + wb.add_item("Type", "Food").unwrap(); + for (month, value) in [("Jan", 3.0), ("Feb", 4.0)] { + wb.model.set_cell( + coord(&[("Month", month), ("Type", "Food")]), + CellValue::Number(value), + ); + } + { + let v = wb.active_view_mut(); + v.set_axis("Month", Axis::Row); + v.set_axis("Type", Axis::Column); + v.prune_empty = true; + v.toggle_group_collapse("Month", "Q1"); + } + let csv = String::from_utf8(super::render_csv(&wb, "Default").unwrap()).unwrap(); + let rows: Vec<&str> = csv.lines().skip(1).collect(); + assert_eq!(rows, ["Q1,7.00"], "one folded row, summing Jan and Feb"); + } + #[test] fn export_csv_unknown_view_returns_error() { let m = two_cat_model(); diff --git a/crates/improvise-protocol/src/basis.rs b/crates/improvise-protocol/src/basis.rs index 4a908268..58d0ce9e 100644 --- a/crates/improvise-protocol/src/basis.rs +++ b/crates/improvise-protocol/src/basis.rs @@ -54,6 +54,9 @@ pub enum Rejection { /// A mutation in the frame refused to apply at all. The frame is undone; /// the refusal's own explanation is in the host's status line. MutationRefused, + /// This session cannot write the document, so it takes no edit at all + /// (improvise-j4qt). An edit it could not keep must never be painted. + ReadOnly, /// A [`ClientFrame::Save`](crate::ClientFrame::Save) reached a host with /// no path (improvise-368h). Decided before anything runs. @@ -109,7 +112,8 @@ impl Rejection { | Self::RecordMoved(_) | Self::RecordUnstated(_) | Self::ChainBroken(_) - | Self::MutationRefused => true, + | Self::MutationRefused + | Self::ReadOnly => true, Self::NoDocumentPath | Self::SaveFailed => false, } } @@ -136,7 +140,8 @@ impl Rejection { Self::RecordMoved(_) => Self::RecordUnstated(sample), Self::RecordUnstated(_) => Self::ChainBroken(sample), Self::ChainBroken(_) => Self::MutationRefused, - Self::MutationRefused => Self::NoDocumentPath, + Self::MutationRefused => Self::ReadOnly, + Self::ReadOnly => Self::NoDocumentPath, Self::NoDocumentPath => Self::SaveFailed, Self::SaveFailed => Self::DocumentReplaced, } diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index 8abc5ec4..1e178f92 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/crates/improvise-protocol/src/lib.rs @@ -14,6 +14,8 @@ mod reduce; pub mod render; pub mod resolve; #[cfg(any(test, feature = "test-support"))] +pub mod source_scan; +#[cfg(any(test, feature = "test-support"))] pub mod variants; pub mod viewmodel; #[cfg(test)] diff --git a/crates/improvise-protocol/src/reduce.rs b/crates/improvise-protocol/src/reduce.rs index 93f145fe..9e816f01 100644 --- a/crates/improvise-protocol/src/reduce.rs +++ b/crates/improvise-protocol/src/reduce.rs @@ -577,4 +577,100 @@ mod tests { assert_eq!(cursor, 7, "{panel:?}'s cursor is not the field it names"); } } + + /// improvise-hkbc: every field of every `ModelCommand` names its target by + /// name, by path or by surrogate. A row or column index fails to COMPILE. + fn position_free(cmd: &ModelCommand) -> usize { + fn name(_: &str) -> usize { + 1 + } + fn surrogate(_: &RecordHash) -> usize { + 1 + } + fn record(RecordCell { record, measure }: &RecordCell) -> usize { + surrogate(record) + optional_name(measure) + } + fn path(_: &std::path::Path) -> usize { + 1 + } + fn value(_: &CellValue) -> usize { + 1 + } + fn axis(_: &Axis) -> usize { + 1 + } + fn optional_name(_: &Option) -> usize { + 1 + } + // No `..` anywhere: a field added to a variant must be classified here. + match cmd { + ModelCommand::SetCell { cell, value: v } => record(cell) + value(v), + ModelCommand::ClearCell(cell) => record(cell), + ModelCommand::AddItem { category, item } + | ModelCommand::RemoveItem { category, item } => name(category) + name(item), + ModelCommand::AddItemInGroup { + category, + item, + group, + } => name(category) + name(item) + name(group), + ModelCommand::AddFormula { + raw, + target_category, + } => name(raw) + name(target_category), + ModelCommand::RemoveFormula { + target, + target_category, + } => name(target) + name(target_category), + ModelCommand::CleanEmptyRecords | ModelCommand::SortData => 0, + ModelCommand::AddCategory(category) + | ModelCommand::RemoveCategory(category) + | ModelCommand::CreateView(category) + | ModelCommand::DeleteView(category) => name(category), + ModelCommand::SetAxis { + view, + category, + axis: a, + } => name(view) + name(category) + axis(a), + ModelCommand::CycleAxis { view, category } => name(view) + name(category), + ModelCommand::TransposeAxes { view } | ModelCommand::TogglePruneEmpty { view } => { + name(view) + } + ModelCommand::SetPageSelection { + view, + category, + item, + } + | ModelCommand::HideItem { + view, + category, + item, + } + | ModelCommand::ShowItem { + view, + category, + item, + } => name(view) + name(category) + name(item), + ModelCommand::ToggleGroup { + view, + category, + group, + } => name(view) + name(category) + name(group), + ModelCommand::SetNumberFormat { view, format } => name(view) + name(format), + ModelCommand::LoadModel(p) => path(p), + ModelCommand::ImportJsonHeadless { + path: p, + model_name, + array_path, + } => path(p) + optional_name(model_name) + optional_name(array_path), + } + } + + /// The guard is the signature above; this only keeps it compiled and reached. + #[test] + fn every_model_command_field_is_position_free() { + assert_eq!( + position_free(&ModelCommand::TransposeAxes { view: "V".into() }), + 1 + ); + } } diff --git a/crates/improvise-protocol/src/source_scan.rs b/crates/improvise-protocol/src/source_scan.rs new file mode 100644 index 00000000..419705e8 --- /dev/null +++ b/crates/improvise-protocol/src/source_scan.rs @@ -0,0 +1,346 @@ +//! Production Rust source, for the guards that read source text: no comments, +//! no literals, no test code. The one home of that rule, `context/code/protocol.md` §2. + +use std::collections::BTreeSet; +use std::path::{Path, PathBuf}; + +/// Every `.rs` file under `roots` that is compiled outside tests, with its +/// comments, literals and test-only items blanked or dropped. +pub fn production_code(roots: &[PathBuf]) -> Vec<(PathBuf, String)> { + let mut files = Vec::new(); + for root in roots { + collect_rust(root, &mut files); + } + let code: Vec<(PathBuf, String)> = files + .into_iter() + .map(|file| { + let text = std::fs::read_to_string(&file).expect("a listed source reads"); + let code = code_only(&text); + (file, code) + }) + .collect(); + let test_modules: BTreeSet = code + .iter() + .flat_map(|(file, code)| test_module_paths(file, code)) + .collect(); + code.into_iter() + .filter(|(file, _)| !test_modules.iter().any(|m| is_module_file(m, file))) + .map(|(file, code)| (file, strip_test_items(&code))) + .collect() +} + +/// One file's text as [`production_code`] cleans it. Which *files* are test +/// modules needs their declaring file too, so only `production_code` knows. +pub fn production_text(text: &str) -> String { + strip_test_items(&code_only(text)) +} + +fn collect_rust(dir: &Path, into: &mut Vec) { + let Ok(entries) = std::fs::read_dir(dir) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + match path.is_dir() { + true => collect_rust(&path, into), + false if path.extension().is_some_and(|e| e == "rs") => into.push(path), + false => {} + } + } +} + +/// `m` is a module path without extension: its file is `m.rs`, `m/mod.rs`, +/// or anything under `m/`. +fn is_module_file(m: &Path, file: &Path) -> bool { + file == m.with_extension("rs") || file.starts_with(m) +} + +/// The out-of-line modules `file` declares under a test-only `cfg`, as paths +/// without extension (`#[cfg(test)] mod x;` is `/x`). +fn test_module_paths(file: &Path, code: &str) -> Vec { + let dir = match file.file_stem().and_then(|s| s.to_str()) { + Some("lib" | "main" | "mod") => file.parent().map(Path::to_path_buf), + Some(stem) => file.parent().map(|p| p.join(stem)), + None => None, + } + .expect("a source file has a directory and a stem"); + let lines: Vec<&str> = code.lines().map(str::trim).collect(); + let mut found = Vec::new(); + for (i, line) in lines.iter().enumerate() { + if !is_test_cfg(line) { + continue; + } + let item = lines[i + 1..] + .iter() + .find(|l| !l.starts_with("#[") && !l.is_empty()); + if let Some(name) = item.and_then(|l| out_of_line_mod(l)) { + found.push(dir.join(name)); + } + } + found +} + +/// `mod x;`, `pub mod x;`, `pub(crate) mod x;` → `x`. +fn out_of_line_mod(line: &str) -> Option<&str> { + let rest = line.split_once("mod ")?.1.strip_suffix(';')?; + let prefix = line.split_once("mod ")?.0.trim(); + (prefix.is_empty() || prefix.starts_with("pub")).then_some(rest.trim()) +} + +/// A `cfg` attribute that compiles its item only under `test`. `cfg(not(test))` +/// is production, and `"test-support"` is a literal, already blanked. +fn is_test_cfg(line: &str) -> bool { + let line = line.trim(); + line.starts_with("#[cfg(") + && !line.contains("not(") + && line + .split(|c: char| !c.is_alphanumeric() && c != '_') + .any(|word| word == "test") +} + +/// Drop every test-only item. rustfmt puts an item's closing brace at the +/// attribute's own indentation, which is what makes this a line scan (§2). +fn strip_test_items(text: &str) -> String { + let lines: Vec<&str> = text.lines().collect(); + let mut kept = String::new(); + let mut i = 0; + while i < lines.len() { + let line = lines[i]; + match is_test_cfg(line) { + false => { + kept.push_str(line); + kept.push('\n'); + i += 1; + } + true => { + let indent = line.len() - line.trim_start().len(); + let close = format!("{}}}", " ".repeat(indent)); + i += 1; + while i < lines.len() && lines[i].trim_start().starts_with("#[") { + i += 1; + } + match lines.get(i).is_some_and(|l| l.trim_end().ends_with(';')) { + true => i += 1, + false => { + while i < lines.len() && lines[i].trim_end() != close { + i += 1; + } + i += 1; + } + } + } + } + } + kept +} + +/// `text` with comments, string, byte-string, raw-string and char literals +/// replaced by spaces. Newlines survive, so line-based scans still line up. +pub fn code_only(text: &str) -> String { + let chars: Vec = text.chars().collect(); + let mut out = String::with_capacity(text.len()); + let mut i = 0; + while i < chars.len() { + let end = literal_or_comment_end(&chars, i); + match end { + Some(end) => { + for &c in &chars[i..end] { + out.push(if c == '\n' { '\n' } else { ' ' }); + } + i = end; + } + None => { + out.push(chars[i]); + i += 1; + } + } + } + out +} + +/// Where the comment or literal starting at `i` ends, if one starts there. +fn literal_or_comment_end(c: &[char], i: usize) -> Option { + let at = |k: usize| c.get(k).copied(); + let word_before = i > 0 && (c[i - 1].is_alphanumeric() || c[i - 1] == '_'); + match (at(i), at(i + 1)) { + (Some('/'), Some('/')) => Some(line_end(c, i)), + (Some('/'), Some('*')) => Some(block_end(c, i)), + (Some('"'), _) => Some(quoted_end(c, i + 1, '"')), + (Some('b'), Some('"')) if !word_before => Some(quoted_end(c, i + 2, '"')), + (Some('r' | 'b'), Some('r' | '#' | '"')) if !word_before => raw_end(c, i), + (Some('\''), _) => char_end(c, i), + _ => None, + } +} + +fn line_end(c: &[char], from: usize) -> usize { + (from..c.len()).find(|&k| c[k] == '\n').unwrap_or(c.len()) +} + +/// Block comments nest in Rust. +fn block_end(c: &[char], from: usize) -> usize { + let mut depth = 0usize; + let mut k = from; + while k + 1 < c.len() { + match (c[k], c[k + 1]) { + ('/', '*') => { + depth += 1; + k += 2; + } + ('*', '/') => { + depth -= 1; + k += 2; + if depth == 0 { + return k; + } + } + _ => k += 1, + } + } + c.len() +} + +fn quoted_end(c: &[char], mut k: usize, quote: char) -> usize { + while k < c.len() { + match c[k] { + '\\' => k += 2, + q if q == quote => return k + 1, + _ => k += 1, + } + } + c.len() +} + +/// `r"…"`, `r#"…"#`, `br#"…"#`. `None` when the `r` is only an identifier. +fn raw_end(c: &[char], i: usize) -> Option { + let mut k = i + usize::from(c[i] == 'b'); + if c.get(k) != Some(&'r') { + return None; + } + k += 1; + let hashes = c[k..].iter().take_while(|&&h| h == '#').count(); + k += hashes; + if c.get(k) != Some(&'"') { + return None; + } + k += 1; + while k < c.len() { + if c[k] == '"' + && c[k + 1..] + .iter() + .take(hashes) + .filter(|&&h| h == '#') + .count() + == hashes + { + return Some(k + 1 + hashes); + } + k += 1; + } + Some(c.len()) +} + +/// A char literal, or `None` for a lifetime (`'a` with no closing quote). +fn char_end(c: &[char], i: usize) -> Option { + match (c.get(i + 1), c.get(i + 2)) { + (Some('\\'), _) => Some(quoted_end(c, i + 1, '\'')), + (Some(_), Some('\'')) => Some(i + 3), + _ => None, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The walk itself drops a `#[cfg(test)] mod x;` file, not only the parts + /// that find one. A fixture tree, because the wiring is the thing tested. + #[test] + fn a_test_only_module_file_is_not_production_code() { + let root = std::env::temp_dir().join(format!("source_scan_{}", std::process::id())); + let src = root.join("src"); + std::fs::create_dir_all(src.join("t")).unwrap(); + std::fs::write(src.join("lib.rs"), "mod p;\n#[cfg(test)]\nmod t;\n").unwrap(); + std::fs::write(src.join("p.rs"), "fn production() {}\n").unwrap(); + std::fs::write(src.join("t.rs"), "fn test_only() {}\nmod deeper;\n").unwrap(); + std::fs::write(src.join("t").join("deeper.rs"), "fn test_too() {}\n").unwrap(); + let mut found: Vec = production_code(std::slice::from_ref(&src)) + .into_iter() + .map(|(file, _)| file.strip_prefix(&src).unwrap().display().to_string()) + .collect(); + found.sort(); + std::fs::remove_dir_all(&root).unwrap(); + assert_eq!(found, vec!["lib.rs", "p.rs"]); + } + + #[test] + fn test_only_items_and_modules_are_not_production() { + let text = concat!( + "fn kept() {}\n", + "#[cfg(test)]\n", + "mod inline {\n", + " fn gone_inline() {}\n", + "}\n", + "#[cfg(test)]\n", + "mod out_of_line;\n", + "#[cfg(any(test, feature = \"x\"))]\n", + "pub mod gated;\n", + "#[cfg(not(test))]\n", + "fn production_only() {}\n", + "fn also_kept() {}\n", + ); + let code = code_only(text); + let stripped = strip_test_items(&code); + for kept in ["fn kept", "fn production_only", "fn also_kept"] { + assert!(stripped.contains(kept), "{kept} was dropped: {stripped}"); + } + assert!(!stripped.contains("gone_inline"), "{stripped}"); + let lib = Path::new("/c/src/lib.rs"); + assert_eq!( + test_module_paths(lib, &code), + vec![ + PathBuf::from("/c/src/out_of_line"), + PathBuf::from("/c/src/gated") + ] + ); + let nested = Path::new("/c/src/ui/reduce.rs"); + assert_eq!( + test_module_paths(nested, &code)[0], + PathBuf::from("/c/src/ui/reduce/out_of_line") + ); + assert!(is_module_file( + Path::new("/c/src/gated"), + Path::new("/c/src/gated.rs") + )); + assert!(is_module_file( + Path::new("/c/src/gated"), + Path::new("/c/src/gated/a.rs") + )); + assert!(!is_module_file( + Path::new("/c/src/gated"), + Path::new("/c/src/gatedx.rs") + )); + } + + #[test] + fn comments_and_literals_are_blanked_and_code_is_kept() { + let text = concat!( + "let a = Command::Kept(1); // Command::LineComment(2)\n", + "/* Command::Block( /* nested */ Command::StillBlock ) */\n", + "let s = \"Command::InString(\\\"x\\\")\";\n", + "let r = r#\"Command::InRaw(\"q\")\"#;\n", + "let q = '\"'; let l: &'static str = \"\"; Command::AfterChar(3)\n", + ); + let kept = code_only(text); + assert!(kept.contains("Command::Kept(1)")); + assert!(kept.contains("Command::AfterChar(3)")); + assert!( + kept.contains("&'static str"), + "a lifetime is not a char literal" + ); + for gone in ["LineComment", "Block", "StillBlock", "InString", "InRaw"] { + assert!(!kept.contains(gone), "{gone} survived: {kept}"); + } + assert_eq!(kept.lines().count(), text.lines().count()); + } +} diff --git a/crates/improvise-protocol/src/vocabulary_is_composed.rs b/crates/improvise-protocol/src/vocabulary_is_composed.rs index 5902d835..559ff02d 100644 --- a/crates/improvise-protocol/src/vocabulary_is_composed.rs +++ b/crates/improvise-protocol/src/vocabulary_is_composed.rs @@ -5,6 +5,7 @@ use std::collections::BTreeSet; use std::path::{Path, PathBuf}; use crate::reduce::Command; +use crate::source_scan::{production_code, production_text}; /// A variant production composes nowhere, and why. Checked in **both** /// directions, so a composed entry fails here too (improvise-m0sx). @@ -62,6 +63,24 @@ fn the_uncomposed_list_names_only_real_variants() { ); } +/// The #160 review's fault: a variant named only in a comment or a string +/// is not composed. See `context/test-evidence/protocol.md`. +#[test] +fn a_variant_named_only_in_a_comment_or_string_is_not_composed() { + let text = concat!( + "fn f() {\n", + " // TODO: Command::SetYanked(value)\n", + " /* Command::SetSearchMode(true) */\n", + " let s = \"Command::ChangeMode(AppMode::Normal)\";\n", + "}\n", + ); + assert_eq!(composers_in(text), Vec::::new()); + assert_eq!( + composers_in("let c = Command::SetYanked(None);"), + vec!["SetYanked"] + ); +} + /// The scan must find *something*, or a broken walk reads as a clean bill of /// health — the failure direction `check_the_checker` exists for. #[test] @@ -74,21 +93,22 @@ fn the_scan_reaches_production_code_at_all() { ); } -/// Every `Command::` in a **value** position, in non-test source. -fn composed_in_production() -> BTreeSet { - let mut found = BTreeSet::new(); - for file in production_sources() { - let text = std::fs::read_to_string(&file).expect("a listed source reads"); - for name in constructed_in(&strip_test_items(&text)) { - found.insert(name); - } - } - found +/// One file's composers: no comment, literal or test item is one (improvise-c7mu). +fn composers_in(text: &str) -> Vec { + constructed_in(&production_text(text)) } -/// Rust under `src/`, workspace-wide. `tests/` is excluded by construction: +/// Every `Command::` in a **value** position, in production source. +fn composed_in_production() -> BTreeSet { + production_code(&source_roots()) + .iter() + .flat_map(|(_, code)| constructed_in(code)) + .collect() +} + +/// `src/` of every workspace crate. `tests/` is excluded by construction: /// a variant only a test composes is exactly what this file looks for. -fn production_sources() -> Vec { +fn source_roots() -> Vec { let root = Path::new(env!("CARGO_MANIFEST_DIR")) .parent() .and_then(Path::parent) @@ -98,53 +118,7 @@ fn production_sources() -> Vec { for entry in std::fs::read_dir(root.join("crates")).expect("crates/ is readable") { roots.push(entry.expect("a crates/ entry reads").path().join("src")); } - let mut files = Vec::new(); - for dir in roots { - collect_rust(&dir, &mut files); - } - files -} - -fn collect_rust(dir: &Path, into: &mut Vec) { - let Ok(entries) = std::fs::read_dir(dir) else { - return; - }; - for entry in entries.flatten() { - let path = entry.path(); - match path.is_dir() { - true => collect_rust(&path, into), - false if path.extension().is_some_and(|e| e == "rs") => into.push(path), - false => {} - } - } -} - -/// Drop every `#[cfg(test)]` item. rustfmt puts an item's closing brace at the -/// attribute's own indentation, which is what makes this a line scan (§2). -fn strip_test_items(text: &str) -> String { - let lines: Vec<&str> = text.lines().collect(); - let mut kept = String::new(); - let mut i = 0; - while i < lines.len() { - let line = lines[i]; - match line.trim_start().starts_with("#[cfg(test)]") { - false => { - kept.push_str(line); - kept.push('\n'); - i += 1; - } - true => { - let indent = line.len() - line.trim_start().len(); - let close = format!("{}}}", " ".repeat(indent)); - i += 1; - while i < lines.len() && lines[i] != close { - i += 1; - } - i += 1; - } - } - } - kept + roots } /// `Command::` occurrences that are not `match` patterns. A pattern is diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index 8025d36b..fa48a6bd 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -42,11 +42,11 @@ pub struct RenderModel { pub categories: Vec, /// Whether the panel is open, so the page does not decide. pub category_panel_open: bool, - /// The view panel's rows, **empty unless it is open** — the same rule the - /// help overlay follows, so the page asks no question about the mode. - pub views: Vec, - /// The formula panel's rows, empty unless it is open. - pub formulas: Vec, + /// The view panel's rows, **`None` when it is closed** and possibly empty + /// when open: an absence is not an empty list (improvise-df2w). + pub views: Option>, + /// The formula panel's rows, on the same terms as `views`. + pub formulas: Option>, /// The tile bar, drawn on every frame as the TUI draws it. Which tile is /// `selected` is the mode's answer, not the page's. pub tiles: Vec, @@ -341,41 +341,39 @@ fn page_axes(state: &ClientState) -> Vec { .collect() } -/// The view panel's rows while it is open. The axis line is +/// The view panel's rows, or `None` while it is closed. The axis line is /// `viewmodel::view_axis_summary`, which the TUI's own panel calls too. -fn view_rows(state: &ClientState) -> Vec { - if !state.view.view_panel_open { - return Vec::new(); - } - state - .views - .views - .iter() - .enumerate() - .map(|(i, summary)| RenderViewRow { - name: summary.name.clone(), - axes: improvise_protocol::viewmodel::view_axis_summary(summary), - active: summary.name == state.view.active_view, - selected: i == state.view.view_panel_cursor, - }) - .collect() +fn view_rows(state: &ClientState) -> Option> { + state.view.view_panel_open.then(|| { + state + .views + .views + .iter() + .enumerate() + .map(|(i, summary)| RenderViewRow { + name: summary.name.clone(), + axes: improvise_protocol::viewmodel::view_axis_summary(summary), + active: summary.name == state.view.active_view, + selected: i == state.view.view_panel_cursor, + }) + .collect() + }) } -/// The formula panel's rows while it is open. -fn formula_rows(state: &ClientState) -> Vec { - if !state.view.formula_panel_open { - return Vec::new(); - } - state - .formulas - .formulas - .iter() - .enumerate() - .map(|(i, text)| RenderFormulaRow { - text: text.clone(), - selected: i == state.view.formula_cursor, - }) - .collect() +/// The formula panel's rows, or `None` while it is closed. +fn formula_rows(state: &ClientState) -> Option> { + state.view.formula_panel_open.then(|| { + state + .formulas + .formulas + .iter() + .enumerate() + .map(|(i, text)| RenderFormulaRow { + text: text.clone(), + selected: i == state.view.formula_cursor, + }) + .collect() + }) } /// The tile bar, always. Only the *selection* is conditional, exactly as @@ -797,34 +795,48 @@ mod tests { client } - /// An empty list is what says a panel is closed — the same rule the help - /// overlay follows, so the page asks no question about the mode. + /// `None` is what says a panel is closed, so the page asks no question + /// about the mode. #[test] fn a_closed_panel_projects_no_rows_and_an_open_one_does() { let mut client = client_with_panels(); let closed = client.model(); assert!( - closed.views.is_empty(), + closed.views.is_none(), "the cache is full but the panel is shut" ); - assert!(closed.formulas.is_empty()); + assert!(closed.formulas.is_none()); client.state_mut().view.view_panel_open = true; client.state_mut().view.formula_panel_open = true; client.state_mut().view.view_panel_cursor = 1; client.state_mut().view.active_view = "Default".into(); let open = client.model(); - assert_eq!(open.views.len(), 2); + let (views, formulas) = (open.views.unwrap(), open.formulas.unwrap()); + assert_eq!(views.len(), 2); assert_eq!( - open.views[0].axes, "R:Region", + views[0].axes, "R:Region", "the axis line is the shared derivation, and _Dim is not part of it" ); - assert!( - open.views[0].active, - "active is ViewState's, not the cache's" - ); - assert!(open.views[1].selected, "and selected is the panel cursor's"); - assert_eq!(open.formulas[0].text, "Profit = Revenue - Cost"); + assert!(views[0].active, "active is ViewState's, not the cache's"); + assert!(views[1].selected, "and selected is the panel cursor's"); + assert_eq!(formulas[0].text, "Profit = Revenue - Cost"); + } + + /// Open with nothing in it is not closed: the page must be able to tell + /// them apart from the model alone (improvise-df2w). + #[test] + fn an_open_empty_panel_is_not_a_closed_one() { + let mut client = init(); + let closed = client.model(); + client.state_mut().view.formula_panel_open = true; + let formulas_open = client.model(); + assert_ne!(formulas_open, closed, "an open, empty formula panel"); + + client.state_mut().view.formula_panel_open = false; + client.state_mut().view.view_panel_open = true; + let views_open = client.model(); + assert_ne!(views_open, closed, "an open, empty view panel"); } /// The tile bar is drawn always; only the *selection* waits for the mode, diff --git a/crates/improvise-wasm-client/src/socket.rs b/crates/improvise-wasm-client/src/socket.rs index 2d5a21b0..868f01b5 100644 --- a/crates/improvise-wasm-client/src/socket.rs +++ b/crates/improvise-wasm-client/src/socket.rs @@ -29,6 +29,10 @@ fn rejection_message(rejection: &Rejection) -> String { Rejection::MutationRefused => { "part of that batch was refused — the batch was not applied".into() } + Rejection::ReadOnly => { + "this tab is read-only: another tab has the document open — your edit was not applied" + .into() + } // Not staleness, so neither offers a retry: they are worded as facts // about the host, because that is what they are (improvise-368h). Rejection::NoDocumentPath => { diff --git a/crates/improvise-worker-server/src/bindings.rs b/crates/improvise-worker-server/src/bindings.rs index 232e5d08..ca71fb7d 100644 --- a/crates/improvise-worker-server/src/bindings.rs +++ b/crates/improvise-worker-server/src/bindings.rs @@ -111,7 +111,7 @@ async fn read_only(storage: &OpfsFs, path: PathBuf, why: String) -> Result, + access: Access, +} + +/// Whether this session may change the document. A read-only boot still +/// answers every view command; `context/code/clients.md` says why. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Access { + Writable, + /// Another context holds the handle, so no edit could ever be kept. + ReadOnly, +} + +impl Access { + /// Decided before the host sees the frame, so nothing is applied and + /// nothing projected: an edit that cannot be saved is never painted. + fn refuses(self, frame: &ClientFrame) -> bool { + match (self, frame) { + (Self::ReadOnly, ClientFrame::Commands(cmds)) => { + cmds.iter().any(|c| matches!(c, Command::Model { .. })) + } + (Self::ReadOnly, _) | (Self::Writable, _) => false, + } + } } impl Worker { /// Take an `App` already pointed at its document and storage backend; /// building that is [`crate::vfs`]'s job and the shim's, not this loop's. pub fn new(app: App) -> Self { + Self::with_access(app, Access::Writable) + } + + /// A session over a document another context holds (improvise-i0v9). It + /// paints and navigates, and refuses every edit (improvise-j4qt). + pub fn read_only(app: App) -> Self { + Self::with_access(app, Access::ReadOnly) + } + + fn with_access(app: App, access: Access) -> Self { Self { host: Host::new(app), client: None, notice: None, + access, } } @@ -48,6 +82,9 @@ impl Worker { // Anything else before the handshake: composed against a document // the client was never sent, so nothing to apply and nothing to say. (None, _) => Vec::new(), + (Some(_), frame) if self.access.refuses(&frame) => { + vec![ServerFrame::Rejected(Rejection::ReadOnly)] + } (Some(id), frame) => { // A save the client asked for is already answered; retrying it // here double-writes. See `context/code/clients.md`. @@ -272,6 +309,71 @@ mod tests { ); } + /// The read-only boot `bindings::read_only` performs: the document's bytes + /// over a backend that refuses every write, because another tab holds it. + fn read_only_worker() -> Worker { + let fs = MemoryFs::new(); + let path = PathBuf::from("demo.improv"); + fs.write(&path, &demo_bytes()).unwrap(); + let locked = + crate::vfs::ReadOnly::new(fs, "another tab or window is holding this document"); + let wb = vfs::open(&locked, &Opening::named(&path)).unwrap().workbook; + let mut app = App::with_storage(wb, Some(path), Box::new(locked)); + app.rebuild_layout(); + Worker::read_only(app) + } + + /// improvise-j4qt: a read-only tab applied an edit and PAINTED a value the + /// document never had. Evidence: `context/test-evidence/clients.md`. + #[test] + fn a_read_only_session_refuses_an_edit_before_anything_is_painted() { + let mut worker = read_only_worker(); + let mut grid = connect(&mut worker); + let was = grid.cells[0][0].text.clone(); + let cmd = reduce::caught_up( + worker.app(), + ModelCommand::SetCell { + cell: addressed(&grid, 0, 0), + value: CellValue::Number(4242.0), + }, + ); + + let out = worker.on_frame(ClientFrame::command(cmd)); + let _ = folded(&mut grid, &out); + + assert_eq!( + out, + vec![ServerFrame::Rejected(Rejection::ReadOnly)], + "a session that cannot save must refuse the edit, and say only that" + ); + assert_eq!(grid.cells[0][0].text, was, "the refused value was painted"); + assert_eq!( + worker.take_notice(), + None, + "nothing was applied, so no persist ran and none failed" + ); + assert!( + !worker.app().model_state.dirty, + "the edit reached the document" + ); + } + + /// The control: read-only is about the DOCUMENT. Moving the cursor writes + /// only this client's slice, so it must still be answered. + #[test] + fn a_read_only_session_still_answers_a_view_command() { + let mut worker = read_only_worker(); + let _ = connect(&mut worker); + let out = worker.on_frame(ClientFrame::command(Command::SetSelected { + row: 1, + col: 0, + })); + assert!( + !out.iter().any(|f| matches!(f, ServerFrame::Rejected(_))), + "a read-only tab refused a cursor move: {out:?}" + ); + } + /// improvise-zre1: the failure stayed in a `status_msg` the worker has no /// UI for, so the client could not tell "save failed" from "save pending". #[test] diff --git a/harness/tests/contract.spec.js b/harness/tests/contract.spec.js index 34dd6743..718a4121 100644 --- a/harness/tests/contract.spec.js +++ b/harness/tests/contract.spec.js @@ -70,7 +70,7 @@ test.describe("every field the render model projects reaches the screen", () => await expect(page.locator("#categories")).toBeVisible(); await page.keyboard.press("Escape"); - // `views`: empty unless open, so the page asks no question about the mode. + // `views`: `null` unless open, so the page asks no question about the mode. await expect(page.locator("#views")).toBeHidden(); await page.keyboard.press("Shift+V"); await expect(page.locator("#views li")).toHaveCount(2); @@ -356,6 +356,27 @@ test.describe("a pointer event reaches the client and its answer is painted", () }); }); +test.describe("an open panel is drawn even when it has nothing in it", () => { + // improvise-df2w: `formulas` said "closed" by being empty, so a document with + // no formulas could open the panel and never see it. `tall.improv` has none. + test("the formula panel opens over a document with no formulas", async ({ page }) => { + await page.goto("/app/index.html?doc=df2w-empty.improv&seed=tall.improv"); + await expect(page.locator("#grid table")).toBeVisible({ timeout: 20_000 }); + await page.locator("#grid").focus(); + await expect(page.locator("#formulas")).toBeHidden(); + + await page.keyboard.press("Shift+F"); + await expect(page.locator("#mode")).toHaveText("FORMULAS"); + await expect(page.locator("#formulas")).toBeVisible(); + await expect(page.locator("#formulas li")).toHaveCount(0); + + // And closed is still closed: the other direction, which a page that + // simply never hid the panel would fail. + await page.keyboard.press("Shift+F"); + await expect(page.locator("#formulas")).toBeHidden(); + }); +}); + test.describe("the selection is on screen", () => { // improvise-jq8l: `web/app.js` called `scrollIntoView` NOWHERE, so a selection // moved past the bottom of `#grid` simply stayed there, invisible, with no way diff --git a/harness/tests/grouped.spec.js b/harness/tests/grouped.spec.js index a5fbd8df..55295ca5 100644 --- a/harness/tests/grouped.spec.js +++ b/harness/tests/grouped.spec.js @@ -146,18 +146,21 @@ test.describe("a grouped, multi-level document", () => { expect(await groupRows.locator("td").count()).toBe(0); await expect(groupRows.first().locator("th")).toHaveCount(1); - // The collapse is the document's, and it must have REMOVED rows: Upland's - // two regions are absent while Inland's two are here. Without this, a - // page that drew the indicator and ignored the frame would pass. + // The collapse is the document's: Upland's two regions are gone and the + // group is ONE row in their place (improvise-6qet, core.md §14). const labels = await page.locator("#grid tbody tr th[scope=row]").allTextContents(); expect(labels).toContain("East"); expect(labels).toContain("West"); + expect(labels).toContain("Upland"); expect(labels).not.toContain("Ridge"); expect(labels).not.toContain("Valley"); - // 4 regions visible x 2 stores. Stated as a count as well, because a row - // drawn with empty labels would satisfy the four assertions above. - await expect(page.locator("#grid tbody tr").filter({ hasNot: page.locator("th.group") })).toHaveCount( - 8, - ); + // (4 regions + the Upland fold) x 2 stores. A count too, because a row + // drawn with empty labels would satisfy the assertions above. + const dataRows = page.locator("#grid tbody tr").filter({ hasNot: page.locator("th.group") }); + await expect(dataRows).toHaveCount(10); + // The fold is an aggregate: no record carries Region=Upland, so nothing + // in its rows may open an editor, where a stored row beside it does. + await expect(dataRows.first().locator("td.editable")).not.toHaveCount(0); + await expect(dataRows.last().locator("td.editable")).toHaveCount(0); }); }); diff --git a/harness/tests/shell.spec.js b/harness/tests/shell.spec.js index 9648ea92..9e393226 100644 --- a/harness/tests/shell.spec.js +++ b/harness/tests/shell.spec.js @@ -257,6 +257,40 @@ test.describe("the standalone deployment", () => { await first.close(); }); + // improvise-j4qt: the read-only tab used to APPLY an edit to its own copy + // and paint it, a number the document never had. It must refuse the edit. + test("a read-only tab refuses an edit and never paints it", async ({ context }) => { + const doc = `read-only-edit-${Date.now()}.improv`; + const first = await context.newPage(); + await first.goto(`/app/index.html?doc=${doc}`); + await expect(first.locator("#grid table")).toBeVisible({ timeout: 20_000 }); + + const second = await context.newPage(); + await second.goto(`/app/index.html?doc=${doc}`); + await expect(second.locator("#grid table")).toBeVisible({ timeout: 20_000 }); + await expect(second.locator("#status")).toContainText("read-only"); + + const cell = second.locator("#grid td.editable").first(); + const before = await cell.textContent(); + await cell.click(); + const input = cell.locator("input"); + await expect(input).toBeFocused(); + await input.fill("4242"); + await input.press("Enter"); + + // The refusal is what arrives, and a document message is a toast. Waiting + // on it first means the grid below is asserted AFTER the answer landed. + await expect(second.locator("#toast")).toContainText("read-only", { timeout: 10_000 }); + await expect(cell).toHaveText(before); + await expect(second.locator("#grid")).not.toContainText("4,242"); + // The boot's reason stands; no failed save replaced it (the old symptom). + await expect(second.locator("#status")).toContainText("read-only"); + await expect(second.locator("#status")).not.toContainText("NOT SAVED"); + + await second.close(); + await first.close(); + }); + // What lands on screen is the HOST's value, not the string typed — the // round trip through a real worker, which no unit test can stand in for. test("committing an edit sends one frame and shows the host's value", async ({ diff --git a/scripts/kb b/scripts/kb index bd029e86..f4465e6f 100755 --- a/scripts/kb +++ b/scripts/kb @@ -72,7 +72,7 @@ acquire_lock() { scratch=() new_cand() { cand="$(mktemp "$KB.cand.XXXXXX")" - scratch+=("$cand" "$cand.delta" "$cand.removed" "$cand.ledger" "$cand.base") + scratch+=("$cand" "$cand.delta" "$cand.removed" "$cand.ledger" "$cand.base" "$cand.stamped") } # What `kb rm` removed that HEAD still has: the record that tells an intended @@ -83,6 +83,14 @@ ledger_path() { fi } +# A ledger speaks only for the kb FILE this tool last published, named on its +# first line; bytes alone are not enough, since a restore can reproduce them. +file_stamp() { + printf '%s %s' "$(stamp_of "$1")" \ + "$(stat -c '%i %.9Y' "$1" 2>/dev/null || stat -f '%i %Fm' "$1")" +} +ledger_holds() { [ -f "$1" ] && [ "$(head -n 1 "$1")" = "% kb-stamp $(file_stamp "$2")" ]; } + # The committed knowledge base, which no write may end up short of. # KB_BASELINE is the test seam, naming a file. baseline_to() { @@ -140,19 +148,22 @@ commit() { # $1 = candidate, $2 = stamp of the kb as read, $3 = keys a rm remove echo "kb: cannot read the committed kb.pl to compare against; $KB is unchanged" >&2 exit 3 fi - { if [ -f "$ledger" ]; then cat "$ledger"; fi + { if ledger_holds "$ledger" "$KB"; then cat "$ledger"; fi if [ -n "$removing" ]; then cat "$removing"; fi; } > "$cand.removed" if ! merge complete "$cand.base" "$cand" "$cand.removed" "$cand.ledger"; then echo "kb: REFUSED -- clauses HEAD has are gone, with no kb rm recorded; $KB is unchanged." >&2 - echo "kb: restore it from HEAD and re-apply your deltas, or commit the removal." >&2 + echo "kb: restore it from HEAD (git show HEAD:prolog-sessions/kb.pl) and re-apply your deltas." >&2 + echo "kb: do NOT commit this kb.pl: a committed loss becomes the floor this check compares against." >&2 + echo "kb: to drop a clause on purpose, restore first and then kb rm it." >&2 exit 1 fi if ! check_file "$cand"; then echo "kb: REFUSED -- kb_consistent fails on the result; $KB is unchanged" >&2 exit 1 fi + { printf '%% kb-stamp %s\n' "$(file_stamp "$cand")"; cat "$cand.ledger"; } > "$cand.stamped" kill_hook "$cand" - mv -f "$cand.ledger" "$ledger" + mv -f "$cand.stamped" "$ledger" mv -f "$cand" "$KB" } diff --git a/scripts/kb-merge.pl b/scripts/kb-merge.pl index 2985f912..38ac2461 100644 --- a/scripts/kb-merge.pl +++ b/scripts/kb-merge.pl @@ -35,8 +35,11 @@ run(remove, KbFile, DeltaFile, OutFile) :- halt(2) ), foldl_remove(Funcs, Doomed, Old, Kept), - length(Old, NOld), length(Doomed, NDoomed), - ExpectK is NOld - NDoomed, + length(Old, NOld), + findall(x, ( member(O, Old), clause_key(Funcs, O, KO), + member(T, Doomed), clause_key(Funcs, T, KT), same_key(KO, KT) ), Hits), + length(Hits, NHits), + ExpectK is NOld - NHits, check_total(Kept, ExpectK), write_terms(OutFile, Kept), verify(OutFile, Kept), @@ -325,24 +328,27 @@ count_only(KbFile) :- % Consistency is not completeness: a kb that lost 276 clauses still passed % kb_consistent. Why by key and against HEAD: context/code/kb-tool.md. +% Keys are counted, not collected: a merge can commit two clauses under one +% key, and a set of keys cannot see one of them go (improvise-ih3k). complete(BaseFile, CandFile, ExcFile, LedgerOut) :- terms_of(BaseFile, Base), terms_of(CandFile, Cand), terms_of(ExcFile, Exc), - ground_keys(Cand, CandKeys), + key_counts(Base, BaseCounts), + key_counts(Cand, CandCounts), ground_keys(Exc, ExcKeys), - findall(T, ( member(T, Base), ground_key(T, K), - \+ memberchk(K, CandKeys), \+ memberchk(K, ExcKeys) ), Missing), + findall(K-D, ( member(K-NBK, BaseCounts), count_of(K, CandCounts, NCK), + D is NBK - NCK, D > 0 ), Short), + findall(K-D, ( member(K-D, Short), \+ memberchk(K, ExcKeys) ), Unexcused), + findall(T, ( member(K-_, Unexcused), lost_under(K, Base, Cand, T) ), Missing), length(Base, NB), length(Cand, NC), - ( Missing == [] - -> ground_keys(Base, BaseKeys), - findall(E, ( member(E, Exc), ground_key(E, K), - memberchk(K, BaseKeys), \+ memberchk(K, CandKeys) ), Kept), + ( Unexcused == [] + -> findall(E, ( member(E, Exc), ground_key(E, K), memberchk(K-_, Short) ), Kept), write_terms(LedgerOut, Kept), length(Kept, NK), format("kb-merge: complete -- baseline ~w, result ~w, ~w removal(s) recorded~n", [NB, NC, NK]) - ; length(Missing, NM), + ; foldl(add_deficit, Unexcused, 0, NM), format("kb-merge: REFUSED -- the baseline has ~w clauses and the result ~w; ~w of the baseline's are missing and no kb rm recorded their removal:~n", [NB, NC, NM]), forall(( nth1(I, Missing, T), I =< 10 ), format(" ~q~n", [T])), @@ -356,5 +362,29 @@ ground_keys(Terms, Sorted) :- findall(C, ( member(T, Terms), ground_key(T, C) ), Cs), sort(Cs, Sorted). +key_counts(Terms, Counts) :- + findall(C, ( member(T, Terms), ground_key(T, C) ), Cs), + msort(Cs, Sorted), + run_lengths(Sorted, Counts). + +run_lengths([], []). +run_lengths([K|T], [K-N|Rest]) :- + same_run(K, T, 1, N, After), + run_lengths(After, Rest). + +same_run(K, [X|T], N0, N, After) :- X == K, !, N1 is N0 + 1, same_run(K, T, N1, N, After). +same_run(_, After, N, N, After). + +count_of(K, Counts, N) :- ( memberchk(K-N0, Counts) -> N = N0 ; N = 0 ). + +add_deficit(_-D, N0, N) :- N is N0 + D. + +% What to name for a short key: the baseline's clauses under it that the +% result holds no variant of, or the first one when every text survives. +lost_under(K, Base, Cand, T) :- + findall(B, ( member(B, Base), ground_key(B, KB), KB == K ), Under), + findall(B, ( member(B, Under), \+ ( member(C, Cand), same_key(B, C) ) ), Gone), + ( Gone == [] -> Under = [T|_] ; member(T, Gone) ). + die(Msg) :- format("kb-merge: ~w~n", [Msg]), halt(2). die_file(F) :- format("kb-merge: cannot read ~w~n", [F]), halt(2). diff --git a/scripts/kb-test.sh b/scripts/kb-test.sh index 52797816..eda7bd5d 100755 --- a/scripts/kb-test.sh +++ b/scripts/kb-test.sh @@ -553,6 +553,17 @@ else bad "the truncation was refused, but not by the completeness guard" fi +# 19b. ...and the refusal must not offer committing the loss as the way out: +# a committed loss becomes the floor, and is never reported again +# (improvise-za97). +if said "$work/out" "commit the removal"; then + bad "the refusal still offers committing an unexplained loss" +elif said "$work/out" "do NOT commit"; then + ok "the refusal warns against committing the loss" +else + bad "the refusal says nothing about committing the short kb" +fi + # 20. rm is how the count legitimately falls, and the ledger is how a LATER # write knows it: rm a clause HEAD has, then add -- the add must land. A # ledger that is never written, or is ignored, refuses here. @@ -568,13 +579,19 @@ fi # 20b. ...and the record excuses exactly what it names. A second, unrecorded # loss on top of a recorded rm must still refuse; a guard that stands down -# whenever a ledger exists passes 20 and fails here. -sed -i.bak '/^location(add_command,/d' "$work/kb.pl"; rm -f "$work/kb.pl.bak" +# whenever a ledger exists passes 20 and fails here. The second loss is +# made by giving the BASELINE a clause, not by editing the kb: an edit +# voids the ledger (case 24), which would test that instead. +awk -v x="location(a_probe_only_head_has,'src/nowhere.rs',probe)." ' + { line[NR] = $0 } /^location\(/ { last = NR } + END { for (i = 1; i <= NR; i++) { print line[i]; if (i == last) print x } } +' "$KB" > "$work/head-plus.pl" cp "$work/kb.pl" "$work/twice.pl" printf "doctrine(another_probe_after_the_rm,'x').\n" > "$work/new2.pl" -if kb add "$work/new2.pl" >"$work/out" 2>&1; then +if KB_BASELINE="$work/head-plus.pl" kb add "$work/new2.pl" >"$work/out" 2>&1; then bad "a recorded rm excused an unrecorded loss beside it" -elif cmp -s "$work/kb.pl" "$work/twice.pl" && said "$work/out" "1 of the baseline's are missing"; then +elif cmp -s "$work/kb.pl" "$work/twice.pl" && said "$work/out" "1 of the baseline's are missing" \ + && said "$work/out" "a_probe_only_head_has"; then ok "a recorded rm excuses only the clause it removed" else bad "the unrecorded loss was refused, but not by name, or the kb changed" @@ -611,5 +628,131 @@ else ok "a write killed before its rename leaves the old kb byte-identical" fi +echo "kb-test: what the ledger and the key comparison can be fooled by" + +# 23. Keys compared as a SET cannot see one of two clauses sharing a key +# (improvise-ih3k). work_done/3 keys on (id, kind) and is not functional/1, +# so a git merge can commit two clauses under one key. Dropping one then +# leaves the key present and the loss unseen. The baseline carries the pair. +awk -v a="work_done('improvise-zzzz',bug_fix,'first fix, merged from branch A')." \ + -v b="work_done('improvise-zzzz',bug_fix,'review fix, merged from branch B')." ' + { line[NR] = $0 } /^work_done\(/ { last = NR } + END { for (i = 1; i <= NR; i++) { print line[i]; if (i == last) { print a; print b } } } +' "$KB" > "$work/pair-base.pl" +grep -v "^work_done('improvise-zzzz',bug_fix,'review fix" "$work/pair-base.pl" > "$work/pair-lost.pl" +pkb() { KB_FILE="$work/kb.pl" KB_BASELINE="$work/pair-base.pl" "$here/kb" "$@"; } +fresh +if ! KB_FILE="$work/pair-base.pl" "$here/kb" check >/dev/null 2>&1; then + bad "the paired baseline fails kb_consistent -- case 23 proves nothing" +elif [ "$(grep -c "^work_done('improvise-zzzz'" "$work/pair-lost.pl")" -ne 1 ]; then + bad "the lossy fixture does not hold exactly one of the pair -- case 23 proves nothing" +else + cp "$work/pair-lost.pl" "$work/kb.pl" + if pkb add "$work/new.pl" >"$work/out" 2>&1; then + bad "add wrote onto a kb that lost one of two clauses sharing a key" + elif ! cmp -s "$work/kb.pl" "$work/pair-lost.pl"; then + bad "the lost twin was refused but the kb was MODIFIED" + elif said "$work/out" "review fix, merged from branch B"; then + ok "losing one of two clauses under one key is refused, naming the one lost" + else + bad "the lost twin was refused, but the refusal does not name it" + fi + # The pair itself is not a loss: an add onto the intact pair must land, or + # the guard has turned "HEAD holds a duplicate" into "nothing may be written". + cp "$work/pair-base.pl" "$work/kb.pl" + if pkb add "$work/new.pl" >"$work/out" 2>&1; then + ok "an add onto a kb that still holds both clauses of a pair is accepted" + else + bad "the intact pair was refused: $(tail -1 "$work/out")" + fi + # And the pair has a way out through the tool: rm the key (both go, recorded), + # add back the survivor, and a later write must still land. + printf "work_done('improvise-zzzz',bug_fix,_).\n" > "$work/pair-key.pl" + printf "work_done('improvise-zzzz',bug_fix,'the fix, both halves').\n" > "$work/pair-one.pl" + printf "doctrine(a_probe_after_the_pair_is_resolved,'x').\n" > "$work/after-pair.pl" + if ! pkb rm "$work/pair-key.pl" >"$work/out" 2>&1; then + bad "rm of a key HEAD holds twice was refused: $(tail -1 "$work/out")" + elif ! pkb add "$work/pair-one.pl" >"$work/out" 2>&1; then + bad "re-adding one clause for a removed pair was refused: $(tail -1 "$work/out")" + elif ! pkb add "$work/after-pair.pl" >"$work/out" 2>&1; then + bad "a write after resolving the pair was refused: $(tail -1 "$work/out")" + else + ok "a pair is resolved by rm then add, and later writes still land" + fi +fi + +# 24. A ledger entry excuses only the kb it was recorded against +# (improvise-yea8). rm X, then restore kb.pl the way the refusal message +# recommends -- not a write, so nothing prunes -- then lose X some other +# way. The ledger entry from the abandoned rm must not excuse that loss. +fresh +printf "location(cli_args,'src/main.rs',_).\n" > "$work/drop-x.pl" +grep -v "^location(cli_args," "$KB" > "$work/lost-x.pl" +if ! kb rm "$work/drop-x.pl" >"$work/out" 2>&1; then + bad "rm of a clause HEAD has was refused -- case 24 proves nothing" +elif [ ! -s "$work/kb.pl.removed" ]; then + bad "rm left no ledger -- case 24 proves nothing" +else + cp "$work/lost-x.pl" "$work/kb.pl" + if kb add "$work/new.pl" >"$work/out" 2>&1; then + bad "a ledger entry from an abandoned rm excused a later loss of that clause" + elif ! cmp -s "$work/kb.pl" "$work/lost-x.pl"; then + bad "the stale excuse was refused but the kb was MODIFIED" + elif said "$work/out" "location(cli_args,'src/main.rs',clap)"; then + ok "a ledger entry does not outlive the kb its rm produced" + else + bad "the loss was refused, but not by naming the lost clause" + fi +fi + +# 24b. ...and still excuses it across the tool's own writes: the stamp moves +# with each write, so rm then add then add must all land. A ledger stamped +# only by rm passes 24 and refuses the second add here. +fresh +printf "doctrine(a_second_probe_after_the_rm,'x').\n" > "$work/new3.pl" +if ! kb rm "$work/drop-x.pl" >"$work/out" 2>&1; then + bad "rm refused -- case 24b proves nothing" +elif ! kb add "$work/new.pl" >"$work/out" 2>&1; then + bad "the first add after an rm was refused: $(tail -1 "$work/out")" +elif ! kb add "$work/new3.pl" >"$work/out" 2>&1; then + bad "the second add after an rm was refused: the ledger did not follow the write" +else + ok "a recorded rm stays excused across the tool's later writes" +fi + +# 25. The production paths, which every case above bypasses by setting KB_FILE: +# the baseline is `git show HEAD` and the ledger is `git rev-parse +# --git-path`. Run in a throwaway repo holding a copy of the tool. +r="$work/repo" +mkdir -p "$r/scripts" "$r/prolog-sessions" +cp "$here/kb" "$here/kb-merge.pl" "$r/scripts/" +cp "$KB" "$r/prolog-sessions/kb.pl" +g() { git -C "$r" -c user.name=kb-test -c user.email=kb-test@invalid "$@"; } +rkb() { (unset KB_FILE KB_BASELINE; "$r/scripts/kb" "$@"); } +if ! { g init -q && g add -A && g commit -qm base; } >/dev/null 2>&1; then + bad "could not build the throwaway repo -- case 25 proves nothing" +elif ! rkb rm "$work/drop-x.pl" >"$work/out" 2>&1; then + bad "rm in a real repo was refused: $(tail -1 "$work/out")" +else + ledger="$(g rev-parse --path-format=absolute --git-path kb-removed.pl)" + if ! grep -q "^location(cli_args," "$ledger" 2>/dev/null; then + bad "rm in a real repo did not record its removal at the git-path ledger" + elif [ "$(g status --porcelain --untracked-files=all)" != " M prolog-sessions/kb.pl" ]; then + bad "the real-repo write left something git sees: $(g status --porcelain --untracked-files=all | tr '\n' ' ')" + elif ! rkb add "$work/new.pl" >"$work/out" 2>&1; then + bad "the git-path ledger did not excuse its own rm: $(tail -1 "$work/out")" + else + g checkout -q -- prolog-sessions/kb.pl + cp "$work/lost-x.pl" "$r/prolog-sessions/kb.pl" + if rkb add "$work/new.pl" >"$work/out" 2>&1; then + bad "after git checkout restored the kb, the git-path ledger excused a new loss" + elif said "$work/out" "location(cli_args,'src/main.rs',clap)"; then + ok "the git-path ledger and HEAD baseline work, and a loss after a restore is refused" + else + bad "the real-repo loss was refused, but not by naming it" + fi + fi +fi + printf '\nkb-test: %d passed, %d failed\n' "$pass" "$fail" [ "$fail" -eq 0 ] diff --git a/web/app.js b/web/app.js index 2aeed8d0..b3c0149c 100644 --- a/web/app.js +++ b/web/app.js @@ -594,12 +594,12 @@ function edit(td, row, col, seed) { }); } -/** A side panel. An empty row list is what says it is closed — the page asks - * no question about the mode, exactly as `paintHelp` does not. */ +/** A side panel. `null` is what says it is closed, and `[]` is open and empty: + * two facts, never inferred from a count (improvise-df2w). */ function paintPanel(host, rows, build) { - host.hidden = rows.length === 0; + host.hidden = rows === null; const list = document.createElement("ul"); - rows.forEach((row, index) => { + (rows ?? []).forEach((row, index) => { const li = build(row); li.dataset.index = String(index); list.appendChild(li);