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/cli.md b/context/code/cli.md index e1c20ca3..d4cc9a68 100644 --- a/context/code/cli.md +++ b/context/code/cli.md @@ -151,7 +151,75 @@ atomicity is preserved by keeping the existing whole-line `parse_line` as a still parses, and a line that did not still does not, so the only behavioural change is which cursor the commands see. +**That atomicity claim was false as written** (improvise-2bac), and §8 is what +makes it true. The gate runs the pure arm; the loop runs the interactive arm. A +gate is only a gate if *every argument list the pure arm accepts, the +interactive arm accepts too* — and `commit-cell-edit` broke it: +`add-category Foo . commit-cell-edit editing 5 Foo/x` on an empty document +passed the gate, applied the first segment, and then refused the second with +`no cell at cursor`. Half the line landed. + The TUI's `:` line had the same defect and is fixed the same way, with one limitation that is not new: `ExecuteCommand` returns one effect batch, so the segments of `:a . b` all see the pre-batch cursor, exactly as a `Binding::Sequence` does (gotchas 21 and 23). + +## 8 — the interactive arm is `parse`, then context; it is never a second parser + +#151 fixed three instances of *a registry with two construction arms demotes +whichever caller reaches the weaker one*, one patch each. A post-merge review +found the fourth (improvise-2bac): `commit-cell-edit `, +whose interactive arm read only ``, took the value from the `edit` buffer +and the key from the cursor. So a script that wrote 5 to `Region/East` +**deleted the cell under the cursor** — the empty buffer, committed — with exit +0 and nothing on stderr. The fifth was already in the tree: +`toggle-panel-and-focus false false` ignored both flags and toggled. + +The shape rather than the instances is what changed. `CmdRegistry::register` +used to take two free functions, `parse(args)` and `interactive(args, ctx)`, +and nothing related them; each command re-derived its arguments twice and every +disagreement was silent. It now takes three pieces, and the interactive arm is +**built from `parse`**, not beside it: + +- `parse(args) -> C` — the only function that reads arguments a caller supplied. +- `contextualize(C, &CmdContext) -> C` — fills the fields that are context and + never argument (a `Move`'s origin cursor, a `MovePanelCursor`'s bound). It is + handed the built command and **not the args**, so it cannot discard them. +- `fallback: Option<(args, ctx) -> C>` — reached **only** when `parse` refused. + This is where "no coordinates means the cursor" lives: `clear-cell` bare, and + `commit-cell-edit ` bare, which is what the keymap binds. + +Two consequences hold by construction rather than by test. **Whatever `parse` +accepts, the interactive arm accepts** — `contextualize` is infallible — so the +§7 gate cannot pass a segment the loop then refuses. And **an accepted argument +list cannot be overridden by the cursor**, because the only function that sees +both args and context never runs on an accepted list. `register_pure` and +`register_nullary` already had one arm; they are the `contextualize = identity, +fallback = None` case. + +What is still representable, stated rather than hidden: + +- `contextualize` can overwrite a field `parse` set. It would have to name the + field, which is a visible line in review, where the old `|_args, ctx|` was an + invisible omission. `commit_cell_edit_honours_an_explicit_value_and_cell_over_the_cursor` + fails if `commit-cell-edit`'s does. +- A `fallback` sees the args `parse` refused and could ignore some of them. + `commit-cell-edit`'s refuses anything but a lone mode, so the malformed + `commit-cell-edit editing 5` is an error on the `:` line — which has no pure + gate — rather than a commit of the edit buffer to the cursor cell. +- The gate is **stricter** than the loop wherever a fallback exists: a bare + `clear-cell` is refused in a script. That is the safe direction — the whole + line is refused — and it is unchanged from before #151. + +`toggle-panel-and-focus` did not fit `contextualize`, because its pure arm +defaulted the omitted flags to `true` and a contextualizer cannot tell an +omitted flag from an explicit one. So the omission is now in the type — +`target: Option<(bool, bool)>`, `None` meaning *toggle* — and the toggle is +resolved in `execute` against the same context. That is the honest home: the +decision depends on context, and `execute` is where context is. + +Guards: `every_argument_list_the_gate_accepts_the_run_accepts_too` enumerates +**every registration** against every candidate argument list, in a context whose +cursor sits on no cell — the context in which the old `commit-cell-edit` arm +refused. It lists no commands, so a new registration is covered the day it +lands. It is backed by the process-level tests in `tests/headless_cli.rs`. diff --git a/context/code/command.md b/context/code/command.md index 02de2469..987df348 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -2489,12 +2489,13 @@ line separated by `.`, coordinate pairs written `Category/Item`, and quoted stri available for the rare command that needs to enumerate all views, such as the view panel. -`InteractiveFn` constructs a `Cmd` from the interactive context (keymap dispatch). -It receives both the keymap args and the interactive context, so commands can -combine text arguments (a panel name) with runtime state (whether that panel is -currently open). `register_pure` registers a command that does not need interactive -context: called interactively with args it delegates to `parse`, and without args it -errors. +The interactive arm (keymap dispatch, scripts, the `:` line) is built **from** +`parse`: `register` takes `parse`, a `contextualize` that fills context-only +fields of the command `parse` built without seeing the args, and an optional +`fallback` reached only for argument lists `parse` refused. So an accepted +argument list is never overridden by context (improvise-2bac; the argument is +`context/code/cli.md` §8). `register_pure` is the case with nothing to +contextualize and no fallback: both arms are `parse`. `write_buffer` is the effect that writes a named buffer — the mirror of `read_buffer`, and the only place that knows "search" is not an entry in the buffers diff --git a/context/code/protocol.md b/context/code/protocol.md index 7b797818..fdbf9e8e 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -552,8 +552,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/cli.md b/context/test-evidence/cli.md index bf5f7818..4c89ddee 100644 --- a/context/test-evidence/cli.md +++ b/context/test-evidence/cli.md @@ -229,12 +229,94 @@ the syntax gate and then cleared whatever the cursor was sitting on — the orig So improvise-6uva's own lesson had a **third** instance in its own fix: two construction arms that mean different things demote whichever caller reaches the weaker one, and here the "weaker" one was the arm that had the context. The four -arms now go through `key_from_args_or_cursor`: explicit coordinates win, the +arms went through `key_from_args_or_cursor`: explicit coordinates win, the cursor is the fallback, which is also what the keymap path wants since a binding -carries no arguments. +carries no arguments. That helper is gone since improvise-2bac: the cursor is +now the registry's `fallback`, reached only when `parse` refused, so explicit +coordinates win by construction (`context/code/cli.md` §8). The fixture writes three cells and clears the one the cursor is **not** on, so the assertion separates "cleared the named cell" from "cleared the cursor cell" — clearing `North` by name would have passed against the broken code. Restoring the arg-discarding arm fails it: `the cursor cell was cleared instead of the named one: ["Region=East = 3", "Region=South = 2"]`. + +### commit_cell_edit_writes_the_named_cell_and_spares_the_cursors + +improvise-2bac, found by the post-merge adversarial review of PR #151: the +fourth instance of that PR's own lesson. `commit-cell-edit` was registered with +two free arms. The pure arm read ` `; the interactive arm +read only ``, took the value from the `edit` buffer and the key from the +cursor. Once #151 made scripts construct through the interactive arm, +`commit-cell-edit editing 5 Region/East` passed the syntax gate and then +committed an **empty** buffer to the cursor cell — deleting it — with exit 0 +and nothing on stderr. + +Reproduced at `40b8913` before the fix, through the real binary: the document +came back as `["Region=East = 3", "Region=North = 1"]` — `South`, the cell the +cursor had been moved onto, was gone and `East` was unchanged. + +The fixture writes three cells and **moves the cursor to `South`** in the same +line before committing to `East`, so the cursor sits on neither the target nor +the origin: a test with the cursor on the target is right by accident, and one +that only asserts the target misses the destruction. It asserts all three. + +Injected rivals, run: (a) `contextualize` for `CommitAndAdvance` overwriting +`key` and `value` from the cursor and the buffer — fails this test and +`a_line_whose_gate_passes_runs_whole`; (b) the registry dispatching to the +fallback first whenever one exists (the pre-fix shape) — fails this test, +`a_line_whose_gate_passes_runs_whole`, `an_explicit_coordinate_still_beats_the_cursor` +and both registry unit tests below. + +### a_line_whose_gate_passes_runs_whole + +improvise-2bac's second symptom, reported on the ticket: `context/code/cli.md` +§7 said the whole-line `parse_line` gate preserved line atomicity. It did not, +wherever the interactive arm refused an argument list the pure arm accepted. +`add-category Foo . commit-cell-edit editing 5 Foo/x` on an empty document +printed `Parse error: commit-cell-edit: no cell at cursor` after `Foo` had +already been added and saved: half the line landed. + +The assertion is on `Parse error` and on the committed cell, **not** on the exit +code. The line exits 1 both before #151 and after this fix, for a different and +pre-existing reason: the grid has no rows, so `commit-cell-edit`'s advance +emits `AbortChain`, and the CLI maps an aborted batch to exit 1 +(`context/code/cli.md` §3). Asserting success would have made this test fail on +the correct behaviour. + +### a_malformed_commit_line_is_refused_whole + +The other half of atomicity, which must survive the fix: a segment the gate +refuses (`commit-cell-edit editing 5` — a value and no coordinates) refuses the +whole line, so the preceding `add-category Foo` never lands. It passed before +the fix and must keep passing; its job is to catch a "fix" that loosens the +pure arm to make the gate agree. + +### commit_cell_edit_honours_an_explicit_value_and_cell_over_the_cursor + +The unit-level form of the headless test, against `CmdRegistry::interactive` +directly: cursor on `Type=Food, Month=Jan`, a decoy `edit` buffer, and an +explicit `5` for `Type/Clothing Month/Feb`. It asserts the built command's key +and value. Rival (a) above — a `contextualize` that overwrites the parsed +fields — fails it; this is the one hole the new registry shape leaves +representable, so it is the one that needs a test. + +### every_argument_list_the_gate_accepts_the_run_accepts_too + +Enumerates **every** registration (`CmdRegistry::names`, no hand-kept list) +against every candidate argument list, and requires the interactive arm to +accept whatever the pure arm accepts, in a context whose cursor sits on **no +cell** — a category with no items. That context is what made the old +`commit-cell-edit` arm refuse, so a context with a cell under the cursor would +have passed the bug. The assertion that the cursor has no cell is checked +first; a plain `Workbook::new` does not satisfy it, which is how the fixture +came to add the empty category. Rival (b) fails it. + +### explicit_open_and_focus_arguments_beat_the_toggle + +The fifth instance, named on improvise-2bac as plausible and not yet run: +`toggle-panel-and-focus false false` ignored both flags and toggled, so +on a **closed** panel it opened it. The fixture closes every panel first, +because on an open-and-focused panel the toggle also closes it and the test +would pass against the bug. Rival: `execute` ignoring `target` and always +toggling — fails this test for every panel. diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index e130266a..e73939f4 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4087,17 +4087,18 @@ Demonstrated at `e7de19d`: the first test below reports `Some("")`. ### `an_editing_mode_with_no_edit_buffer_shows_the_cell_rather_than_a_blank` -The fallback is `improvise_protocol::edit_seed`, which is the **existing** home -of "what an editor opened here starts with" — `seed_at` at the cursor, the -function `enter-advance` already seeds from. A second reconciliation of the -scroll offset inside `render.rs` would have been gotcha 42's own trap, so the -one-line fallback cost one `pub` rather than a copy. +**Superseded in part by improvise-ktlk** (entry at the end of this file). As +first landed, the fallback lived in `render.rs`: `editing` fell back to +`edit_seed` while `commit` and `append-char` still read the absent buffer as +`""`, so the overlay showed `1200` and Enter cleared it. The fallback now lives +in the fold (`ClientState::settle_editor`), which seeds the buffer itself, and +`render.rs` reads the same buffer `commit` does. The test's first assertion +changed with it: it used to check that the buffer was still absent after the +fold, and now checks that the fold seeded it. -The test asserts three things in order, and the middle one is why it is not -vacuous: that the buffer really is absent, that the cell really holds `1200`, -and only then what `editing` says. Without the first two a fixture that happened -to carry an `edit` buffer, or a grid whose cell was blank anyway, would pass -against the bug. +The test asserts three things in order: what the fold put in the buffer, that +the cell really holds `1200`, and only then what `editing` says. Without the +second, a grid whose cell was blank anyway would pass against the bug. **Fault injected:** `unwrap_or_default()`, the original expression. Fails with `Some("")`. @@ -4147,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 @@ -4223,3 +4230,99 @@ and converged. **Fault injected:** the same `Binding::Prefix` → `Upstream` reversion. Fails on the second press. + +## improvise-ktlk — the overlay and the commit read one buffer + +PR #162 (improvise-xg1z) changed what the overlay **draws** for an `Editing` +mode that arrived without its `edit` buffer, and not what Enter **commits**. +`render.rs` painted `edit_seed(state)`, while `resolve.rs::commit` and +`append-char` read `read_buffer(&state.view, "edit")`, which is `""` for an +absent key. So the overlay showed the value, Enter composed `ClearCell`, and one +typed character replaced the shown text rather than extending it. #162's tests +only looked at the drawing, and none of them pressed a key. + +The fix gives the fact one home. The fold seeds the missing buffer +(`ClientState::settle_editor`, called after every fold, bootstrap and refusal, +beside `settle_mode`), and `render.rs` reads the buffer through `read_buffer`, +as `commit` does. `edit_seed` is `pub(crate)` again and no renderer calls it. +Seeding happens only when there is a cell under the cursor that has +`edit_text`. With no such cell the buffer stays absent, which reads as `""` +on both paths, and `commit` declines anyway for want of a cell. + +### `an_editor_entered_without_its_buffer_commits_and_extends_the_cell` + +The fixture's display text (`1,200.50`) and edit text (`1200.5`) **differ**. +With `CellDisplay::number("1200")`, as #162 used, a fallback that seeds from the +formatted `text` passes too (gotcha 8). The test folds `ChangeMode(editing)` +with no `SetBuffer`, then presses the keys: Enter must compose +`SetCell(Number(1200.5))`, and after a typed `5`, Enter must compose +`SetCell(Number(1200.55))`. + +**Faults injected, each run:** +- The tree before this fix (render-only fallback). The buffer assertion fails + with `""`. With that assertion removed, the Enter assertion fails on its own + with `None`, because the first command is `ClearCell`. +- `edit_seed` returning the drawn `text` instead of `edit_text`. Fails with + `"1,200.50"`. +- 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 4c7f0346..c958621d 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -2391,6 +2391,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 @@ -4934,9 +4959,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". 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/command/cmd/core.rs b/crates/improvise-command/src/command/cmd/core.rs index 4af2470f..491718cb 100644 --- a/crates/improvise-command/src/command/cmd/core.rs +++ b/crates/improvise-command/src/command/cmd/core.rs @@ -139,9 +139,12 @@ pub trait Cmd: Debug + Send + Sync { /// Factory that constructs a Cmd from text arguments (headless/script). pub type ParseFn = fn(&[String]) -> Result, String>; -/// Factory constructing a Cmd from the interactive context. It takes both the keymap -/// args and the context, so a command can combine text args with runtime state. -pub type InteractiveFn = fn(&[String], &CmdContext) -> Result, String>; +/// Fills the context-only fields of a command `parse` already built. It never +/// sees the args, so it cannot discard them — `context/code/cli.md` §8. +pub type ContextualizeFn = fn(C, &CmdContext) -> C; + +/// Builds a command from context, reached ONLY for args `parse` refused. +pub type FallbackFn = fn(&[String], &CmdContext) -> Result; type BoxParseFn = Box Result, String>>; type BoxInteractiveFn = Box Result, String>>; @@ -183,13 +186,26 @@ impl CmdRegistry { name } - /// Register a command with both a text parser and an interactive constructor. - /// The name is derived from a prototype command instance. - pub fn register(&mut self, prototype: &dyn Cmd, parse: ParseFn, interactive: InteractiveFn) { + /// Register a command whose interactive arm is `parse` then `contextualize`, so + /// args `parse` accepts are honoured whole — `context/code/cli.md` §8. + pub fn register( + &mut self, + prototype: &C, + parse: fn(&[String]) -> Result, + contextualize: ContextualizeFn, + fallback: Option>, + ) { self.entries.push(CmdEntry { name: prototype.name(), - parse: Box::new(parse), - interactive: Box::new(interactive), + parse: Box::new(move |args| parse(args).map(|c| Box::new(c) as Box)), + interactive: Box::new(move |args, ctx| { + let built = match (parse(args), fallback) { + (Ok(cmd), _) => Ok(contextualize(cmd, ctx)), + (Err(_), Some(fallback)) => fallback(args, ctx), + (Err(err), None) => Err(err), + }; + built.map(|c| Box::new(c) as Box) + }), }); } diff --git a/crates/improvise-command/src/command/cmd/panel.rs b/crates/improvise-command/src/command/cmd/panel.rs index 29d1153d..1d27a42f 100644 --- a/crates/improvise-command/src/command/cmd/panel.rs +++ b/crates/improvise-command/src/command/cmd/panel.rs @@ -20,8 +20,7 @@ mod tests { let ctx = make_ctx(&m, &layout, ®); let cmd = TogglePanelAndFocus { panel: effect::Panel::Formula, - open: true, - focused: true, + target: Some((true, true)), }; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 2); @@ -40,8 +39,7 @@ mod tests { let ctx = make_ctx(&m, &layout, ®); let cmd = TogglePanelAndFocus { panel: effect::Panel::Formula, - open: false, - focused: false, + target: Some((false, false)), }; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 2); @@ -531,20 +529,24 @@ mod tests { #[derive(Debug)] pub struct TogglePanelAndFocus { pub panel: Panel, - pub open: bool, - pub focused: bool, + /// Explicit `(open, focused)`; `None` toggles: close if open and focused, else open and focus. + pub target: Option<(bool, bool)>, } impl Cmd for TogglePanelAndFocus { fn name(&self) -> &'static str { "toggle-panel-and-focus" } - fn execute(&self, _ctx: &CmdContext) -> Vec> { + fn execute(&self, ctx: &CmdContext) -> Vec> { + let (open, focused) = self.target.unwrap_or_else(|| { + let shown = ctx.panel_open(self.panel) && ctx.mode.panel() == Some(self.panel); + (!shown, !shown) + }); let mut effects: Vec> = Vec::new(); effects.push(Box::new(effect::SetPanelOpen { panel: self.panel, - open: self.open, + open, })); - if self.focused { + if focused { effects.push(effect::change_mode(self.panel.mode())); } else { effects.push(effect::change_mode(AppMode::Normal)); diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index de1d2985..6ef243dc 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -22,15 +22,50 @@ use super::text_buffer::*; use super::tile::*; use super::wizard::*; -/// Explicit coordinates win; the cursor is the fallback — `context/code/cli.md`. -fn key_from_args_or_cursor(args: &[String], ctx: &CmdContext) -> Result { - if args.is_empty() { - ctx.cell_key().clone().ok_or("no cell at cursor".into()) - } else { - Ok(parse_cell_key_from_args(args)) +/// The cell under the cursor: what a coordinate command falls back to when given none. +fn cursor_key(ctx: &CmdContext) -> Result { + ctx.cell_key().clone().ok_or("no cell at cursor".into()) +} + +/// A `Move` built from args takes the live cursor as its origin. +fn move_from_cursor(cmd: Move, ctx: &CmdContext) -> Move { + Move { + cursor: CursorState::from_ctx(ctx), + ..cmd } } +/// A commit built from args still advances from the live cursor. +fn commit_from_cursor(cmd: CommitAndAdvance, ctx: &CmdContext) -> CommitAndAdvance { + CommitAndAdvance { + cursor: CursorState::from_ctx(ctx), + ..cmd + } +} + +/// `` alone: commit the edit buffer to the cell under the cursor. +fn commit_at_cursor( + args: &[String], + ctx: &CmdContext, + advance: AdvanceDir, +) -> Result { + let [mode] = args else { + return Err("takes a mode alone, or a mode, a value and coordinates".into()); + }; + Ok(CommitAndAdvance { + edit_mode: parse_mode_name(mode)?, + value: read_buffer(ctx, "edit"), + key: cursor_key(ctx)?, + advance, + cursor: CursorState::from_ctx(ctx), + }) +} + +/// For a command with no context-only fields: `parse`'s result is used as built. +fn as_parsed(cmd: C, _ctx: &CmdContext) -> C { + cmd +} + /// Build the default command registry with all commands. /// Registry names MUST match the `Cmd::name()` return value. pub fn default_registry() -> CmdRegistry { @@ -51,14 +86,16 @@ pub fn default_registry() -> CmdRegistry { if args.is_empty() { return Err("clear-cell requires at least one Cat/Item coordinate".into()); } - Ok(Box::new(ClearCellCommand { + Ok(ClearCellCommand { key: parse_cell_key_from_args(args), - })) - }, - |args, ctx| { - let key = key_from_args_or_cursor(args, ctx)?; - Ok(Box::new(ClearCellCommand { key })) + }) }, + as_parsed, + Some(|_, ctx| { + Ok(ClearCellCommand { + key: cursor_key(ctx)?, + }) + }), ); r.register_pure(&AddFormulaCmd(vec![]), AddFormulaCmd::parse); r.register_pure(&FitMeasure::prototype(), FitMeasure::parse); @@ -80,16 +117,8 @@ pub fn default_registry() -> CmdRegistry { r.register_pure(&ExportCmd(vec![]), ExportCmd::parse); r.register_pure(&WriteCmd(vec![]), WriteCmd::parse); r.register_pure(&HelpCmd(vec![]), HelpCmd::parse); - r.register( - &HelpPageNextCmd(vec![]), - HelpPageNextCmd::parse, - |_args, _ctx| Ok(Box::new(HelpPageNextCmd(vec![]))), - ); - r.register( - &HelpPagePrevCmd(vec![]), - HelpPagePrevCmd::parse, - |_args, _ctx| Ok(Box::new(HelpPagePrevCmd(vec![]))), - ); + r.register_pure(&HelpPageNextCmd(vec![]), HelpPageNextCmd::parse); + r.register_pure(&HelpPagePrevCmd(vec![]), HelpPagePrevCmd::parse); // ── Navigation (unified Move) ────────────────────────────────────── r.register( @@ -102,22 +131,14 @@ pub fn default_registry() -> CmdRegistry { require_args(args, 2)?; let dr = args[0].parse::().map_err(|e| e.to_string())?; let dc = args[1].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { + Ok(Move { kind: MoveKind::Relative(dr, dc), cursor: CursorState::default(), cmd_name: "move-selection", - })) - }, - |args, ctx| { - require_args(args, 2)?; - let dr = args[0].parse::().map_err(|e| e.to_string())?; - let dc = args[1].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { - kind: MoveKind::Relative(dr, dc), - cursor: CursorState::from_ctx(ctx), - cmd_name: "move-selection", - })) + }) }, + move_from_cursor, + None, ); // Jump-to-edge commands: first/last row/col macro_rules! reg_jump { @@ -133,7 +154,7 @@ pub fn default_registry() -> CmdRegistry { cmd_name: $name, }, |_| { - Ok(Box::new(Move { + Ok(Move { kind: if $to_end { MoveKind::ToEnd($is_row) } else { @@ -141,19 +162,10 @@ pub fn default_registry() -> CmdRegistry { }, cursor: CursorState::default(), cmd_name: $name, - })) - }, - |_, ctx| { - Ok(Box::new(Move { - kind: if $to_end { - MoveKind::ToEnd($is_row) - } else { - MoveKind::ToStart($is_row) - }, - cursor: CursorState::from_ctx(ctx), - cmd_name: $name, - })) + }) }, + move_from_cursor, + None, ); }; } @@ -170,21 +182,14 @@ pub fn default_registry() -> CmdRegistry { |args| { require_args(args, 1)?; let n = args[0].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { + Ok(Move { kind: MoveKind::Relative(n, 0), cursor: CursorState::default(), cmd_name: "scroll-rows", - })) - }, - |args, ctx| { - require_args(args, 1)?; - let n = args[0].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { - kind: MoveKind::Relative(n, 0), - cursor: CursorState::from_ctx(ctx), - cmd_name: "scroll-rows", - })) + }) }, + move_from_cursor, + None, ); r.register( &Move { @@ -195,28 +200,21 @@ pub fn default_registry() -> CmdRegistry { |args| { require_args(args, 1)?; let dir = args[0].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { + Ok(Move { kind: MoveKind::Page(dir), cursor: CursorState::default(), cmd_name: "page-scroll", - })) - }, - |args, ctx| { - require_args(args, 1)?; - let dir = args[0].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { - kind: MoveKind::Page(dir), - cursor: CursorState::from_ctx(ctx), - cmd_name: "page-scroll", - })) + }) }, + move_from_cursor, + None, ); r.register( &EnterAdvance { cursor: CursorState::default(), }, |_| { - Ok(Box::new(EnterAdvance { + Ok(EnterAdvance { cursor: CursorState { row: 0, col: 0, @@ -227,13 +225,12 @@ pub fn default_registry() -> CmdRegistry { visible_rows: 20, visible_cols: 8, }, - })) + }) }, - |_, ctx| { - Ok(Box::new(EnterAdvance { - cursor: CursorState::from_ctx(ctx), - })) + |_, ctx| EnterAdvance { + cursor: CursorState::from_ctx(ctx), }, + None, ); // ── Cell operations ────────────────────────────────────────────────── @@ -245,14 +242,16 @@ pub fn default_registry() -> CmdRegistry { if args.is_empty() { return Err("yank requires at least one Cat/Item coordinate".into()); } - Ok(Box::new(YankCell { + Ok(YankCell { key: parse_cell_key_from_args(args), - })) - }, - |args, ctx| { - let key = key_from_args_or_cursor(args, ctx)?; - Ok(Box::new(YankCell { key })) + }) }, + as_parsed, + Some(|_, ctx| { + Ok(YankCell { + key: cursor_key(ctx)?, + }) + }), ); r.register( &PasteCell { @@ -262,14 +261,16 @@ pub fn default_registry() -> CmdRegistry { if args.is_empty() { return Err("paste requires at least one Cat/Item coordinate".into()); } - Ok(Box::new(PasteCell { + Ok(PasteCell { key: parse_cell_key_from_args(args), - })) - }, - |args, ctx| { - let key = key_from_args_or_cursor(args, ctx)?; - Ok(Box::new(PasteCell { key })) + }) }, + as_parsed, + Some(|_, ctx| { + Ok(PasteCell { + key: cursor_key(ctx)?, + }) + }), ); // clear-cell is registered above (unified: ctx.cell_key() or explicit coords) @@ -313,21 +314,19 @@ pub fn default_registry() -> CmdRegistry { if args.is_empty() { return Err("drill-into-cell requires Cat/Item coordinates".into()); } - Ok(Box::new(DrillIntoCell { + Ok(DrillIntoCell { key: parse_cell_key_from_args(args), - })) - }, - |args, ctx| { - let key = key_from_args_or_cursor(args, ctx)?; - Ok(Box::new(DrillIntoCell { key })) + }) }, + as_parsed, + Some(|_, ctx| { + Ok(DrillIntoCell { + key: cursor_key(ctx)?, + }) + }), ); r.register_nullary(|| Box::new(ViewNavigate { forward: false })); - r.register( - &ViewNavigate { forward: true }, - |_| Ok(Box::new(ViewNavigate { forward: true })), - |_, _| Ok(Box::new(ViewNavigate { forward: true })), - ); + r.register_nullary(|| Box::new(ViewNavigate { forward: true })); r.register_pure(&EnterMode(AppMode::Normal), |args| { require_args(args, 1)?; Ok(Box::new(EnterMode(parse_mode_name(&args[0])?))) @@ -345,38 +344,20 @@ pub fn default_registry() -> CmdRegistry { r.register( &TogglePanelAndFocus { panel: Panel::Formula, - open: true, - focused: true, + target: None, }, |args| { // Parse: toggle-panel-and-focus [open] [focused] require_args(args, 1)?; let panel = parse_panel(&args[0])?; - let open = args.get(1).map(|s| s == "true").unwrap_or(true); - let focused = args.get(2).map(|s| s == "true").unwrap_or(open); - Ok(Box::new(TogglePanelAndFocus { - panel, - open, - focused, - })) - }, - |args, ctx| { - require_args(args, 1)?; - let panel = parse_panel(&args[0])?; - // Default interactive: if already open+focused -> close, else open+focus - let currently_open = ctx.panel_open(panel); - let currently_focused = ctx.mode.panel() == Some(panel); - let (open, focused) = if currently_open && currently_focused { - (false, false) // close - } else { - (true, true) // open + focus - }; - Ok(Box::new(TogglePanelAndFocus { - panel, - open, - focused, - })) + let target = args.get(1).map(|s| { + let open = s == "true"; + (open, args.get(2).map(|s| s == "true").unwrap_or(open)) + }); + Ok(TogglePanelAndFocus { panel, target }) }, + as_parsed, + None, ); r.register( &TogglePanelVisibility { @@ -386,20 +367,16 @@ pub fn default_registry() -> CmdRegistry { |args| { require_args(args, 1)?; let panel = parse_panel(&args[0])?; - Ok(Box::new(TogglePanelVisibility { + Ok(TogglePanelVisibility { panel, currently_open: false, - })) + }) }, - |args, ctx| { - require_args(args, 1)?; - let panel = parse_panel(&args[0])?; - let currently_open = ctx.panel_open(panel); - Ok(Box::new(TogglePanelVisibility { - panel, - currently_open, - })) + |cmd, ctx| TogglePanelVisibility { + currently_open: ctx.panel_open(cmd.panel), + ..cmd }, + None, ); r.register( &CyclePanelFocus { @@ -408,19 +385,18 @@ pub fn default_registry() -> CmdRegistry { view_open: false, }, |_| { - Ok(Box::new(CyclePanelFocus { + Ok(CyclePanelFocus { formula_open: false, category_open: false, view_open: false, - })) + }) }, - |_, ctx| { - Ok(Box::new(CyclePanelFocus { - formula_open: ctx.formula_panel_open, - category_open: ctx.category_panel_open, - view_open: ctx.view_panel_open, - })) + |_, ctx| CyclePanelFocus { + formula_open: ctx.formula_panel_open, + category_open: ctx.category_panel_open, + view_open: ctx.view_panel_open, }, + None, ); r.register( &MovePanelCursor { @@ -433,29 +409,26 @@ pub fn default_registry() -> CmdRegistry { require_args(args, 2)?; let panel = parse_panel(&args[0])?; let delta = args[1].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(MovePanelCursor { + Ok(MovePanelCursor { panel, delta, current: 0, max: 0, - })) + }) }, - |args, ctx| { - require_args(args, 2)?; - let panel = parse_panel(&args[0])?; - let delta = args[1].parse::().map_err(|e| e.to_string())?; - let (current, max) = match panel { + |cmd, ctx| { + let (current, max) = match cmd.panel { Panel::Formula => (ctx.formula_cursor, ctx.model.formulas().len()), Panel::Category => (ctx.cat_panel_cursor, ctx.cat_tree_len()), Panel::View => (ctx.view_panel_cursor, ctx.workbook.views.len()), }; - Ok(Box::new(MovePanelCursor { - panel, - delta, + MovePanelCursor { current, max, - })) + ..cmd + } }, + None, ); r.register_nullary(|| Box::new(DeleteFormulaAtCursor)); r.register_nullary(|| Box::new(AddRecordRow)); @@ -490,11 +463,7 @@ pub fn default_registry() -> CmdRegistry { // ── Grid operations ────────────────────────────────────────────────── r.register_nullary(|| Box::new(ToggleGroupAtCursor { is_row: true })); - r.register( - &ToggleGroupAtCursor { is_row: false }, - |_| Ok(Box::new(ToggleGroupAtCursor { is_row: false })), - |_, _| Ok(Box::new(ToggleGroupAtCursor { is_row: false })), - ); + r.register_nullary(|| Box::new(ToggleGroupAtCursor { is_row: false })); r.register_nullary(|| Box::new(HideSelectedRowItem)); // ── Text buffer ────────────────────────────────────────────────────── @@ -537,27 +506,16 @@ pub fn default_registry() -> CmdRegistry { return Err("commit-cell-edit requires a mode, value, and coords".into()); } let edit_mode = parse_mode_name(&args[0])?; - Ok(Box::new(CommitAndAdvance { + Ok(CommitAndAdvance { key: parse_cell_key_from_args(&args[2..]), value: args[1].clone(), advance: AdvanceDir::Down, cursor: CursorState::default(), edit_mode, - })) - }, - |args, ctx| { - require_args(args, 1)?; - let edit_mode = parse_mode_name(&args[0])?; - let value = read_buffer(ctx, "edit"); - let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; - Ok(Box::new(CommitAndAdvance { - key, - value, - advance: AdvanceDir::Down, - cursor: CursorState::from_ctx(ctx), - edit_mode, - })) + }) }, + commit_from_cursor, + Some(|args, ctx| commit_at_cursor(args, ctx, AdvanceDir::Down)), ); r.register( &CommitAndAdvance { @@ -568,19 +526,8 @@ pub fn default_registry() -> CmdRegistry { edit_mode: AppMode::editing(), }, |_| Err("commit-and-advance-right requires context".into()), - |args, ctx| { - require_args(args, 1)?; - let edit_mode = parse_mode_name(&args[0])?; - let value = read_buffer(ctx, "edit"); - let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; - Ok(Box::new(CommitAndAdvance { - key, - value, - advance: AdvanceDir::Right, - cursor: CursorState::from_ctx(ctx), - edit_mode, - })) - }, + commit_from_cursor, + Some(|args, ctx| commit_at_cursor(args, ctx, AdvanceDir::Right)), ); r.register( &CommitBuffer { @@ -589,13 +536,14 @@ pub fn default_registry() -> CmdRegistry { |args| { require_args(args, 1)?; let kind = parse_commit_kind(&args[0], &AppMode::Normal)?; - Ok(Box::new(CommitBuffer { kind })) + Ok(CommitBuffer { kind }) }, - |args, ctx| { + as_parsed, + Some(|args, ctx| { require_args(args, 1)?; let kind = parse_commit_kind(&args[0], ctx.mode)?; - Ok(Box::new(CommitBuffer { kind })) - }, + Ok(CommitBuffer { kind }) + }), ); r.register_nullary(|| Box::new(CommitExport)); r.register_nullary(|| Box::new(CommitSave)); @@ -641,6 +589,9 @@ mod tests { vec!["1", "1"], vec!["formula", "1"], vec!["editing", "5", "Region", "East"], + vec!["editing", "5", "Region/East"], + vec!["formula", "false", "false"], + vec!["Region/East"], ] .into_iter() .map(|v| v.into_iter().map(String::from).collect()) @@ -802,6 +753,67 @@ mod tests { "only {checked} ex-command examples were found; the scan has stopped seeing them" ); } + + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn commit_cell_edit_honours_an_explicit_value_and_cell_over_the_cursor() { + let mut app = crate::ui::app::App::new(two_cat_model(), None); + app.view_state.buffers.insert("edit".into(), "decoy".into()); + let ctx = app.cmd_context( + crate::command::key::KeyCode::Null, + crate::command::key::KeyModifiers::NONE, + ); + let target = CellKey::new(vec![ + ("Month".into(), "Feb".into()), + ("Type".into(), "Clothing".into()), + ]); + assert_ne!( + ctx.cell_key().as_ref(), + Some(&target), + "the cursor is elsewhere" + ); + let args: Vec = ["editing", "5", "Type/Clothing", "Month/Feb"] + .map(String::from) + .into(); + let dbg = format!( + "{:?}", + ctx.registry + .interactive("commit-cell-edit", &args, &ctx) + .expect("commit-cell-edit resolves") + ); + assert!( + dbg.contains(&format!("key: {target:?}")), + "wrong cell: {dbg}" + ); + assert!(dbg.contains("value: \"5\""), "wrong value: {dbg}"); + } + + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn every_argument_list_the_gate_accepts_the_run_accepts_too() { + let r = default_registry(); + let mut wb = crate::workbook::Workbook::new("Empty"); + wb.add_category("Foo").expect("a category with no items"); + let app = crate::ui::app::App::new(wb, None); + let ctx = app.cmd_context( + crate::command::key::KeyCode::Null, + crate::command::key::KeyModifiers::NONE, + ); + assert!(ctx.cell_key().is_none(), "the cursor must sit on no cell"); + let mut accepted = 0; + for name in r.names().collect::>() { + for args in candidate_args() { + if r.parse(name, &args).is_err() { + continue; + } + accepted += 1; + if let Err(e) = r.interactive(name, &args, &ctx) { + panic!("the gate accepts `{name} {args:?}` but the run refuses it: {e}"); + } + } + } + assert!(accepted > 50, "only {accepted} accepted pairs were tried"); + } } #[cfg(test)] @@ -880,6 +892,7 @@ mod panel_open_tests { ctx.registry .interactive("toggle-panel-and-focus", &args, &ctx) .expect("toggle-panel-and-focus resolves") + .execute(&ctx) ); assert!( dbg.contains("open: false"), @@ -897,6 +910,7 @@ mod panel_open_tests { ctx.registry .interactive("toggle-panel-and-focus", &args, &ctx) .expect("toggle-panel-and-focus resolves") + .execute(&ctx) ); assert!( dbg.contains("open: true"), @@ -905,4 +919,29 @@ mod panel_open_tests { ); } } + + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn explicit_open_and_focus_arguments_beat_the_toggle() { + for panel in Panel::ALL { + let mut app = App::new(two_cat_model(), None); + app.view_state.formula_panel_open = false; + app.view_state.category_panel_open = false; + app.view_state.view_panel_open = false; + let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); + let args = [panel.name().to_string(), "false".into(), "false".into()]; + let dbg = format!( + "{:?}", + ctx.registry + .interactive("toggle-panel-and-focus", &args, &ctx) + .expect("toggle-panel-and-focus resolves") + .execute(&ctx) + ); + assert!( + dbg.contains("open: false") && !dbg.contains("open: true"), + "`{} false false` on a closed panel toggled it open: {dbg}", + panel.name() + ); + } + } } 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 7481fd7b..1fe8526e 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-protocol/src/client.rs b/crates/improvise-protocol/src/client.rs index 8010e920..599f21e4 100644 --- a/crates/improvise-protocol/src/client.rs +++ b/crates/improvise-protocol/src/client.rs @@ -74,6 +74,7 @@ impl ClientState { self.apply_projection(projection); } self.settle_mode(); + self.settle_editor(); self.revision += 1; } @@ -85,6 +86,7 @@ impl ClientState { other => reduce_view(&mut self.view, other), } self.settle_mode(); + self.settle_editor(); self.revision += 1; } @@ -124,6 +126,7 @@ impl ClientState { } self.last_rejection = Some(rejection); self.settle_mode(); + self.settle_editor(); self.revision += 1; } @@ -158,6 +161,21 @@ impl ClientState { self.view.mode = settled_mode(&self.view.mode, self.grid.records); } + /// An editor that arrived without its buffer is seeded here, so what is + /// drawn and what Enter commits read one buffer (improvise-ktlk). + fn settle_editor(&mut self) { + let absent = self + .view + .mode + .minibuffer() + .filter(|_| self.view.mode.is_editing()) + .map(|mb| mb.buffer_key.clone()) + .filter(|key| !self.view.buffers.contains_key(key)); + if let (Some(key), Some(seed)) = (absent, crate::resolve::edit_seed(self)) { + self.view.buffers.insert(key, seed); + } + } + /// The widget dispatch: a projection names the widget it is addressed to. /// The `CategoryTreeCache`/`TileBarCache` overlap is deliberate (gotcha 32). fn apply_projection(&mut self, projection: &Projection) { diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index 53544924..8abc5ec4 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/crates/improvise-protocol/src/lib.rs @@ -34,7 +34,7 @@ pub use render::{ WizardProposal, WizardStepKind, header_level_widths, }; pub use resolve::{ - Click, KeyOutcome, Request, click, edit_seed, read_buffer, resolve, typed, typed_cell_command, + Click, KeyOutcome, Request, click, read_buffer, resolve, typed, typed_cell_command, }; pub use viewmodel::{CatTreeEntry, flatten_cat_tree, view_axis_summary}; // `improvise-core`'s, because the index that resolves one lives in 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 be549bf3..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() { @@ -955,11 +953,10 @@ fn record_at_cursor(state: &ClientState) -> Option { } } -/// What an editor opened on the cursor starts with. The one home, so a -/// renderer does not reconcile the scroll offset again (gotcha 42). -pub fn edit_seed(state: &ClientState) -> String { - let (row, col) = state.view.cursor().selected; - seed_at(state, row, col) +/// What an editor opened on the cursor starts with, or `None` with no cell +/// there to seed from. Read by the fold, never a renderer (improvise-ktlk). +pub(crate) fn edit_seed(state: &ClientState) -> Option { + cell_at_cursor(state)?.edit_text.clone() } /// The display text at an absolute grid position, as the cache holds it. @@ -3028,6 +3025,55 @@ mod tests { )); } + /// improvise-ktlk: what an editor shows and what Enter commits are one + /// fact. See `context/test-evidence/clients.md`. + #[test] + fn an_editor_entered_without_its_buffer_commits_and_extends_the_cell() { + let mut client = client_on(record_cell()); + let mut cell = CellDisplay::number("1,200.50"); + cell.edit_text = Some("1200.5".into()); + cell.id = Some(record_cell()); + client.fold(&Command::Project(Projection::Grid(GridUpdate::Cells( + vec![((0, 0), cell)], + )))); + client.fold(&Command::ChangeMode(AppMode::editing())); + assert_eq!( + read_buffer(&client.view, "edit"), + "1200.5", + "the buffer is what the overlay paints and what a commit reads" + ); + + let committed = |client: &ClientState| { + let KeyOutcome::Local(commands) = resolve(client, ev(KeyCode::Enter)) else { + panic!("Enter in Editing must resolve here"); + }; + match &commands[0] { + Command::Model { + cmd: ModelCommand::SetCell { value, .. }, + .. + } => Some(value.clone()), + _ => None, + } + }; + assert_eq!( + committed(&client), + Some(CellValue::Number(1200.5)), + "Enter on an editor showing the cell must not clear it" + ); + + let KeyOutcome::Local(typed) = resolve(&client, ev(KeyCode::Char('5'))) else { + panic!("a character in Editing is append-char"); + }; + for command in &typed { + client.fold(command); + } + assert_eq!( + committed(&client), + Some(CellValue::Number(1200.55)), + "a typed character extends what the editor showed, not replaces it" + ); + } + /// At the bottom-right corner `EnterAdvance` returns `AbortChain`, so the /// commit leaves the user in normal mode. See the evidence file. #[test] @@ -3150,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-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index b73785d0..8025d36b 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -294,16 +294,13 @@ pub fn render_model(state: &ClientState) -> RenderModel { help: help_rows(&state.view), help_page: state.view.help_page, help_titles: help_titles(&state.view), - // An ABSENT buffer is not an empty one: blanking the overlay over a - // real value is one Enter from clearing it (improvise-xg1z). - editing: state.view.mode.is_editing().then(|| { - state - .view - .buffers - .get("edit") - .cloned() - .unwrap_or_else(|| improvise_protocol::edit_seed(state)) - }), + // The buffer Enter commits, never a second derivation of it: the fold + // seeds an absent one (improvise-xg1z, improvise-ktlk). + editing: state + .view + .mode + .is_editing() + .then(|| improvise_protocol::read_buffer(&state.view, "edit")), minibuffer: state.view.mode.minibuffer().map(|mb| RenderMinibuffer { prompt: mb.prompt.clone(), value: improvise_protocol::read_buffer(&state.view, &mb.buffer_key), @@ -912,10 +909,11 @@ mod tests { client .state_mut() .fold(&Command::ChangeMode(improvise_protocol::AppMode::editing())); - assert!( - !client.state().view.buffers.contains_key("edit"), - "the fixture must really be missing the buffer, or this proves \ - nothing about the fallback" + assert_eq!( + client.state().view.buffers.get("edit").map(String::as_str), + Some("1200"), + "the fold seeds the missing buffer, so the overlay and a commit \ + read one thing (improvise-ktlk)" ); assert_eq!( client.model().rows[0].cells()[0].text, 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/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/tests/headless_cli.rs b/tests/headless_cli.rs index f5ae9bb6..f0751f16 100644 --- a/tests/headless_cli.rs +++ b/tests/headless_cli.rs @@ -357,3 +357,86 @@ fn an_explicit_coordinate_still_beats_the_cursor() { "the named cell survived: {left:?}" ); } + +/// North=1, South=2, East=3 on the row axis; the cursor starts on North. +fn three_cells(scratch: &Scratch, name: &str) -> PathBuf { + let doc = three_rows(scratch, name); + run_cmd( + &[ + "set-cell 1 Region/North", + "set-cell 2 Region/South", + "set-cell 3 Region/East", + ], + &doc, + ); + assert_eq!(cell_lines(&doc).len(), 3, "the fixture wrote three cells"); + doc +} + +fn value_of(doc: &Path, item: &str) -> Option { + cell_lines(doc) + .into_iter() + .find(|l| l.contains(&format!("Region={item}"))) + .and_then(|l| l.rsplit(" = ").next().map(str::to_string)) +} + +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn commit_cell_edit_writes_the_named_cell_and_spares_the_cursors() { + let scratch = Scratch::new("commit-explicit"); + let doc = three_cells(&scratch, "commit.improv"); + + let out = run_cmd( + &["move-selection 1 0 . commit-cell-edit editing 5 Region/East"], + &doc, + ); + assert!(out.status.success(), "the line failed: {}", stderr_of(&out)); + let cells = cell_lines(&doc); + assert_eq!(value_of(&doc, "East").as_deref(), Some("5"), "{cells:?}"); + assert_eq!( + value_of(&doc, "South").as_deref(), + Some("2"), + "the cell under the cursor was touched: {cells:?}" + ); + assert_eq!( + value_of(&doc, "North").as_deref(), + Some("1"), + "the origin cell was touched: {cells:?}" + ); +} + +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_line_whose_gate_passes_runs_whole() { + let scratch = Scratch::new("commit-atomic"); + let doc = scratch.path("atomic.improv"); + let out = run_cmd( + &["add-category Foo . commit-cell-edit editing 5 Foo/x"], + &doc, + ); + assert!( + !stderr_of(&out).contains("Parse error"), + "a line the gate accepted was refused part-way: {}", + stderr_of(&out) + ); + let text = std::fs::read_to_string(&doc).expect("the document"); + assert!( + text.lines() + .any(|l| l.contains("Foo=x") && l.ends_with("= 5")), + "the commit did not land: {text}" + ); +} + +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_malformed_commit_line_is_refused_whole() { + let scratch = Scratch::new("commit-malformed"); + let doc = seeded(&scratch, "malformed.improv"); + let out = run_cmd(&["add-category Foo . commit-cell-edit editing 5"], &doc); + assert!(!out.status.success(), "a malformed line exited 0"); + let text = std::fs::read_to_string(&doc).expect("the document"); + assert!( + !text.contains("Foo"), + "the first half of a refused line landed: {text}" + ); +}