diff --git a/.agents/skills/wave/SKILL.md b/.agents/skills/wave/SKILL.md index dffa68a2..fbb9bd74 100644 --- a/.agents/skills/wave/SKILL.md +++ b/.agents/skills/wave/SKILL.md @@ -7,6 +7,8 @@ description: Dispatch parallel agents to work through open beads and shepherd ea An orchestrator chunks the open backlog, fans agents out, reviews what comes back, and merges. `AGENTS.md` has the history; this is the operating procedure. +This file is the brief checklist's one home, and `agent-rules.md` beside it +holds the rules each agent follows. ## Every agent gets its own worktree. No exceptions. @@ -17,9 +19,8 @@ here. If you discover it mid-wave, stop every agent, have each create a worktree and copy across **only files it can prove it touched**, then `git checkout --` those files in the primary tree. -In the worktree: `direnv allow`, or `bd` reports a version *mismatch* rather than -failing cleanly. `direnv status` prints `Found RC allowed 0`, and `0` means -allowed — it reads like the opposite. +In the worktree `.envrc` must be allowed, or `bd` fails confusingly: +`agent-rules.md`, "Discipline per ticket". ## Partitioning @@ -33,32 +34,41 @@ it must cross. Two agents adding a workspace member will still collide in ## What every brief must carry +**Point the agent at [`agent-rules.md`](agent-rules.md) and tell it to read it in +full first.** That file is the agent's standing rules: base and push mechanics, +the gate list, the claim, the vacuity shapes, evidence, the files it must not +write, GitHub conventions, the hand-back and disk cleanup. Do not restate them +in the brief. A restated rule drifts from its source, and the brief is the copy +nobody corrects. The brief adds only what varies per wave: + 1. **The expected base commit, by name.** Worktrees do not reliably branch from - HEAD — one was created three merges behind and only noticed because it was - told to check. -2. **The baseline**, so "N passing" is checkable rather than asserted: Rust test - count, harness counts, node counts. -3. **Its ground, and the ground that is not.** -4. **Never write** `.beads/**`, `prolog-sessions/kb.pl`, `context/repo-map.md`. - Agents read them freely and report proposed text; the orchestrator applies it. -5. **The commit trailers and PR footer**, spelled out. -6. **Do not push to `main`, do not merge.** The orchestrator merges. -7. **What to report**: branch and head SHA, PR number, what changed per ticket, - *what could not be verified and why*, proposed repo-map/kb text, and **what it - learned that the ticket did not predict** — consistently the most valuable - part, and omitted unless asked for explicitly. + HEAD. One was created three merges behind and was only noticed because it + was told to check. The agent checks against it: `agent-rules.md` rule 1. +2. **A reference baseline**, so "N passing" is checkable and a drop is + visible: Rust test count, harness counts, node counts, **and the machine**. + The same commit once measured `improvise-io`'s binary at 1.4s on one machine + and 3.7–5.2s on another, so a figure with no machine is a claim about the + suite, which it is not. The agent still measures its own. +3. **Its ground, and the ground that is not.** Name any orchestrator-only file + it is exceptionally allowed to edit. +4. **Its `BEADS_ACTOR` and its tickets.** +5. **The attribution line, commit trailers and PR footer**, spelled out. + They name the agent (Claude's line opens `_Posted by Claude ()…_`) + and this session's URL, which is why the shared `agent-rules.md` cannot + hold them. +6. **An explicit ask for what the ticket did not predict.** `agent-rules.md` + lists it in the hand-back. Say it in the brief anyway: it is consistently + the most valuable part of a hand-back, and agents omit it unless asked. -## Method to require, because this repo's defect is a test that passes +When a hand-back teaches a new rule, add it to `agent-rules.md` in the same +wave, not to the next brief. -- Failing test first, then the fix. -- Every guard proved by **injecting the fault it names** and watching it fail - **by name**. An injection that does not compile reads as a clean run, so - require a positive signal the build ran. -- `cargo test --workspace` is **fail-fast**; `--no-fail-fast` for any claim about - blast radius. -- A textually clean merge can **delete tests** with no conflict marker, and - `cargo check` will not notice. After any merge that moves a test module, diff - test **names** — a count is masked by the other side's additions. +## Method to require + +`agent-rules.md`, "Discipline per ticket" and "Gates": failing test first, a +rival injected and watched failing by name, `--no-fail-fast`, and test *names* +diffed after any merge. Hold the hand-back to it. This repo's characteristic +defect is a test that passes. ## Shepherding @@ -72,7 +82,9 @@ it must cross. Two agents adding a workspace member will still collide in commit reads as a pass. - Merge on GitHub, then `git fetch gh && git merge --ff-only gh/main`. - Anything posted to GitHub goes out under the owner's account: open every - comment with a line saying a Codex agent wrote it. + comment with a line naming the model and saying an agent wrote it. That + wording is agent-neutral because this one file serves Claude and Codex + (`.claude/skills/wave` is a symlink to `.agents/skills/wave`). ## Between waves diff --git a/.agents/skills/wave/agent-rules.md b/.agents/skills/wave/agent-rules.md new file mode 100644 index 00000000..28245bc9 --- /dev/null +++ b/.agents/skills/wave/agent-rules.md @@ -0,0 +1,198 @@ +# Standing rules for a wave agent + +Every work-unit agent reads this **in full before its first command**. A brief +points here and adds only what is specific to its wave: the expected base +commit, the agent's ground, its `BEADS_ACTOR`, and its tickets. Every rule below +was bought by a failure. + +This file holds each agent-side rule **once**. The brief checklist is +`SKILL.md` beside it. Where `AGENTS.md` ("The Wave Workflow") carries a longer +history, the rule here points there. Change a rule here, not in a brief, and +not in a second copy. + +## Base, branch, push + +1. **Verify your base against the commit your brief names, first.** Run + `git log --oneline -1` and compare. Worktrees do not reliably branch from + the orchestrator's HEAD: one was created three merges behind, and it was + only noticed because the agent was told to look. +2. **Branch from `gh/main`, never `main` or `origin/main`.** `git fetch gh && + git checkout -b gh/main`. A worktree handed to you may already sit + on unpushed commits. Why: `AGENTS.md`, "Shipping a wave" (the two remotes + both spelled `main`, and the worktree-base vector). +3. **Before opening a PR, check `git diff --numstat gh/main...`.** Do + not use `gh pr view --json files`: that is the union of your commits' files, + not the effective diff. +4. **Push with an explicit refspec:** `git push gh HEAD:refs/heads/`. A + branch cut from `gh/main` tracks `gh/main`, so a bare push targets `main` and + is refused. +5. **Before pushing a fix to an existing PR, check `gh pr view --json + state`.** GitHub deletes a merged PR's branch. A push to it then prints + `* [new branch]` and lands nowhere. Why: `AGENTS.md`, "Shipping a wave". +6. **The "Session Completion" block near the end of `AGENTS.md` does not apply + to you.** It says `git merge origin/main` and then a bare `git push`. On a + wave branch the first is the #145 trap and the second is refused (rule 4). + Rules 1–5 replace it. The block sits between bd's generated + `BEGIN/END BEADS INTEGRATION` markers, so it is not edited by hand + (improvise-x1uk). + +## Commit early: a worktree is not durable storage + +Commit a WIP as soon as your first test run is green, and again after every +meaningful step. Push your branch (rule 4) once it is worth keeping. An agent +finished a whole ticket with every gate green and lost all of it, uncommitted, +when its worktree was deleted in a disk cleanup. A local commit survives a +deleted worktree. Tidying history before the PR is fine; losing it is not. + +## Gates + +``` +direnv exec . cargo test --workspace --no-fail-fast +direnv exec . cargo clippy --workspace --all-targets -- -D warnings +direnv exec . ./scripts/fmt-workspaces.sh # NOT cargo fmt --all; exit 2 = the gate did not run +direnv exec . python3 scripts/check-comment-runs.py --check-evidence +direnv exec . ./scripts/checkers.sh +direnv exec . ./scripts/node-test.sh 'web/tests/*.test.js' +``` + +- **Never `cargo fmt --all`.** It reaches only **one** of this repo's two + workspace roots, because `harness/opfs-contract` declares its own + `[workspace]`. The obvious repair is worse: `cargo fmt -p opfs-contract` from + the repo root exits **1**, *"not a member of the workspace"*, which looks + exactly like a real formatting failure (improvise-b4ks). Both spellings fail + in the direction that looks like success. +- **Run every gate under `direnv exec .`.** `./scripts/checkers.sh` scrubs the + environment to an allowlist, so a bare run drops `tpl` off `PATH` and reports + spurious failures. +- **Never pipe a gate.** `$?` must be the gate's own. And + `check-comment-runs.py --check-evidence` prints two lines with the comment + cap **first**, so `| tail -1` silently drops half of it. +- **`--no-fail-fast`**, because plain `cargo test --workspace` stops at the + first failing binary. Use it for any claim about blast radius. +- If you touch a browser-bundle crate (core, formula, io, protocol, command, + wasm-client, worker-server), also run + `cargo check --target wasm32-unknown-unknown -p ` **for each crate**. + CI checks each one separately, so feature unification cannot hide a leak. +- If you touch `web/` or `harness/`, run the Playwright suite in `harness/`. +- **Measure your own baseline** before changing anything, and report it with + the machine it was measured on. A figure from the brief is a reference, not + your baseline. +- **Merge `gh/main` and re-run every gate before calling a PR green.** Then + check that no test *disappeared*. Diff test **names**, not counts: a + textually clean merge can delete a test module with no conflict marker, and + the other side's new tests mask the loss in a count. +- **The shell guard refusing a compound command means NEITHER part ran.** Split + the command. After a refusal, re-check the state it was meant to produce. +- **Re-run the full suite after reverting an injected fault.** A revert applied + to the wrong occurrence once shipped two faults past every other gate. + +## Discipline per ticket + +- **Claim first:** `export BEADS_ACTOR=` in every shell, then + `bd update --claim`. Exit 13 means another actor holds it. Pick another; + do not retry. +- **`bd` needs `.envrc` allowed in your worktree.** Without it you get a + **version mismatch**, not a clean failure. Run `direnv allow` in the worktree, + or rely on the `[whitelist] prefix` in `~/.config/direnv/direnv.toml`. + `direnv status` reports `Found RC allowed 0`, and `0` means **allowed** (the + enum is 0=Allowed, 1=NotAllowed). It reads like the opposite. +- **Reproduce at HEAD before fixing.** A dozen tickets were stale-open in one + campaign, one fixed two days before it was filed. If it does not reproduce, + find the fixing commit, confirm with `git merge-base --is-ancestor HEAD`, + report the SHA, and close nothing. +- **Write a failing test first, then inject a rival and watch it fail.** Run + it; do not reason about it. Report *where* it failed, by case name. An + injection that does not compile reads as a clean run, so get a positive + signal that the build ran. +- **Check your test against every vacuity shape seen here:** + - an assertion the default state already satisfies; + - `f(x) == f(x)`: calling a pure `Cmd` twice against one immutable context; + - a disjunctive assertion whose other branch is the bug's own symptom; + - a rival that fails the *setup*, which proves the neighbouring question + (e.g. setup and subject share an `EffectKind`); + - a fixture that is right by accident (expected value 1, group at row 0, + bystander already on the relayed value); + - a fixture or stub that lacks the structure the hazard needs, or fails in a + wider shape than the real fault; + - hand-picked fixture cases where the set could be **enumerated**: four + vacuities in `fmt-workspaces-test.sh` were each a case nobody thought of + (improvise-90ea); + - a rendered-output test at a width where the thing asserted is never drawn; + - a fold test that batches differently from the real producer (a + `Binding::Sequence` is ONE effect vec); + - **the cursor at index 0**: "the item under the cursor" and "the first item" + are indistinguishable there, and that was four of five findings in one + review. Seat the cursor at a nonzero index of a list with at least two + elements; + - a determinism check against ONE context instance: every `HashMap` iterates + the same way twice. Build two equal contexts independently + (improvise-hzjo); + - two derivations of one fact where the fixture makes them agree (drawn vs + committed, edit text vs value, `1200` vs `1.23456789`). Use a value where + they disagree. +- **Record each bug in `context/test-evidence/.md`**, keyed by test name + (module-qualify the name when the leaf is not unique). It is append-at-EOF, so + **expect a conflict**. Keep both sides and verify line by line against both + parents: `--check-evidence` cannot see a dropped entry. +- **Comment runs are at most two lines.** Reasoning goes in `context/`, not in + comments (`context/design-principles.md` §6). +- **If the acceptance criteria look wrong, argue it** rather than following + them silently. Three agents inverted theirs in one campaign, and all three + were right. + +## Files you must not write + +`.beads/**` (never commit `issues.jsonl`, never `git checkout` it), +`prolog-sessions/kb.pl`, `context/repo-map.md`: `AGENTS.md`, "Who owns what". +Also `AGENTS.md`, unless your brief grants you a named section. Report proposed +text for any of these in your hand-back. + +## GitHub + +- `gh` is shell-aliased to a different tool. Use + `nix shell nixpkgs#gh --command gh …`, and pass bodies with `-F body=@file`. +- **Everything posted goes out under the repository owner's account.** Open + every PR body and comment with the attribution line your brief gives you, + verbatim. It names your agent and model, which is how our posts are told + apart from an automated Codex review (`AGENTS.md`, "Shipping a wave"). +- The attribution line, commit trailers and PR footer all come from your brief. + They name the agent and the session, so this shared file cannot hold them. +- Read reviews with `./scripts/pr-reviews `, and do not pipe it. How to tell + a ChatGPT Codex review from ours, and why Copilot is not coming: `AGENTS.md`, + "Shipping a wave". Answer each Codex review on its thread. +- **Do not merge, and do not close beads.** Avoid stacked PRs. If you must + stack, never let the base branch be deleted. + +## Hand-back, per ticket + +Report: + +- your branch, head SHA and PR number. Review is dispatched against the SHA; +- what changed, per ticket; +- the claim result; +- your measured baseline; +- whether the ticket reproduced (the SHA if it did not); +- the rival and where it failed; +- what you could not verify, and why; +- proposed repo-map and `kb.pl` text; +- **what the ticket did not predict**, consistently the most valuable part. + +## Disk + +Each worktree's `target/` grows to 3–6G, and a full disk (ENOSPC) once stopped +every agent. **When you hand back, delete your own build output, and only +yours.** + +Run `pwd` and `git rev-parse --show-toplevel` first, and read the answer. Then +delete by **literal absolute path**, typed out: +`rm -rf /Users/.../.claude/worktrees/agent-/target`. + +**Never interpolate a computed path into `rm -rf`.** One agent's worktree had +been removed, so `$(git rev-parse --show-toplevel)` printed nothing and the +command became `rm -rf "/target"`. A guarded one-liner was tried, but the +worktree shell guard refuses it as too complex, which pushed agents back to an +unguarded form. A literal path is the version that is both allowed and safe. + +Keep your branch and any uncommitted work. Delete only `target/` (and +`harness/opfs-contract/target` if you built it). If you see ENOSPC, stop and +report rather than deleting anything outside your worktree. diff --git a/.claude/skills/wave b/.claude/skills/wave new file mode 120000 index 00000000..0c2044da --- /dev/null +++ b/.claude/skills/wave @@ -0,0 +1 @@ +../../.agents/skills/wave \ No newline at end of file diff --git a/.claude/skills/wave/SKILL.md b/.claude/skills/wave/SKILL.md deleted file mode 100644 index 91b1ef74..00000000 --- a/.claude/skills/wave/SKILL.md +++ /dev/null @@ -1,83 +0,0 @@ ---- -name: wave -description: Dispatch parallel agents to work through open beads and shepherd each to a merged PR. Use when an orchestrator is running a wave, partitioning the backlog, or writing a brief for a work-unit agent. ---- - -# Running a wave - -An orchestrator chunks the open backlog, fans agents out, reviews what comes -back, and merges. `AGENTS.md` has the history; this is the operating procedure. - -## Every agent gets its own worktree. No exceptions. - -Dispatch with `isolation: "worktree"`. Without it every agent edits the **same** -working tree: one `git status` shows four agents' half-finished files, any commit -sweeps up the others' work, and any `git checkout` destroys it. This has happened -here. If you discover it mid-wave, stop every agent, have each create a worktree -and copy across **only files it can prove it touched**, then `git checkout --` -those files in the primary tree. - -In the worktree: `direnv allow`, or `bd` reports a version *mismatch* rather than -failing cleanly. `direnv status` prints `Found RC allowed 0`, and `0` means -allowed — it reads like the opposite. - -## Partitioning - -By **crate or directory**, never by ticket count. **Tickets that share a file go -to one agent**, to be done sequentially — four widget migrations all touching -`src/draw.rs` are a guaranteed conflict split four ways. Group first, then count. - -Tell each agent what ground is *not* theirs, and to report rather than guess when -it must cross. Two agents adding a workspace member will still collide in -`Cargo.toml`/`Cargo.lock`; keep both sides. - -## What every brief must carry - -1. **The expected base commit, by name.** Worktrees do not reliably branch from - HEAD — one was created three merges behind and only noticed because it was - told to check. -2. **The baseline**, so "N passing" is checkable rather than asserted: Rust test - count, harness counts, node counts. -3. **Its ground, and the ground that is not.** -4. **Never write** `.beads/**`, `prolog-sessions/kb.pl`, `context/repo-map.md`. - Agents read them freely and report proposed text; the orchestrator applies it. -5. **The commit trailers and PR footer**, spelled out. -6. **Do not push to `main`, do not merge.** The orchestrator merges. -7. **What to report**: branch and head SHA, PR number, what changed per ticket, - *what could not be verified and why*, proposed repo-map/kb text, and **what it - learned that the ticket did not predict** — consistently the most valuable - part, and omitted unless asked for explicitly. - -## Method to require, because this repo's defect is a test that passes - -- Failing test first, then the fix. -- Every guard proved by **injecting the fault it names** and watching it fail - **by name**. An injection that does not compile reads as a clean run, so - require a positive signal the build ran. -- `cargo test --workspace` is **fail-fast**; `--no-fail-fast` for any claim about - blast radius. -- A textually clean merge can **delete tests** with no conflict marker, and - `cargo check` will not notice. After any merge that moves a test module, diff - test **names** — a count is masked by the other side's additions. - -## Shepherding - -- Wait on checks with `scripts/pr-watch --expect `. Never hand-roll a - poll; see the `pr-watch` skill for the three ways they report green while wrong. -- Review before merging. Copilot's quota may be exhausted and its request is a - **silent no-op**; an absent review and an unrequested one both read as - "pending". Dispatch an adversarial reviewer and give it the head SHA, the base, - and a test-merge, so it reviews the tree that will exist. -- A re-review is of the **delta**, at a named SHA. A review of a superseded - commit reads as a pass. -- Merge on GitHub, then `git fetch gh && git merge --ff-only gh/main`. -- Anything posted to GitHub goes out under the owner's account: open every - comment with a line saying a Claude agent wrote it. - -## Between waves - -Run the code-review skill on the merged range. The characteristic find here is -not a wrong line, it is **a test that passes while the thing it names is -broken** — a vacuous fold test, a one-category fixture, a helper that fabricates -what the code under test needs. Ask the reviewer to inject the fault and watch -the check fail. diff --git a/AGENTS.md b/AGENTS.md index 935a7099..4be26a9d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -100,52 +100,19 @@ Two agents each adding a workspace member will still collide in ### Every wave agent's brief must include -1. **Verify your base commit first.** Worktrees do **not** reliably branch from - the orchestrator's HEAD — one was created three merges behind and only - noticed because it was told to look. `git log --oneline -1` against a named - commit, reset onto `main` if behind. Name the expected commit in the brief. -2. **The baseline test count**, so "1090 passing" is checkable rather than - asserted, and a drop is visible. Tell the agent to **measure its own** and - cite the machine: the same commit measured `improvise-io`'s binary at 1.4s on - one machine and 3.7–5.2s on another, so a figure with no machine is a claim - about the suite, which it is not. -3. **The gate list, and it must not say `cargo fmt --all`.** That reaches only - **one** of this repo's two workspace roots — `harness/opfs-contract` declares - its own `[workspace]` — and the obvious repair is worse: `cargo fmt - -p opfs-contract` from the repo root exits **1**, *"not a member of the - workspace"*, which is indistinguishable from a real formatting failure - (improvise-b4ks). Both spellings fail in the direction that looks like - success. The gate is: +The wave skill holds both halves, each in one place (improvise-1f8e): - ``` - direnv exec . ./scripts/fmt-workspaces.sh # exit 2 = the gate did not run - ``` +- **The brief checklist** — what an orchestrator must put in a brief — is + `.agents/skills/wave/SKILL.md`, "What every brief must carry". +- **The agent's standing rules** — base and push, gates, claim, vacuity shapes, + evidence, forbidden files, GitHub, hand-back, disk, commit early — are + `.agents/skills/wave/agent-rules.md`. A brief points there rather than + restating them. - Every gate goes through `direnv exec .` — `./scripts/checkers.sh` scrubs the - environment to an allowlist, so a bare invocation drops `tpl` off `PATH` and - reports spurious failures. And **never pipe a gate**: `$?` must be the gate's - own, and `check-comment-runs.py --check-evidence` prints two lines with the - comment cap **first**, so `| tail -1` silently drops half of it. -4. **Commit frequently, so the work is safe in git.** A WIP commit after the - first green test, and again after every meaningful step. A worktree is not - durable storage: an agent finished a whole ticket with every gate green and - lost all of it, uncommitted, when its worktree was deleted during a disk - cleanup. Tidying the history before the PR is fine; losing it is not. -5. **Do not merge to `main`.** The orchestrator merges. Pushing the agent's own - branch (`git push gh HEAD:refs/heads/`) is encouraged once it is - worth keeping. -6. The commit-message trailers, spelled out. -7. **What to report**: branch and commits, what changed per ticket, *what could - not be verified and why*, and proposed repo-map/`kb.pl` text. Ask explicitly - for the thing the agent learned that the ticket did not predict — that is - consistently the most valuable part of a hand-back, and agents omit it unless - asked. - -`bd` works in any worktree, but `.envrc` must be allowed there or you get a -**version mismatch** rather than a clean failure — `direnv allow` in the -worktree, or rely on the `[whitelist] prefix` in `~/.config/direnv/direnv.toml`. -Note `direnv status` reports `Found RC allowed 0`, and `0` means **allowed** -(the enum is 0=Allowed, 1=NotAllowed). It reads like the opposite. +`.claude/skills/wave` is a symlink to `.agents/skills/wave`, so Claude and Codex +read the same files. A new rule goes into one of those two files, not here and +not into the next brief: a restated rule is the copy nobody corrects, and this +list was once a third copy that drifted from the skill's. ### Between waves diff --git a/context/code/clients.md b/context/code/clients.md index b535916a..995b99f4 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1491,6 +1491,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/protocol.md b/context/code/protocol.md index 7b797818..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 @@ -552,8 +624,39 @@ client over the same grid, and they disagreed by a comma. `numeric` and `error` are decided by the server for the same reason: it is the side that knows which it was, and re-deriving client-side is how the two -ends drift apart. `CellDisplay` is deliberately not a `CellValue` — by the -time a cell reaches a client the number formatting has already happened. +ends drift apart. `CellDisplay` is deliberately not *drawn from* a `CellValue` +— by the time a cell reaches a client the number formatting has already +happened, and `text` is what a renderer paints. + +### `CellDisplay::value` — what `yank` copies (improvise-dsxq) + +The value the host evaluated at this position, the same `Option` +that `text` was formatted from: `evaluate_aggregated` over the view's +`none_cats` in a pivot grid, `records_value` in a records grid. It exists +because the browser's `yy` has to copy what the TUI's `YankCell` copies, and +neither string on the cell can be turned back into it: + +- `edit_text` is `CellValue::to_string()`, which writes a non-whole number + `{:.4}` — `1.23456789` becomes `1.2346`. +- Re-parsing any text with `typed()` turns number-like text into a number — + `Text("007")` becomes `Number(7.0)`. +- In a pivot grid `edit_text` is the *stored* cell (`get_cell`), so an + aggregate or a formula cell has none and yanked nothing at all. + +So the value is **projected**, not re-derived (gotcha 47): a client that +wants the value asks for the value. It is kept separate from `edit_text` +rather than replacing it, because the two answer different questions — what +an editor starts with is a *string*, deliberately the one the TUI's buffer +holds, and changing it would change what the TUI's own editor shows. + +It cost `Eq`: `CellValue` holds an `f64`, so `CellDisplay`, `GridCache` and +`GridUpdate` are `PartialEq` only. Nothing hashed or `Eq`-bounded them. + +In a records grid the TUI does **not** yank this value: `YankCell` and +`PasteCell` resolve the synthetic `(_Index, _Dim)` key, so the TUI yanks +`None` and pastes a junk cell into the document (improvise-6p8q). The browser +is right there and the TUI is wrong, which is why the lockstep yank test stays +on a pivot grid. ### `AxisFrame` — the grid's frame, and why it is *one* type diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index e60db1ab..e8c8bc86 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 @@ -4148,6 +4148,12 @@ so the paste is evidence about the yank rather than about a hand-seeded field. **Fault injected:** the `"yank"` arm of `composed` renamed. Fails on the yank. +**Superseded in part by improvise-dsxq** (entry at the end of this file). The +fixture's `1200` is the one kind of value on which the edit text and the value +agree, so this test could not see that the arm yanked the edit text re-parsed. +The arm now yanks `CellDisplay::value`, and the test sets that field on its +hand-built cell. The parity claim lives in the lockstep tests, not here. + ### `a_second_key_that_matches_nothing_still_spends_the_prefix` `App::dispatch_key` `take()`s the prefix before looking the second key up, so @@ -4225,6 +4231,33 @@ and converged. **Fault injected:** the same `Binding::Prefix` → `Upstream` reversion. Fails on the second press. +### `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` @@ -4261,3 +4294,62 @@ with no `SetBuffer`, then presses the keys: Enter must compose - The over-broad seed, "absent **or empty**, refill". This test passes, and `an_edit_buffer_the_user_has_emptied_is_not_refilled_from_the_cell` fails, as it did against the same rival in `render.rs`. + +## improvise-dsxq — the browser's `yy` copies the value, not a string + +PR #161 (improvise-lut7) made the browser's `yank` arm compose +`SetYanked(typed(edit_text))`. The TUI's `YankCell` yanks +`evaluate_aggregated(key)`, the value itself. Those differ in three ways, and +#161's only fixture, `1200`, hits none of them: + +- `edit_text` is `CellValue::to_string()`, which rounds a non-whole number to + four places. +- `typed()` turns number-like text into a number. +- In a pivot grid `edit_text` is the stored cell only, so an aggregate or a + formula cell has none and the browser yanked `None`. + +The fix projects the evaluated value as `CellDisplay::value`, and the arm yanks +that (`context/code/protocol.md` §3). Reproduced at `40b8913` by both tests +below before the fix: the browser yanked `Number(1.2346)` for `1.23456789`, and +`None` for the aggregate. + +### `yy_then_p_copies_the_value_the_host_copies` + +Three cases, one per disagreement, each on a real `App` and a `ClientState` +seeded by `grid_of`. The host presses `y y p` through `reduce_key`; a twin +host applies exactly what the client composed. Each case asserts the host +yanked the expected value (so a fixture that yanks nothing cannot pass), that +the client yanked the same, that the host's paste landed, and that the two +documents match. Sources sit at row 2 or 3 and targets at row 1, never row 0, +and the first column is an unused `Units` measure, so no cursor is at index 0. + +- `1.23456789`, a non-whole number. +- `Text("007")`, number-like text. +- `Profit = Revenue - Cost` over `10.123456` and `0.5`, a formula's value, + pasted into a stored `Revenue` cell. + +**Faults injected, each run:** +- The pre-fix arm, `typed(edit_text)`. Fails on the first case with + `Number(1.2346)`. +- The arm yanking `typed(&value.to_string())`, the projected value re-rendered. + Fails on the first case with `Number(1.2346)`. +- The producer coercing number-like `Text` through `typed()`. The first case + passes, and the second fails with `Number(7.0)`. +- The producer projecting the raw `get_cell` instead of the evaluated value. + The first two cases pass, and the third fails with `None`. + +### `yy_over_an_aggregate_copies_the_sum_the_host_copies` + +`Product` on the None axis with two stored records, `1.23456789` and `1.0`, so +the cell at row 1 is an `Aggregate` with no stored value. Yank only, since an +all-aggregate grid has no record cell to paste into. The host must yank +`Number(2.23456789)`, which rules out a fixture that is not an aggregate. + +**Faults injected, each run:** the pre-fix arm (fails with `None`), the +re-rendered value (fails with `Number(2.2346)`), and the raw `get_cell` +producer (fails with `None`). + +Records mode is deliberately not covered. There the TUI yanks `None` and its +`p` writes a junk `_Dim`/`_Index` cell, because it resolves the synthetic key +(improvise-6p8q). The browser is right and the TUI is wrong, so a lockstep +case there would assert the bug. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index efcda9b0..ebfbb841 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -2382,6 +2382,31 @@ keystroke to land in. Injected fault: dropping `self.view_state = snapshot.view_state` from `App::restore` fails this test and only this test. +**Addendum (improvise-gho5).** Improvise-gho5 found that this +test and `a_rolled_back_load_tells_nobody_and_puts_every_mirror_back` passed +against a host that **never ran the load**. The review's fault, reproduced +here, was placed at the top of `Host::commands`: refuse any multi-command frame +that contains a `LoadModel` with `Rejected(DocumentReplaced)`, before +`CheckedBatch` runs. Both tests stayed green. A frame that never got that far +leaves the generation, the view, the status line and every mirror untouched, +which is exactly what the tests asserted. + +Both tests now assert that the asker's whole reply is +`the_load_ran_then_the_write_broke()`, which is `Rejected(ChainBroken(North))`. +They also assert, before the frame, that `North` resolves. Together these make +the rejection a witness that the load ran. `ChainBroken` comes only from +`reduce::broken_chain`, which runs inside `mutating_frame` just before each +mutation. It names `North` only once the document no longer holds that +record, and only an applied `LoadModel` in this frame can have removed it. + +With the same fault re-injected, both tests fail. The left side is +`[Rejected(DocumentReplaced)]` and the right is +`[Rejected(ChainBroken())]`. The fault was reverted. + +The residual: a rogue up-front check that forged `ChainBroken` specifically +would still pass. That is the same forgery a hand-rolled client could commit +against any basis check, and a test cannot guard against it. + ### a_refused_mutation_rolls_back_its_predecessor_and_the_abort_flag The `Rejection::MutationRefused` arm — the one the first hand-back wrongly called @@ -4885,9 +4910,14 @@ Three things make it non-vacuous: `LoadModel` silently failed, `Applied::Rebased` would never run and every assertion here would pass over nothing. The control asserts the generation moves and that *both* clients are owed a `Snapshot`. -- **The rollback is asserted before anything is concluded from it** — the - `document_generation` equality is checked first, so "the frame was rejected" - cannot be confused with "the frame never got that far". +- **The load is witnessed to have RUN before anything is concluded from the + rollback** (improvise-gho5). The asker's whole reply must be + `Rejected(ChainBroken(North))`. `North` is asserted to resolve before the + frame, so only `broken_chain` can give that answer, and only after the + `LoadModel` has replaced the document. `CheckedBatch` never produces + `ChainBroken`. This bullet used to say the `document_generation` equality + did the job, and that was false: generation equality is exactly what holds + when the frame never got that far. - **A bystander is seated where a wrong relay could move it.** A single-client fixture cannot tell "the mirrors were restored" from "there was one mirror and it happened to be the asker's". @@ -4903,3 +4933,42 @@ The control for the test above, and it is stated separately rather than folded in so that a change making the fixture's load stop rebasing fails *here*, where the message says what actually broke, rather than turning the rollback test green by emptying it. + +### ui::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 `[]`. 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 38c28214..2d350c15 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -391,6 +391,8 @@ green suite over a vacuous assertion. **What is not covered.** The script asks `cargo metadata` for members, so a root whose metadata succeeds but whose member list is wrong is not detectable here. +*Narrowed by improvise-90ea, below:* a list that **omits** a package in the tree +is now refused; one that names a package it should not is still not detected. And `harness/opfs-contract`'s own `cargo fmt -p opfs-contract` step in the `harness-lint` job is left as it is: it resolves correctly because that step sets `working-directory`, and the ticket's own note confirms CI as written was @@ -415,9 +417,11 @@ getting wrong. The line the split actually needs is **"did the gate run"**: - exit 1 — rustfmt did not certify a root clean. A diff or a refusal to parse; either way **a file is named** and the tree is what is wrong. - exit 2 — the gate did not run. Usage, no cargo, no rustfmt, `cargo metadata` - failed, no `[workspace]` root. **No file is named.** Every such cause is - checked *above* the loop, which is what makes the narrowing true rather than - hopeful. + failed, no `[workspace]` root. **No source file is named.** Every such cause + is checked *above* the loop, which is what makes the narrowing true rather + than hopeful. (Since improvise-90ea, a package no root reaches is also exit 2, + and it names that package's `Cargo.toml`, which is the reason no verdict + exists. The line is "no verdict", not "no output"; improvise-ximf.) Both directions are now pinned, and two further rivals were injected: @@ -439,6 +443,67 @@ stub must fail in the **shape** of the real fault, not merely fail — a stub th breaks more than the real thing breaks satisfies the test through the wrong arm, and reads exactly like coverage. +#### The root package, and why the fourth fix is structural — `scripts/fmt-workspaces-test.sh` + +A post-merge review of PR #159 found a **fourth** fixture vacuity +(improvise-90ea). The fixture's outer root is a package (`root-pkg`) as well as +a workspace, which is right, but no case ever dirtied **the root package's own +`src/lib.rs`**. Only the member `a/` and `nested/` were dirtied. So a script +that formats every member *except* the root package passed 17/17. In this repo +the root package is `improvise`, the whole TUI. Reproduced at `40b8913` with +the review's rival in `members_of` (drop the package whose manifest sits at +`workspace_root`): the suite passed 17/17 and `--list` left `improvise` out. + +Adding one more hand-picked case would have fixed the fourth and not the fifth. +Four vacuities in one checker all have the same cause: **the cases were chosen +by hand, one per hazard somebody had thought of.** So the fix is structural, +and it has two halves because it has to cover two sets of packages: + +- **Every package the fixture has.** The two hand-picked fault cases are + replaced by a loop over every `[package]` manifest in the fixture. Each one + gets its own misformatted `src/lib.rs`, must exit 1, and must be named + **exactly once**, so the `--all` boundary check now covers every package + rather than one. The enumeration is a `grep` for `[package]`, deliberately + *not* the script's `[workspace]` + `cargo metadata` discovery, since that + discovery is what is under test. A count assertion (three packages) stops + the loop going vacuous if the fixture loses a package. A package added to + the fixture gets its case automatically. +- **Every package the tree has.** The loop proves only that the script handles + the fixture's *shapes*. A real tree can hold a package no root lists (an + `exclude`d crate, say), and nothing in the fixture would say so. So the gate + itself now checks an invariant **before it formats anything**: every + `[package]` manifest the find reaches must be a member of some root, or it + exits **2** and names the package. That is exit 2 and not 1 because no + verdict was produced for that package, which is what exit 2 means here. + Paths are compared through `realpath`, because on macOS `mktemp` lives under + `/var` while cargo reports `/private/var`. + +**Rivals injected, and where each failed** (the suite went from 17 to 21 cases): + +- **The review's skip-the-root rival.** It now fails the fixture at `a fault in + src/lib.rs is reported` (exit 0). With the invariant in place it fails + **six** cases (exit 2, "no workspace root reaches"). It also fails the + **real repo** with no fixture involved: `fmt-workspaces.sh` exits 2 naming + `Cargo.toml`. That last one is the property worth having. The gate refuses + its own blindness in CI, not only in its test suite. +- **Disabling the invariant** (`if false`). It fails `a package no workspace + root reaches is exit 2, not a pass` with **exit 0**, even though that + package's `src/lib.rs` is misformatted. That exit 0 *is* the hazard: an + unreached package is silently clean. +- **The two earlier rivals, re-run against the loop.** `--all` for `-p` fails + both `a/src/lib.rs` and `src/lib.rs` "named exactly once". The second of + those was not predicted: the nested root's `--all` also reaches the root + package. The bare manifest-path form fails `a fault in a/src/lib.rs is + reported`. + +The real-repo control from the ticket still holds: a misformatted fn appended +to `src/ui/text.rs` is reported (`Diff in …/src/ui/text.rs:72`). + +**What is still not covered.** A root whose member list *includes* something it +should not is not detected, and a package in a directory the find excludes +(`target/`, `.claude/`, `node_modules/`) is outside the invariant by +construction. + ### The kb tool's completeness guard and atomic write — `scripts/kb-test.sh` cases 18–22 improvise-usus: `scripts/kb add` wrote onto a working-tree `kb.pl` that had lost diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 50d7b392..166f16e0 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -1049,6 +1049,12 @@ mod tests { let was_view = host.app().view_state.active_view.clone(); let was_status = host.app().view_state.status_msg.clone(); let basis = frame_basis(&host, &["North"]); + assert!( + host.app() + .resolve_record(addressed("North").record) + .is_some(), + "North must resolve BEFORE the load, or a refusal naming it proves nothing" + ); // The load lands, then the write names a record the incoming document // has never had — so the frame fails at its second command. @@ -1069,11 +1075,10 @@ mod tests { ]), ); - assert!( - answer - .iter() - .any(|a| matches!(a.msg, ServerFrame::Rejected(_))), - "the fixture must actually refuse the frame: {answer:?}" + assert_eq!( + to(&answer, asker), + vec![the_load_ran_then_the_write_broke()], + "only a load that RAN can refuse the write this way: {answer:?}" ); assert_eq!( host.app().view_state.active_view, @@ -1097,6 +1102,12 @@ mod tests { ); } + /// `North` resolves in the outgoing document, so only `broken_chain` AFTER the + /// load applied can answer this. Evidence: `context/test-evidence/command.md`. + fn the_load_ran_then_the_write_broke() -> ServerFrame { + ServerFrame::Rejected(Rejection::ChainBroken(addressed("North").record)) + } + /// Everything one client was told, flattened to the projections it carries. fn projections(frames: &[Addressed], id: SessionId) -> Vec { to(frames, id) @@ -1147,6 +1158,12 @@ mod tests { let (bystander, _) = host.connect(10, 10); let was_generation = host.app().document_generation(); let basis = frame_basis(&host, &["North"]); + assert!( + host.app() + .resolve_record(addressed("North").record) + .is_some(), + "North must resolve BEFORE the load, or a refusal naming it proves nothing" + ); // The load lands and rebases every mirror; the write then names a // record the incoming document has never had, so the frame fails. @@ -1167,6 +1184,11 @@ mod tests { ]), ); + assert_eq!( + to(&answer, asker), + vec![the_load_ran_then_the_write_broke()], + "only a load that RAN can refuse the write this way: {answer:?}" + ); assert_eq!( host.app().document_generation(), was_generation, diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 7d33657b..441af17e 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -282,6 +282,7 @@ fn cell_display( numeric: matches!(value, Some(CellValue::Number(_))), error: matches!(value, Some(CellValue::Error(_))), id: cell_id(app, layout, row, col), + value: value.clone(), }; } let key = layout.cell_key(row, col); @@ -293,6 +294,7 @@ fn cell_display( // The *unformatted* value the TUI seeds its edit buffer from. Projecting it // beats a client un-formatting `text` (gotcha 47). edit_text: key.map(|key| app.display_value_in(layout, &key)), + value: value.clone(), } } 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-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/render.rs b/crates/improvise-protocol/src/render.rs index dbcad0d1..57f0d0c5 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/crates/improvise-protocol/src/render.rs @@ -4,6 +4,7 @@ use serde::{Deserialize, Serialize}; use unicode_width::UnicodeWidthStr; +use improvise_core::model::cell::CellValue; use improvise_core::model::record::{AggregateHash, RecordHash}; use improvise_core::view::Axis; use improvise_core::view::layout::PageAxis; @@ -64,9 +65,9 @@ pub enum CreateKind { Category, } -/// One cell as the client draws it. Deliberately not a `CellValue` — the +/// One cell as the client draws it. Drawn from `text`, never from `value` — the /// formatting has already happened, and re-deriving it is how ends drift. -#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] pub struct CellDisplay { pub text: String, /// Right-align numerics, left-align text — decided by the server, which @@ -80,6 +81,9 @@ pub struct CellDisplay { /// What an editor should start with — the value as the user last /// **typed** it, not as it is drawn. The difference corrupts data (§3). pub edit_text: Option, + /// The value the host evaluated here, unrounded and untyped-by-text: what + /// `yank` copies. Neither `text` nor `edit_text` can be re-parsed into it (§3). + pub value: Option, } impl CellDisplay { @@ -98,6 +102,7 @@ impl CellDisplay { numeric: true, error: false, id: None, + value: None, } } @@ -110,6 +115,7 @@ impl CellDisplay { error: true, id: None, edit_text: None, + value: None, } } @@ -416,7 +422,7 @@ fn shows(items: &[&Vec], idx: usize, first_rendered: usize, level: usize /// The grid widget's copy of the visible cells (improvise-n10), indexed by /// position within the **viewport** and never by `CellKey`. -#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] pub struct GridCache { /// The row axis's frame: levels, label tuples and groups. **Not** a list /// of strings — see [`AxisFrame`]. @@ -563,7 +569,7 @@ enum LabelAxis { /// What can change about the grid: cell values, the frames that name them, /// the widths, the totals, the page bar and the extent. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub enum GridUpdate { /// Display values for cells, by viewport position. Cells(Vec<((usize, usize), CellDisplay)>), diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index e01a44ee..3a86f346 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -530,12 +530,10 @@ fn composed( state, ModelCommand::ClearCell(record_cell_at_cursor(state)?), )?]), - // `YankCell::execute`: the value, then the status. `edit_text` is - // what the host evaluated, which is what the TUI yanks. + // `YankCell::execute`: the value, then the status. The projected + // value, never text re-parsed: improvise-dsxq. "yank" => Some(vec![ - Command::SetYanked(typed( - cell_at_cursor(state)?.edit_text.as_deref().unwrap_or(""), - )), + Command::SetYanked(cell_at_cursor(state)?.value.clone()), Command::SetStatus("Yanked".to_string()), ]), "paste" => Some(match state.view.yanked.clone() { @@ -3198,6 +3196,7 @@ mod tests { #[test] fn yy_yanks_the_cell_under_the_cursor_and_p_pastes_what_it_yanked() { let mut state = client_on(record_cell()); + state.grid.cells[0][0].value = Some(CellValue::Number(1200.0)); arm(&mut state, ev(KeyCode::Char('y'))); let KeyOutcome::Local(yank) = resolve(&state, ev(KeyCode::Char('y'))) else { panic!("`yy` is bound to yank"); 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/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index db9abcef..a3e0115c 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1581,3 +1581,177 @@ fn opens_an_editor(client: &ClientState, key: KeyEvent) -> bool { _ => false, } } + +// Yank and paste (improvise-dsxq). + +fn region_measure(region: &str, measure: &str) -> improvise_command::model::cell::CellKey { + improvise_command::model::cell::CellKey::new(vec![ + ("Region".to_string(), region.to_string()), + ("_Measure".to_string(), measure.to_string()), + ]) +} + +/// Four rows and four measures, the last a formula, holding values on which the +/// edit text and the value disagree. Evidence file. +fn yank_host() -> App { + use improvise_protocol::CellValue::{Number, Text}; + let mut wb = Workbook::new("Test"); + wb.add_category("Region").unwrap(); + for item in ["West", "North", "South", "East"] { + wb.add_item("Region", item).unwrap(); + } + for item in ["Units", "Revenue", "Cost"] { + wb.add_item("_Measure", item).unwrap(); + } + wb.model.add_formula( + improvise_command::formula::parse_formula("Profit = Revenue - Cost", "_Measure").unwrap(), + ); + wb.active_view_mut().set_axis("Region", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + for (region, measure, value) in [ + ("North", "Revenue", Number(1.0)), + ("North", "Cost", Number(2.0)), + ("South", "Revenue", Number(1.23456789)), + ("South", "Cost", Text("007".into())), + ("East", "Revenue", Number(10.123456)), + ("East", "Cost", Number(0.5)), + ] { + wb.model.set_cell(region_measure(region, measure), value); + } + let mut app = App::new(wb, None); + app.term_height = 4 + 8; + app.rebuild_layout(); + app +} + +/// One unmodified character on both ends: the host through its keymap, a twin +/// through exactly what the client composed. +fn press(app: &mut App, twin: &mut App, client: &mut ClientState, key: char) { + let event = KeyEvent::new(KeyCode::Char(key), KeyModifiers::NONE); + let KeyOutcome::Local(cmds) = resolve(client, event) else { + panic!("{key}: the client must answer this key locally"); + }; + for cmd in &cmds { + client.fold(cmd); + } + apply_on(twin, &cmds); + reduce_key(app, event); +} + +fn seat(app: &mut App, client: &mut ClientState, at: (usize, usize)) { + app.cursor_mut().selected = at; + client.view.cursor_mut().selected = at; +} + +/// `yy` then `p` copies the value the TUI copies, not its edit text re-parsed. +/// Never an integer: the one kind on which the two agree. Evidence file. +#[test] +fn yy_then_p_copies_the_value_the_host_copies() { + use improvise_protocol::CellValue::{Number, Text}; + let cases = [ + ( + "a non-whole number", + (2, 1), + (1, 1), + "Revenue", + Number(1.23456789), + ), + ( + "number-like text", + (2, 2), + (1, 2), + "Cost", + Text("007".into()), + ), + ( + "a formula's value", + (3, 3), + (1, 1), + "Revenue", + Number(10.123456 - 0.5), + ), + ]; + for (what, from, to, measure, expected) in cases { + let mut app = yank_host(); + let mut twin = yank_host(); + let mut client = client_ready_to_edit(&app, (4, 4)); + + seat(&mut app, &mut client, from); + press(&mut app, &mut twin, &mut client, 'y'); + press(&mut app, &mut twin, &mut client, 'y'); + assert_eq!( + app.view_state.yanked, + Some(expected.clone()), + "{what}: the host must yank the value itself, or the fixture proves nothing" + ); + assert_eq!( + client.view.yanked, app.view_state.yanked, + "{what}: client and host yanked different values" + ); + + seat(&mut app, &mut client, to); + press(&mut app, &mut twin, &mut client, 'p'); + assert_eq!( + app.model_state + .workbook + .model + .get_cell(®ion_measure("North", measure)), + Some(&expected), + "{what}: the host's paste must have landed" + ); + assert_eq!( + document_text(&twin), + document_text(&app), + "{what}: the client's paste wrote a different document" + ); + } +} + +/// A true aggregate: `Product` on no axis, so the cell sums two records and has +/// no stored value. Yank only: an all-aggregate grid has nowhere to paste. +#[test] +fn yy_over_an_aggregate_copies_the_sum_the_host_copies() { + let mut wb = Workbook::new("Test"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "North").unwrap(); + wb.add_item("Region", "South").unwrap(); + wb.add_category("Product").unwrap(); + wb.add_item("Product", "Widget").unwrap(); + wb.add_item("Product", "Gadget").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); + wb.active_view_mut().set_axis("Product", Axis::None); + for (product, value) in [("Widget", 1.23456789), ("Gadget", 1.0)] { + wb.model.set_cell( + region_measure("South", "Revenue").with("Product", product), + improvise_protocol::CellValue::Number(value), + ); + } + let mut app = App::new(wb, None); + app.term_height = 2 + 8; + app.rebuild_layout(); + let mut client = client_of(&app, (2, 1)); + + app.cursor_mut().selected = (1, 0); + client.view.cursor_mut().selected = (1, 0); + for _ in 0..2 { + let event = KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("y must resolve locally over an aggregate too"); + }; + for cmd in &cmds { + client.fold(cmd); + } + reduce_key(&mut app, event); + } + assert_eq!( + app.view_state.yanked, + Some(improvise_protocol::CellValue::Number(1.23456789 + 1.0)), + "the host yanks the sum, or the fixture is not an aggregate" + ); + assert_eq!( + client.view.yanked, app.view_state.yanked, + "client and host yanked different values over an aggregate" + ); +} 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/scripts/fmt-workspaces-test.sh b/scripts/fmt-workspaces-test.sh index 7e2f292d..ce15d6ce 100755 --- a/scripts/fmt-workspaces-test.sh +++ b/scripts/fmt-workspaces-test.sh @@ -76,36 +76,37 @@ run_case() { scaffold run_case "both workspaces clean is a pass" 0 "$work/repo" -# The load-bearing case. `cargo fmt --all` at the outer root cannot reach this -# file, and `cargo fmt -p inner-pkg` from the outer root exits 1 saying `not a -# member of the workspace` -- the usage error this whole script exists to split -# from a real verdict. +# One fault case PER PACKAGE, enumerated from the fixture rather than +# hand-picked: three hand-picked shapes each missed a rival, and the fourth +# missed the root package itself (improvise-90ea). Reasoning: +# context/test-evidence/tooling.md. The enumeration is a find over +# `[package]` manifests, deliberately NOT the script's own `[workspace]` + +# `cargo metadata` discovery, which is the thing under test. scaffold -printf '%s' "$DIRTY" > "$work/repo/nested/src/lib.rs" -if run_case "a fault in the NESTED workspace is reported" 1 "$work/repo"; then - case "$OUT" in - *nested/src/lib.rs*) ok " and the nested file is named" ;; - *) bad " and the nested file is named" "output did not name it: ${OUT:0:300}" ;; - esac +pkg_manifests=() +while IFS= read -r m; do pkg_manifests+=("$m"); done < <( + cd "$work/repo" && grep -rl --include=Cargo.toml '^\[package\]' . | sort) +# An empty or shrunken enumeration would make the loop below vacuous. +if [ "${#pkg_manifests[@]}" = 3 ]; then + ok "the fixture enumerates all three packages (root, member, nested)" +else + bad "the fixture enumerates all three packages" "found: ${pkg_manifests[*]:-none}" fi - -scaffold -printf '%s' "$DIRTY" > "$work/repo/a/src/lib.rs" -if run_case "a fault in the OUTER workspace is reported" 1 "$work/repo"; then - case "$OUT" in - *a/src/lib.rs*) ok " and the outer file is named" ;; - *) bad " and the outer file is named" "output did not name it: ${OUT:0:300}" ;; - esac - # The rival: spelling the per-root check `--all` instead of `-p ` - # walks PAST the nested workspace boundary (measured), so the OUTER file is - # reported a second time under the inner root. Counting is what sees that. - n="$(printf '%s\n' "$OUT" | grep -c 'a/src/lib.rs')" - if [ "$n" = 1 ]; then - ok " and it is named ONCE -- no root reaches past its own boundary" - else - bad " and it is named ONCE" "named $n times; a root is walking past its boundary" +for m in "${pkg_manifests[@]}"; do + rel="$(dirname "${m#./}")/src/lib.rs"; rel="${rel#./}" + scaffold + printf '%s' "$DIRTY" > "$work/repo/$rel" + if run_case "a fault in $rel is reported" 1 "$work/repo"; then + # Counting, not matching: `--all` for `-p` walks PAST the nested boundary + # (measured) and names an outer file a second time under the inner root. + n="$(printf '%s\n' "$OUT" | grep -cF "$work/repo/$rel")" + if [ "$n" = 1 ]; then + ok " and $rel is named exactly once" + else + bad " and $rel is named exactly once" "named $n times; output: ${OUT:0:300}" + fi fi -fi +done # A usage error must NOT wear a formatting failure's exit code. This is the # whole ticket: `cargo fmt -p opfs-contract` from the repo root exits 1. @@ -121,6 +122,32 @@ run_case "a tree with no workspace root is a usage error, not a pass" 2 "$work/e run_case "--help is a usage error, so a typo cannot pass" 2 "$work/repo" --help +# The loop above covers every package the FIXTURE has; this covers every +# package the TREE has. A package no root lists is checked by nothing, so the +# gate must refuse a verdict -- and name the package -- rather than pass. +scaffold +mkdir -p "$work/repo/orphan/src" +printf '[package]\nname = "orphan-pkg"\nversion = "0.1.0"\nedition = "2021"\n' \ + > "$work/repo/orphan/Cargo.toml" +printf '%s' "$DIRTY" > "$work/repo/orphan/src/lib.rs" +cat > "$work/repo/Cargo.toml" <<'EOF' +[workspace] +members = ["a"] +exclude = ["orphan"] +resolver = "2" + +[package] +name = "root-pkg" +version = "0.1.0" +edition = "2021" +EOF +if run_case "a package no workspace root reaches is exit 2, not a pass" 2 "$work/repo"; then + case "$OUT" in + *"no workspace root reaches"*orphan/Cargo.toml*) ok " and the unreached package is named" ;; + *) bad " and the unreached package is named" "output: ${OUT:0:300}" ;; + esac +fi + # The narrowed contract, pinned so it is not merely prose (ChatGPT Codex on # PR #159). rustfmt exits 1 for a source it cannot parse, exactly as for a # diff -- so exit 1 is "rustfmt did not certify this root clean", and what diff --git a/scripts/fmt-workspaces.sh b/scripts/fmt-workspaces.sh index abb6fec9..98bc8370 100755 --- a/scripts/fmt-workspaces.sh +++ b/scripts/fmt-workspaces.sh @@ -1,7 +1,7 @@ #!/usr/bin/env bash # Format-checks every Cargo workspace root in the repo, from any directory. -# `cargo fmt --all` reaches only ONE of them. Reasoning: context/code/tooling.md -# section 4 -- including why `-p` scoped by `--manifest-path` is the only +# `cargo fmt --all` reaches only ONE of them. Reasoning: context/test-evidence/ +# tooling.md, "The two-workspace fmt gate" -- including why `-p` scoped by `--manifest-path` is the only # spelling that is neither blind nor boundary-crossing (improvise-b4ks). set -uo pipefail @@ -18,8 +18,9 @@ exit 1 rustfmt did not certify a root clean: a file differs from rustfmt's output, or rustfmt refused to parse one. Either way a FILE IS NAMED and the tree is what is wrong. exit 2 the gate did not run: usage error, no cargo, no rustfmt, cargo - metadata failed, or no [workspace] root found. NO verdict was - produced and NO file is named. + metadata failed, no [workspace] root found, or a [package] that no + root has as a member. NO verdict was produced. No SOURCE file is + named; the unreached-package case names its Cargo.toml. The split is "did the gate run", not "is every exit-1 cause a diff". That is narrower than it sounds and it is deliberate -- see the note on rc=1 below. @@ -38,8 +39,8 @@ EOF # The find is RELATIVE to the tree being scanned: an absolute `-path` filter # matches the ancestors too, and this repo is itself checked out under # `.claude/worktrees/` in every agent worktree, where that excluded everything. -workspace_roots() { - local root="$1" +manifests_with() { + local root="$1" section="$2" ( cd "$root" && find . -name Cargo.toml \ -not -path './target/*' -not -path '*/target/*' \ -not -path './.git/*' \ @@ -47,11 +48,12 @@ workspace_roots() { -not -path './.claude/*' ) \ | sort \ | while IFS= read -r rel; do - if grep -q '^\[workspace\]' "$root/${rel#./}"; then + if grep -q "^\\[$section\\]" "$root/${rel#./}"; then printf '%s\n' "$root/${rel#./}" fi done } +workspace_roots() { manifests_with "$1" workspace; } # The members of ONE workspace, named by cargo rather than parsed out of the # manifest: a `members = [...]` list may carry globs and path dependencies. @@ -61,10 +63,10 @@ members_of() { printf '%s' "$meta" | python3 -c ' import json, sys doc = json.load(sys.stdin) -names = [p["name"] for p in doc["packages"]] -if not names: +rows = [p["name"] + "\t" + p["manifest_path"] for p in doc["packages"]] +if not rows: sys.exit(1) -print("\n".join(names)) +print("\n".join(rows)) ' || return 1 } @@ -106,15 +108,39 @@ main() { return 2 fi - local rc=0 manifest member - for manifest in "${roots[@]}"; do - local -a pkgs=() - while IFS= read -r member; do [ -n "$member" ] && pkgs+=("$member"); done \ - < <(members_of "$manifest") - if [ ${#pkgs[@]} -eq 0 ]; then - printf 'fmt-workspaces: cargo metadata failed for %s\n' "$manifest" >&2 + # Pass 1: every root's members, before anything is formatted, so that a + # package NO root reaches is exit 2 rather than silently never checked. + local rc=0 manifest member i m + local -a members=() covered=() + for i in "${!roots[@]}"; do + local rows + rows="$(members_of "${roots[$i]}")" || rows="" + if [ -z "$rows" ]; then + printf 'fmt-workspaces: cargo metadata failed for %s\n' "${roots[$i]}" >&2 return 2 fi + members[$i]="$(printf '%s\n' "$rows" | cut -f1 | tr '\n' ' ')" + while IFS= read -r m; do covered+=("$m"); done < <(printf '%s\n' "$rows" | cut -f2) + done + local orphans + orphans="$(manifests_with "$root" package | python3 -c ' +import os, sys +covered = {os.path.realpath(p) for p in sys.argv[1:]} +for line in sys.stdin: + m = line.rstrip("\n") + if m and os.path.realpath(m) not in covered: + print(m) +' "${covered[@]}")" + if [ -n "$orphans" ]; then + printf 'fmt-workspaces: no workspace root reaches these packages, so nothing checks them:\n%s\n' \ + "$orphans" >&2 + return 2 + fi + + for i in "${!roots[@]}"; do + manifest="${roots[$i]}" + local -a pkgs=() + read -r -a pkgs <<< "${members[$i]}" if [ "$list" = 1 ]; then printf '%s: %s\n' "$manifest" "${pkgs[*]}" 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);