diff --git a/.agents/skills/code-review/SKILL.md b/.agents/skills/code-review/SKILL.md new file mode 100644 index 00000000..faef7aa8 --- /dev/null +++ b/.agents/skills/code-review/SKILL.md @@ -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>`. 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/.agents/skills/kb/SKILL.md b/.agents/skills/kb/SKILL.md new file mode 100644 index 00000000..85c0ffc7 --- /dev/null +++ b/.agents/skills/kb/SKILL.md @@ -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 # 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 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. diff --git a/.agents/skills/pr-watch/SKILL.md b/.agents/skills/pr-watch/SKILL.md new file mode 100644 index 00000000..cf304a2b --- /dev/null +++ b/.agents/skills/pr-watch/SKILL.md @@ -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 [--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. diff --git a/.agents/skills/wave/SKILL.md b/.agents/skills/wave/SKILL.md new file mode 100644 index 00000000..dffa68a2 --- /dev/null +++ b/.agents/skills/wave/SKILL.md @@ -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 --expect `. Never hand-roll a + poll; see the `pr-watch` skill for the three ways they report green while wrong. +- Review before merging. Copilot's quota may be exhausted and its request is a + **silent no-op**; an absent review and an unrequested one both read as + "pending". Dispatch an adversarial reviewer and give it the head SHA, the base, + and a test-merge, so it reviews the tree that will exist. +- A re-review is of the **delta**, at a named SHA. A review of a superseded + commit reads as a pass. +- Merge on GitHub, then `git fetch gh && git merge --ff-only gh/main`. +- Anything posted to GitHub goes out under the owner's account: open every + comment with a line saying a 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. diff --git a/.codex/config.toml b/.codex/config.toml index 146af7eb..95de0aeb 100644 --- a/.codex/config.toml +++ b/.codex/config.toml @@ -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"]