chore: add skills in less claude-specific locations

2026-09-21 13:40:54 -07:00
parent 856a5f84eb
commit 074569e150
5 changed files with 355 additions and 0 deletions
@@ -0,0 +1,139 @@
 ---
 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.
@@ -0,0 +1,86 @@
 ---
 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 verifies the result reads back as intended and runs `kb_consistent`
 on the candidate, so a bad delta leaves the file untouched. `count` before and
 after is still the cheapest evidence a write did what you meant.
 `context/code/kb-tool.md` has the argument. `scripts/kb-test.sh` proves each
 refusal by injecting the fault it names.
@@ -0,0 +1,43 @@
 ---
 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.
@@ -0,0 +1,83 @@
 ---
 name: wave
 description: Dispatch parallel agents to work through open beads and shepherd each to a merged PR. Use when an orchestrator is running a wave, partitioning the backlog, or writing a brief for a work-unit agent.
 ---
 # Running a wave
 An orchestrator chunks the open backlog, fans agents out, reviews what comes
 back, and merges. `AGENTS.md` has the history; this is the operating procedure.
 ## Every agent gets its own worktree. No exceptions.
 Dispatch with `isolation: "worktree"`. Without it every agent edits the **same**
 working tree: one `git status` shows four agents' half-finished files, any commit
 sweeps up the others' work, and any `git checkout` destroys it. This has happened
 here. If you discover it mid-wave, stop every agent, have each create a worktree
 and copy across **only files it can prove it touched**, then `git checkout --`
 those files in the primary tree.
 In the worktree: `direnv allow`, or `bd` reports a version *mismatch* rather than
 failing cleanly. `direnv status` prints `Found RC allowed 0`, and `0` means
 allowed — it reads like the opposite.
 ## Partitioning
 By **crate or directory**, never by ticket count. **Tickets that share a file go
 to one agent**, to be done sequentially — four widget migrations all touching
 `src/draw.rs` are a guaranteed conflict split four ways. Group first, then count.
 Tell each agent what ground is *not* theirs, and to report rather than guess when
 it must cross. Two agents adding a workspace member will still collide in
 `Cargo.toml`/`Cargo.lock`; keep both sides.
 ## What every brief must carry
 1. **The expected base commit, by name.** Worktrees do not reliably branch from
   HEAD — one was created three merges behind and only noticed because it was
   told to check.
 2. **The baseline**, so "N passing" is checkable rather than asserted: Rust test
   count, harness counts, node counts.
 3. **Its ground, and the ground that is not.**
 4. **Never write** `.beads/**`, `prolog-sessions/kb.pl`, `context/repo-map.md`.
   Agents read them freely and report proposed text; the orchestrator applies it.
 5. **The commit trailers and PR footer**, spelled out.
 6. **Do not push to `main`, do not merge.** The orchestrator merges.
 7. **What to report**: branch and head SHA, PR number, what changed per ticket,
   *what could not be verified and why*, proposed repo-map/kb text, and **what it
   learned that the ticket did not predict** — consistently the most valuable
   part, and omitted unless asked for explicitly.
 ## Method to require, because this repo's defect is a test that passes
 - Failing test first, then the fix.
 - Every guard proved by **injecting the fault it names** and watching it fail
  **by name**. An injection that does not compile reads as a clean run, so
  require a positive signal the build ran.
 - `cargo test --workspace` is **fail-fast**; `--no-fail-fast` for any claim about
  blast radius.
 - A textually clean merge can **delete tests** with no conflict marker, and
  `cargo check` will not notice. After any merge that moves a test module, diff
  test **names** — a count is masked by the other side's additions.
 ## Shepherding
 - Wait on checks with `scripts/pr-watch <pr> --expect <sha>`. Never hand-roll a
  poll; see the `pr-watch` skill for the three ways they report green while wrong.
 - Review before merging. Copilot's quota may be exhausted and its request is a
  **silent no-op**; an absent review and an unrequested one both read as
  "pending". Dispatch an adversarial reviewer and give it the head SHA, the base,
  and a test-merge, so it reviews the tree that will exist.
 - A re-review is of the **delta**, at a named SHA. A review of a superseded
  commit reads as a pass.
 - Merge on GitHub, then `git fetch gh && git merge --ff-only gh/main`.
 - Anything posted to GitHub goes out under the owner's account: open every
  comment with a line saying a Codex agent wrote it.
 ## Between waves
 Run the code-review skill on the merged range. The characteristic find here is
 not a wrong line, it is **a test that passes while the thing it names is
 broken** — a vacuous fold test, a one-category fixture, a helper that fabricates
 what the code under test needs. Ask the reviewer to inject the fault and watch
 the check fail.
@@ -1,2 +1,6 @@
 [features]
 hooks = true
 [mcp_servers.prolog-mcp]
 command = "node"
 args = ["/Users/edwlan/git_repos/github.com/adamrybinski/prolog-mcp/dist/index.js"]