diff --git a/.agents/skills/wave/SKILL.md b/.agents/skills/wave/SKILL.md index b9b17b86..f52ce28a 100644 --- a/.agents/skills/wave/SKILL.md +++ b/.agents/skills/wave/SKILL.md @@ -37,7 +37,8 @@ it must cross. Two agents adding a workspace member will still collide in **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 +write, GitHub conventions (including how authors and reviewers answer reviews +on the PR thread), 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: @@ -77,10 +78,15 @@ defect is a test that passes. - 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. + and a test-merge, so it reviews the tree that will exist. Point it at + `agent-rules.md`, "GitHub", for what a reviewer reads and answers first. +- An ADR PR merges on the user's approval, not on a review: + `AGENTS.md`, "ADR pull requests". - 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`. +- Merge on GitHub, then `git fetch gh && git merge --ff-only gh/main`. Close + the bead only once the merge has landed. That is when the work is done: + `AGENTS.md`, "Repository policy over the Beads blocks". - Anything posted to GitHub goes out under the owner's account: open every 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 diff --git a/.agents/skills/wave/agent-rules.md b/.agents/skills/wave/agent-rules.md index 4c9a5bf8..7a4d3f52 100644 --- a/.agents/skills/wave/agent-rules.md +++ b/.agents/skills/wave/agent-rules.md @@ -29,12 +29,12 @@ not in a second copy. 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). +6. **The Beads "Session Completion" block near the end of `AGENTS.md` does not + set your git policy.** This repository does not opt in to the block's + "team-maintainer" profile. On a wave branch that profile's pull and bare + `git push` would be refused (rule 4), or worse. Rules 1–5 are your git + policy. The block is bd's and bd regenerates it whole, so it is not edited + by hand: `AGENTS.md`, "Repository policy over the Beads blocks" (improvise-x1uk). ## Commit early: a worktree is not durable storage @@ -47,14 +47,33 @@ 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 . cargo clippy --workspace --all-targets -- -D warnings +direnv exec . cargo test --workspace --no-fail-fast +direnv exec . python3 scripts/check-comment-runs.py --self-test +direnv exec . python3 scripts/check-comment-runs.py --check-evidence # also answers the comment cap +direnv exec . python3 scripts/check-ci-graph.py --self-test +direnv exec . python3 scripts/check-ci-graph.py +direnv exec . ./scripts/checkers.sh # every suite in scripts/, its own self-test included +direnv exec . tpl prolog-sessions/kb.pl -g "(kb_consistent -> halt(0) ; (kb_report, halt(1)))" direnv exec . ./scripts/node-test.sh 'web/tests/*.test.js' ``` +This list is the `test`, `js` and `kb` jobs of `.github/workflows/ci.yml`, +step for step, and this file is the one place it is written down. Change it in +the same PR that changes CI. The other CI jobs are conditional, below, or left +out on purpose: + +- **`wasm`**: when you touch a crate it checks (next bullet). +- **`harness-lint`**: when you touch `harness/opfs-contract`, run + `cargo clippy --target wasm32-unknown-unknown --all-targets -- -D warnings` + from that directory. Its format half is already `fmt-workspaces.sh`. +- **`build` and `browser`**: when you touch `web/`, `harness/`, or the surface + the page calls in `improvise-wasm-client` or `improvise-worker-server` + (third bullet). +- **`pages-deploy`** is left out. It deploys, and it runs only on a push to + `main`. What guards it locally is `check-ci-graph.py`, which is in the list. + - **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 @@ -70,10 +89,17 @@ direnv exec . ./scripts/node-test.sh 'web/tests/*.test.js' - **`--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 + wasm-client, worker-server) or `improvise-gp`, 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/`. +- If you touch `web/`, `harness/` or the page's wasm surface, run + `./harness/build.sh`, then + `./scripts/node-test.sh 'web/tests/bundle/*.test.js'`, then `npm ci` and + `npx playwright test` in `harness/`. The bundle tests sit outside the + `web/tests/*.test.js` glob and read the built `web/pkg`, so the standard list + never runs them. They failed only in CI once. A fresh worktree has no + `harness/node_modules`, and Playwright then reports a missing + `@playwright/test`, which reads like a broken checkout. - **If you change the grid's row or column count, also grep `harness/fixtures/*.improv` and `harness/tests/*.spec.js`.** On PR #176 a Playwright spec pinned the old collapsed-group row count. `cargo test` cannot @@ -165,9 +191,19 @@ text for any of these in your hand-back. 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. + "Shipping a wave". +- **As a PR's author, reply on the PR to every review finding you address.** + Name the commit that fixes it, or say why you declined it. Putting it only in + your hand-back to the orchestrator is not enough: the PR thread is where the + next reader looks. +- **As a reviewer, read the existing reviews first**, with + `./scripts/pr-reviews `, unpiped. Reply on its thread to every Codex + review: fixed (name the commit), declined (say why), or already one of our + tickets (name it). Every review you post states the head SHA it reviewed. +- **Do not merge, and do not close beads.** Your part ends at an open PR plus + your hand-back. What "done" means is in `AGENTS.md`, "Repository policy over + the Beads blocks". Avoid stacked PRs. If you must stack, never let the base + branch be deleted. ## Hand-back, per ticket diff --git a/AGENTS.md b/AGENTS.md index 4be26a9d..8a11780c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -57,9 +57,9 @@ cp -rf source dest # NOT: cp -r source dest ## The Wave Workflow Most work here is done by **waves**: an orchestrator chunks the open backlog, -fans several agents out into their own git worktrees, merges what comes back, -reviews it, and pushes the wave as one PR. This section is what that costs to -get right. Everything in it was learned by getting it wrong first. +fans several agents out into their own git worktrees, and shepherds each +agent's pull request through review to a merge on GitHub. This section is what +that costs to get right. Everything in it was learned by getting it wrong first. ### Who owns what @@ -75,11 +75,13 @@ badly: Wave agents **read** all three freely (`bd show`, `bd ready`, grep `kb.pl`) and **write** none of them. They report proposed repo-map lines and `kb.pl` facts as -*text* in their hand-back, and the orchestrator applies them — batched, at the -end of the wave, after the review. **`kb.pl` is written by `scripts/kb add|replace|rm` and by nothing else** — not +*text* in their hand-back, and the orchestrator applies them. Repo-map lines +are batched at the end of the wave, after the review, because they conflict. +`kb.pl` facts are not batched: the orchestrator writes each one when it arrives. +**`kb.pl` is written by `scripts/kb add|replace|rm` and by nothing else** — not the Prolog MCP, not an editor. Every write is gated on `kb_consistent` against -the candidate, so a bad delta leaves the file untouched. Facts do not need -batching: write one when you learn it. There is a `kb` skill; read it before +the candidate, so a bad delta leaves the file untouched, which is why a fact +can be written the moment it is learned. There is a `kb` skill; read it before your first write. Reading is unaffected — `loadSession` at session start is still how a session gets the model in hand. @@ -127,18 +129,21 @@ and watch the check fail. ### Shipping a wave -**Do not push `main` to `gh/main`.** Work reaches GitHub as a branch and a pull -request, and lands on `main` by being merged there. The orchestrator still -merges agent branches into its *local* `main` to integrate and run the gates — -that is how conflicts get found early — but the push is a branch push, not a -`main` push. `origin` and `gitea` are mirrors and take `main` directly. +**Do not push `main` to `gh/main`.** Each agent's work reaches GitHub as its own +branch and its own pull request, and lands on `main` by being merged there, once +CI is green and an adversarial review agent has passed the PR's head SHA. The +orchestrator does not integrate agent branches locally: its `main` only +fast-forwards to `gh/main` after a merge. The procedure is +`.agents/skills/wave/SKILL.md`, "Shepherding". `origin` and `gitea` are mirrors +and take `main` directly. -**So `origin/main` runs ahead of `gh/main`, and a branch destined for a GitHub -PR must merge `gh/main` — never `origin/main`.** This is a trap in this repo's -own topology rather than a slip: the two remotes are both spelled `main`, and -nothing about `git merge origin/main` looks wrong. The session-completion -protocol below literally says `git merge origin/main`, which is right for -`main` itself and wrong for a PR branch. It cost a review round on PR #145, +**Because the mirrors take `main` directly, `origin/main` can run ahead of +`gh/main`, and a branch destined for a GitHub PR must merge `gh/main` — never +`origin/main`.** This is a trap in this repo's own topology rather than a slip: +the two remotes are both spelled `main`, and nothing about +`git merge origin/main` looks wrong. bd's generated Session Completion block +once prescribed exactly that command (see "Repository policy over the Beads blocks" below). +It cost a review round on PR #145, where four unmerged commits — including another PR's whole diff — were pulled into a branch based on `gh/main` and had to be rebuilt by cherry-picking. ChatGPT Codex caught it; nothing in CI could, because the result compiles and @@ -153,15 +158,13 @@ finishing. It happened in this repo: 27 commits across six agent branches — including a new crate, a new protocol type, and a type moving between crates — went straight to `main` unreviewed, while `AGENTS.md` already said not to. If you notice a batch has bypassed the loop, the cheap repair is a **review-only -PR**: base a branch at the last PR merge, open a PR with `main` as the head, let -Copilot review the diff, and land the findings as an ordinary PR afterwards. It +PR**: base a branch at the last PR merge, open a PR with `main` as the head, +have an adversarial review agent review the diff, and land the findings as an +ordinary PR afterwards. It cannot be merged — the commits are already in — so close it once its findings are filed or fixed. Rewinding the default branch to fake a mergeable PR is not worth it on a repo with other collaborators. -One PR per wave, built by a **dedicated PR agent in its own worktree**, so waves -pipeline against it: the next wave runs while the PR absorbs review fixes. - **Read the PR's reviews before merging it.** This is separate from CI and from the adversarial agent, and it has been missed: two ChatGPT Codex reviews landed on merged PRs and were not read until afterwards, one of them a live P2 @@ -250,9 +253,20 @@ a PR you have not looked at in a few minutes, rather than after. Merge only once a review has passed — an adversarial agent's while Copilot is absent, and it must name the head SHA it reviewed. Prefer a **merge commit** — -`AGENTS.md` says merges over rebases, and a wave's commits are individually +`AGENTS.md` says merges over rebases, and a PR's commits are individually meaningful. +### ADR pull requests + +An ADR (`context/adr/`) merges on the user's approval, not on a review. The +steps, modelled on PR #122 (ADR 004): + +1. The PR proposes the ADR with its **open questions numbered**. +2. The user answers them on the PR. +3. The answers are folded into the ADR, in a section headed **"What the user + decided"**. +4. The PR merges only after the user comments **"Approved"**. + ### Never hand-edit `.beads/issues.jsonl` bd manipulates that file and auto-exports to it. Editing it — or even @@ -269,15 +283,37 @@ session before an agent tried one and found nothing. Read the ID back after filing, and before pushing docs run the audit: ```bash -grep -rohE 'improvise-[a-z0-9]{3,8}\b' \ +git ls-files -z --cached --others --exclude-standard -- \ context README.md AGENTS.md CLAUDE.md crates src tests examples web harness docs \ + .agents .claude \ + | xargs -0 grep -d skip -ohE 'improvise-[a-z0-9]{3,8}\b' \ | sort -u \ - | grep -vE '^improvise-(core|formula|io|protocol|command|gp|ws|worker|wasm|harness|registry|side|web)$' \ + | grep -vE '^improvise-(core|formula|io|protocol|command|gp|ws|worker|wasm|harness|registry|report|side|web)$' \ > /tmp/bd-audit-ids.txt bd show $(cat /tmp/bd-audit-ids.txt) 2>&1 \ - | grep -iE 'not found|no such issue' || echo "all resolve" + | grep -E '^(Issue [^ ]+ not found|[Nn]o such issue)' || echo "all resolve" ``` +`git ls-files` picks the files, and grep only reads them. The file list is the +tracked files plus the untracked ones, minus anything git ignores. That leaves +out three things a `grep -r` over the same roots reads: + +- **Build output.** After `./harness/build.sh`, `harness/serve/` holds + rendered copies of `context/`, and `grep -r` reported three dead ids from them + that no tracked file contains (improvise-8haj). +- **Other agents' worktrees.** In the orchestrator's checkout + `.claude/worktrees/` holds every agent's worktree, dozens of copies of the + repository with their unpushed work. +- **The skills twice.** `.claude/skills/*` are symlinks to directories under + `.agents/skills/`. `-d skip` passes over them, so the skills are read once, + through `.agents`. + +Plain `git grep` is not a substitute: on macOS its `-E` silently ignores `\b` +and finds a fraction of the ids. + +The last line matches at the start of a line because `bd show` prints each +ticket's body indented, and a body can itself contain the words "not found". + Three things the earlier version of this recipe got wrong, each of which let a real defect through: @@ -293,7 +329,8 @@ real defect through: - **The exclusion list named only `core` and `wasm`**, so every other crate name would have been reported missing. The exclusions above also cover the non-ticket tokens this repo really contains: a package name, a temp file - name, and the prose word "improvise-side". **They are anchored `^…$` for the + name, the prose word "improvise-side", and the example crate name in the + `kb` skill's `crate(...)` fact. **They are anchored `^…$` for the same reason the recipe is read as code**: unanchored, the `gp` alternative is a *prefix* match, so a future real ticket whose id happens to begin `gp` is silently excluded and the audit reports nothing while skipping it. An @@ -327,8 +364,32 @@ in progress. Two duplicates were filed in this repo within minutes of the hand-backs that prompted them. `bd search` the core concept *and* scan in-progress issues first. +## Repository policy over the Beads blocks - +Everything between a `BEGIN BEADS …` marker and its `END` is generated by bd, +and bd replaces it whole. `bd init` and `bd setup factory|opencode|mux` rewrite +the integration block, and the last three call it "stale" and invite the +rewrite whenever bd's template has moved on. `bd setup codex` rewrites the +Codex block. So a hand edit inside the markers is silently reverted by the next +regeneration. Repository policy lives here, outside them: + +- **Work is not done until it is in a pull request and that pull request has + merged.** A pushed branch is not done, and neither is a green one. In a wave, + the merge is the orchestrator's, after an adversarial review passes the head + SHA ("Shipping a wave"). So a wave agent's part ends at an open PR plus its + hand-back, and the ticket is done, and its bead closed, only when the merge + lands. +- **This repository does not opt in to the Beads "team-maintainer" profile.** + So the pull (`git pull`, or `git pull --rebase` in bd's "full" profile) and + the bare `git push` that the block lists for that profile are not + instructions here. Work reaches `main` as "Shipping a wave" describes, + and a wave agent pushes with the explicit refspec in + `.agents/skills/wave/agent-rules.md`, "Base, branch, push". +- **The no-markdown-TODO and no-`MEMORY.md` rules have one exception:** + `context/design-principles.md` and `context/repo-map.md` are kept by hand + (the repo map by the orchestrator only; see "Who owns what"). + + ## Beads Issue Tracker This project uses **bd (beads)** for issue tracking. Run `bd prime` to see full workflow context and commands. @@ -344,35 +405,44 @@ bd close # Complete work ### Rules -- Use `bd` for ALL task tracking — do NOT use TodoWrite, TaskCreate, or markdown TODO lists (the exception being that you must maintain context/design-principles.md and context/repo-map.md) +- Use `bd` for ALL task tracking — do NOT use TodoWrite, TaskCreate, or markdown TODO lists - Run `bd prime` for detailed command reference and session close protocol -- Use `bd remember` for persistent knowledge — do NOT use MEMORY.md files (the exception being that you must maintain context/design-principles.md and context/repo-map.md) +- Use `bd remember` for persistent knowledge — do NOT use MEMORY.md files + +**Architecture in one line:** issues live in a local Dolt DB; sync uses `refs/dolt/data` on your git remote; `.beads/issues.jsonl` is a passive export. See https://github.com/gastownhall/beads/blob/main/docs/core-concepts/sync-concepts.md for details and anti-patterns. + +## Agent Context Profiles + +The managed Beads block is task-tracking guidance, not permission to override repository, user, or orchestrator instructions. + +- **Conservative (default)**: Use `bd` for task tracking. Do not run git commits, git pushes, or Dolt remote sync unless explicitly asked. At handoff, report changed files, validation, and suggested next commands. +- **Minimal**: Keep tool instruction files as pointers to `bd prime`; use the same conservative git policy unless active instructions say otherwise. +- **Team-maintainer**: Only when the repository explicitly opts in, agents may close beads, run quality gates, commit, and push as part of session close. A current "do not commit" or "do not push" instruction still wins. ## Session Completion -**When ending a work session**, you MUST complete ALL steps below. Work is NOT complete until `git push` succeeds. +This protocol applies when ending a Beads implementation workflow. It is subordinate to explicit user, repository, and orchestrator instructions. -**MANDATORY WORKFLOW:** - -1. **File issues for remaining work** - Create issues for anything that needs follow-up +1. **File issues for remaining work** - Create beads for anything that needs follow-up 2. **Run quality gates** (if code changed) - Tests, linters, builds 3. **Update issue status** - Close finished work, update in-progress items -4. **PUSH TO REMOTE** - This is MANDATORY: +4. **Handle git/sync by active profile**: ```bash - git merge origin/main + # Conservative/minimal/default: report status and proposed commands; wait for approval. + git status + + # Team-maintainer opt-in only, unless current instructions forbid it: + git pull bd dolt push git push - git status # MUST show "up to date with origin" + git status ``` -5. **Clean up** - Clear stashes, prune remote branches -6. **Verify** - All changes committed AND pushed -7. **Hand off** - Provide context for next session +5. **Hand off** - Summarize changes, validation, issue status, and any blocked sync/commit/push step -**CRITICAL RULES:** -- Work is NOT complete until `git push` succeeds -- NEVER stop before pushing - that leaves work stranded locally -- NEVER say "ready to push when you are" - YOU must push -- If push fails, resolve and retry until it succeeds +**Critical rules:** +- Explicit user or orchestrator instructions override this Beads block. +- Do not commit or push without clear authority from the active profile or the current user request. +- If a required sync or push is blocked, stop and report the exact command and error. diff --git a/Cargo.toml b/Cargo.toml index 86f54e65..bda00df3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,7 +40,9 @@ improvise-command = { path = "crates/improvise-command" } ratatui = "0.30" crossterm = "0.29" serde = { version = "1", features = ["derive"] } -serde_json = "1" +# Parses JSON a user's numbers cross, so it must read back the f64 written: per crate, +# because each build stands alone (improvise-6yvg, context/code/protocol.md). +serde_json = { version = "1", features = ["float_roundtrip"] } anyhow = "1" thiserror = "2" indexmap = { version = "2", features = ["serde"] } diff --git a/context/code/cli.md b/context/code/cli.md index 330b95c8..0200feca 100644 --- a/context/code/cli.md +++ b/context/code/cli.md @@ -98,7 +98,8 @@ The message states the **shape** rather than the arity, because ## 6 — the help's ex-commands were the unguarded surface `help.rs` derives `HelpLine::Keys` rows from the keymap and drops any row whose -command does not resolve, so a key row cannot go stale. `HelpLine::Literal` and +command does not resolve, so a key row cannot go stale. `HelpLine::Literal` (an +ex-command only, since improvise-wk18: built by `ex!`) and `HelpLine::Accent` rows are hand-written prose and had nothing checking them — which is how all three documented spellings of `:formula` came to be ones the parser refuses. diff --git a/context/code/command.md b/context/code/command.md index b6cefb51..d6d2756f 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -935,6 +935,52 @@ There is deliberately **no `dirty = true`** in it: it declares of `MarkDirty` — harmless, and a second home for the one thing that commit set out to give exactly one. +### `RecordRewrite` — a row's edits composed, not replayed (improvise-uo2p) + +A staged drill edit is a statement about the record **as the snapshot showed +it**. It is not a statement about the model as the previous edit left it. The +edits in one row do not commute when they are replayed one at a time. A rename +moves every cell in the row (gotcha 1). A measure edit replayed after it writes +the old key, which leaves a ghost record there and a stale value in the moved +record. A second rename replayed after the first finds nothing at the old keys, +so it is dropped. `ApplyAndClearDrill` replayed them in `HashMap` order, so the +outcome was different from one App to the next. That is gotcha 18's class of +bug, found in an effect rather than an index. + +**Sorting is not the fix.** "Measures before renames" repairs the ticket's own +case and still drops a second rename every time (rival 1 in the evidence). Any +fixed order makes the corruption reproducible instead of removing it. So the +rule is **build each row's final record once, then write it**: + +- `RecordRewrite::of(model, row, edits)` collects the row's renames and measure + edits into maps keyed by column, and reads each unedited value off the model + **before anything is written**. It then states the rewrite: clear every old + key, and write every cell at the key with *all* renames applied. A measure + edit that is staged replaces the carried value. The result does not depend on + the order of the edits. A drill stages at most one edit per `(row, column)`, + so no two edits in a row name the same column. A blank rename is refused, and + the row's other edits still land at the unrenamed coordinates, as before. +- `RecordRewrite::merged` concatenates the rows' rewrites **by phase**: every + refusal, then every item added, then every clear, then every write. The clears + come first so that a swap is a swap. Rows are taken in snapshot row order + (`BTreeMap`), so where two final records land on one cell, **the later row + wins**. That is the answer plain records mode gives for the same renames made + top to bottom. + +The collision rule is a **choice**, and it still loses the earlier row's values +silently. Plain records mode loses them the same way when a rename lands on an +existing record. Refusing the colliding rows instead would be a change to both +paths. + +**One copy.** The plain records path (`commit.rs::commit_plain_records_edit`) +calls the same `RecordRewrite::of` with its one edit, so `rename_record_coordinate` +and the drill's `apply_coordinate_rename`/`apply_measure_edit` are gone. This is +the consolidation improvise-fdk1 asked for, "regardless" of its wire half. For a +single edit the output is the effects the plain path used to emit, reordered from +`clear, set` per cell to all clears then all sets, which touches distinct keys. +One behaviour moved on the drill side: it used to discard `add_item`'s error, and +now goes through `effect::AddItem`, which refuses when the category has gone. + ### `Save` / `SaveAs` abort the batch (improvise-8tk / improvise-lae) A failed save aborts the rest of the batch, so the `Quit` that `wq`/`ZZ` chains @@ -2629,8 +2675,10 @@ At bottom-right `EnterAdvance` has no place to go, so it emits a single `AbortCh stages in drill pending edits while in drill mode, or applies directly in plain records mode; for real keys it writes to the model. -`rename_record_coordinate` renames one coordinate of a records row. In wide format a -row is a group of cells — one per measure — so the rename has to move **every** one +`commit_plain_records_edit` hands a coordinate or measure edit to +`effect::RecordRewrite::of`, the one home it shares with the drill (improvise-uo2p; +see `RecordRewrite` above). It replaced `rename_record_coordinate`. In wide format a +row is a group of cells — one per measure — so a rename has to move **every** one of them, not just the cell under the cursor (improvise-c5v, gotcha 1). Every cell in the row moves, and a row seeded by `add-record-row` has no measure cells yet: its coordinates live on a lone measure-less cell, which must move too or the row loses @@ -2874,6 +2922,43 @@ flag: tidy-up goes where an abort cannot reach it. stays in `TileSelect` mode so the user can adjust several tiles. `axis: None` cycles; `axis: Some(a)` sets. +**The tile bar offers `_Index` and `_Dim`, and that is deliberate (improvise-jgzw).** +Every tile call site passes `Model::category_names()`, virtual categories included, +so tile cursor 0 is `_Index`. The ticket asked whether that was a leak. It is not, +for three reasons, each checked rather than argued: + +- **It is a decision the user already made.** improvise-kos: "`_Dim` and `_Index` + should work as pivot axes outside Records/Drill view". A category that works as a + pivot axis is one the axis picker offers. +- **The tile bar is the only way out of a records layout that was saved.** + improvise-jgzw's "check what it does" found that hand-assigning `_Index` → Row and + `_Dim` → Column in a real view (tile bar or `:set-axis`) is **written** to the + `.improv`, because a real view persists every axis, and it reopens as records mode. + (`VIRTUAL_VIEW_NAMES`' "never-persisted `_Index`/`_Dim` on axes" is true only of + `_Records`/`_Drill`.) In that document, `R` (`toggle-records-mode`) emits + `ViewBack` on an empty back stack, so it leaves the layout as it was and sets the + status to "Pivot mode". That is filed as improvise-gj5p. Moving the two axes back through + the tile bar is what gets out. In the browser, which declines + `toggle-records-mode`, the tile bar is also the only way in, and improvise-bd9o + already made that route safe instead of closing it + (`the_tile_bar_route_into_records_mode_does_not_strand_the_user`). +- **What filtering would cost was measured.** With `View::tileable_cats` filtering + on `CategoryKind::is_index_or_dim`, that bd9o test and the TUI's + `renders_each_tile_with_its_axis_label` fail, as well as the two characterization + tests here. + +What the default cursor does cost, stated: one cycle key on tile 0 puts `_Index` +on Row alone, and in a pivot view that empties the grid (0 rows on +`web/demo.improv`). The status line says `_Index → Row`, and three more presses +undo it. If that turns out to matter, the fix is the order the tile bar lists +categories in, not taking entries out of it. + +**`_Measure` must stay tileable.** It is on an axis in every shipped document, so +`regular_category_names()`, the advice for *prompts*, is the wrong filter here. +`the_tile_bar_offers_every_category_by_name_measure_and_records_axes_included` +names the whole offered list with `_Measure` on a column, and fails if either +decision changes. + ### `cmd/effect_cmds.rs` The parseable model-mutation commands are thin `Cmd` wrappers around effects, @@ -3150,8 +3235,8 @@ the cursor**. The result was silent corruption of an unrelated record: appending a row changes `view_frame`'s hash, the frame observation inside `apply_effects` invalidates the cursor to the origin *mid-batch* (gotcha 24), and `EnterEditAtCursor` then pre-fills the editor with row 0's category coordinate. -Enter commits through `rename_record_coordinate`, which by gotcha 1 moves every -measure cell of that row. +Enter commits through `rename_record_coordinate` (since improvise-uo2p, +`RecordRewrite::of`), which by gotcha 1 moves every measure cell of that row. Two things are worth carrying past this fix. @@ -3225,3 +3310,84 @@ key the two questions agree, because a cursor key never carries a None-axis category. The **collapsed-group fold** half of improvise-l4mp is not covered: its key carries the group's name as an ordinary coordinate, so no question about the key alone can see it — it needs `GridLayout::folds_a_group`. + +## A command is a function of its context — the determinism law (improvise-hzjo) + +`cmd/determinism.rs` holds the law the old `law_toggle_group_involution` claimed +and could not state. For **every** registered command and every argument list +that builds it, the effects `execute` returns must be equal, by `Debug` text, +across contexts built **independently from equal data**: four `App`s, each +parsed from `web/demo.improv` and given the same buffers, expanded categories, +yank, panels and cursor. + +**Why the instances must be distinct.** The old test called `execute` twice +against one `CmdContext`. Every `HashMap` it reached was the same instance with +the same `RandomState`, so it iterated the same way both times: `f(x) == f(x)`, +which nothing can fail. Separately built `App`s give every map its own seed. So a +command whose effects follow hash iteration order disagrees with itself. Hash +order is the one kind of nondeterminism this repo has actually shipped (gotcha +18). This law can see it where a command's `execute` builds it, as the drill +snapshot's record order was. It cannot see the other shipped instance, the +analyzer's `HashSet`: `analyze_records` is reached only from inside +`Effect::apply`, which is outside this law (see below). The companion test +`only_distinct_instances_can_see_a_command_that_follows_hash_order` keeps that +property checked: a test-only command that emits `ctx.buffers` in iteration +order must agree with itself on one context and disagree across two. + +**Four instances, not two.** A rival that takes *the first* entry of a map, not +all of them, agrees across two seeds whenever both happen to put the same entry +first. With 21 buffers that is roughly one chance in 21, and three comparisons +make it negligible. The fixture's values are also chosen to stay distinct under +the edits commands make. A first version used `v0`…`v15`, which `pop()` collapses +to `v`, and so it let `command-mode-backspace` reading an arbitrary buffer pass. + +**The store needs several records to order, so the drills come from the +document (improvise-e1sm).** The registry's `candidate_args` are extended with +names the demo really has, because coordinates that match nothing never reach +the records a drill or a fit would order. A hand-picked name is not enough. The +first version's only real drill was `Product/Latte, Quarter/Q2`: one record, two +cells. With the store's sort removed, two contexts put those two cells in the +same order about half the time, and with three comparisons the law passed about +one run in eight (measured: 46 of 400). So the arguments now include **every +item of every regular category as a one-coordinate drill**, read off the parsed +document. A one-coordinate drill spans every other category whole, so it matches +the whole slice of records under that item, not one record picked by name. In +the demo that is four records and eight cells for each of the eight drills. The +test +also asserts that the widest candidate it actually passes matches at least four +records. That guard catches a demo with too little data, and it catches the +derived drills being dropped from the argument list. It does **not** catch the +comparison losing its wide drills some other way, because it measures what the +argument lists match, not what the built commands ordered (improvise-b9f2). The +review showed this with a rival: a `drill-into-cell` parser that refuses fewer +than two arguments sends every one-coordinate drill to the one-record cursor +fallback. The guard still reports four records, and with the store's sort +removed the law passed 38 of 300 runs. With the store's sort removed and the +parser intact, the law fails 400 of 400 runs. + +**What it cannot see.** Only commands and arguments the fixture reaches. A +command whose nondeterminism needs some other state, such as records mode, a +wizard or a drill in progress, is outside this fixture. And `Debug` equality is +text equality: two effects that differ only in something their `Debug` omits are +equal here. + +**Its scope is what `execute` returns, not what `Effect::apply` does.** The law +compares the effects a command hands back. Many effects are unit structs that +decide all their work inside `apply`: `ApplyAndClearDrill`, +`SortData`, `CleanEmptyRecords`, `Save`, `RebuildLayout`, `AcceptWizardModel`. +Hash-order iteration inside an `apply` is therefore invisible here, however +often the law runs. improvise-uo2p is the worked example. +`ApplyAndClearDrill::apply` walks `drill.pending_edits`, a `HashMap`, in hash +order. A measure edit and a category rename staged on one row can then write a +ghost record. The command that emits it passes this law, because the effect's +`Debug` is just its name. So "the one kind of nondeterminism this repo has +shipped" is guarded here only on the `execute` side. The `apply` side needs its +own test per effect, or a law over `App` state after `apply_effects`. + +**The exemption list.** `OUTSIDE_THE_CONTEXT` names commands that legitimately +depend on something outside the context, such as a clock or an entropy source, +each with its reason. It is empty: `fit-measure`'s seed is an argument with a +fixed default, not a clock read. The list is checked in both directions. An +unlisted command that diverges fails the test, and so does a listed one that +does not diverge. A stale entry therefore cannot linger, and a new registration +is covered the day it lands. diff --git a/context/code/protocol.md b/context/code/protocol.md index f4cfb754..032feab5 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -756,28 +756,54 @@ coming back one ULP off, both in `CellDisplay::value` (host to client, so `yy` copied a neighbour) and in a `SetCell` (client to host, so a typed or pasted number landed as a neighbour). -The feature is on for **every crate that parses a frame**: `improvise-ws-server` -(`transport.rs`), `improvise-worker-server` (`worker.rs`) and -`improvise-wasm-client` (`socket.rs`). Each crate's own manifest names it, -because each browser bundle is built from one crate alone. The `wasm` CI job -and `wasm-pack` both invoke per crate, and feature unification cannot reach -across separate invocations (`context/repo-map.md`, `## Wasm`). Enabling it in -one crate and relying on the others to inherit it works in `cargo test` and -nowhere else. +The feature is on for **every workspace crate with a normal dependency on +serde_json** (improvise-dz8c). Each crate's own manifest names it, because each +build is made from one crate alone: the `wasm` CI job and `wasm-pack` invoke +per crate, and feature unification cannot reach across separate invocations +(`context/repo-map.md`, `## Wasm`). Enabling it in one crate and relying on the +others to inherit it works in `cargo test` and nowhere else. -That is also why the end-to-end tests cannot guard this alone. They run in one -unified build with `improvise-ws-server` as a dev-dependency, so they stay -green with the feature removed from the browser bundle. -`every_build_that_parses_a_frame_enables_float_roundtrip` asks `cargo tree` -per crate over normal edges instead. A new crate that parses frames belongs in -its list. +**Why "can parse" and not "parses a frame".** The first version of the rule +covered only the three crates that parse frames and listed them by hand. +Frames are not the only place a user's number crosses a JSON parser. +`improvise-io`'s legacy-JSON `.improv` load, `improvise-command`'s JSON import +(`StartImportWizard`, `ImportJsonHeadless`) and the root binary's +`improvise import` all parsed with the lossy default, and in the TUI binary +nothing enabled the feature (improvise-6yvg, reproduced: `0.24815714285714285` +stored as `…283`). The worker bundle happened to be exact through unification, +so one import gave different numbers in the two front ends. "Parses JSON input" +cannot be read off a manifest, but "has a normal serde_json dependency" can, and +it over-approximates in the cheap direction: the feature changes only parsing, +so a crate that only serializes pays nothing for it. Dev-dependencies are out, +because a test build ships nowhere. That also leaves out `gp-tree`, whose only +serde_json is a dev-dependency (its example CLI reads a saved model). Every +number it persists is an `f32`, and 3M random `f32`s all read back exactly +through the lossy parser, against about 30% of random `f64`s. -Not covered, and filed separately rather than widened into this change -(improvise-6yvg): `improvise-io`'s JSON import and `improvise-command`'s JSON -reads also parse numbers with the default parser. They are file formats, not -frames. In the TUI binary nothing enables the feature. In the worker bundle -unification happens to reach them, so one import can give different numbers -in the two front ends. +**Why a declaration and not the build graph.** The first check asked +`cargo tree` whether each listed crate's *build* had the feature. That answer +can be yes by accident. `improvise-command` depends on `improvise-io`, so with +the feature removed from command's manifest, command's own test build still +has it, and its import test stays green. The rule requires the crate's own +manifest to say it, so a later change to a dependency cannot take it away. + +**Why not enable it once, in `improvise-protocol`.** That removes the list, but +it would give the protocol a normal serde_json dependency it has no use for +(its repo-map entry says core and serde only). It would also reach only builds +that include the protocol, and `improvise-io` does not depend on it. Under this +rule the protocol's one serde_json dependency, the optional `test-support` one, +carries the feature too. That is off in every shipped build, so it costs +nothing. + +`every_crate_that_can_parse_json_enables_float_roundtrip` (`tests/json_numbers.rs`) +reads `cargo metadata --no-deps` and names every package, and every +target-specific entry, whose normal serde_json dependency lacks the feature. A +new crate is covered the day it declares the dependency. It does not reach +`harness/opfs-contract`, which is its own workspace and only serializes. + +The end-to-end tests cannot guard this alone. They run in one unified build +with `improvise-ws-server` as a dev-dependency, so they stay green with the +feature removed from the browser bundle. ### `AxisFrame` — the grid's frame, and why it is *one* type @@ -2064,14 +2090,38 @@ nothing in the table to look up, and its hint is a hand-written tail. That is a real hole and it is where it is visible, not papered over: a keymap cannot describe keys a command resolves privately. -**`gg`, `yy`, `ZZ` and the ex-commands.** A prefix sequence is two addresses and -not one binding (gotcha 13), and `:q`/`:w` are parsed text rather than keys. -Both are `HelpLine::Literal`, which says out loud that the row is spelled rather -than derived. +**The ex-commands.** `:q`/`:w` are parsed text rather than keys, so they are +`HelpLine::Literal` — built only by the `ex!` macro, whose leading colon is its +own — and the parser, not the keymap, is what checks them +(`every_ex_command_the_help_pages_show_is_one_the_parser_accepts`). -**The captions.** `i:edit` where `i` entered records mode would still pass every -test here. A caption is English beside a command name and only a reader judges -it — the same limit gotcha 14 recorded for the old hint test. +This paragraph used to say the same of `gg`, `yy` and `ZZ`: "a prefix sequence +is two addresses and not one binding". Two addresses is true and was no reason +to spell them by hand — `Keymap::prefixed_keys_running` looks the pair up the +way `keys_running` looks up one key, and `keys()` asks both (improvise-wk18). A +key string with no command name was also the one row no guard could reach. + +**Every row the pages ask for is visible to a guard.** The page builders take +the `keys` lookup as a parameter; `pages()` passes the real one, and +`help_bindings()` (tests only) passes one that records `(mode, command, args, +description)`. So a check over the help overlay reads the *same builders* the +overlay is drawn from, never a list kept beside them. That premise is a type, +not a convention (improvise-v29m): `HelpLine::Keys` carries a `KeyRow` whose +only constructor does the lookup, so a builder cannot type a key row the +recorder would never see; and `ex!` refuses at compile time any usage with a +bare word after the command name, so an ex row cannot smuggle a key either. +`resolve::tests::every_advertised_command_answers_in_its_own_mode` walks it +beside `advertised`, which is how the browser's black holes on the help page +became visible (improvise-uln5). + +**The captions.** `i:edit` where `i` entered records mode would still pass most +tests here: a caption is English beside a command name and only a reader judges +it — the same limit gotcha 14 recorded for the old hint test. Two checks now +bite at its edges. The browser guard requires an advertised key to *act*, so a +key whose caption promises an action and which only refuses must be named +(improvise-3t5x); and `only_the_edit_key_is_captioned_as_editing_the_cell` asks +by command that no other Normal row claims to edit (improvise-jhdf). A false +caption in words neither looks for still passes. ### The browser half diff --git a/context/design-principles.md b/context/design-principles.md index 8d3e29b1..23c31b1b 100644 --- a/context/design-principles.md +++ b/context/design-principles.md @@ -311,10 +311,9 @@ candidate for the store. - **And an argument must not live only in a commit message.** A message is write-once and unauditable: it cannot be corrected without rewriting merged - history, and no check in this repo can read one — the ID audit greps - `context`, `README.md`, `AGENTS.md`, `CLAUDE.md`, `crates`, `src`, `tests`, - `examples`, `web`, `harness` and `docs`, and a commit message is in none of - them. So a reason recorded only there is a reason nothing can ever find wrong. + history, and no check in this repo can read one — the ID audit + (`AGENTS.md`, "Cite a ticket ID you have read back") greps files in the + tree, and a commit message is in none of them. So a reason recorded only there is a reason nothing can ever find wrong. When one turns out to be wrong, correct `context/` and say in `context/` that the message is superseded; that is the fix rather than a compromise. PR #51's `7d25762` is the worked example, corrected by #56. diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index dec9d4e1..11ef493a 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -1458,6 +1458,17 @@ is prose, and nothing checks it against the command it documents. That is a neighbour of improvise-wk18, not the same gap — wk18 is about `literal()` rows whose *keys* go unchecked; here the keys were right and the words were wrong. +**improvise-jhdf closed the paraphrase, and the paragraph above is what it +found.** Rewording Enter's caption to "Edit the cell (same as i)" — the same lie +in new words — passed both help tests. The test now asks by **command**, which +wk18's `help::help_bindings()` made possible: every Normal-mode help row whose +caption says "edit" in any case must run `edit-or-drill`. It fails on that +rival naming `(Normal, "enter-advance", None, "Edit the cell (same as i)")`. +Two controls stop it passing by looking away: `edit-or-drill` and +`enter-advance` must each still have a Normal row, or the filter has nothing to +catch. What it still cannot see is a false caption that avoids the word — "Opens +the cell" — which is the honest limit of any check over prose. + ### a_view_row_click_focuses_moves_and_switches_to_the_row_clicked improvise-iioi, and it is the **same trap** as the category click one level @@ -2071,6 +2082,50 @@ mode `declined` refuses at the door, so a browser never stands in front of the hint; the other two are improvise-gcvb's records-cell commit, which needs `record_col`. +**improvise-3t5x and improvise-wk18 widened it again, in two directions, and the +list above is superseded by the one in the test.** + +*What "answers" means.* It counted any `Local` as answered, so a lone +`SetStatus` refusal passed — which is why improvise-s8h7's false "Edit current +cell" stayed green. Injected to show it: make `edit_or_drill`'s `Record` arm +return only `SetStatus("RIVAL")`, so `i` refuses on every cell — the old guard +**passed**. The line drawn now: a caption is a promise, so an advertised key +must *act*. A refusal is legitimate — a feature a browser honestly lacks should +say so rather than go silent — but it is not what the caption says, so it has to +be **named** in the list with its reason, like a black hole. The test therefore +sorts every key into three outcomes, `Acts`, `Declines` (said something, did +nothing) and `Silent`, and lists the last two. The same injection now fails +naming `NORMAL: a/i edit-or-drill Declines`. + +*Judged by what moved, not by the variant.* The first version of the new +classifier called anything but an all-status `Local` an action, and a rival that +unhooked `jump-first-row` from `composed` **passed** — because a chord's second +key always returns `Local([SetTransientPrefix(None), ..])`, the disarm, even +when nothing else happens. That is the laundering this entry already warns +about, one level down: `KeyOutcome::of` launders an empty vec into `Ignored`, +and `chord` launders a no-op into a `Local`. The disarm is now ignored when +deciding whether a key did anything, and the same rival fails naming `NORMAL: +gg jump-first-row Silent`. + +*Which keys.* It read `help::advertised`, the status-bar table, and the help +overlay is a second body of advertising the browser also paints +(improvise-uln5). It now also reads `help::help_bindings()`, which records every +keys row the page builders ask for — the same builders `pages()` renders, so the +two cannot drift — including prefix sequences (`gg`, `yy`, `ZZ`), which it +presses by arming the prefix first. That is what added the five host-only +black holes (`/`, `n`, `H`, `z`, Normal's `Enter`) to the list: each was already +in `the_command_names_that_still_need_the_host_are_these`, and the help page +advertised it anyway. + +*Which cells.* `furnished` stood on a record cell only, so `i` on an aggregate +(s8h7's whole subject) was outside its reach. Normal mode is now pressed on a +record, an aggregate and a derived cell, and a non-record cell is reported only +where it changes the answer, so the list does not triple. `furnished_on` also +carries a history in both directions and a yanked value, so `<`, `,` and `p` +are not refused by an empty stack — state refusals are correct behaviour and +not this test's subject, the same argument that gave `furnished` three of +everything. + ### a_locally_committed_creating_column_agrees_with_the_host improvise-gcvb. `resolve`'s commit returned `None` for the whole of records @@ -3341,9 +3396,22 @@ over `Ctrl-R` is in `context/code/tui.md`), the test is renamed, and the second half now asserts the binding **exists** — with `view-back` kept as the control, which now guards the opposite failure: a table that binds everything, or a sweep that reports names it never read. A third assertion pins the key itself, -`composed(&state, ev(Char(',')), "view-forward", &[]) == [Command::ViewForward]`, -so the binding cannot drift onto some other key while the presence check stays -happy. +`resolve(&state, ev(Char(','))) == Local([Command::ViewForward])`, so the +binding cannot drift onto some other key while the presence check stays happy. + +**That third assertion was vacuous as first written, and the paragraph above +said it was not** (improvise-3ij0). It called `composed(&state, ev(Char(',')), +"view-forward", &[])`, and `composed` dispatches on the command **name**: the +`view-forward` arm never reads the event, so the `,` was decoration and the +assertion restated the first half of the test. Rebinding `view-forward` from `,` +to `~` in `keymap.rs` left it green; the key was pinned only by +`ui::app::tests::the_view_history_walks_both_ways_from_the_keyboard`, in another +crate. It now goes through `resolve`, which looks the key up in the shared +keymap before anything reads a name — so the rival fails **here**, at this +assertion (resolve.rs, "and the key is `,`"), and the pre-fix code under the +same rival was run and passes. The general shape: a helper that takes a key +*and* a name, and dispatches on the name, cannot pin the key; only the lookup +can. **The rival: bind `,` to `view-back` instead** — the misspelling the original control was written against, and the one a copy-paste actually produces. It @@ -4522,21 +4590,78 @@ before #178 too, as the review noted. **Fault injected:** the same pre-fix tree. Fails: the host stored `4598108823619341467` in South/Cost. -### `every_build_that_parses_a_frame_enables_float_roundtrip` +**Retired: `every_build_that_parses_a_frame_enables_float_roundtrip`.** It asked +`cargo tree` about three crates listed by hand. improvise-dz8c replaced it with +`every_crate_that_can_parse_json_enables_float_roundtrip`, whose entry is in +`tooling.md`. The injections recorded for it still hold: with the feature +removed from `improvise-wasm-client` alone, the two tests above stay green, so +they cannot guard the bundle alone. -The two tests above run in **one** feature-unified test build, which includes -`improvise-ws-server` and `improvise-command` as dev-dependencies. So they pass -whenever **any** crate in that build enables the feature. The shipped browser -bundle is built from one crate alone, so it would still be lossy. That was -demonstrated: with the feature removed from `improvise-wasm-client` only, both -tests stayed green while `cargo tree` showed no `float_roundtrip` in the wasm -bundle. This test asks cargo instead, once per crate that parses a frame: -`cargo tree --locked --edges normal,features --invert serde_json --package -`, with `--target wasm32-unknown-unknown` for the two bundles. It uses -normal edges only, because a dev edge would unify the feature back in -(`context/repo-map.md`, `## Wasm`). +### resolve::tests::every_grid_move_lands_where_its_binding_says -**Faults injected:** the feature removed from `improvise-wasm-client` alone, -which fails naming that crate while the two tests above pass. Then removed from -`improvise-ws-server` alone, which fails naming it. The native path runs -without `--target`, so the two arms are different invocations. +improvise-6562, found reviewing PR #180 at 949810c, and it is the same +laundering as the disarm, one level over. `heard()` said it judged by what moved +and in fact judged by command **type**: any `Local` command besides a status or a +disarm counted as `Acts`. The fixture sat at every edge — a 2x1 grid with the +cursor at (0,0), one tile, help page 0 — so about twenty movement keys that +clamp to nothing were counted as acting. Rebinding `jump-last-col` to +`GridMove::ToStart(false)`, so `$` and `End` go to the *first* column, left the +whole workspace green. + +Two changes, because either alone is not enough. `heard()` now **folds** the +outcome and compares the view slice before and after (status and disarm aside), +counting a `Command::Model` as sent and a `Request` as an act; and `furnished_on` +stands on a 3x3 grid with the cursor at (1,1), three tiles with the cursor on +the middle one, a page axis on its middle item and help page 1, so no move is +clamped by an edge. Injecting `jump-last-col` as a `SetSelected` to where the +cursor already is now fails `every_advertised_command_answers_in_its_own_mode` +naming `NORMAL: $/End jump-last-col Silent`. + +**The reviewer's own rival still passes that guard, and should.** From (1,1), +`ToStart(false)` moves to (1,0) — the view moved, so the key "acts". No +did-anything-happen check can see *direction*; only an expected destination can. +This test supplies one without a second table: a `move-selection` binding's +expected landing is read off its own args (the deltas), and the four jumps are +named by what they are called. Every key that runs one is pressed from the +centre — fourteen of them, with a floor so a blind walk fails. Under the rival +it fails naming `End` and `$`. The general point: the fixture that made the +laundering invisible (the edge) is also the one that makes a wrong-direction +jump visible, so moving off the edge fixed one blind spot and opened the other, +and each needs its own question. + +**improvise-w7qd, and the floor was the hole.** As first written the test +covered `move-selection` and a four-entry jump table, behind `pressed >= 14`. +Two things passed the whole workspace, both found in re-review of #180 at +544744c: negating the `scroll-rows` and `page-scroll` arms, so `Ctrl-d`/`Ctrl-u` +and `PgDn`/`PgUp` all scroll the wrong way (advertised, and still "acting" from +the centre); and dropping the jump table *with* `$` sent to the first column, +because the sixteen `move-selection` sequences met the floor on their own. The +test now walks **every** command Normal binds. `scroll-rows` and `page-scroll` +state their landing from their own argument exactly as `move-selection` does — +from the centre of a 3x3 grid any step of at least one lands at `1 + d` +clamped, so a sign is visible whatever the page size — and a command that moves +the cursor (in the same view) without a stated landing **fails** rather than +being skipped. So a dropped table entry is caught by derivation, not by a count: +under the second rival it fails naming every jump as moving with nothing stating +where. The first rival fails on `PgUp` (`page-scroll ["-1"]`) and, alone, on +`Ctrl-u` (`scroll-rows ["-5"]`). A last assertion that each kind was pressed at +least once guards the other direction — a binding vanishing — and is a named +list on purpose, since that is the one thing no walk over bindings can see. + +**improvise-5kdr: the 3x3 fixture saw only a sign.** From the centre of a 3x3 +grid every step of one or more clamps to an edge, so the w7qd version stated +`page-scroll` as `1 + d` — one row, which is not what it does — and passed +because the true landing clamped to the same edge. Measured on #198 at 8772db2: +`page-scroll` ignoring the page size together with `scroll-rows` moving one row, +or `move-selection` moving `2d`, left the test green. The walk now stands at +(10,10) of a 21x21 grid with an 8x8 window, far from every edge, so a landing +shows magnitude: `10+d` for `move-selection` and `scroll-rows`, `10 + page·d` +for `page-scroll`, 0/20 for the jumps. The page size is not written into the +test: `grid_move`'s page rule moved into `page_rows(window_rows)` and the test +asks it, so the rule has one home. Rivals, each run: `move-selection` doubled +fails on `Up` (`move-selection ["-1","0"]`); the page-size-and-one-row pair +fails on `PgUp` (`page-scroll ["-1"]`); the one-row `scroll-rows` alone fails on +`Ctrl-u` (`scroll-rows ["-5"]`); the jump-table rival still fails naming every +jump. The residual, stated: because the test reads `page_rows`, a change to the +page rule itself moves the test with it — whether that rule is the TUI's is +`tests/lockstep.rs`'s question, not this one's. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 5663d672..6e2f8db9 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5314,6 +5314,167 @@ before `refuse_and_stop` was applied to its arms. For `load`, the injected fault is removing its abort (`refuse_and_stop` back to `app.refuse`), and the test then fails with "`load absent.improv . q` quit having loaded nothing". +### ui::effect::tests::a_drill_rows_measure_edits_follow_its_rename + +improvise-uo2p. `ApplyAndClearDrill` walked `pending_edits`, a `HashMap`, and +applied each edit alone against the model as the previous one left it. A rename +moves the row's cells. A measure edit then wrote `row.cell_key(measure)`, the +snapshot's old key. So when the rename came first, a ghost record appeared at the +old coordinates carrying the new value, and the moved record kept the stale one. +The reviewer saw this in 16 of 40 fresh Apps. The fixture stages two measure edits +and a rename on drill row 1, so a wrong answer needs only one of the three orders +that put the rename last. Each assertion runs over 32 fresh Apps and compares the +**whole** store, so the ghost and the stale value are both visible. Before the +fix it failed on five of five runs. Rival 3 (the rewrite addresses measure edits +at the unrenamed key) fails it. + +### ui::effect::tests::a_drill_row_renamed_twice_and_edited_lands_as_one_record + +improvise-uo2p. This is the case that fails **every** time, not with some +probability. Each rename read the snapshot's old keys. The second rename found +nothing there, because the first had moved it, so one rename was always dropped +and the row split into a `Clothing/Feb` record and a `Drink/Jan` record. Rival 1 +is the ticket's proposed fix direction: measures before renames, each edit +applied alone. It passes the ticket's own case and fails here. So "a fixed +order" is not a fix; the renames have to be composed. + +### ui::effect::tests::a_drill_that_swaps_two_records_coordinates_swaps_them + +improvise-uo2p. Row 0 is renamed onto row 1's coordinates and row 1 onto row 0's. +If the rows are applied in sequence, the first move overwrites the other record +and the second move carries the overwritten values back, so one record's data +is lost in either order. Rival 2 composes each row correctly but writes it +before reading the next row, and it fails here and nowhere else. That is why +`RecordRewrite::merged` reads every row off the model first, then clears every +old key, then writes. + +### ui::effect::tests::a_drill_edit_addresses_its_own_record_not_the_one_renamed_onto_its_coordinates + +improvise-uo2p. Row 1's staged `Cost` belongs to row 1's record, even though row +0 is being renamed away from coordinates and row 1 onto them. Before the fix, +some hash orders left `Clothing/Jan/Cost = 999` as a ghost. Rival 3 fails it. + +### ui::effect::tests::two_drill_rows_renamed_onto_one_record_resolve_to_the_later_row + +improvise-uo2p. Two rows renamed onto the same coordinates collide per cell. The +rule chosen is that the later snapshot row wins, which is what plain records +mode gives when the same renames are made top to bottom. Before the fix the +winner was the hash order's choice. Rival 4 reverses the row order in `merged` +and fails here. The rule is a choice, and it still loses data silently. See +`context/code/command.md`, `RecordRewrite`. + +### ui::effect::tests::a_record_rewrite_is_the_same_under_every_order_of_its_edits + +improvise-uo2p. The composition is order-independent **by construction**, so it +is asserted over all 24 orders of four edits on one row: two renames, a measure +write and a measure clear. The test pins one value as well as the equality, so +a rewrite that is the same wrong answer in every order still fails. Rival 3 +fails it. + +## `command/cmd/tile.rs` — the tile bar's offered set, by name (improvise-jgzw) + +### the_tile_bar_offers_every_category_by_name_measure_and_records_axes_included + +This test pins a decision, not a bug. The argument is in `context/code/command.md` +under `cmd/tile.rs`. Before this test, no tile assertion named the whole offered +set. The two older ones named only the category at cursor 0, so a filter that +changed what the bar offers past the cursor was invisible. The fixture puts +`_Measure` on **Column**, as every shipped document does. The test then walks the +cursor past the end and asserts the exact list +`[_Index, _Dim, _Measure, Type, Month]`. + +Rivals, injected, full workspace run: + +- **`regular_category_names()` in `TileAxisOp`**, the obvious "no virtual + categories in pickers" fix, which drops `_Measure` too. It fails this test + (`["Type", "Month"]`), the two characterization tests above it, and + `lockstep.rs::a_locally_resolved_tile_axis_change_agrees_with_the_host`. +- **`View::tileable_cats` filtering `is_index_or_dim`**, the ticket's option (b). + With the list asserted this test fails by construction, so the point is what + *else* fails: `renders_each_tile_with_its_axis_label` (TUI) and + `the_tile_bar_route_into_records_mode_does_not_strand_the_user`. The second one + is improvise-bd9o's own test of the route this decision keeps. + +## `command/cmd/determinism.rs` — the determinism law over every command (improvise-hzjo) + +The reasoning is in `context/code/command.md`, "A command is a function of its +context". This entry records what each test can see. + +### every_command_gives_equal_effects_in_independently_built_equal_contexts + +It runs all 89 registrations, 1500+ (command, args) pairs, in four contexts built +from four separately parsed copies of `web/demo.improv`, and requires equal +effect `Debug` text. It passed at `ae96b8c` with no exemptions. It is not a +regression test for a live bug. It restores a law, and the evidence is which +rivals it catches: + +- **`command-mode-backspace` reading `ctx.buffers.values().next()`** instead of + the `command` buffer: a real command following hash order. Injected, full + workspace run: this test is the **only** failure, because every + `text_buffer.rs` test builds a one-entry buffer map. With the comparison + switched to one reused context (`compare(&a, from_ref(&a))`) the same rival + **passes**. That is the half the ticket required: the distinct construction + is what does the work. +- **`DataStore::matching_positions` without its sort**: gotcha 18 re-shipped. + This entry first said the test catches it through `drill-into-cell` and + `fit-measure`'s record order. That was wrong (improvise-e1sm). `fit-measure` + never diverged, in 60 failing runs in review and in 354 here. And the catch + was a matter of luck. The only real drill was `Product/Latte, Quarter/Q2`, + which is one record with two cells, so each other context agrees with + probability 1/2. With the sort removed and the test binary run in a loop, the + law **passed 46 of 400 runs** at `fc0b13c`, and `drill-into-cell + ["Product/Latte", "Quarter/Q2"]` was the only pair that ever diverged. The + arguments now include every item of every regular category as a + one-coordinate drill, read off the parsed document (four records, eight cells + each in the demo). With the same injection the law **passed 0 of 400**, and + all eight new drills diverged in about 1195 of 1200 pairwise comparisons each. + Without the injection it passed 400 of 400. The three `cell.rs` order tests + still pin the store directly. +- **The derived drills dropped from the argument list** (the + `all.extend(one_coordinate_drills(model))` line commented out): the test + fails before comparing anything, with "the widest drill orders 1 records". + That is the only way of losing the wide drills the guard can see. It measures + what the argument lists match, not what the built commands ordered, so it can + pass while the comparison has lost its wide drills (improvise-b9f2). Review + rival: `drill-into-cell`'s parser changed to refuse fewer than two arguments, + which sends every one-coordinate drill to the one-record cursor fallback. The + guard stays silent and, with gotcha 18 re-injected, the law passed 38 of 300 + runs. +- **A stale exemption**: `("quit", "stale")` listed in `OUTSIDE_THE_CONTEXT`. + It fails as "listed … but did not". + +**A vacuity this test itself had on its first version.** The fixture's filler +buffers held `v0`…`v15`. The `command-mode-backspace` rival pops a character, +so every value collapsed to `v` (or `v1`), and the rival passed across two +distinct contexts. This is the "two derivations the fixture makes agree" shape. +The values are now `value {i} of 16`. The comparison was also widened from one +other instance to three, because a take-the-first rival agrees across two seeds +about one time in 21. + +### only_distinct_instances_can_see_a_command_that_follows_hash_order + +The check-the-checker behind the law. A test-only `EmitsInHashOrder` emits one +`SetBuffer` per `ctx.buffers` entry in iteration order. It must equal itself on +one context, which is why the old single-context law was vacuous, and differ +across two independently built ones. If a future refactor made the fixture +share one map, or share one `RandomState`, the second assertion fails. Without +this test the law would then pass everything and look healthy. + +## JSON import numbers (improvise-6yvg) + +### `a_json_import_stores_its_numbers_bit_for_bit` + +`ImportJsonHeadless` imports `[{"Region":"East","Revenue":0.24815714285714285}]` +and the test reads Region=East, _Measure=Revenue back by `f64::to_bits`. +`StartImportWizard` parses through the same `serde_json::from_slice` call, so +one test covers the crate's feature. + +**Red at `1158c23`** in `cargo test -p improvise-command`: stored +`0.24815714285714283`. That reproduces improvise-6yvg. **Limitation:** this test +cannot see the feature removed from `improvise-command` alone, because +`improvise-io`, a normal dependency, enables it in the same build. That case is +held by `every_crate_that_can_parse_json_enables_float_roundtrip` (`tooling.md`). + ### the_grid_and_a_csv_export_agree_on_a_fold_row The projection-level pin PR #203's review asked to keep. The review's own diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index 82a5bf80..1be0b13e 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -1362,6 +1362,29 @@ fold is the row `z` names the group from. Note the narrowing this implies: an its header goes with it. lhi4 kept that header because it was the only way back to the rows; the fold is now that way back. +### records_steps_are_the_spine_then_every_non_index_dim_category_on_none + +improvise-t0ma. Before this, `records_steps` was guarded by one incidental test: the +`is_index_or_dim() → is_measure()` rival failed only +`save_with_unfinished_record_is_readable`, a persistence test that happens to enter +records mode. This asserts the **steps**, not the effects `cmd/grid.rs` translates +them into, because the steps are the model-side answer a browser will also consume. +The expected list filters `_Index`/`_Dim` by **name**, not by `CategoryKind`, so it +is not a second copy of the predicate under test, and the setup asserts `_Measure` +and a regular category are both present, since a model with neither could not tell +the two predicates apart. Rival: `is_measure()` in place of `is_index_or_dim()`. +This test fails, because `_Measure` is dropped and `_Index`/`_Dim` are put on `None` +after the spine that just placed them. + +### drill_steps_filter_each_coordinate_and_free_the_rest + +improvise-t0ma's companion for `drill_steps`. The fixture includes a `none_cat` that +the key also fixes (`Region`) and one that is virtual (`_Index`), so both exclusions +are exercised. It also lists the key's coordinates out of sorted order, which checks +that the steps follow `CellKey`'s canonical order. Rival: `Axis::Page` in place of +`Axis::Filter` for a fixed coordinate, which reopens improvise-dz8 (a drilled +coordinate the user could page away from). This test fails. + ### a_folds_ratio_formula_is_the_ratio_of_its_folded_inputs improvise-eccl. `Margin = (Revenue - Cost) / Revenue` with Jan `10/5` and Feb `30/3` diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index daf7a527..48e8bd5a 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -1434,3 +1434,17 @@ rather than defaulted: the alternative — expanding the group in the export — reintroduce the iot6 divergence between what the user sees and what they export. On the pre-6qet tree the body is empty (the group had no row, and pruning left nothing), so the test fails there. + +## Legacy JSON numbers (improvise-6yvg) + +### `a_legacy_json_workbook_loads_its_numbers_bit_for_bit` + +A workbook with Food/Jan holding `0.24815714285714285` is serialized with +`serde_json` and loaded through `load_from`. A leading `{` sends it down the +legacy JSON path. The test first asserts that the text holds the exact decimal, +so what it measures is the parse. It compares `f64::to_bits`. Found by the +review of PR #190, not by the ticket. + +**Red at `1158c23`** in `cargo test -p improvise-io`: loaded +`0.24815714285714283`. Green once `improvise-io`'s own manifest enables +`float_roundtrip`, and red again with only that line reverted. diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index e1dc17a6..aa7d04d9 100644 --- a/context/test-evidence/protocol.md +++ b/context/test-evidence/protocol.md @@ -1100,3 +1100,94 @@ and the text between the inner quotes leaked. It failed as `InRawC survived`. `c"…"` and `cr#"…"#` are now literals. A first version of this test used a C string with no inner quotes and passed against the bug, because the ordinary lexing happened to blank it too. + +### help::tests::no_help_row_spells_a_key_by_hand + +improvise-wk18. The help pages had one kind of row whose keys the keymap never +saw: `literal("gg", …)`, `literal("yy", …)`, `literal("ZZ", …)`, and a dim tip +reading "press Tab or l/n". Every other key row comes off `keys_for`, so a +rebinding reaches it; these would have gone on saying `gg` after `g g` stopped +meaning anything, and no guard could see them because a string carries no +command name. At HEAD the test failed naming all four. + +The fix is structural rather than a second scanner. The three prefix rows are +ordinary `keys(..)` rows now — `keys` also asks `Keymap::prefixed_keys_running` +for two-key sequences. The tip is **gone** rather than derived: made a `keys` row +in `ModeKey::Help`, it gave the Welcome page its one row with a description, +and `harness/tests/help.spec.js` ("? opens help, and its rows carry the keys and +the words") failed in CI on `welcome.every(row => row.description === null)` — +Welcome is prose by contract. The Help mode's own status hint already derives +`next-page` from the keymap, so the literal was a second table (improvise-rr8n's +rule: keep the one derived from the thing it describes). +`literal()` and `dim()` are gone; the only way left to build a +`HelpLine::Literal` is the `ex!` macro, which writes the leading colon itself, so +what remains hand-written is ex-command text — checked against the parser by +`every_ex_command_the_help_pages_show_is_one_the_parser_accepts`, which reads +exactly the text a user would type. This test is what stops a direct +`HelpLine::Literal { keys: "gg", .. }` coming back past the macro. + +### help::tests::every_keys_row_the_pages_ask_for_finds_its_keys + +A `keys` row whose lookup finds nothing is **dropped**, by design — a page never +advertises a key that does not exist. The cost of that design is that a broken +lookup looks like a shorter page: `every_help_page_key_row_names_keys_that_exist` +only checks the rows that survived, plus a floor of twenty. So this asks the +question from the other end, over `help_bindings()` — every row the builders +*asked for* — and requires each to find a key. + +The injection: comment out the `prefixed_labels` extension in `keys`. The three +prefix rows vanish from the page, every other help test stays green, and this +fails naming `Normal jump-first-row`, `Normal yank` and `Normal wq`. + +### help::tests::an_ex_row_can_name_placeholders_and_nothing_else + +improvise-v29m, found reviewing PR #180 at 949810c. `no_help_row_spells_a_key_by_hand` +read only `HelpLine::Literal`, and its premise — that `keys()` is the only way to +make a key row — was a convention: `HelpLine::Keys { keys: Vec, .. }` +was a public variant with public fields, and `help_bindings()` records only what +the builders *ask* for, so `HelpLine::Keys { keys: vec!["Q"], description: "Quit +instantly" }` typed straight into `panels()`, with `Q` bound to nothing, passed +the whole workspace. So did `ex!("q or press Q", ..)`: the parser test splits on +colons and `parse_line` accepts trailing words. + +Both are now **compile errors**, which is why this test is not where the guard +lives. `HelpLine::Keys` carries a `KeyRow` whose fields are private to a module +whose one constructor, `KeyRow::looked_up`, does the keymap lookup; the injected +row fails with E0559/E0451. `ex!` evaluates `ex_usage_is_placeholders` in a +`const` item, and the injected usage fails with `evaluation panicked: assertion +failed: ex_usage_is_placeholders("q or press Q")`. The rule cost one row: +`:add-items a b c ...` had three bare words and is now +`:add-items ...`. This test pins the rule the macro asks, in both +directions, including that shipped usage and the rival. + +### help::tests::every_keys_row_drawn_is_one_the_builders_asked_for + +improvise-ssoe, found in re-review of #180 at 544744c. `KeyRow` closed +hand-typed keys, but `KeyRow::looked_up` was `pub(super)`, so a page builder +could call it directly instead of through its `keys` parameter — and +`help_bindings()` records only what goes through the parameter. Injected into +`editing()`: `KeyRow::looked_up(n, "clear-cell", None, "Edit the current +cell")`, which captions `x` as editing the cell — the exact lie `jhdf`'s test +exists for — and the whole workspace stayed green, because both guards that +would object read `help_bindings()`. + +Two changes. `looked_up` is now private to the `key_row` module, whose one +caller is the module's own `keys` — the lookup `pages()` hands every builder — +so the reviewer's injection is `E0624: associated function looked_up is +private`. That narrows the door without closing it: a builder can still reach +the module-level `keys` by path (`self::keys(..)`) rather than through its +shadowing parameter. So this test states the premise directly — the number of +keys rows drawn equals the number of rows the builders asked for (every asked +row finds its keys, by `every_keys_row_the_pages_ask_for_finds_its_keys`) — +and the `self::keys` bypass fails it `51 != 50`. + +**improvise-aszt: a count is not the premise.** The test above compared +*counts*, so a bypass that also drops one real row keeps them equal. Injected in +`editing()`: call the parameter `keys(e, "clear-buffer", …)` and discard it with +`let _ =`, then return `self::keys(n, "clear-cell", None, "Edit the current +cell")` in its place. The asked row is recorded and never drawn, the drawn row +is never recorded, 50 = 50, and the whole workspace stayed green. The test now +compares **content in order**: the drawn keys rows must equal +`help_bindings()` looked up through the same `keys`, row for row. The same +injection now fails. What still passes is a bypass that draws exactly what the +builders asked for, in the order they asked — which is harmless by definition. diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 0bdaca76..cd8ff234 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -681,3 +681,41 @@ reproduction. alone fails, exiting 0. All 44 others pass. That is the review's point: the older `a head that moves mid-wait is not a pass` starts from MERGEABLE, so it cannot see this rival. + +## Every JSON parser reads numbers exactly (improvise-dz8c) + +### `every_crate_that_can_parse_json_enables_float_roundtrip` + +It replaces a check over three crates listed by hand. It reads +`cargo metadata --no-deps` and fails naming every workspace package, and +every target-specific entry, whose **normal** serde_json dependency lacks +`float_roundtrip`. The rule and why it was chosen over the alternatives are in +`context/code/protocol.md`. It is red at `1158c23`, naming `improvise`, +`improvise-command`, `improvise-io` and `improvise-protocol`. + +**Faults injected, each run:** + +- **The feature removed from `improvise-io` alone.** The check fails naming + `improvise-io (all targets)`, and so does + `a_legacy_json_workbook_loads_its_numbers_bit_for_bit`. + `a_json_import_stores_its_numbers_bit_for_bit` stays green, because + `improvise-command` enables the feature and unification reaches io in its + build. +- **The feature removed from `improvise-command` alone.** The check fails naming + `improvise-command (all targets)`. The command crate's own import test + **stays green**: `cargo tree -p improvise-command` shows io's declaration + switching the feature on. So the manifest check is the only guard that + sees this case. It is also the case the retired build-graph check would + have passed. +- **A throwaway workspace member**, `zz-throwaway`, with normal dependencies on + `improvise-protocol` and `serde_json` and no feature (the lockfile updated, + as a real new crate would be). The check fails naming + `zz-throwaway (all targets)`. With the unconditional dependency carrying the + feature and a `cfg(target_arch = "wasm32")` one lacking it, the check fails + naming `zz-throwaway (cfg(target_arch = "wasm32"))`. That is stricter than + the build needs, since a wasm build unifies the two entries, and cheap to + satisfy. The crate was then removed. + +The `checked.len() >= 2` assertion is the positive signal. If a cargo change +renamed the metadata fields, the check would find no serde_json dependency and +pass vacuously without it. diff --git a/crates/improvise-command/Cargo.toml b/crates/improvise-command/Cargo.toml index 5f1bd1d9..acc61a1f 100644 --- a/crates/improvise-command/Cargo.toml +++ b/crates/improvise-command/Cargo.toml @@ -14,7 +14,9 @@ improvise-io = { path = "../improvise-io" } improvise-gp = { path = "../improvise-gp" } anyhow = "1" serde = { version = "1", features = ["derive"] } -serde_json = "1" +# Parses JSON a user's numbers cross, so it must read back the f64 written: per crate, +# because each build stands alone (improvise-6yvg, context/code/protocol.md). +serde_json = { version = "1", features = ["float_roundtrip"] } unicode-width = "^0.2" # Instant::now() panics on wasm32-unknown-unknown; web_time re-exports # std::time off-wasm and uses performance.now() on it (improvise-ywd). diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index 6846f198..86e4e55d 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -1,8 +1,8 @@ use crate::model::category::MEASURE_CATEGORY; -use crate::model::cell::{CellKey, CellValue}; +use crate::model::cell::CellKey; use crate::ui::app::AppMode; use crate::ui::effect::{self, Effect}; -use crate::view::{RecordCol, RecordRow}; +use crate::view::RecordCol; use super::core::{Cmd, CmdContext, read_buffer}; use super::grid::AddRecordRow; @@ -14,6 +14,7 @@ mod tests { use super::*; use crate::command::cmd::test_helpers::*; + use crate::model::cell::CellValue; use crate::workbook::Workbook; fn commit(kind: CommitKind) -> CommitBuffer { @@ -622,47 +623,8 @@ fn stage_drill_edit(record_idx: usize, col: RecordCol, value: &str) -> Box>, -) { - if value.is_empty() { - effects.push(effect::refuse(effect::RECORD_COORDS_CANNOT_BE_EMPTY)); - return; - } - effects.push(Box::new(effect::AddItem { - category: category.to_string(), - item: value.to_string(), - })); - - // Every cell in the row moves. A row seeded by `add-record-row` carries its - // coordinates on a lone measure-less cell, which must move too. - let cells: Vec<(CellKey, CellValue)> = if row.values.is_empty() { - model - .get_cell(&row.coords) - .map(|v| vec![(row.coords.clone(), v.clone())]) - .unwrap_or_default() - } else { - row.values - .iter() - .map(|(measure, v)| (row.cell_key(measure), v.clone())) - .collect() - }; - for (old_key, cell_value) in cells { - effects.push(Box::new(effect::ClearCell(old_key.clone()))); - effects.push(Box::new(effect::SetCell( - old_key.with(category, value), - cell_value, - ))); - } -} - -/// Apply a records-mode edit directly to the model (no drill state). +/// Apply a records-mode edit directly to the model (no drill state). A drill's staged +/// edits go through the same `RecordRewrite`, all of a row's at once (gotcha 19). fn commit_plain_records_edit( ctx: &CmdContext, record_idx: usize, @@ -674,11 +636,8 @@ fn commit_plain_records_edit( return; }; match col { - RecordCol::Category(category) => { - rename_record_coordinate(ctx.model, row, category, value, effects); - } - RecordCol::Measure(measure) => { - commit_regular_cell_value(&row.cell_key(measure), value, effects); + RecordCol::Category(_) | RecordCol::Measure(_) => { + effects.extend(effect::RecordRewrite::of(ctx.model, row, [(col, value)]).effects()); } RecordCol::NewMeasure => add_measure(value, effects), RecordCol::NewCategory => add_category(value, effects), diff --git a/crates/improvise-command/src/command/cmd/determinism.rs b/crates/improvise-command/src/command/cmd/determinism.rs new file mode 100644 index 00000000..8f0a71e5 --- /dev/null +++ b/crates/improvise-command/src/command/cmd/determinism.rs @@ -0,0 +1,233 @@ +//! A command is a function of its context: two contexts built independently from +//! equal data give equal effects — `context/code/command.md`, improvise-hzjo. + +use std::collections::{BTreeSet, HashSet}; + +use super::core::{Cmd, CmdContext}; +use super::registry::tests::candidate_args; +use crate::command::key::{KeyCode, KeyModifiers}; +use crate::model::Model; +use crate::model::cell::{CellKey, CellValue}; +use crate::model::record::record_of; +use crate::ui::app::App; +use crate::ui::effect::{self, Effect}; + +/// Commands whose effects depend on something outside the context, with why. +const OUTSIDE_THE_CONTEXT: &[(&str, &str)] = &[]; + +const DOCUMENT: &str = include_str!("../../../../../web/demo.improv"); + +/// One `App` over the shipped demo. Every context map holds several entries that +/// stay distinct under an edit like `pop()` — see the evidence. +fn fixture() -> App { + let workbook = crate::persistence::parse_md(DOCUMENT).expect("the demo parses"); + let mut app = App::new(workbook, None); + let vs = &mut app.view_state; + for i in 0..16 { + vs.buffers + .insert(format!("b{i}"), format!("value {i} of 16")); + } + for (name, text) in [ + ("edit", "42"), + ("formula", "Margin = Profit / Revenue"), + ("category", "Region"), + ("command", "add-item Product Mocha"), + ("export", "/tmp/improvise-hzjo.csv"), + ] { + vs.buffers.insert(name.into(), text.into()); + } + for cat in ["Product", "Quarter", "_Measure", "_Index", "_Dim"] { + vs.expanded_cats.insert(cat.into()); + } + vs.yanked = Some(CellValue::Number(7.0)); + vs.formula_panel_open = true; + vs.category_panel_open = true; + vs.view_panel_open = true; + vs.cat_panel_cursor = 2; + vs.view_panel_cursor = 1; + app.cursor_mut().selected = (1, 1); + app +} + +/// Every item as a one-coordinate drill: each spans the other categories whole, +/// so the store has several records to order — see the evidence. +fn one_coordinate_drills(model: &Model) -> Vec> { + let mut drills = Vec::new(); + for cat in model.regular_category_names() { + for item in model.category(cat).expect("listed").ordered_item_names() { + drills.push(vec![format!("{cat}/{item}")]); + } + } + drills +} + +/// How many distinct records the widest all-`Cat/Item` candidate matches. +fn widest_drill(model: &Model, candidates: &[Vec]) -> usize { + let coordinates = |args: &Vec| -> Option> { + args.iter() + .map(|a| { + a.split_once('/') + .map(|(c, i)| (c.to_string(), i.to_string())) + }) + .collect() + }; + candidates + .iter() + .filter(|args| !args.is_empty()) + .filter_map(coordinates) + .map(|partial| { + let cells = model.data.matching_cells(&partial); + let records: HashSet = cells.iter().map(|(k, _)| record_of(k)).collect(); + records.len() + }) + .max() + .unwrap_or(0) +} + +/// The registry's candidates, plus names the demo really has: a drill or a fit +/// over coordinates that match nothing never reaches the records it would order. +fn args_the_document_names(model: &Model) -> Vec> { + let mut all = candidate_args(); + all.extend(one_coordinate_drills(model)); + all.extend( + [ + vec!["Revenue"], + vec!["Product/Latte", "Quarter/Q2"], + vec!["Product/Latte", "Quarter/Q2", "_Measure/Cost"], + vec![ + "editing", + "5", + "Product/Latte", + "Quarter/Q2", + "_Measure/Cost", + ], + ] + .into_iter() + .map(|v| v.into_iter().map(String::from).collect()), + ); + all +} + +fn effects_of(name: &str, args: &[String], ctx: &CmdContext) -> Option { + let cmd = ctx.registry.interactive(name, args, ctx).ok()?; + Some(format!("{:?}", cmd.execute(ctx))) +} + +/// What running every registered command under `a` and under each of `others` found. +#[derive(Default)] +struct Comparison { + diverged: BTreeSet<&'static str>, + shown: Vec, + never_built: Vec<&'static str>, + compared: usize, +} + +fn compare(a: &CmdContext, others: &[CmdContext]) -> Comparison { + let mut found = Comparison::default(); + let candidates = args_the_document_names(a.model); + for name in a.registry.names().collect::>() { + let before = found.compared; + for args in &candidates { + let Some(left) = effects_of(name, args, a) else { + continue; + }; + found.compared += 1; + for b in others { + let right = effects_of(name, args, b).unwrap_or_default(); + if left != right { + found.diverged.insert(name); + found + .shown + .push(format!("{name} {args:?}:\n {left}\n {right}")); + } + } + } + if found.compared == before { + found.never_built.push(name); + } + } + found +} + +/// Evidence: `context/test-evidence/command.md`. +#[test] +fn every_command_gives_equal_effects_in_independently_built_equal_contexts() { + let apps: Vec = (0..4).map(|_| fixture()).collect(); + let contexts: Vec = apps + .iter() + .map(|app| app.cmd_context(KeyCode::Null, KeyModifiers::NONE)) + .collect(); + let model = contexts[0].model; + let widest = widest_drill(model, &args_the_document_names(model)); + assert!( + widest >= 4, + "the widest drill orders {widest} records, so the store's order is nearly a coin toss" + ); + let found = compare(&contexts[0], &contexts[1..]); + assert!( + found.never_built.is_empty(), + "no candidate args build these in the fixture, so the law never ran them: {:?}", + found.never_built + ); + let listed: BTreeSet<&str> = OUTSIDE_THE_CONTEXT.iter().map(|(n, _)| *n).collect(); + let unlisted: Vec<&str> = found + .shown + .iter() + .filter(|s| !listed.iter().any(|n| s.starts_with(&format!("{n} ")))) + .map(String::as_str) + .collect(); + assert!( + unlisted.is_empty(), + "effects depend on more than the context ({} pairs compared):\n{}", + found.compared, + unlisted.join("\n") + ); + let stale: Vec<&&str> = listed + .iter() + .filter(|n| !found.diverged.contains(**n)) + .collect(); + assert!( + stale.is_empty(), + "listed as depending on something outside the context, but did not: {stale:?}" + ); +} + +/// Iterates a context `HashMap` and emits in that order: the one nondeterminism +/// this repo has shipped (gotcha 18). The law must see it; a reused context cannot. +#[derive(Debug)] +struct EmitsInHashOrder; +impl Cmd for EmitsInHashOrder { + fn name(&self) -> &'static str { + "emits-in-hash-order" + } + fn execute(&self, ctx: &CmdContext) -> Vec> { + ctx.buffers + .iter() + .map(|(name, value)| { + Box::new(effect::SetBuffer { + name: name.clone(), + value: value.clone(), + }) as Box + }) + .collect() + } +} + +/// Evidence: `context/test-evidence/command.md`. +#[test] +fn only_distinct_instances_can_see_a_command_that_follows_hash_order() { + let (first, second) = (fixture(), fixture()); + let a = first.cmd_context(KeyCode::Null, KeyModifiers::NONE); + let b = second.cmd_context(KeyCode::Null, KeyModifiers::NONE); + let run = |ctx: &CmdContext| format!("{:?}", EmitsInHashOrder.execute(ctx)); + assert_eq!( + run(&a), + run(&a), + "one reused context iterates its maps the same way twice" + ); + assert_ne!( + run(&a), + run(&b), + "two independently built contexts must iterate differently, or the law is vacuous" + ); +} diff --git a/crates/improvise-command/src/command/cmd/mod.rs b/crates/improvise-command/src/command/cmd/mod.rs index c7f86714..07edab36 100644 --- a/crates/improvise-command/src/command/cmd/mod.rs +++ b/crates/improvise-command/src/command/cmd/mod.rs @@ -13,6 +13,9 @@ pub mod text_buffer; pub mod tile; pub mod wizard; +#[cfg(test)] +mod determinism; + // Re-export items used by external code pub use self::core::{Cmd, CmdContext, CmdRegistry}; pub use registry::default_registry; diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index 0534709e..dbd86d68 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -498,13 +498,13 @@ pub fn default_registry() -> CmdRegistry { } #[cfg(test)] -mod tests { +pub(super) mod tests { use super::*; use crate::command::cmd::test_helpers::*; /// Argument vectors to try when constructing a command, most specific /// first. A command is exercised by the first one its parser accepts. - fn candidate_args() -> Vec> { + pub(in crate::command::cmd) fn candidate_args() -> Vec> { [ vec![], vec!["1"], diff --git a/crates/improvise-command/src/command/cmd/tile.rs b/crates/improvise-command/src/command/cmd/tile.rs index 1ff05418..c4a71cf9 100644 --- a/crates/improvise-command/src/command/cmd/tile.rs +++ b/crates/improvise-command/src/command/cmd/tile.rs @@ -59,6 +59,26 @@ mod tests { assert!(effects.is_empty()); } + /// improvise-jgzw: `_Index`/`_Dim` stay tileable on purpose, and `_Measure` on + /// a column must too — `context/test-evidence/command.md`. + #[test] + fn the_tile_bar_offers_every_category_by_name_measure_and_records_axes_included() { + let mut m = two_cat_model(); + m.active_view_mut().set_axis("_Measure", Axis::Column); + let layout = make_layout(&m); + let reg = make_registry(); + let mut ctx = make_ctx(&m, &layout, ®); + let mut offered = Vec::new(); + for idx in 0..8 { + ctx.tile_cat_idx = idx; + let dbg = effects_debug(&TileAxisOp { axis: None }.execute(&ctx)); + if let Some(rest) = dbg.split("CycleAxis(Active, \"").nth(1) { + offered.push(rest.split('"').next().unwrap_or_default().to_string()); + } + } + assert_eq!(offered, ["_Index", "_Dim", "_Measure", "Type", "Month"]); + } + #[test] fn move_tile_cursor_right() { let m = two_cat_model(); diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 18528e69..f07adea5 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -1,3 +1,4 @@ +use std::collections::BTreeMap; use std::fmt::Debug; use std::path::PathBuf; @@ -663,24 +664,19 @@ impl Effect for ApplyAndClearDrill { let Some(drill) = app.drill_state.take() else { return; }; - if drill.pending_edits.is_empty() { - return; - } + let mut rows: BTreeMap> = BTreeMap::new(); for ((record_idx, col), new_value) in &drill.pending_edits { - let Some(row) = drill.records.get(*record_idx) else { - continue; - }; - match col { - RecordCol::Measure(measure) => { - apply_measure_edit(app, &row.cell_key(measure), new_value); - } - RecordCol::Category(category) => { - apply_coordinate_rename(app, row, category, new_value); - } - // Never staged: `commit_cell_value` applies a new measure or - // category immediately — a schema change is not a cell edit. - RecordCol::NewMeasure | RecordCol::NewCategory => {} - } + rows.entry(*record_idx) + .or_default() + .push((col, new_value.as_str())); + } + let model = &app.model_state.workbook.model; + let rewrite = RecordRewrite::merged(rows.iter().filter_map(|(record_idx, edits)| { + let row = drill.records.get(*record_idx)?; + Some(RecordRewrite::of(model, row, edits.iter().copied())) + })); + for effect in rewrite.effects() { + effect.apply(app); } // No `dirty = true` here: this declares `Model(CellData)` and // `App::apply_effects` raises the flag off that (improvise-gv9r). @@ -690,51 +686,114 @@ impl Effect for ApplyAndClearDrill { } } -/// Write one measure cell, or clear it when the edit is blank. -fn apply_measure_edit(app: &mut App, key: &CellKey, new_value: &str) { - let model = &mut app.model_state.workbook.model; - match new_value { - "" => { - model.clear_cell(key); - } - v => { - let value = v - .parse::() - .map(CellValue::Number) - .unwrap_or_else(|_| CellValue::Text(v.to_string())); - model.set_cell(key.clone(), value); - } - } +/// A records row's edits composed into the one record they mean, read off the model +/// before anything is written. Why composed: `context/code/command.md`, `RecordRewrite`. +#[derive(Debug, Default, PartialEq)] +pub(crate) struct RecordRewrite { + refused: bool, + items: Vec<(String, String)>, + clears: Vec, + sets: Vec<(CellKey, CellValue)>, } -/// Rename one coordinate of a records row. The row is a group of cells — -/// one per measure — so every one of them moves (improvise-c5v). -fn apply_coordinate_rename(app: &mut App, row: &RecordRow, category: &str, new_value: &str) { - if new_value.is_empty() { - app.refuse(RECORD_COORDS_CANNOT_BE_EMPTY); - return; - } - let keys: Vec = if row.values.is_empty() { - vec![row.coords.clone()] - } else { - row.values - .iter() - .map(|(measure, _)| row.cell_key(measure)) - .collect() - }; - let _ = app - .model_state - .workbook - .add_item(category, new_value.to_string()); - for old_key in keys { - let Some(value) = app.model_state.workbook.model.get_cell(&old_key).cloned() else { - continue; +impl RecordRewrite { + /// `row`'s edits, each naming a distinct column. The result does not depend on + /// their order: every coordinate is renamed before any cell is placed. + pub(crate) fn of<'a>( + model: &crate::model::Model, + row: &RecordRow, + edits: impl IntoIterator, + ) -> Self { + let mut refused = false; + let mut renames: BTreeMap<&str, &str> = BTreeMap::new(); + let mut measures: BTreeMap<&str, Option> = BTreeMap::new(); + for (col, value) in edits { + match col { + RecordCol::Category(_) if value.is_empty() => refused = true, + RecordCol::Category(category) => { + renames.insert(category.as_str(), value); + } + RecordCol::Measure(measure) => { + measures.insert(measure.as_str(), improvise_protocol::typed(value)); + } + // Never staged: a new measure or category is a schema change, + // applied at once by `commit_cell_value`. + RecordCol::NewMeasure | RecordCol::NewCategory => {} + } + } + let renamed = |key: &CellKey| { + renames.iter().fold(key.clone(), |key, (category, item)| { + key.with(*category, *item) + }) }; - app.model_state.workbook.model.clear_cell(&old_key); - app.model_state - .workbook - .model - .set_cell(old_key.with(category, new_value), value); + let moved: Vec<(Option<&str>, CellKey)> = match (renames.is_empty(), row.values.is_empty()) + { + (true, _) => Vec::new(), + (false, true) => vec![(None, row.coords.clone())], + (false, false) => row + .values + .iter() + .map(|(measure, _)| (Some(measure.as_str()), row.cell_key(measure))) + .collect(), + }; + let mut rewrite = RecordRewrite { + refused, + items: renames + .iter() + .map(|(category, item)| (category.to_string(), item.to_string())) + .collect(), + ..Self::default() + }; + for (measure, old) in &moved { + let value = match measure.and_then(|m| measures.remove(m)) { + Some(edit) => edit, + None => model.get_cell(old).cloned(), + }; + rewrite.clears.push(old.clone()); + rewrite.sets.extend(value.map(|v| (renamed(old), v))); + } + for (measure, edit) in measures { + let key = renamed(&row.cell_key(measure)); + match edit { + Some(value) => rewrite.sets.push((key, value)), + None => rewrite.clears.push(key), + } + } + rewrite + } + + /// Several rows' rewrites as one: every clear lands before any write, so a + /// swap is a swap, and where two rows land on one cell the later row wins. + pub(crate) fn merged(rewrites: impl IntoIterator) -> Self { + rewrites.into_iter().fold(Self::default(), |mut all, one| { + all.refused |= one.refused; + all.items.extend(one.items); + all.clears.extend(one.clears); + all.sets.extend(one.sets); + all + }) + } + + pub(crate) fn effects(self) -> Vec> { + let refusal = self.refused.then(|| refuse(RECORD_COORDS_CANNOT_BE_EMPTY)); + let items = self + .items + .into_iter() + .map(|(category, item)| Box::new(AddItem { category, item }) as Box); + let clears = self + .clears + .into_iter() + .map(|key| Box::new(ClearCell(key)) as Box); + let sets = self + .sets + .into_iter() + .map(|(key, value)| Box::new(SetCell(key, value)) as Box); + refusal + .into_iter() + .chain(items) + .chain(clears) + .chain(sets) + .collect() } } @@ -3071,6 +3130,209 @@ mod tests { assert_eq!(app.model_state.workbook.model.get_cell(&key), None); } + // ── Composing a drill's staged edits (improvise-uo2p) ─────────────── + + fn drill_cell(ty: &str, month: &str, measure: &str) -> CellKey { + CellKey::new(vec![ + ("Type".into(), ty.into()), + ("Month".into(), month.into()), + ("_Measure".into(), measure.into()), + ]) + } + + /// Two records, each carrying Revenue and Cost: row 0 is Food/Jan (1, 2), + /// row 1 is Clothing/Jan (42, 17). Every edit below is staged on row 1 at least. + fn two_record_drill() -> App { + let mut app = test_app(); + let cells = [ + (drill_cell("Food", "Jan", "Revenue"), CellValue::Number(1.0)), + (drill_cell("Food", "Jan", "Cost"), CellValue::Number(2.0)), + ( + drill_cell("Clothing", "Jan", "Revenue"), + CellValue::Number(42.0), + ), + ( + drill_cell("Clothing", "Jan", "Cost"), + CellValue::Number(17.0), + ), + ]; + for (key, value) in cells.clone() { + app.model_state.workbook.model.set_cell(key, value); + } + let records = RecordRow::group(cells); + assert_eq!(records.len(), 2, "one row per coordinate tuple"); + assert_eq!(records[1].coords.get("Type"), Some("Clothing")); + StartDrill(records).apply(&mut app); + app + } + + fn stage(app: &mut App, record_idx: usize, col: RecordCol, value: &str) { + SetDrillPendingEdit { + record_idx, + col, + new_value: value.to_string(), + } + .apply(app); + } + + /// The whole store, as `(Type, Month, measure) = n`, so a ghost cannot hide. + fn drill_store(app: &App) -> std::collections::BTreeSet { + app.model_state + .workbook + .model + .data + .iter_cells() + .map(|(key, value)| { + let coord = |c: &str| key.get(c).unwrap_or("-").to_string(); + let n = value.as_f64().expect("every drill cell here is a number"); + format!( + "{}/{}/{} = {n}", + coord("Type"), + coord("Month"), + coord("_Measure") + ) + }) + .collect() + } + + fn category(name: &str) -> RecordCol { + RecordCol::Category(name.into()) + } + + fn measure(name: &str) -> RecordCol { + RecordCol::Measure(name.into()) + } + + /// Stage `edits` and leave the drill, in 32 fresh Apps: each `HashMap` gets its + /// own `RandomState`, and one map iterates the same way twice. + fn assert_drill_commits_to(edits: &[(usize, RecordCol, &str)], expected: &[&str]) { + let expected: std::collections::BTreeSet = + expected.iter().map(|s| s.to_string()).collect(); + for attempt in 0..32 { + let mut app = two_record_drill(); + for (row, col, value) in edits { + stage(&mut app, *row, col.clone(), value); + } + app.apply_effects(vec![Box::new(ApplyAndClearDrill)]); + assert_eq!(drill_store(&app), expected, "fresh App #{attempt}"); + } + } + + #[test] + fn a_drill_row_renamed_twice_and_edited_lands_as_one_record() { + assert_drill_commits_to( + &[ + (1, measure("Cost"), "999"), + (1, category("Type"), "Drink"), + (1, category("Month"), "Feb"), + ], + &[ + "Food/Jan/Revenue = 1", + "Food/Jan/Cost = 2", + "Drink/Feb/Revenue = 42", + "Drink/Feb/Cost = 999", + ], + ); + } + + #[test] + fn a_drill_rows_measure_edits_follow_its_rename() { + assert_drill_commits_to( + &[ + (1, measure("Revenue"), "100"), + (1, measure("Cost"), "999"), + (1, category("Type"), "Drink"), + ], + &[ + "Food/Jan/Revenue = 1", + "Food/Jan/Cost = 2", + "Drink/Jan/Revenue = 100", + "Drink/Jan/Cost = 999", + ], + ); + } + + #[test] + fn a_drill_that_swaps_two_records_coordinates_swaps_them() { + assert_drill_commits_to( + &[ + (0, category("Type"), "Clothing"), + (1, category("Type"), "Food"), + ], + &[ + "Clothing/Jan/Revenue = 1", + "Clothing/Jan/Cost = 2", + "Food/Jan/Revenue = 42", + "Food/Jan/Cost = 17", + ], + ); + } + + #[test] + fn a_drill_edit_addresses_its_own_record_not_the_one_renamed_onto_its_coordinates() { + assert_drill_commits_to( + &[ + (0, category("Type"), "Drink"), + (1, category("Type"), "Food"), + (1, measure("Cost"), "999"), + ], + &[ + "Drink/Jan/Revenue = 1", + "Drink/Jan/Cost = 2", + "Food/Jan/Revenue = 42", + "Food/Jan/Cost = 999", + ], + ); + } + + #[test] + fn a_record_rewrite_is_the_same_under_every_order_of_its_edits() { + let app = two_record_drill(); + let row = app.drill_state.as_ref().unwrap().records[1].clone(); + let edits = [ + (measure("Cost"), "999"), + (category("Type"), "Drink"), + (measure("Revenue"), ""), + (category("Month"), "Feb"), + ]; + let model = &app.model_state.workbook.model; + let mut seen = Vec::new(); + for a in 0..4 { + for b in (0..4).filter(|&b| b != a) { + for c in (0..4).filter(|&c| c != a && c != b) { + let d = 6 - a - b - c; + let order = [a, b, c, d].map(|i| (&edits[i].0, edits[i].1)); + seen.push(RecordRewrite::of(model, &row, order)); + } + } + } + assert_eq!(seen.len(), 24); + let first = &seen[0]; + assert_eq!( + first.clears, + vec![ + drill_cell("Clothing", "Jan", "Revenue"), + drill_cell("Clothing", "Jan", "Cost") + ] + ); + assert_eq!( + first.sets, + vec![(drill_cell("Drink", "Feb", "Cost"), CellValue::Number(999.0))] + ); + assert!(seen.iter().all(|rewrite| rewrite == first)); + } + + #[test] + fn two_drill_rows_renamed_onto_one_record_resolve_to_the_later_row() { + assert_drill_commits_to( + &[ + (0, category("Type"), "Drink"), + (1, category("Type"), "Drink"), + ], + &["Drink/Jan/Revenue = 42", "Drink/Jan/Cost = 17"], + ); + } + // ── Toggle effects ────────────────────────────────────────────────── #[test] @@ -3283,6 +3545,38 @@ mod tests { assert_eq!(app.view_state.status_msg, "Imported successfully"); } + #[test] + fn a_json_import_stores_its_numbers_bit_for_bit() { + const LOSSY: f64 = 0.248_157_142_857_142_85; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("data.json"); + std::fs::write( + &path, + r#"[{"Region":"East","Revenue":0.24815714285714285}]"#, + ) + .unwrap(); + + let mut app = test_app(); + ImportJsonHeadless { + path, + model_name: None, + array_path: None, + } + .apply(&mut app); + + let key = CellKey::new(vec![ + ("Region".to_string(), "East".to_string()), + ("_Measure".to_string(), "Revenue".to_string()), + ]); + let Some(CellValue::Number(n)) = app.model_state.workbook.model.get_cell(&key) else { + panic!( + "the import must store the number; status {:?}", + app.view_state.status_msg + ); + }; + assert_eq!(n.to_bits(), LOSSY.to_bits(), "stored {n:?}, not {LOSSY:?}"); + } + // ── json_import_records (improvise-oaq helper) ────────────────────── #[test] diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index 833d7b9e..126f6315 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -1927,4 +1927,66 @@ mod tests { assert_eq!(grid_value(&mut wb, "Q1", "Food"), None, "no Q1 row"); assert_eq!(column_total(&wb), 100.0); } + + /// improvise-t0ma: the steps themselves, not their translation into effects — + /// see context/test-evidence/core.md. + #[test] + fn records_steps_are_the_spine_then_every_non_index_dim_category_on_none() { + use super::{ViewStep, records_spine, records_steps}; + let mut m = crate::model::Model::new("T"); + m.add_category("Region").unwrap(); + m.add_category("Product").unwrap(); + let none = |c: &str| ViewStep::Axis { + category: c.to_string(), + axis: Axis::None, + }; + let mut expected = records_spine(); + expected.extend( + m.categories + .keys() + .filter(|c| !matches!(c.as_str(), "_Index" | "_Dim")) + .map(|c| none(c)), + ); + assert!( + expected.contains(&none("_Measure")) && expected.contains(&none("Region")), + "setup: the model carries _Measure and a regular category" + ); + assert_eq!(records_steps(&m), expected); + } + + /// improvise-t0ma: a drill FIXES its coordinates on Filter and frees only the + /// unfixed, non-virtual aggregated categories. + #[test] + fn drill_steps_filter_each_coordinate_and_free_the_rest() { + use super::{ViewStep, drill_steps, records_spine}; + let key = CellKey::new(vec![ + ("Region".into(), "North".into()), + ("Month".into(), "Feb".into()), + ("_Measure".into(), "Revenue".into()), + ]); + let none_cats: Vec = ["Product", "Region", "_Index", "Customer"] + .map(String::from) + .to_vec(); + let fixed = |c: &str, i: &str| { + [ + ViewStep::Axis { + category: c.into(), + axis: Axis::Filter, + }, + ViewStep::PageSelection { + category: c.into(), + item: i.into(), + }, + ] + }; + let mut expected = records_spine(); + expected.extend(fixed("Month", "Feb")); + expected.extend(fixed("Region", "North")); + expected.extend(fixed("_Measure", "Revenue")); + expected.extend(["Product", "Customer"].map(|c| ViewStep::Axis { + category: c.into(), + axis: Axis::None, + })); + assert_eq!(drill_steps(&key, &none_cats), expected); + } } diff --git a/crates/improvise-io/Cargo.toml b/crates/improvise-io/Cargo.toml index 1fdfcaf1..1ec96762 100644 --- a/crates/improvise-io/Cargo.toml +++ b/crates/improvise-io/Cargo.toml @@ -17,7 +17,9 @@ indexmap = { version = "2", features = ["serde"] } pest = "2.9.2" pest_derive = "2.9.2" serde = { version = "1", features = ["derive"] } -serde_json = "1" +# Parses JSON a user's numbers cross, so it must read back the f64 written: per crate, +# because each build stands alone (improvise-6yvg, context/code/protocol.md). +serde_json = { version = "1", features = ["float_roundtrip"] } # The browser backend. Only the wasm build pulls these in; the native # binary never sees them (improvise-i34). diff --git a/crates/improvise-io/src/persistence/mod.rs b/crates/improvise-io/src/persistence/mod.rs index ab355b1e..505dfa2c 100644 --- a/crates/improvise-io/src/persistence/mod.rs +++ b/crates/improvise-io/src/persistence/mod.rs @@ -205,6 +205,28 @@ mod tests { assert_eq!(m2.model.name, "Budget"); } + #[test] + fn a_legacy_json_workbook_loads_its_numbers_bit_for_bit() { + use crate::storage::{MemoryFs, Storage}; + const LOSSY: f64 = 0.248_157_142_857_142_85; + let key = coord(&[("Type", "Food"), ("Month", "Jan")]); + let mut wb = two_cat_model(); + wb.model.set_cell(key.clone(), CellValue::Number(LOSSY)); + let json = serde_json::to_string(&wb).unwrap(); + assert!( + json.contains("0.24815714285714285"), + "the file holds the exact text" + ); + let fs = MemoryFs::new(); + let path = std::path::Path::new("legacy.improv"); + fs.write(path, json.as_bytes()).unwrap(); + let loaded = super::load_from(&fs, path).unwrap(); + let Some(CellValue::Number(n)) = loaded.model.get_cell(&key) else { + panic!("the cell must load as a number"); + }; + assert_eq!(n.to_bits(), LOSSY.to_bits(), "loaded {n:?}, not {LOSSY:?}"); + } + // ── save/load roundtrip via file ──────────────────────────────────── #[test] diff --git a/crates/improvise-protocol/Cargo.toml b/crates/improvise-protocol/Cargo.toml index c114f10c..62c4a498 100644 --- a/crates/improvise-protocol/Cargo.toml +++ b/crates/improvise-protocol/Cargo.toml @@ -14,7 +14,8 @@ serde = { version = "1", features = ["derive"] } # `char`, and the TUI has always measured it this way. unicode-width = "^0.2" # Only for `variants`, which is off unless a test build asks for it. -serde_json = { version = "1", optional = true } +# float_roundtrip because every normal serde_json dep carries it (improvise-dz8c). +serde_json = { version = "1", optional = true, features = ["float_roundtrip"] } [features] # `variants` reaches OUT of this crate: improvise-command bounds its own diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index 724a373c..117f4fde 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -304,14 +304,11 @@ pub enum HelpLine { Heading(&'static str), Text(&'static str), Accent(&'static str), - /// A keys-and-description row whose keys came off the keymap, so it - /// cannot go stale against the bindings it documents. - Keys { - keys: Vec, - description: &'static str, - }, - /// A row naming something the keymap does not own: an ex-command, or a - /// prefix sequence, which is two addresses and not one binding. + /// A keys-and-description row whose keys came off the keymap. A [`KeyRow`] + /// has no public constructor, so no builder can type one (improvise-v29m). + Keys(KeyRow), + /// An ex-command, which the keymap does not own. Built only by `ex!`, whose + /// colon is what stops a key being typed here (improvise-wk18). Literal { keys: &'static str, description: &'static str, @@ -326,38 +323,127 @@ pub struct HelpPage { pub lines: Vec, } -/// A keys row, looked up in `mode`. An unbound command yields no keys and the -/// row is dropped, so the page never advertises a key that does not exist. -fn keys( - mode: &ModeKey, - command: &str, - args: Option<&[&str]>, - description: &'static str, -) -> Option { - let args: Option> = args.map(|a| a.iter().map(|s| (*s).to_string()).collect()); - let found = keys_for(mode, command, args.as_deref()); - match found.is_empty() { - true => None, - false => Some(HelpLine::Keys { - keys: found.iter().map(|(c, m)| key_label(c, *m)).collect(), - description, - }), +pub use key_row::KeyRow; +use key_row::keys; + +/// The one door to a keys row: its fields are private to this module, and its +/// only constructor looks the keys up. +mod key_row { + use super::{HelpLine, ModeKey, key_label, keys_for, prefixed_labels}; + + #[derive(Debug, Clone, PartialEq, Eq)] + pub struct KeyRow { + keys: Vec, + description: &'static str, + } + + impl KeyRow { + pub fn keys(&self) -> &[String] { + &self.keys + } + + pub fn description(&self) -> &'static str { + self.description + } + + /// An unbound command yields no keys and no row, so the page never + /// advertises a key that does not exist. + fn looked_up( + mode: &ModeKey, + command: &str, + args: Option<&[String]>, + description: &'static str, + ) -> Option { + let mut keys: Vec = keys_for(mode, command, args) + .iter() + .map(|(c, m)| key_label(c, *m)) + .collect(); + keys.extend(prefixed_labels(mode, command, args)); + (!keys.is_empty()).then_some(Self { keys, description }) + } + } + + /// A keys row, looked up in `mode`: the one caller of [`KeyRow::looked_up`], + /// and the lookup `pages` hands every builder (improvise-ssoe). + pub(super) fn keys( + mode: &ModeKey, + command: &'static str, + args: Option<&[&str]>, + description: &'static str, + ) -> Option { + let args: Option> = args.map(|a| a.iter().map(|s| (*s).to_string()).collect()); + KeyRow::looked_up(mode, command, args.as_deref(), description).map(HelpLine::Keys) } } -fn literal(keys: &'static str, description: &'static str) -> HelpLine { - HelpLine::Literal { - keys, - description, - dim: false, +/// `gg`, `yy`, `ZZ`: a prefix key and the key after it, both off the keymap. +fn prefixed_labels(mode: &ModeKey, command: &str, args: Option<&[String]>) -> Vec { + let mut labels: Vec = match Keymaps::shared().get(mode) { + Some(keymap) => keymap.prefixed_keys_running(command, args), + None => Vec::new(), } + .iter() + .map(|[(c1, m1), (c2, m2)]| format!("{}{}", key_label(c1, *m1), key_label(c2, *m2))) + .collect(); + labels.sort(); + labels.dedup(); + labels } -fn dim(keys: &'static str, description: &'static str) -> HelpLine { - HelpLine::Literal { - keys, - description, - dim: true, +/// How a page asks for a keys row. `pages` answers with [`keys`]; a test +/// answers by recording the question, so a guard reads commands, not text. +type KeysRow<'a> = + &'a dyn Fn(&ModeKey, &'static str, Option<&[&str]>, &'static str) -> Option; + +/// An ex-command row. The colon is the macro's, and every word after the +/// command's name must be a placeholder, checked when the page compiles. +macro_rules! ex { + ($text:literal, $description:expr $(,)?) => {{ + const _: () = assert!(ex_usage_is_placeholders($text)); + HelpLine::Literal { + keys: concat!(":", $text), + description: $description, + dim: false, + } + }}; + (dim $text:literal, $description:expr $(,)?) => {{ + const _: () = assert!(ex_usage_is_placeholders($text)); + HelpLine::Literal { + keys: concat!(" :", $text), + description: $description, + dim: true, + } + }}; +} + +/// After the name, every word opens with `<`, `[` or `"`, or is `...`: so an +/// ex row cannot also advertise a key ("q or press Q"). Quotes span spaces. +const fn ex_usage_is_placeholders(text: &str) -> bool { + let b = text.as_bytes(); + let mut i = 0; + while i < b.len() && b[i] != b' ' { + i += 1; + } + loop { + while i < b.len() && b[i] == b' ' { + i += 1; + } + if i == b.len() { + return true; + } + let start = i; + let mut quoted = false; + while i < b.len() && (quoted || b[i] != b' ') { + if b[i] == b'"' { + quoted = !quoted; + } + i += 1; + } + let ellipsis = + i - start == 3 && b[start] == b'.' && b[start + 1] == b'.' && b[start + 2] == b'.'; + if !(ellipsis || b[start] == b'<' || b[start] == b'[' || b[start] == b'"') { + return false; + } } } @@ -367,30 +453,48 @@ pub fn pages() -> Vec { let pages = vec![ HelpPage { title: "Welcome", - lines: welcome(), + lines: welcome(&keys), }, HelpPage { title: "Navigation", - lines: navigation(), + lines: navigation(&keys), }, HelpPage { title: "Editing", - lines: editing(), + lines: editing(&keys), }, HelpPage { title: "Panels & Views", - lines: panels(), + lines: panels(&keys), }, HelpPage { title: "Commands", - lines: commands(), + lines: commands(&keys), }, ]; debug_assert_eq!(pages.len(), HELP_PAGE_COUNT); pages } -fn welcome() -> Vec { +/// Every keys row the pages ask for, as `(mode, command, args, description)` +/// — the same builders, so a guard over it cannot drift from what is drawn. +#[cfg(test)] +pub(crate) fn help_bindings() -> Vec<(ModeKey, &'static str, Option>, &'static str)> { + let asked = std::cell::RefCell::new(Vec::new()); + let record: KeysRow = &|mode, command, args, description| { + let args = args.map(|a| a.iter().map(|s| (*s).to_string()).collect()); + asked + .borrow_mut() + .push((mode.clone(), command, args, description)); + None + }; + for build in [welcome, navigation, editing, panels, commands] { + build(record); + } + asked.into_inner() +} + +fn welcome(_: KeysRow) -> Vec { vec![ HelpLine::Blank, HelpLine::Banner(" improvise"), @@ -427,12 +531,10 @@ fn welcome() -> Vec { ), HelpLine::Text(" Tile The row/column/page assignment of a category."), HelpLine::Text(" Formula A computed item: derives its value from other items."), - HelpLine::Blank, - dim("Tip: press Tab or l/n to go to the next page.", ""), ] } -fn navigation() -> Vec { +fn navigation(keys: KeysRow) -> Vec { let n = &ModeKey::Normal; [ Some(HelpLine::Blank), @@ -445,7 +547,7 @@ fn navigation() -> Vec { None, "Move down, wrapping to the next column", ), - Some(literal("gg", "Jump to first row")), + keys(n, "jump-first-row", None, "Jump to first row"), keys(n, "jump-last-row", None, "Jump to last row"), keys(n, "jump-first-col", None, "Jump to first column"), keys(n, "jump-last-col", None, "Jump to last column"), @@ -512,7 +614,7 @@ fn navigation() -> Vec { .collect() } -fn editing() -> Vec { +fn editing(keys: KeysRow) -> Vec { let n = &ModeKey::Normal; let e = &ModeKey::Editing; [ @@ -555,7 +657,7 @@ fn editing() -> Vec { Some(HelpLine::Blank), Some(HelpLine::Heading("Copy and paste")), Some(HelpLine::Blank), - Some(literal("yy", "Yank (copy) the current cell value")), + keys(n, "yank", None, "Yank (copy) the current cell value"), keys( n, "paste", @@ -593,7 +695,7 @@ fn editing() -> Vec { .collect() } -fn panels() -> Vec { +fn panels(keys: KeysRow) -> Vec { let n = &ModeKey::Normal; [ Some(HelpLine::Blank), @@ -741,14 +843,14 @@ fn panels() -> Vec { "Toggle collapse of nearest group above cursor", ), keys(n, "hide-selected-row-item", None, "Hide current row item"), - Some(dim(" :show-item ", "Restore a hidden item")), + Some(ex!(dim "show-item ", "Restore a hidden item")), ] .into_iter() .flatten() .collect() } -fn commands() -> Vec { +fn commands(keys: KeysRow) -> Vec { let n = &ModeKey::Normal; [ Some(HelpLine::Blank), @@ -763,55 +865,40 @@ fn commands() -> Vec { Some(HelpLine::Blank), Some(HelpLine::Heading("File operations")), Some(HelpLine::Blank), - Some(literal( - ":w [path]", - "Save (path optional after first save)", - )), - Some(literal(":wq", "Save and quit")), - Some(literal(":q", "Quit (warns if unsaved changes)")), - Some(literal(":q!", "Force quit without saving")), - Some(literal("ZZ", "Save and quit (same as :wq)")), + Some(ex!("w [path]", "Save (path optional after first save)",)), + Some(ex!("wq", "Save and quit")), + Some(ex!("q", "Quit (warns if unsaved changes)")), + Some(ex!("q!", "Force quit without saving")), + keys(n, "wq", None, "Save and quit (same as :wq)"), keys(n, "save", None, "Save (same as :w)"), Some(HelpLine::Blank), Some(HelpLine::Heading("Import and export")), Some(HelpLine::Blank), - Some(literal(":import ", "Open the JSON/CSV import wizard")), - Some(literal( - ":export [path.csv]", - "Export the active view to CSV", - )), + Some(ex!("import ", "Open the JSON/CSV import wizard")), + Some(ex!("export [path.csv]", "Export the active view to CSV",)), Some(HelpLine::Blank), Some(HelpLine::Heading("Model building")), Some(HelpLine::Blank), - Some(literal(":add-cat ", "Add a new category")), - Some(literal( - ":add-item ", - "Add one item to a category", - )), - Some(literal( - ":add-items a b c ...", + Some(ex!("add-cat ", "Add a new category")), + Some(ex!("add-item ", "Add one item to a category",)), + Some(ex!( + "add-items ...", "Add multiple items at once", )), - Some(literal( - ":formula \" = \"", - "Add a formula", - )), + Some(ex!("formula \" = \"", "Add a formula",)), Some(HelpLine::Blank), Some(HelpLine::Heading("Views")), Some(HelpLine::Blank), - Some(literal(":add-view [name]", "Create a new view")), + Some(ex!("add-view [name]", "Create a new view")), Some(HelpLine::Blank), Some(HelpLine::Heading("Display")), Some(HelpLine::Blank), - Some(literal( - ":set-format ", - "Set number format (e.g. ',.2')", - )), + Some(ex!("set-format ", "Set number format (e.g. ',.2')",)), Some(HelpLine::Blank), Some(HelpLine::Heading("Other keys")), Some(HelpLine::Blank), keys(n, "enter-mode", Some(&["help"]), "Open this help screen"), - Some(literal(":help", "Open this help screen")), + Some(ex!("help", "Open this help screen")), ] .into_iter() .flatten() @@ -884,8 +971,12 @@ mod tests { for page in &pages { assert!(!page.title.is_empty()); for line in &page.lines { - if let HelpLine::Keys { keys, description } = line { - assert!(!keys.is_empty(), "{description} resolved to no keys"); + if let HelpLine::Keys(row) = line { + assert!( + !row.keys().is_empty(), + "{} resolved to no keys", + row.description() + ); rows += 1; } } @@ -896,6 +987,84 @@ mod tests { ); } + /// improvise-wk18: a key the pages name is looked up, never typed. The only + /// hand-written row left is an ex-command. See `context/test-evidence/protocol.md`. + #[test] + fn no_help_row_spells_a_key_by_hand() { + let spelled: Vec<&str> = pages() + .iter() + .flat_map(|page| page.lines.iter()) + .filter_map(|line| match line { + HelpLine::Literal { keys, .. } => Some(*keys), + _ => None, + }) + .filter(|keys| !keys.trim_start().starts_with(':')) + .collect(); + assert!( + spelled.is_empty(), + "these rows name keys the keymap never saw: {spelled:?}" + ); + } + + /// improvise-v29m: `ex!` refuses a usage with a bare word in it at compile + /// time; this pins the rule it asks. See `context/test-evidence/protocol.md`. + #[test] + fn an_ex_row_can_name_placeholders_and_nothing_else() { + for usage in [ + "w [path]", + "q!", + "formula \" = \"", + "add-items ...", + ] { + assert!(ex_usage_is_placeholders(usage), "{usage} is refused"); + } + for usage in ["q or press Q", "add-items a b c", "help ?"] { + assert!(!ex_usage_is_placeholders(usage), "{usage} is accepted"); + } + } + + /// improvise-ssoe: a keys row drawn but never asked for through the builders' + /// parameter is invisible to every guard. See `context/test-evidence/protocol.md`. + #[test] + fn every_keys_row_drawn_is_one_the_builders_asked_for() { + let drawn: Vec = pages() + .into_iter() + .flat_map(|page| page.lines) + .filter(|line| matches!(line, HelpLine::Keys(_))) + .collect(); + // Content, in order, not a count: a bypass that also drops a real row + // keeps the count and changes this (improvise-aszt). + let asked: Vec = help_bindings() + .iter() + .filter_map(|(mode, command, args, description)| { + let args: Option> = args + .as_ref() + .map(|a| a.iter().map(String::as_str).collect()); + keys(mode, command, args.as_deref(), description) + }) + .collect(); + assert_eq!(drawn, asked); + } + + /// A row whose lookup finds no key is dropped silently, so a broken lookup + /// looks like a shorter page. See `context/test-evidence/protocol.md`. + #[test] + fn every_keys_row_the_pages_ask_for_finds_its_keys() { + let asked = help_bindings(); + assert!(asked.len() > 20, "only {} rows asked", asked.len()); + let dropped: Vec = asked + .iter() + .filter(|(mode, command, args, description)| { + let args: Option> = args + .as_ref() + .map(|a| a.iter().map(String::as_str).collect()); + keys(mode, command, args.as_deref(), description).is_none() + }) + .map(|(mode, command, _, _)| format!("{mode:?} {command}")) + .collect(); + assert!(dropped.is_empty(), "rows with no key: {dropped:?}"); + } + /// `compact`'s three shapes, including the one no shipped mode reaches: /// keys that are all chords leave nothing to draw. #[test] @@ -966,7 +1135,7 @@ mod tests { .into_iter() .flat_map(|page| page.lines) .filter_map(|line| match line { - HelpLine::Keys { keys, description } => Some((keys, description)), + HelpLine::Keys(row) => Some((row.keys().to_vec(), row.description())), _ => None, }) .collect() @@ -998,16 +1167,26 @@ mod tests { /// See `context/test-evidence/clients.md`. #[test] fn only_the_edit_key_is_captioned_as_editing_the_cell() { - let edit = labels_of("edit-or-drill", Some(&["editing".to_string()])); - let claims: Vec<_> = rows() + let normal: Vec<_> = help_bindings() .into_iter() - .filter(|(_, d)| d.contains("Edit current cell")) + .filter(|(mode, ..)| *mode == ModeKey::Normal) .collect(); - assert!(!claims.is_empty(), "the check needs a row to check"); - for (keys, description) in claims { + // By COMMAND, not phrase (improvise-jhdf): any wording of "edit" on a + // Normal row that is not edit-or-drill's is the lie s8h7 removed. + let liars: Vec<_> = normal + .iter() + .filter(|(_, command, _, d)| { + d.to_lowercase().contains("edit") && *command != "edit-or-drill" + }) + .collect(); + assert!( + liars.is_empty(), + "captioned as editing but do not: {liars:?}" + ); + for command in ["edit-or-drill", "enter-advance"] { assert!( - keys.iter().all(|k| edit.contains(k)), - "{keys:?} is captioned {description:?} but only {edit:?} edit" + normal.iter().any(|(_, c, ..)| *c == command), + "{command} has no Normal help row, so the check above cannot see it" ); } } @@ -1024,8 +1203,8 @@ mod tests { println!("\n=== {} ===", page.title); for line in &page.lines { match line { - HelpLine::Keys { keys, description } => { - println!(" {:<28}{description}", keys.join(" ")) + HelpLine::Keys(row) => { + println!(" {:<28}{}", row.keys().join(" "), row.description()) } HelpLine::Literal { keys, description, .. diff --git a/crates/improvise-protocol/src/keymap.rs b/crates/improvise-protocol/src/keymap.rs index a375048b..6b7aef43 100644 --- a/crates/improvise-protocol/src/keymap.rs +++ b/crates/improvise-protocol/src/keymap.rs @@ -235,6 +235,32 @@ impl Keymap { keys } + /// Every two-key sequence here or in a parent that runs `command`: a + /// prefix key, then a key of the sub-keymap it opens (flat, gotcha 13). + pub fn prefixed_keys_running( + &self, + command: &str, + args: Option<&[String]>, + ) -> Vec<[(KeyCode, KeyModifiers); 2]> { + let mut keys: Vec<[(KeyCode, KeyModifiers); 2]> = self + .bindings + .iter() + .filter_map(|(pattern, binding)| match (pattern, binding) { + (KeyPattern::Key(code, mods), Binding::Prefix(sub)) => Some((*code, *mods, sub)), + _ => None, + }) + .flat_map(|(code, mods, sub)| { + sub.keys_running(command, args) + .into_iter() + .map(move |second| [(code, mods), second]) + }) + .collect(); + if let Some(parent) = &self.parent { + keys.extend(parent.prefixed_keys_running(command, args)); + } + keys + } + /// Look up the binding for a key in this keymap's own bindings. fn lookup_local(&self, key: KeyCode, mods: KeyModifiers) -> Option<&Binding> { self.bindings diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 0e5d0c6e..d88b4e23 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -619,6 +619,11 @@ enum GridMove { Page(i32), } +/// Rows one `page-scroll` step moves: three quarters of the window, at least one. +fn page_rows(window_rows: usize) -> i32 { + (window_rows as i32 * 3 / 4).max(1) +} + fn grid_move(state: &ClientState, kind: GridMove) -> Option> { let GridExtent { rows, cols } = state.grid.extent; let (window_rows, window_cols) = state.window; @@ -641,7 +646,7 @@ fn grid_move(state: &ClientState, kind: GridMove) -> Option> { GridMove::ToEnd(true) => (row_max as usize, cursor.selected.1), GridMove::ToEnd(false) => (cursor.selected.0, col_max as usize), GridMove::Page(dir) => { - let delta = (window_rows as i32 * 3 / 4).max(1) * dir; + let delta = page_rows(window_rows) * dir; ((row + delta).clamp(0, row_max) as usize, cursor.selected.1) } }; @@ -2044,18 +2049,60 @@ mod tests { // ── what is bound and what is not ─────────────────────────────── + /// A three-by-three grid with `id` at (1,1) and the cursor on it: no move + /// from there is clamped by an edge (improvise-6562). + fn centred_on(id: CellId) -> ClientState { + let mut centre = CellDisplay::number("1200"); + centre.id = Some(id); + let mut cells: Vec<_> = (0..3) + .flat_map(|r| (0..3).map(move |c| ((r, c), CellDisplay::number("7")))) + .collect(); + cells[4].1 = centre; + let mut client = ClientState::default(); + client.bootstrap(Snapshot { + view: ViewState::default(), + seed: vec![ + Projection::Grid(GridUpdate::RowFrame(AxisFrame::flat([ + "North", "South", "East", + ]))), + Projection::Grid(GridUpdate::ColFrame(AxisFrame::flat([ + "Revenue", "Cost", "Profit", + ]))), + Projection::Grid(GridUpdate::Cells(cells)), + Projection::Grid(GridUpdate::Extent(GridExtent { rows: 3, cols: 3 })), + Projection::Basis(published()), + ], + }); + client.window = (3, 3); + client.view.cursor_mut().selected = (1, 1); + client + } + /// A client whose every widget has something in it, so an `Upstream` names /// an **unhandled command** and never a state that said no (improvise-rmvx). fn furnished(mode: AppMode) -> ClientState { - let mut state = client_on(record_cell()); + furnished_on(mode, record_cell()) + } + + /// `furnished`, with `cell` under the cursor. A history in both directions + /// and a yanked value, so no `Cmd` is refused by an empty stack (3t5x). + fn furnished_on(mode: AppMode, cell: CellId) -> ClientState { + let mut state = centred_on(cell); + let frame = |name: &str| crate::ViewFrame { + view_name: name.into(), + mode: AppMode::Normal, + }; + state.view.view_back_stack = vec![frame("Second")]; + state.view.view_forward_stack = vec![frame("Third")]; + state.view.yanked = Some(CellValue::Number(5.0)); // A records MODE over a non-records GRID is a state no host can // produce, so the fixture projects both (improvise-e5pf). crate::WidgetCache::apply(&mut state.grid, &GridUpdate::Records(mode.is_records())); state.view.mode = mode; state.grid.page_coords = vec![crate::PageAxis { cat: "Quarter".into(), - selected: "Q1".into(), - items: Some(vec!["Q1".into(), "Q2".into()]), + selected: "Q2".into(), + items: Some(vec!["Q1".into(), "Q2".into(), "Q3".into()]), }]; // THREE of everything, every panel cursor seated in the MIDDLE: an edge // makes `move-panel-cursor` answer `Ignored` for the wrong reason. @@ -2087,10 +2134,15 @@ mod tests { axes: Vec::new(), }) .collect(); - state.tiles.tiles = vec![crate::TileEntry { - category: "Region".into(), - axis: Axis::Row, - }]; + state.tiles.tiles = ["Region", "Product", "Quarter"] + .into_iter() + .map(|category| crate::TileEntry { + category: category.into(), + axis: Axis::Row, + }) + .collect(); + state.view.tile_cat_idx = 1; + state.view.help_page = 1; state.view.cat_panel_cursor = 1; state.view.formula_cursor = 1; state.view.view_panel_cursor = 1; @@ -2107,59 +2159,298 @@ mod tests { state } - /// improvise-rmvx. An advertised key that resolves upstream is a black hole - /// (gotcha 47). See `context/test-evidence/clients.md`. + /// What a user sees an advertised key do. A status alone is `Declines`: + /// honest, but not what the caption promised (improvise-3t5x). + #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] + enum Heard { + Acts, + Declines, + Silent, + } + + /// Judged by what the fold MOVED, never by a command's type: a clamped move + /// or a lone disarm changes nothing (improvise-6562). `Model` is sent. + fn heard(state: &mut ClientState, outcome: KeyOutcome) -> Heard { + let commands = match outcome { + KeyOutcome::Upstream(_) | KeyOutcome::Ignored => return Heard::Silent, + KeyOutcome::Request(_) => return Heard::Acts, + KeyOutcome::Local(commands) => commands, + }; + let quiet = |view: &ViewState| ViewState { + status_msg: String::new(), + transient_prefix: None, + ..view.clone() + }; + let before = quiet(&state.view); + commands.iter().for_each(|c| state.fold(c)); + let sent = commands.iter().any(|c| matches!(c, Command::Model { .. })); + let said = commands.iter().any(|c| matches!(c, Command::SetStatus(_))); + match (sent || quiet(&state.view) != before, said) { + (true, _) => Heard::Acts, + (false, true) => Heard::Declines, + (false, false) => Heard::Silent, + } + } + + /// Every `(mode, search, command, args)` either table advertises: the + /// status-bar hint and the help pages (improvise-wk18). + fn advertised_everywhere() -> Vec<(AppMode, bool, &'static str, Option>)> { + let mut rows = Vec::new(); + for mode in AppMode::all() { + for (command, args, _) in crate::help::advertised(&mode) { + rows.push((mode.clone(), false, command, args)); + } + } + for (key, command, args, _) in crate::help::help_bindings() { + let (mode, search) = match key { + ModeKey::SearchMode => (AppMode::Normal, true), + key => match AppMode::all() + .into_iter() + .find(|m| ModeKey::from_app_mode(m, false).as_ref() == Some(&key)) + { + Some(mode) => (mode, false), + None => panic!("a help row names {key:?}, which no mode reaches"), + }, + }; + rows.push((mode, search, command, args)); + } + rows.sort_by_key(|(m, s, c, a)| (crate::mode_label(m), *s, *c, a.clone())); + rows.dedup(); + rows + } + + /// Every key sequence that runs `command` in `keymap`: one key, or a + /// prefix and the key after it. + fn sequences_running( + keymap: &Keymap, + command: &str, + args: Option<&[String]>, + ) -> Vec> { + let one = keymap + .keys_running(command, args) + .into_iter() + .map(|(code, mods)| vec![KeyEvent::new(code, mods)]); + let two = keymap + .prefixed_keys_running(command, args) + .into_iter() + .map(|pair| pair.map(|(code, mods)| KeyEvent::new(code, mods)).to_vec()); + one.chain(two).collect() + } + + /// What pressing `sequence` in `state` does, arming any prefix first. + fn press(mut state: ClientState, sequence: &[KeyEvent]) -> Heard { + let (last, prefix) = sequence.split_last().expect("a sequence has a key"); + for key in prefix { + if let KeyOutcome::Local(commands) = resolve(&state, *key) { + commands.iter().for_each(|c| state.fold(c)); + } + } + let outcome = resolve(&state, *last); + heard(&mut state, outcome) + } + + /// improvise-rmvx, wk18, 3t5x. Every advertised key does what it says, or + /// is named below saying why not. See `context/test-evidence/clients.md`. #[test] fn every_advertised_command_answers_in_its_own_mode() { - let mut unanswered = Vec::new(); - for mode in AppMode::all() { - let Some(mode_key) = ModeKey::from_app_mode(&mode, false) else { - continue; + let aggregate = CellId::Aggregate( + serde_json::from_str("\"aa4c8e5e9cbd578e0d388f4b2b5a7879\"").unwrap(), + ); + let derived = CellId::Derived { + target: "Profit".into(), + }; + // (mode, cell, command, heard) -> the keys that heard it. + let mut short: std::collections::BTreeMap<(String, &str, Heard), Vec> = + Default::default(); + for (mode, search, command, args) in advertised_everywhere() { + let key = ModeKey::from_app_mode(&mode, search).expect("a keyed mode"); + let keymap = Keymaps::shared().get(&key).expect("a keymap"); + let at = |cell: &CellId| { + let mut state = furnished_on(mode.clone(), cell.clone()); + state.view.search_mode = search; + state }; - let Some(keymap) = Keymaps::shared().get(&mode_key) else { - continue; - }; - let state = furnished(mode.clone()); - for (command, args, caption) in crate::help::advertised(&mode) { - for (code, mods) in keymap.keys_running(command, args.as_deref()) { - let event = KeyEvent::new(code, mods); - // BOTH silent variants. To a user `Ignored` is the same - // black hole as `Upstream` — nothing is on the screen. - let answered = !matches!( - resolve(&state, event), - KeyOutcome::Upstream(_) | KeyOutcome::Ignored - ); - if !answered { - unanswered.push(format!( - "{}: {:?} → `{command} {args:?}` ({caption})", - crate::mode_label(&mode), - code - )); + let label = crate::mode_label(&mode).to_string() + if search { " SEARCH" } else { "" }; + for sequence in sequences_running(keymap, command, args.as_deref()) { + let keys: String = sequence + .iter() + .map(|k| crate::keymap::key_label(&k.code, k.modifiers)) + .collect(); + let on_record = press(at(&record_cell()), &sequence); + let mut report = vec![(label.clone(), on_record)]; + // Only a grid mode stands on another kind of cell, and a cell + // matters only where it changes the answer. + if mode == AppMode::Normal { + for (on, cell) in [ + (" on an aggregate", &aggregate), + (" on a derived cell", &derived), + ] { + let got = press(at(cell), &sequence); + if got != on_record { + report.push((label.clone() + on, got)); + } } } + for (where_, got) in report.into_iter().filter(|(_, got)| *got != Heard::Acts) { + short + .entry((where_, command, got)) + .or_default() + .push(keys.clone()); + } } } - unanswered.sort(); - unanswered.dedup(); - // The five that remain, each with the reason it is not a black hole. - // Closing one has to come here and say so; opening a sixth fails. - let expected = [ + let short: Vec = short + .into_iter() + .map(|((at, command, got), mut keys)| { + keys.sort(); + keys.dedup(); + format!("{at}: {} {command} {got:?}", keys.join("/")) + }) + .collect(); + // Closing one has to come here and say so; adding one fails. A caption + // is a promise, and each line here is a promise a browser does not keep. + let expected: &[&str] = &[ // Reached only from a mode `declined` refuses at the door, so a - // browser is never standing in front of this hint. - "COMMAND: Enter → `execute-command None` (run)", - "EXPORT: Enter → `commit-export None` (export)", - "SAVE AS: Enter → `commit-save None` (save)", + // browser is never standing in front of these. + "COMMAND: Enter execute-command Silent", + "EXPORT: Enter commit-export Silent", + "SAVE AS: Enter commit-save Silent", + "NORMAL SEARCH: Enter/Esc exit-search-mode Silent", + // Commands the browser has no arm for: upstream, so a black hole + // (improvise-uln5). Each is also in the host-only list below. + "NORMAL: Enter enter-advance Silent", + "NORMAL: H hide-selected-row-item Silent", + "NORMAL: / search Silent", + "NORMAL: n search-navigate Silent", + "NORMAL: z toggle-group-under-cursor Silent", + // Honestly unavailable, and saying so in `declined`'s words. + "FORMULAS: Del/d delete-formula-at-cursor Declines", + "NORMAL: > drill-into-cell Declines", + "NORMAL: : enter-mode Declines", + "NORMAL: P toggle-prune-empty Declines", + "NORMAL: R toggle-records-mode Declines", + "NORMAL: ZZ wq Declines", + "RECORDS: o add-record-row Declines", + "RECORDS: a/i edit-or-drill Declines", + "RECORDS: : enter-mode Declines", + "RECORDS: P toggle-prune-empty Declines", + "RECORDS: R toggle-records-mode Declines", + // `i` on an aggregate IS drill, which the browser lacks (s8h7); on a + // derived cell the browser names the formula instead (ir7l). + "NORMAL on an aggregate: a/i edit-or-drill Declines", + "NORMAL on a derived cell: a/i edit-or-drill Declines", + // No record under the cursor to write. The TUI writes onto the + // aggregate's partial key, which is its own bug (improvise-l4mp). + "NORMAL on an aggregate: p paste Silent", + "NORMAL on an aggregate: x clear-cell Silent", + "NORMAL on a derived cell: p paste Silent", + "NORMAL on a derived cell: x clear-cell Silent", // A records cell commit needs `record_col` (gotcha 1); only the two // creating columns resolve, and this fixture is on a record cell. - "RECORDS INSERT: Enter → `commit-cell-edit None` (commit)", - "RECORDS INSERT: Tab → `commit-and-advance-right None` (commit+right)", + "RECORDS INSERT: Tab commit-and-advance-right Silent", + "RECORDS INSERT: Enter commit-cell-edit Silent", ]; let mut expected: Vec = expected.iter().map(|s| s.to_string()).collect(); expected.sort(); + let mut short = short; + short.sort(); assert_eq!( - unanswered, expected, - "advertised and unanswerable: anything here that is not in the \ - named list above reads as broken rather than as unavailable" + short, + expected, + "advertised keys that do not do what they say, in full:\n{}", + short.join("\n") + ); + } + + /// improvise-6562: "it moved" cannot see a move the WRONG way, so every grid + /// key is pressed from the centre and must land where it says. Evidence file. + #[test] + fn every_grid_move_lands_where_its_binding_says() { + let keymap = Keymaps::shared().get(&ModeKey::Normal).expect("Normal"); + let mut bound = Vec::new(); + bound_commands(keymap, &mut bound); + bound.sort(); + bound.dedup(); + // Far from every edge of a grid far larger than any step, so a landing + // shows magnitude as well as sign (improvise-5kdr); a jump's is its name. + const START: (usize, usize) = (10, 10); + const WINDOW: (usize, usize) = (8, 8); + let d = |a: &String| a.parse::().expect("a delta"); + let at = |base: usize, by: isize| base.checked_add_signed(by).expect("inside the grid"); + let page = page_rows(WINDOW.0) as isize; + let destination = |name: &str, args: &[String]| match name { + "move-selection" => Some((at(START.0, d(&args[0])), at(START.1, d(&args[1])))), + "scroll-rows" => Some((at(START.0, d(&args[0])), START.1)), + "page-scroll" => Some((at(START.0, page * d(&args[0])), START.1)), + "jump-first-row" => Some((0, START.1)), + "jump-last-row" => Some((20, START.1)), + "jump-first-col" => Some((START.0, 0)), + "jump-last-col" => Some((START.0, 20)), + _ => None, + }; + let mut unstated = Vec::new(); + let mut unpressed = Vec::new(); + let mut pressed = std::collections::BTreeSet::new(); + for (name, args) in &bound { + let to = destination(name, args); + let sequences = sequences_running(keymap, name, Some(args.as_slice())); + if to.is_some() && sequences.is_empty() { + unpressed.push(format!("{name} {args:?}")); + } + for sequence in sequences { + let mut state = furnished(AppMode::Normal); + state.grid.extent = GridExtent { rows: 21, cols: 21 }; + state.window = WINDOW; + *state.view.cursor_mut() = crate::ViewCursor { + selected: START, + row_offset: 5, + col_offset: 5, + }; + let view = state.view.active_view.clone(); + for key in &sequence { + if let KeyOutcome::Local(commands) = resolve(&state, *key) { + commands.iter().for_each(|c| state.fold(c)); + } + } + let landed = state.view.cursor().selected; + if to.is_some() { + pressed.insert(name.as_str()); + } + match to { + Some(to) => { + assert_eq!( + landed, to, + "{sequence:?} runs `{name} {args:?}` from {START:?}" + ) + } + None if state.view.active_view == view && landed != START => { + unstated.push(format!("{name} {args:?} -> {landed:?}")) + } + None => {} + } + } + } + assert!( + unpressed.is_empty(), + "stated and never pressed: {unpressed:?}" + ); + assert!( + unstated.is_empty(), + "these move the cursor and nothing states where to: {unstated:?}" + ); + let every = [ + "jump-first-col", + "jump-first-row", + "jump-last-col", + "jump-last-row", + "move-selection", + "page-scroll", + "scroll-rows", + ]; + assert_eq!( + pressed.into_iter().collect::>(), + every, + "each kind is pressed" ); } @@ -2431,8 +2722,8 @@ mod tests { not vacuous over a table that binds everything or nothing" ); assert_eq!( - composed(&state, ev(KeyCode::Char(',')), "view-forward", &[]), - Some(vec![Command::ViewForward]), + resolve(&state, ev(KeyCode::Char(','))), + KeyOutcome::Local(vec![Command::ViewForward]), "and the key is `,` — the unshifted twin of `<`, so both halves of \ the history sit on one physical key" ); diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index 8aa220b4..24ca5e93 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -427,9 +427,9 @@ impl RenderHelpRow { L::Heading(t) => prose("heading", t), L::Text(t) => prose("text", t), L::Accent(t) => prose("accent", t), - L::Keys { keys, description } => Self { - keys: keys.join(" "), - description: description.to_string(), + L::Keys(row) => Self { + keys: row.keys().join(" "), + description: row.description().to_string(), kind: "keys", }, L::Literal { diff --git a/crates/improvise-wasm-client/tests/browser_round_trip.rs b/crates/improvise-wasm-client/tests/browser_round_trip.rs index 56ae17a9..adbc2c7f 100644 --- a/crates/improvise-wasm-client/tests/browser_round_trip.rs +++ b/crates/improvise-wasm-client/tests/browser_round_trip.rs @@ -707,44 +707,6 @@ fn a_yanked_number_crosses_the_serialized_wire_exactly() { ); } -/// The two tests around this one share a feature-unified build, so neither can -/// see ONE frame-parsing crate lacking `float_roundtrip`. Evidence file. -#[test] -fn every_build_that_parses_a_frame_enables_float_roundtrip() { - for (krate, target) in [ - ("improvise-wasm-client", Some("wasm32-unknown-unknown")), - ("improvise-worker-server", Some("wasm32-unknown-unknown")), - ("improvise-ws-server", None), - ] { - let mut cargo = std::process::Command::new(env!("CARGO")); - cargo.current_dir(env!("CARGO_MANIFEST_DIR")).args([ - "tree", - "--locked", - "--edges", - "normal,features", - "--invert", - "serde_json", - "--package", - krate, - ]); - if let Some(target) = target { - cargo.args(["--target", target]); - } - let out = cargo.output().expect("cargo tree must run"); - assert!( - out.status.success(), - "{krate}: cargo tree failed: {}", - String::from_utf8_lossy(&out.stderr) - ); - let tree = String::from_utf8(out.stdout).expect("cargo tree prints UTF-8"); - assert!( - tree.contains(r#"serde_json feature "float_roundtrip""#), - "{krate} parses frames without float_roundtrip, so its numbers land \ - one ULP off:\n{tree}" - ); - } -} - /// A typed number is parsed exactly in the client and re-parsed from JSON by /// the host. Evidence file. #[test] diff --git a/src/ui/help.rs b/src/ui/help.rs index 09055a17..30fffcfd 100644 --- a/src/ui/help.rs +++ b/src/ui/help.rs @@ -69,9 +69,9 @@ fn page_content(page: usize) -> Vec { HelpLine::Heading(text) => plain(text, s.heading), HelpLine::Text(text) => plain(text, s.normal), HelpLine::Accent(text) => plain(text, s.accent), - HelpLine::Keys { keys, description } => Row { - key: format!(" {}", keys.join(" ")), - desc: description, + HelpLine::Keys(row) => Row { + key: format!(" {}", row.keys().join(" ")), + desc: row.description(), style: s.key, }, HelpLine::Literal { diff --git a/tests/json_numbers.rs b/tests/json_numbers.rs new file mode 100644 index 00000000..506a59ec --- /dev/null +++ b/tests/json_numbers.rs @@ -0,0 +1,61 @@ +//! A number read through serde_json must be the number written (improvise-6yvg, +//! improvise-dz8c). The rule and its derivation: `context/code/protocol.md`. + +use serde_json::Value; + +/// Every NORMAL serde_json dependency in the workspace, and those lacking +/// `float_roundtrip`, each spelled `package (target)`. +fn lossy_json_dependencies(metadata: &Value) -> (Vec, Vec) { + let mut checked = vec![]; + let mut lossy = vec![]; + let packages = metadata["packages"] + .as_array() + .expect("metadata lists packages"); + for package in packages { + let name = package["name"].as_str().expect("a package has a name"); + let deps = package["dependencies"] + .as_array() + .expect("a package lists deps"); + for dep in deps { + if dep["name"] != "serde_json" || !dep["kind"].is_null() { + continue; + } + let target = dep["target"].as_str().unwrap_or("all targets"); + let spelled = format!("{name} ({target})"); + let features = dep["features"].as_array().expect("a dep lists features"); + if !features.iter().any(|f| f == "float_roundtrip") { + lossy.push(spelled.clone()); + } + checked.push(spelled); + } + } + (checked, lossy) +} + +/// Derived from `cargo metadata`, not listed: a new crate that can parse JSON +/// is covered the day it declares the dependency. Evidence file. +#[test] +fn every_crate_that_can_parse_json_enables_float_roundtrip() { + let out = std::process::Command::new(env!("CARGO")) + .current_dir(env!("CARGO_MANIFEST_DIR")) + .args(["metadata", "--no-deps", "--format-version", "1"]) + .output() + .expect("cargo metadata must run"); + assert!( + out.status.success(), + "cargo metadata failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + let metadata: Value = serde_json::from_slice(&out.stdout).expect("metadata is JSON"); + let (checked, lossy) = lossy_json_dependencies(&metadata); + assert!( + checked.len() >= 2, + "the derivation found {checked:?}: too few to be reading the metadata right" + ); + assert!( + lossy.is_empty(), + "these parse JSON numbers with serde_json's lossy default, one ULP off \ + for roughly one decimal in seven; add features = [\"float_roundtrip\"] \ + to each crate's own manifest: {lossy:?}" + ); +}