diff --git a/.agents/skills/code-review/SKILL.md b/.agents/skills/code-review/SKILL.md index faef7aa8..618d30c2 100644 --- a/.agents/skills/code-review/SKILL.md +++ b/.agents/skills/code-review/SKILL.md @@ -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 diff --git a/.agents/skills/wave/agent-rules.md b/.agents/skills/wave/agent-rules.md index 28245bc9..4c9a5bf8 100644 --- a/.agents/skills/wave/agent-rules.md +++ b/.agents/skills/wave/agent-rules.md @@ -74,6 +74,12 @@ direnv exec . ./scripts/node-test.sh 'web/tests/*.test.js' `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 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. diff --git a/.claude/skills/code-review b/.claude/skills/code-review new file mode 120000 index 00000000..2d88efdf --- /dev/null +++ b/.claude/skills/code-review @@ -0,0 +1 @@ +../../.agents/skills/code-review \ No newline at end of file diff --git a/.claude/skills/code-review/SKILL.md b/.claude/skills/code-review/SKILL.md deleted file mode 100644 index faef7aa8..00000000 --- a/.claude/skills/code-review/SKILL.md +++ /dev/null @@ -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>`. 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. diff --git a/.claude/skills/kb b/.claude/skills/kb new file mode 120000 index 00000000..3ee2d209 --- /dev/null +++ b/.claude/skills/kb @@ -0,0 +1 @@ +../../.agents/skills/kb \ No newline at end of file diff --git a/.claude/skills/kb/SKILL.md b/.claude/skills/kb/SKILL.md deleted file mode 100644 index ea57a3f6..00000000 --- a/.claude/skills/kb/SKILL.md +++ /dev/null @@ -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 # does the model accept a design you have not built? -scripts/kb count # clauses per keyed predicate -scripts/kb query '' # run a goal; exit 1 if it fails -scripts/kb add # add clauses; refuses if a key already exists -scripts/kb replace # add, overwriting existing keys -scripts/kb rm # 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. diff --git a/.claude/skills/pr-watch b/.claude/skills/pr-watch new file mode 120000 index 00000000..3ef371ce --- /dev/null +++ b/.claude/skills/pr-watch @@ -0,0 +1 @@ +../../.agents/skills/pr-watch \ No newline at end of file diff --git a/.claude/skills/pr-watch/SKILL.md b/.claude/skills/pr-watch/SKILL.md deleted file mode 100644 index cf304a2b..00000000 --- a/.claude/skills/pr-watch/SKILL.md +++ /dev/null @@ -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 [--expect ] [--timeout ] [--interval ] -``` - -``` -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 ` 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.