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
This commit is contained in:
Edward Langley
Symlink
+1
@@ -0,0 +1 @@
|
||||
../../.agents/skills/code-review
|
||||
@@ -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.
|
||||
Symlink
+1
@@ -0,0 +1 @@
|
||||
../../.agents/skills/kb
|
||||
@@ -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.
|
||||
Symlink
+1
@@ -0,0 +1 @@
|
||||
../../.agents/skills/pr-watch
|
||||
@@ -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.
|
||||
Reference in New Issue
Block a user