11 Commits

Author SHA1 Message Date
Ed Langley da2e5ab9e5 Merge pull request #189 from fiddlerwoaroof/fix/5fzx-skill-symlinks
CI / Build all bundles (push) Has been cancelled
CI / Test and lint (host) (push) Has been cancelled
CI / wasm32 check (browser bundle crates) (push) Has been cancelled
CI / Browser JS unit tests (node --test) (push) Has been cancelled
CI / Knowledge base consistency (push) Has been cancelled
CI / Lint the harness crate (push) Has been cancelled
CI / Browser harness (OPFS contract + shell) (push) Has been cancelled
CI / Deploy to GitHub Pages (push) Has been cancelled
skills: symlink the three hand-kept .claude copies; code-review points at the one gate list (improvise-5fzx, improvise-vjus)
2026-09-22 21:50:42 -07:00
Ed Langley 9aa3581c2d Merge pull request #184 from fiddlerwoaroof/fix/6p8q-records-yank-paste
TUI records-mode yy/p resolve the real record; p declines aggregates (improvise-6p8q, l4mp)
2026-09-22 21:50:15 -07:00
Edward Langley ead796890b The aggregate paste decline no longer sends the user into a drill (improvise-48jd)
Inside a drill p declines too, so 'drill in to paste' led to a second refusal.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
2026-09-22 21:43:19 -07:00
Edward Langley 62dba66232 Merge gh/main into fix/6p8q-records-yank-paste
Resolves the append-at-EOF conflict in context/test-evidence/command.md,
keeping gh/main's entries and then this branch's.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
2026-09-22 21:43:13 -07:00
Edward Langley 8641f9a59d code-review skill: point at agent-rules Gates instead of restating it (improvise-vjus); two gate lessons in agent-rules
The restated list said cargo fmt --all --check, which reaches one of two
workspace roots, and omitted direnv exec, checkers.sh and node-test.sh.
agent-rules gains two rules from PRs #176 and #177.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
2026-09-22 21:38:03 -07:00
Edward Langley 2abf89ade9 skills: .claude/skills/{kb,pr-watch,code-review} become symlinks to .agents (improvise-5fzx)
The three were byte-identical hand-kept copies (same blob on gh/main), the
shape improvise-1f8e found drifted in wave. Done the way PR #175 did wave and
beads-merge already was.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
2026-09-22 21:37:29 -07:00
Edward Langley 0ed2e41c32 Merge gh/main into fix/6p8q-records-yank-paste
The TUI's records yank now reads records_value unfiltered, the value the browser
yanks since improvise-dsxq; the blank case is filed as improvise-s1lx.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
2026-09-22 21:28:30 -07:00
Edward Langley 5ea122b3ea improvise-6p8q/l4mp: context reasoning and test evidence
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
2026-09-22 21:24:54 -07:00
Edward Langley 8aaddeea1d WIP improvise-6p8q: records yank/paste lockstep
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
2026-09-22 21:23:32 -07:00
Edward Langley 8df9431199 WIP improvise-6p8q: yank of a coordinate/creating column
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
2026-09-22 21:20:22 -07:00
Edward Langley 9d04b98136 WIP improvise-6p8q/l4mp: yank/paste resolve the real record; paste declines aggregates
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
2026-09-22 21:19:25 -07:00
13 changed files with 535 additions and 319 deletions
+8 -11
View File
@@ -95,17 +95,14 @@ From `design-principles.md` §7 and `doctrine/2`:
## Verifying
Run the gate **CI runs**, flags and all. Clippy exits 0 on warnings, so without
`-D warnings` this file was green on exactly the input CI is red on — a check
that cannot fail in the direction the defect lies.
```bash
cargo test --workspace --no-fail-fast # fail-fast hides the blast radius
cargo clippy --workspace --all-targets -- -D warnings
cargo fmt --all --check
python3 scripts/check-comment-runs.py # the two-line comment cap
python3 scripts/check-comment-runs.py --check-evidence # a DIFFERENT question
```
Run the gate **CI runs**, flags and all. The list is
`.agents/skills/wave/agent-rules.md`, "Gates", and it is kept only there. Run
it from that file rather than from memory or from an older copy. A second
copy of that list lived here and drifted: it said `cargo fmt --all --check`,
which reaches only one of this repo's two workspace roots, and it left out
`direnv exec .`, `scripts/checkers.sh` and `scripts/node-test.sh`
(improvise-vjus). Flags matter. Clippy exits 0 on warnings, so without
`-D warnings` a clippy run was green on exactly the input CI is red on.
**`--check-evidence` is additive**, so it answers the comment cap too and both
verdicts print. It did not always: the flag used to *select*, so the flagged
+6
View File
@@ -74,6 +74,12 @@ direnv exec . ./scripts/node-test.sh 'web/tests/*.test.js'
`cargo check --target wasm32-unknown-unknown -p <crate>` **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 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
see that.
- **A PR that conflicts with its base gets no CI run.** So anything "left to
CI" is unchecked until the PR is mergeable (PR #177).
- **Measure your own baseline** before changing anything, and report it with
the machine it was measured on. A figure from the brief is a reference, not
your baseline.
+1
View File
@@ -0,0 +1 @@
../../.agents/skills/code-review
-139
View File
@@ -1,139 +0,0 @@
---
name: code-review
description: Review a pull request or diff against this repository's own written architecture and testing doctrine — the Cmd/Effect split, exhaustive enums over AppMode/Axis/CategoryKind, crate layering, the CellKey/layout/persistence invariants, and when a green suite is not evidence. Use when reviewing any change to this repo, and before pushing your own work.
---
# Code review
This repo has already written down what "correct shape" means, in more detail
than most. A review here is mostly checking a change against that, rather than
against generic standards.
Two passes, in order:
1. **Correctness** — does the change do what it claims? Wrong logic, missed
edge cases, error paths that swallow failures. Ordinary review judgement.
2. **Conformance** — does it fit the design? That is what the rest of this
file is for, and it is the part a reviewer who has not read `context/`
will miss.
## Read the sources, don't trust this file
This skill is an index. The rules live in files that change, and restating
them here would let the copy drift:
| Source | What it holds |
|---|---|
| `context/design-principles.md` | The "why" — functional-first, polymorphism over conditionals, correctness by construction, layering, §7 testing doctrine |
| `context/repo-map.md` | The "where", plus **numbered gotchas** — read the ones your diff touches |
| `prolog-sessions/kb.pl` | Executable knowledge: `invariant/2`, `doctrine/2`, `forbidden/2`, `decision/3`, and prior `work_done/3` |
| `AGENTS.md` | Working rules: bug-first tests, `Option`/`Result`/`Box` at the edges, merges over rebases |
`kb.pl` is the highest-value and least-obvious of these. Grep it before
reviewing: `grep -n "invariant(\|doctrine(\|decision(" prolog-sessions/kb.pl`.
A `decision/3` fact often explains why the thing you are about to flag looks
the way it does.
## What to check
**Command/effect split.** `Cmd::execute` takes `&CmdContext` and returns
`Vec<Box<dyn Effect>>`. Mutation happens only in `Effect::apply`. A command
reaching for mutable state is the single most common shape error.
**New effects.** `kind()` has no default, so the compiler asks — but check
the classification is right, not just present (`doctrine(derive_then_verify)`
records naive extraction mis-classifying three effects). Note there is no
`changes_mode()` any more: mode settling is observed after the batch, not
predicted per type (gotcha 31, improvise-toc). Don't reintroduce a static
flag for a conditional behaviour.
**Exhaustive matches.** `AppMode`, `Axis`, `BinOp`, `CategoryKind` and friends
should match exhaustively — a `_ =>` arm is how adding a variant silently
skips a site. When a new variant shares one property with an existing variant
but not another, name both as predicates and route call sites through the one
they mean, rather than testing for the variant
(`doctrine(two_questions_one_enum)`; `Axis::filters_data` /
`is_user_assignable` are the worked example).
**Layering.** `forbidden/2` in `kb.pl` is the authority:
`improvise-core` no UI/IO, `improvise-io` no UI/commands, `improvise-command`
no ratatui/crossterm. A ratatui type in core is the usual violation — the fix
is a semantic enum in core that the render layer maps (see `AxisColor`).
**`Option`/`Result`/`Box` at the boundaries.** Wrap at parse/lookup/IO, unwrap
early, keep core logic on concrete values. Heap-container plumbing should be
split from logic (`AGENTS.md`).
**Invariants the type system does not carry.** Check the relevant gotchas
rather than reasoning from first principles — `CellKey` built through its
constructors, `IndexMap` order being load-bearing for persistence, display
rounding never fed back into eval, `app.layout` going stale mid-batch and
needing `RebuildLayout` in front.
**Persistence.** The pest grammar is the source of truth; update it first.
Wire tokens are not UI labels and must not follow them.
## Testing doctrine
From `design-principles.md` §7 and `doctrine/2`:
- **A bug fix needs a test that failed before it.** If the diff claims a fix
with no such test, that is the finding. Ask whether it was run against the
old code.
- Prefer unit tests over integration/e2e; test the observable contract, not
internal storage.
- Suite stays **under 2 seconds**. Proptest stays at 256 cases — constrain the
strategy instead of raising them.
- `doctrine(check_the_checker)`: a new check should be proven by injecting the
fault it targets. A check nobody has seen fail is indistinguishable from one
that always passes.
- `doctrine(populate_before_roundtrip)`: a roundtrip over a default-constructed
value proves almost nothing.
- `doctrine(binding_dump_diff)`: most keybindings have no direct test, so a
green suite is **not** evidence for a keymap refactor — diff the dumped
bindings.
## Verifying
Run the gate **CI runs**, flags and all. Clippy exits 0 on warnings, so without
`-D warnings` this file was green on exactly the input CI is red on — a check
that cannot fail in the direction the defect lies.
```bash
cargo test --workspace --no-fail-fast # fail-fast hides the blast radius
cargo clippy --workspace --all-targets -- -D warnings
cargo fmt --all --check
python3 scripts/check-comment-runs.py # the two-line comment cap
python3 scripts/check-comment-runs.py --check-evidence # a DIFFERENT question
```
**`--check-evidence` is additive**, so it answers the comment cap too and both
verdicts print. It did not always: the flag used to *select*, so the flagged
run skipped the cap and said nothing about having skipped it, and three
separate agents hit that in one campaign — a local gate using only the flagged
form went green while CI went red on a three-line doc comment (improvise-ijql).
Run the bare form anyway when you only want the cap: it is 0.3s and needs no
toolchain, where the flagged one needs a compiled workspace.
The knowledge base is checked in CI as code:
```bash
nix shell nixpkgs#trealla --command tpl prolog-sessions/kb.pl \
-g "(kb_consistent -> halt(0) ; (kb_report, halt(1)))"
```
In a cloud container that needs one step first: `nix shell` resolves nixpkgs
as a GitHub tarball, which is not served there. Run
`scripts/nix-seed-flake-inputs.sh` to seed the locked inputs over git (it does
not modify `flake.lock`), after which the command works. Details and the two
alternatives are in `context/cloud-agents.md`. If you would rather not spend
the time, review `kb.pl` edits by reading and let CI run the check.
## Closing the loop
If the review surfaces a durable fact — a decision, a new invariant, a method
that worked — add it to `prolog-sessions/kb.pl` as `decision/3`,
`invariant/2` or `doctrine/2`. If it changes where something lives or how it
behaves, update `context/repo-map.md`. Those two files and
`context/design-principles.md` are maintained by hand and are the reason this
review is cheap next time.
+1
View File
@@ -0,0 +1 @@
../../.agents/skills/kb
-118
View File
@@ -1,118 +0,0 @@
---
name: kb
description: Read and write the Prolog knowledge base at prolog-sessions/kb.pl. Use when adding a doctrine, invariant, decision or work_done fact, correcting or retiring one, or querying the formal model. Never edit that file by hand and never write it through the Prolog MCP.
---
# Writing the knowledge base
`prolog-sessions/kb.pl` is executable knowledge, gated by CI. **`scripts/kb` is
the only thing that writes it.** `tpl` is assumed on `PATH` from the dev shell.
```
scripts/kb check # kb_consistent, exactly what CI runs
scripts/kb check --with <d.pl># does the model accept a design you have not built?
scripts/kb count # clauses per keyed predicate
scripts/kb query '<goal>' # run a goal; exit 1 if it fails
scripts/kb add <delta.pl> # add clauses; refuses if a key already exists
scripts/kb replace <delta.pl> # add, overwriting existing keys
scripts/kb rm <keys.pl> # remove by key; refuses if a key is absent
scripts/kb fmt # canonical form (idempotent)
```
A delta is an ordinary Prolog file of clauses. For `rm`, a key is a clause head
with the last argument left as `_`:
```prolog
doctrine(a_name_that_went_false,_).
```
## Designing against the model
Before implementing a ticket, state the design as facts and ask whether the
model accepts it:
```prolog
crate(improvise_report,'crates/improvise-report','render a View as a report').
crate_depends_on(improvise_report,improvise_core).
layer(report,improvise_report).
```
`scripts/kb check --with design.pl` merges those into a throwaway candidate and
runs `kb_consistent` over the union. Nothing is written either way.
It derives consequences you would not enumerate: one wrong dependency edge
returns six `layering_inverted` violations through the transitive closure. It
also catches schema mistakes — `layer/2` is `(layer, crate)`, and the wrong
order comes back as `layer_names_unknown_crate` rather than being accepted.
What the model can refuse today: layering and dependency direction
(`illegal_dependency`, `layering_inverted`, `may_use`), `ViewState` membership
(`portable`, `snapshot_safe`, `view_state_admits`, `field_misplaced_in_view_state`),
record obligations, and functional-key conflicts. A design touching those areas
is worth stating; one that touches none of them will be accepted vacuously, so
extend the model rather than trusting the green.
## Rules
**Write a fact when you learn it.** There is no reason to batch.
**Verify the claim against the tree before writing it.** A clause stating the
negation of what the code does is worse than no clause
(`doctrine(a_stale_gotcha_asserts_the_negation)`).
**If the NAME went false, retire it** — `rm` the old key, `add` the new one, and
say in the new clause that it was renamed. A reader who trusts a name never
reads the body.
**Put the argument in the clause.** Doctrine and decision text is where worked
examples and history belong; prose docs should stay operational.
## Traps
- **`assertz` does not work here.** A consulted predicate is static;
`assertz`/`retract` raise `permission_error(modify, static_procedure)`. This
is why the tool reads clauses as data.
- **`kb_consistent` does not check everything.** A clause name cited *inside
another clause's string* is unchecked, so retiring a clause can dangle every
clause referring to it (`improvise-2f97`).
- **Reading through the MCP is fine.** `loadSession` at session start is still
how a session gets the model in hand; only writing moved.
Every write checks three things before it replaces the file: the result reads
back as intended, it still holds every clause **HEAD** has, and `kb_consistent`
passes. So a bad delta leaves the file untouched. `count` before and after is
still the cheapest evidence a write did what you meant.
## When a write is refused
Exit codes: `0` done, `1` refused, `2` usage, `3` HEAD's `kb.pl` could not be
read, so nothing was compared, `127` no `tpl`. Only `1` is a verdict.
**"clauses HEAD has are gone, with no kb rm recorded"** means the working-tree
`kb.pl` is short of the committed one. Your delta did not cause it. The file
was already damaged, by a killed write, a bad merge resolution or an edit. The
refusal lists the clauses that are missing. The check counts clauses **per
key**, so losing one of two clauses that share a key is caught too.
- **Recover:** `git show HEAD:prolog-sessions/kb.pl > prolog-sessions/kb.pl`,
then re-run your `add`/`replace`/`rm` deltas.
- **Do not commit the short file to get past the refusal.** HEAD is what the
check compares against, so a committed loss becomes the new floor and is
never reported again.
- **To remove a clause on purpose, use `kb rm`.** It records the removal in a
per-worktree ledger (`git rev-parse --git-path kb-removed.pl`, never
committed), and that record is what lets later writes accept the missing
clause. A hand deletion has no record, so it is refused.
The ledger vouches only for the exact `kb.pl` file the tool last wrote. Any
rewrite outside the tool voids it, including a restore, `git checkout`, `cp`
or an editor. After an `rm`, a restore therefore also undoes the `rm`, and you
must redo it. That is deliberate. Otherwise the abandoned `rm` would go on
excusing a later, accidental loss of the same clause.
A killed write can leave `kb.pl.cand.*` files and a `kb.pl.lock` directory in
`prolog-sessions/`. Git ignores them. The lock makes the next write wait 30
seconds and then fail, so delete it if nothing else is writing.
`context/code/kb-tool.md` has the argument. `scripts/kb-test.sh` proves each
refusal by injecting the fault it names.
+1
View File
@@ -0,0 +1 @@
../../.agents/skills/pr-watch
-43
View File
@@ -1,43 +0,0 @@
---
name: pr-watch
description: Wait for a GitHub PR's checks to finish before merging or reporting on it. Use whenever you need to know whether a PR is green. Do not hand-roll a polling loop over gh pr view.
---
# Waiting on a PR
```
scripts/pr-watch <pr-number> [--expect <sha>] [--timeout <sec>] [--interval <sec>]
```
```
exit 0 every check COMPLETED and none failed
exit 1 a check failed, or the PR is CONFLICTING
exit 2 the head moved during the wait, or is not --expect
exit 3 timed out with checks still running
exit 4 usage or environment error
```
**Pass `--expect <sha>` whenever you know which commit you are asking about.**
It is the difference between "this PR is green" and "the thing I pushed is
green", and a PR here was once merged at a head that predated its own fix.
## Do not write the loop yourself
Three signals look like "finished" and are not:
- **"nothing pending"** — also true when no check has registered yet. Require a
**non-zero total** as well.
- **`conclusion == null`** — `conclusion` is a *string*, empty while running.
Test `status == "COMPLETED"`.
- **a verdict from before the head moved** — re-check the head, or `--expect`.
`gh` is shell-aliased to a different tool in these shells; `pr-watch` locates a
real one itself. Do not reintroduce a bare `gh` call in a script here.
## Before deciding a PR is stuck
Check suites can take several minutes to appear at all, and a Copilot review
takes about six. Compare `date -u` against the GitHub timeline rather than a
felt interval, and do not close and reopen a PR to unstick it.
`scripts/pr-watch-test.sh` drives every case from fixtures.
+45
View File
@@ -3078,3 +3078,48 @@ and every entry, so *any* append reframes and throws the cursor to the origin
although nothing was renumbered. Placing the cursor masks that at this call
site. Distinguishing "rows appended" from "rows renumbered" is a larger question
and does not belong in a P1 data-loss fix.
## Paste lands on a record or nowhere (improvise-6p8q, improvise-l4mp)
`YankCell` and `PasteCell` carry a `CellKey` like every cell command, and in
records mode the cursor's key is the **synthetic** `(_Index, _Dim)` pair — a
row index and a column index (gotcha 1). Both commands used it as though it
named a cell. `yy` evaluated it and got `None` on every records cell; `p` wrote
a *new* cell keyed `_Dim=|3|, _Index=|0|`, which `format_md` then persisted into
`## Data`. One keystroke corrupted the saved document, and the browser, which
composes its paste against the projected `CellId::Record`, never had the bug.
**Yank reads what the grid draws.** A synthetic key is answered by
`GridLayout::records_value`, the one resolution of a records cell
(improvise-tom1), so a coordinate column yanks its item as text and a measure
column its stored value. That is also what the browser yanks, since
improvise-dsxq made it copy `CellDisplay::value`, which the projection fills
from the same call. One consequence is shared by both ends and is not decided
here: a blank — a creating column, or a measure the record lacks — yanks
`Text("")` where an empty pivot cell yanks `None`. Filtering it in the TUI alone
would be a second derivation, so it is left to the producer (improvise-s1lx).
**Paste writes only where a value is stored.** A synthetic key resolves through
`GridLayout::record_cell_key`; the coordinate columns and both creating columns
store no value, so the paste is declined out loud rather than guessed at. A
coordinate column is deliberately *not* a rename: renaming moves every measure
cell in the row, a different intent from `SetCell`, and the browser does not
offer it either (`project::cell_id` answers `None` there).
**Inside a drill the paste is declined, not staged.** Gotcha 19 asks the
records branch to follow the commit path, whose drill arm stages the edit — but
`pending_edits` holds *strings* and `CellValue`'s `Display` rounds a number to
four places, so staging a pasted `1.23456789` would commit `1.2346`. Writing
straight through would break the drill's atomicity. Declining is the only arm
that loses nothing; a typed staging map is the real fix if drill paste is
wanted.
**An aggregate is a key that leaves out a regular category the layout
aggregates over** (improvise-l4mp). That is asked of the *key*, not of the view,
so `paste Region=West _Measure=Revenue Product=Gadget` from a script still
writes where the current view happens to aggregate `Product` away: a key that
names every coordinate is a record whatever the view shows. For the cursor's
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`.
+16 -1
View File
@@ -4389,4 +4389,19 @@ producer (fails with `None`).
Records mode is deliberately not covered. There the TUI yanks `None` and its
`p` writes a junk `_Dim`/`_Index` cell, because it resolves the synthetic key
(improvise-6p8q). The browser is right and the TUI is wrong, so a lockstep
case there would assert the bug.
case there would assert the bug. Since improvise-6p8q's fix it is covered, by
`yy_then_p_in_records_mode_writes_the_record_the_host_writes` below.
### yy_then_p_in_records_mode_writes_the_record_the_host_writes
improvise-6p8q in lockstep. The yank test in this file deliberately stayed on a
pivot grid because a records case would have asserted the TUI bug. Two records
(North, South), the yank from North and the paste onto South on row 1, the
`Revenue` column being neither the first column nor the last. Rivals, each run:
restoring the synthetic-key paste fails `both_ends`' document comparison on
`p` (the twin applied the client's `SetCell` to South's record, the host wrote
a `_Dim`/`_Index` cell); restoring the synthetic-key yank fails the yank
assertion. The tail yanks off the `Region` and `+` columns and requires the two
registers to agree; a TUI-only blank filter (`Text("")` to `None`) fails it on
`+`, which is why that filter was removed rather than kept. The evidence entries
for the host-side tests are in `command.md`.
+66
View File
@@ -5021,3 +5021,69 @@ bracket-matched `#[…]` before reading the name.
`an_attributed_or_wrapped_effect_is_still_an_effect` pins that on a fixture:
a doc comment, two attributes (one with nested brackets), and a wrapped call.
With the whitespace skip removed it fails with `[]`.
### yy_then_p_in_records_mode_copies_between_real_records
improvise-6p8q. At `gh/main` `9e78051` it failed at the yank: `left: None,
right: Some(Number(700.0))`, because `YankCell` evaluated the synthetic
`(_Index, _Dim)` key. With the yank alone repaired (rival: paste still writes
`SetCell(synthetic key)`), it fails at the paste assertion, and the document
carried `_Dim=|3|, _Index=|0| = …` in `## Data`. The test drives `yy`/`p`
through `App::handle_key` and then saves and reloads, because the persisted
line is the harm. The fixture is chosen so no two answers coincide: two records
by two measures with four distinct values, the yank from row 0 `Cost` and the
paste onto row 1 `Revenue`, so "the first row", "the first measure" and "the
value already there" are each wrong. The persistence assertion is scoped to the
`## Data` section: a view section legitimately writes `_Index: none`, and a
first version of this test failed on that.
### yy_on_a_records_coordinate_column_yanks_the_item_as_text
A coordinate column yanks its item as text (`West`, row 1): the yank reads
`GridLayout::records_value`, the same value the browser yanks off
`CellDisplay::value`. Rival: restoring the synthetic-key yank fails it with
`None`.
### p_on_a_records_column_that_stores_no_value_changes_nothing
`p` on `Region`, `+cat` and `+` at row 1 leaves the document byte-identical and
says why. At `9e78051` it failed on `Region` with `_Dim=|0|, _Index=|1| = 5.5`
appended to `## Data`. The control — the same paste onto `Revenue` writes — is
what keeps the declines from being "this command never writes".
### p_inside_a_drill_neither_writes_nor_stages
At `9e78051` a drilled paste wrote `_Dim=|3|, _Index=|0| = 5.5`. The rival that
resolves the real record but ignores the drill (writing straight through) fails
the document-unchanged assertion at the same line. Why declining rather than
staging: `context/code/command.md`, "Paste lands on a record or nowhere".
### p_onto_a_none_axis_aggregate_writes_nothing
improvise-l4mp, None-axis half. At `9e78051` `p` over the West/Revenue aggregate
(Product on None) wrote `Region=West, _Measure=Revenue = 5.5` — a partial key
that is not a record. The cursor is on row 1, not row 0. The control reruns the
same paste with `Product` paged instead of aggregated and requires the write,
so a paste that declines everywhere fails. Rival: removing the aggregate check
fails the first assertion.
### paste_with_an_explicit_full_key_writes_even_where_the_view_aggregates
Pins that "aggregate" is asked of the key, not of the view. It passes at
`9e78051` (there was no check at all); its job is the rival where the check
reads only whether the view has a regular category on None, which fails it —
that rival would silently turn every scripted `paste` with full coordinates
into a no-op on an aggregating view.
### the_aggregate_decline_does_not_send_the_user_where_paste_also_declines
improvise-48jd, from the #184 review. The aggregate decline said "drill in to
paste into a record", and inside the drill `p` answers `PASTE_INSIDE_DRILL`, so
the advice led to a second refusal. The test follows the advice literally: `p`
on the West aggregate, `>` into its drill, cursor onto the `Revenue` column, `p`
again. It asserts that the drilled paste really is the decline, so the setup
cannot pass by landing on "no cell at cursor". A first version of the test did
exactly that, because West had no record to drill into. It then asserts that
the first message does not mention drilling. At `0ed2e41` it failed with the
old text. The rival is restoring that text. Real drill paste, which means
staging a typed `CellValue`, is still open as improvise-48jd.
@@ -1,5 +1,7 @@
use crate::model::Aggregation;
use crate::model::cell::{CellKey, CellValue};
use crate::ui::effect::{self, Effect, ViewTarget};
use crate::view::synthetic_record_info;
use super::core::{Cmd, CmdContext};
@@ -91,6 +93,276 @@ mod tests {
assert!(effects.is_empty());
}
use crate::command::key::{KeyCode, KeyEvent, KeyModifiers};
use crate::ui::app::App;
use crate::workbook::Workbook;
fn press(app: &mut App, c: char) {
app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
.unwrap();
}
fn record(region: &str, measure: &str) -> CellKey {
CellKey::new(vec![
("Region".into(), region.into()),
("_Measure".into(), measure.into()),
])
}
/// Two records by two measures, every value distinct, in records mode.
fn records_app() -> App {
let mut wb = Workbook::new("T");
wb.add_category("Region").unwrap();
wb.add_item("Region", "East").unwrap();
wb.add_item("Region", "West").unwrap();
wb.add_item("_Measure", "Revenue").unwrap();
wb.add_item("_Measure", "Cost").unwrap();
for (region, measure, n) in [
("East", "Revenue", 1200.0),
("East", "Cost", 700.0),
("West", "Revenue", 900.0),
("West", "Cost", 400.0),
] {
wb.model
.set_cell(record(region, measure), CellValue::Number(n));
}
let mut app = App::new(wb, None);
press(&mut app, 'R');
assert!(app.layout.is_records_mode(), "setup: in records mode");
app
}
fn col_of(app: &App, label: &str) -> usize {
(0..app.layout.col_count())
.find(|&c| app.layout.col_label(c) == label)
.unwrap_or_else(|| panic!("no records column {label}"))
}
fn synthetic_cells(wb: &Workbook) -> Vec<CellKey> {
wb.model
.data
.iter_cells()
.map(|(k, _)| k)
.filter(|k| k.get("_Index").is_some() || k.get("_Dim").is_some())
.collect()
}
#[test]
fn yy_then_p_in_records_mode_copies_between_real_records() {
let mut app = records_app();
let (east, west) = (0, 1);
assert_eq!(
app.layout.record_row(west).unwrap().coords.get("Region"),
Some("West")
);
app.cursor_mut().selected = (east, col_of(&app, "Cost"));
press(&mut app, 'y');
press(&mut app, 'y');
assert_eq!(
app.view_state.yanked,
Some(CellValue::Number(700.0)),
"yy must yank the record's stored value, not the synthetic key's"
);
app.cursor_mut().selected = (west, col_of(&app, "Revenue"));
press(&mut app, 'p');
let wb = &app.model_state.workbook;
assert_eq!(
wb.model.get_cell(&record("West", "Revenue")),
Some(&CellValue::Number(700.0)),
"p must write the real record under the cursor"
);
assert_eq!(
wb.model.get_cell(&record("East", "Cost")),
Some(&CellValue::Number(700.0))
);
assert_eq!(synthetic_cells(wb), Vec::<CellKey>::new());
let text = crate::persistence::format_md(wb);
let data = text.split("## Data").nth(1).expect("a data section");
assert!(
!data.contains("_Index") && !data.contains("_Dim"),
"the saved document carries a synthetic-keyed line:\n{text}"
);
let reloaded = crate::persistence::parse_md(&text).expect("round trip");
assert_eq!(
reloaded.model.get_cell(&record("West", "Revenue")),
Some(&CellValue::Number(700.0))
);
assert_eq!(synthetic_cells(&reloaded), Vec::<CellKey>::new());
}
#[test]
fn yy_on_a_records_coordinate_column_yanks_the_item_as_text() {
let mut app = records_app();
app.cursor_mut().selected = (1, col_of(&app, "Region"));
press(&mut app, 'y');
press(&mut app, 'y');
assert_eq!(app.view_state.yanked, Some(CellValue::Text("West".into())));
}
#[test]
fn p_on_a_records_column_that_stores_no_value_changes_nothing() {
for label in ["Region", "+cat", "+"] {
let mut app = records_app();
app.view_state.yanked = Some(CellValue::Number(5.5));
let before = crate::persistence::format_md(&app.model_state.workbook);
app.cursor_mut().selected = (1, col_of(&app, label));
press(&mut app, 'p');
assert_eq!(
crate::persistence::format_md(&app.model_state.workbook),
before,
"p on the {label} column changed the document"
);
assert!(
!app.view_state.status_msg.is_empty(),
"p on the {label} column declined silently"
);
}
// The control: the same paste onto a measure column does write.
let mut app = records_app();
app.view_state.yanked = Some(CellValue::Number(5.5));
app.cursor_mut().selected = (1, col_of(&app, "Revenue"));
press(&mut app, 'p');
assert_eq!(
app.model_state
.workbook
.model
.get_cell(&record("West", "Revenue")),
Some(&CellValue::Number(5.5)),
"the control never wrote, so the declines above prove nothing"
);
}
#[test]
fn p_inside_a_drill_neither_writes_nor_stages() {
let mut wb = Workbook::new("T");
wb.add_category("Region").unwrap();
wb.add_item("Region", "East").unwrap();
wb.add_item("_Measure", "Revenue").unwrap();
wb.add_item("_Measure", "Cost").unwrap();
wb.model
.set_cell(record("East", "Revenue"), CellValue::Number(1.0));
wb.model
.set_cell(record("East", "Cost"), CellValue::Number(2.0));
let mut app = App::new(wb, None);
press(&mut app, '>');
assert!(app.drill_state.is_some(), "setup: drilled");
let before = crate::persistence::format_md(&app.model_state.workbook);
app.view_state.yanked = Some(CellValue::Number(5.5));
app.cursor_mut().selected = (0, col_of(&app, "Cost"));
press(&mut app, 'p');
assert_eq!(
crate::persistence::format_md(&app.model_state.workbook),
before
);
assert!(
app.drill_state.as_ref().unwrap().pending_edits.is_empty(),
"a paste was staged as text, which loses the value's type"
);
assert_eq!(
synthetic_cells(&app.model_state.workbook),
Vec::<CellKey>::new()
);
assert!(!app.view_state.status_msg.is_empty(), "declined silently");
}
/// Region on rows, Product aggregated away on None: every cell is an aggregate.
fn aggregated_app(product_axis: crate::view::Axis) -> App {
let mut wb = Workbook::new("T");
wb.add_category("Region").unwrap();
wb.add_item("Region", "East").unwrap();
wb.add_item("Region", "West").unwrap();
wb.add_category("Product").unwrap();
wb.add_item("Product", "Widget").unwrap();
wb.add_item("Product", "Gadget").unwrap();
wb.add_item("_Measure", "Revenue").unwrap();
wb.active_view_mut()
.set_axis("Region", crate::view::Axis::Row);
wb.active_view_mut()
.set_axis("_Measure", crate::view::Axis::Column);
wb.active_view_mut().set_axis("Product", product_axis);
wb.model.set_cell(
record("East", "Revenue").with("Product", "Widget"),
CellValue::Number(10.0),
);
let mut app = App::new(wb, None);
app.view_state.yanked = Some(CellValue::Number(5.5));
app.cursor_mut().selected = (1, 0);
app
}
#[test]
fn p_onto_a_none_axis_aggregate_writes_nothing() {
let mut app = aggregated_app(crate::view::Axis::None);
let before = crate::persistence::format_md(&app.model_state.workbook);
press(&mut app, 'p');
assert_eq!(
crate::persistence::format_md(&app.model_state.workbook),
before,
"p wrote a cell at the aggregate's partial key"
);
assert!(!app.view_state.status_msg.is_empty(), "declined silently");
// The control: with Product paged rather than aggregated, p writes.
let mut app = aggregated_app(crate::view::Axis::Page);
press(&mut app, 'p');
assert_eq!(
app.model_state
.workbook
.model
.get_cell(&record("West", "Revenue").with("Product", "Widget")),
Some(&CellValue::Number(5.5))
);
}
#[test]
fn the_aggregate_decline_does_not_send_the_user_where_paste_also_declines() {
let mut app = aggregated_app(crate::view::Axis::None);
app.model_state.workbook.model.set_cell(
record("West", "Revenue").with("Product", "Gadget"),
CellValue::Number(3.0),
);
app.rebuild_layout();
press(&mut app, 'p');
let advice = app.view_state.status_msg.clone();
press(&mut app, '>');
assert!(app.drill_state.is_some(), "setup: the aggregate drills");
app.cursor_mut().selected = (0, col_of(&app, "Revenue"));
press(&mut app, 'p');
let in_drill = app.view_state.status_msg.clone();
assert_eq!(
in_drill, PASTE_INSIDE_DRILL,
"setup: the drilled paste declines"
);
assert!(
!advice.to_lowercase().contains("drill"),
"{advice:?} sends the user into a drill, where p answers {in_drill:?}"
);
}
#[test]
fn paste_with_an_explicit_full_key_writes_even_where_the_view_aggregates() {
let app = aggregated_app(crate::view::Axis::None);
let wb = &app.model_state.workbook;
let layout = make_layout(wb);
let reg = make_registry();
let yanked = Some(CellValue::Number(5.5));
let mut ctx = make_ctx(wb, &layout, &reg);
ctx.yanked = &yanked;
assert!(!ctx.none_cats().is_empty(), "setup: the view aggregates");
let key = record("West", "Revenue").with("Product", "Gadget");
let effects = PasteCell { key: key.clone() }.execute(&ctx);
let mut app = aggregated_app(crate::view::Axis::None);
app.apply_effects(effects);
assert_eq!(
app.model_state.workbook.model.get_cell(&key),
Some(&CellValue::Number(5.5)),
"a key naming every coordinate is a record, whatever the view shows"
);
}
#[test]
fn transpose_produces_transpose() {
let m = two_cat_model();
@@ -146,9 +418,13 @@ impl Cmd for YankCell {
"yank"
}
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
let value = ctx
.model
.evaluate_aggregated(&self.key, Aggregation::of(ctx.none_cats()));
// A records cell is the layout's one resolution, as the browser yanks it (gotcha 1).
let value = match synthetic_record_info(&self.key) {
Some((row, col)) => ctx.layout.records_value(row, col),
None => ctx
.model
.evaluate_aggregated(&self.key, Aggregation::of(ctx.none_cats())),
};
vec![
Box::new(effect::SetYanked(value)),
effect::set_status("Yanked"),
@@ -166,14 +442,47 @@ impl Cmd for PasteCell {
"paste"
}
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
if let Some(value) = ctx.yanked.clone() {
vec![Box::new(effect::SetCell(self.key.clone(), value))]
} else {
vec![]
match ctx.yanked.clone() {
Some(value) => vec![paste_into(ctx, &self.key, value)],
None => vec![],
}
}
}
/// Where a paste at `key` lands. The reasoning, and why a drill declines:
/// `context/code/command.md`, "Paste lands on a record or nowhere".
fn paste_into(ctx: &CmdContext, key: &CellKey, value: CellValue) -> Box<dyn Effect> {
match synthetic_record_info(key) {
Some((row, col)) => paste_into_record(ctx, row, col, value),
None if aggregates(ctx, key) => effect::set_status(PASTE_ONTO_AGGREGATE),
None => Box::new(effect::SetCell(key.clone(), value)),
}
}
fn paste_into_record(
ctx: &CmdContext,
row: usize,
col: usize,
value: CellValue,
) -> Box<dyn Effect> {
match (ctx.layout.record_cell_key(row, col), ctx.has_drill_state) {
(Some(_), true) => effect::set_status(PASTE_INSIDE_DRILL),
(Some(real), false) => Box::new(effect::SetCell(real, value)),
(None, _) => effect::set_status(PASTE_ONTO_NO_VALUE),
}
}
/// Whether `key` leaves out a regular category the layout aggregates over.
fn aggregates(ctx: &CmdContext, key: &CellKey) -> bool {
ctx.none_cats().iter().any(|cat| {
key.get(cat).is_none() && ctx.model.category(cat).is_some_and(|c| c.kind.is_regular())
})
}
const PASTE_ONTO_AGGREGATE: &str = "Cannot paste onto an aggregate — it combines several records, so there is no one cell to write";
const PASTE_ONTO_NO_VALUE: &str = "Paste lands only on a measure column";
const PASTE_INSIDE_DRILL: &str = "Paste is not staged inside a drill — edit the cell instead";
// ── View commands ────────────────────────────────────────────────────────────
#[derive(Debug)]
@@ -1233,6 +1233,81 @@ fn page_selections(app: &App) -> Vec<(String, Option<String>)> {
.collect()
}
/// improvise-6p8q: the TUI yanked and pasted through the synthetic records key.
/// See `context/test-evidence/clients.md`.
#[test]
fn yy_then_p_in_records_mode_writes_the_record_the_host_writes() {
let r = KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE);
let two_records = || {
let mut app = furnished_host();
let south = north_revenue().with("Region", "South");
app.model_state
.workbook
.model
.set_cell(south, improvise_protocol::CellValue::Number(900.0));
app.rebuild_layout();
reduce_key(&mut app, r);
app
};
let (mut app, mut twin) = (two_records(), two_records());
let revenue = (0..app.layout.col_count())
.find(|&c| app.layout.col_label(c) == "Revenue")
.expect("a Revenue column");
let south = 1;
assert_eq!(
app.layout.record_row(south).unwrap().coords.get("Region"),
Some("South")
);
for host in [&mut app, &mut twin] {
host.cursor_mut().selected = (0, revenue);
}
let mut client = records_client(&app);
both_ends(&mut app, &mut twin, &mut client, "y");
both_ends(&mut app, &mut twin, &mut client, "y");
assert_eq!(
(client.view.yanked.clone(), app.view_state.yanked.clone()),
(
Some(improvise_protocol::CellValue::Number(1200.0)),
Some(improvise_protocol::CellValue::Number(1200.0))
),
"both ends yank North's stored Revenue"
);
for host in [&mut app, &mut twin] {
host.cursor_mut().selected = (south, revenue);
}
client.view.cursor_mut().selected = (south, revenue);
both_ends(&mut app, &mut twin, &mut client, "p");
assert_eq!(
app.model_state
.workbook
.model
.get_cell(&north_revenue().with("Region", "South")),
Some(&improvise_protocol::CellValue::Number(1200.0)),
"the paste must land on South's real record"
);
// Off the measure columns the two ends still yank one value.
for label in ["Region", "+"] {
let col = (0..app.layout.col_count())
.find(|&c| app.layout.col_label(c) == label)
.unwrap();
for host in [&mut app, &mut twin] {
host.cursor_mut().selected = (south, col);
}
client.view.cursor_mut().selected = (south, col);
both_ends(&mut app, &mut twin, &mut client, "y");
both_ends(&mut app, &mut twin, &mut client, "y");
assert_eq!(client.view.yanked, app.view_state.yanked, "{label}");
}
assert_eq!(
app.view_state.yanked,
Some(improvise_protocol::CellValue::Text(String::new())),
"the last yank must have run, over the blank creating column"
);
}
/// Both hosts driven into records mode the way a user gets there, so the
/// `_Records` view and its `_Index`/`_Dim` axes are the real ones.
fn records_pair() -> (App, App) {