From 72f0c63fff3560e5f419b537eed756a3786d3105 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 23:33:53 -0700 Subject: [PATCH] docs: one home for the review rules; pin gh/main; audit prose fixes improvise-28ri: -d skip only silences "Is a directory" on the skills' symlinks, which git ls-files lists as files. grep -r never follows them, so nothing was ever read twice. The "spells no id" sentence now says what the recipe actually cites. improvise-oga4: the review-reply and head-SHA rules live only in agent-rules "GitHub" (plus the after-merge clause moved there); AGENTS.md points to it, and the code-review skill names it. An ADR's "Approved" is an extra gate on top of CI, every review answered and the adversarial review. improvise-diho: agent-rules rule 7, pin gh/main to a SHA at task start. Also: build a test-name list in a fresh CARGO_TARGET_DIR, because git archive's commit mtimes make a reused one return a stale list. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .agents/skills/code-review/SKILL.md | 1 + .agents/skills/wave/agent-rules.md | 22 +++++++++++++--- AGENTS.md | 41 +++++++++++++++++++---------- 3 files changed, 47 insertions(+), 17 deletions(-) diff --git a/.agents/skills/code-review/SKILL.md b/.agents/skills/code-review/SKILL.md index 618d30c2..6512d580 100644 --- a/.agents/skills/code-review/SKILL.md +++ b/.agents/skills/code-review/SKILL.md @@ -28,6 +28,7 @@ them here would let the copy drift: | `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 | +| `.agents/skills/wave/agent-rules.md`, "GitHub" | The reviewer's rule: what to read before your first comment, how to answer each existing review, and what every review you post must state. Read it before reviewing a PR | `kb.pl` is the highest-value and least-obvious of these. Grep it before reviewing: `grep -n "invariant(\|doctrine(\|decision(" prolog-sessions/kb.pl`. diff --git a/.agents/skills/wave/agent-rules.md b/.agents/skills/wave/agent-rules.md index 7a4d3f52..7358f704 100644 --- a/.agents/skills/wave/agent-rules.md +++ b/.agents/skills/wave/agent-rules.md @@ -32,9 +32,17 @@ not in a second copy. 6. **The Beads "Session Completion" block near the end of `AGENTS.md` does not set your git policy.** This repository does not opt in to the block's "team-maintainer" profile. On a wave branch that profile's pull and bare - `git push` would be refused (rule 4), or worse. Rules 1–5 are your git - policy. The block is bd's and bd regenerates it whole, so it is not edited - by hand: `AGENTS.md`, "Repository policy over the Beads blocks" (improvise-x1uk). + `git push` would be refused (rule 4), or worse. Rules 1–5 and 7 are your + git policy. The block is bd's and bd regenerates it whole, so it is not + edited by hand: `AGENTS.md`, "Repository policy over the Beads blocks" (improvise-x1uk). +7. **Pin `gh/main` to a literal SHA when the task starts.** Run + `git rev-parse gh/main` once. Use that SHA, not the ref name, for every + reset, merge-base and diff. Remote refs are shared by every worktree of the + repository, so `gh/main` moves whenever another session fetches. An agent + ran `git reset --soft gh/main` mid-task and staged a reverse diff of #196. + It noticed only because it read `git status` (improvise-diho). Re-resolve + only on purpose, for example when you mean to merge a newer main. Rule 3's + check before a PR is such a time. ## Commit early: a worktree is not durable storage @@ -113,6 +121,12 @@ out on purpose: check that no test *disappeared*. Diff test **names**, not counts: a textually clean merge can delete a test module with no conflict marker, and the other side's new tests mask the loss in a count. +- **Build each side's test list in a fresh `CARGO_TARGET_DIR`, every time.** If + a parent's tree comes from `git archive`, a reused target dir gives a wrong + answer, not just a slow one. Archive stamps every file with its commit time, + so cargo judges the sources older than the last build and lists the previous + tree's tests. On #188 that made three of the branch's own tests look + merge-only. - **The shell guard refusing a compound command means NEITHER part ran.** Split the command. After a refusal, re-check the state it was meant to produce. - **Re-run the full suite after reverting an injected fault.** A revert applied @@ -200,6 +214,8 @@ text for any of these in your hand-back. `./scripts/pr-reviews `, unpiped. Reply on its thread to every Codex review: fixed (name the commit), declined (say why), or already one of our tickets (name it). Every review you post states the head SHA it reviewed. +- **Answer a review even if the PR has already merged.** Reviews arrive late, + and the thread is still where the next reader looks. - **Do not merge, and do not close beads.** Your part ends at an open PR plus your hand-back. What "done" means is in `AGENTS.md`, "Repository policy over the Beads blocks". Avoid stacked PRs. If you must stack, never let the base diff --git a/AGENTS.md b/AGENTS.md index 8a11780c..db781f6c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -184,8 +184,8 @@ earlier head, and the finding may already be fixed — verify rather than assume in either direction. Reviews arrive **as the owner's quota allows**, so absence is not evidence. -Wait where a review is plausible, and when one is found, answer it on the -thread even if the PR has already merged: that is where the next reader looks. +Wait where a review is plausible. How a review is answered once found, by the +author and by a reviewer, is `.agents/skills/wave/agent-rules.md`, "GitHub". **Copilot is not reviewing this repository, and the request is a silent no-op.** Measured 2026-09-20: PRs #12, #21 and #27 carry 2, 8 and 4 Copilot reviews; #61, @@ -231,8 +231,8 @@ normally.) Across four PRs Copilot has found genuine bugs and no false positives, so its findings are worth the loop. **But judge each one against the repo's doctrine -rather than applying it reflexively**, and when you decline, reply on the thread -with the reason. The distinction that has mattered: a finding can be *correct* +rather than applying it reflexively**. How a declined finding is answered is +agent-rules, "GitHub". The distinction that has mattered: a finding can be *correct* and still be wrong to fix in that PR. On #12, a non-ASCII `truncate` panic was applied (byte-identical on ASCII, so it cost no evidence) while a hint-line padding bug in the same file was declined — fixing it would have rewritten the @@ -251,21 +251,27 @@ is to branch from `gh/main`, cherry-pick, open a follow-up PR, and delete the stray re-creation. Check `gh pr view --json state` *before* pushing a fix to a PR you have not looked at in a few minutes, rather than after. -Merge only once a review has passed — an adversarial agent's while Copilot is -absent, and it must name the head SHA it reviewed. Prefer a **merge commit** — +Merge only once a review of the PR's current head has passed — an adversarial +agent's while Copilot is absent. What a review must state is agent-rules, +"GitHub". Prefer a **merge commit** — `AGENTS.md` says merges over rebases, and a PR's commits are individually meaningful. ### ADR pull requests -An ADR (`context/adr/`) merges on the user's approval, not on a review. The +An ADR (`context/adr/`) merges on the user's approval, **in addition to** green +CI, every review answered, and a passing adversarial review ("Shipping a +wave"). The approval is an extra gate. It replaces none of the others. The steps, modelled on PR #122 (ADR 004): 1. The PR proposes the ADR with its **open questions numbered**. 2. The user answers them on the PR. 3. The answers are folded into the ADR, in a section headed **"What the user decided"**. -4. The PR merges only after the user comments **"Approved"**. +4. The PR merges only after the user comments **"Approved"**, and only while + the other gates still hold. "Read the PR's reviews before merging it" still + applies after the approval. On #122 the user's own review landed as the PR + merged. ### Never hand-edit `.beads/issues.jsonl` @@ -296,7 +302,7 @@ bd show $(cat /tmp/bd-audit-ids.txt) 2>&1 \ `git ls-files` picks the files, and grep only reads them. The file list is the tracked files plus the untracked ones, minus anything git ignores. That leaves -out three things a `grep -r` over the same roots reads: +out two things a `grep -r` over the same roots reads: - **Build output.** After `./harness/build.sh`, `harness/serve/` holds rendered copies of `context/`, and `grep -r` reported three dead ids from them @@ -304,9 +310,14 @@ out three things a `grep -r` over the same roots reads: - **Other agents' worktrees.** In the orchestrator's checkout `.claude/worktrees/` holds every agent's worktree, dozens of copies of the repository with their unpushed work. -- **The skills twice.** `.claude/skills/*` are symlinks to directories under - `.agents/skills/`. `-d skip` passes over them, so the skills are read once, - through `.agents`. + +`-d skip` is there for the skills' symlinks, and it changes no answer. The +`.claude/skills/*` entries are symlinks to directories under `.agents/skills/`. +`git ls-files` lists each one as a file, and grep, handed a symlink to a +directory, prints `grep: : Is a directory` on stderr. `-d skip` silences +those lines. The skills are read once, through `.agents`, with or without the +flag: neither BSD nor GNU `grep -r` follows a symlink it meets while recursing, +so no id is ever counted twice. Plain `git grep` is not a substitute: on macOS its `-E` silently ignores `\b` and finds a fraction of the ids. @@ -346,8 +357,10 @@ real defect through: added to the exclusion list in the same commit**, or it arrives as a dead id that no amount of `bd` archaeology explains. -Note the recipe's own prose deliberately spells no live or dead id: a document -that lists ids as examples makes itself a source of audit noise. +The recipe's own prose spells no id as an *example*, because a document that +lists example ids makes itself a source of audit noise. The ids it does cite, +improvise-8haj and improvise-oujb, are the tickets behind two of the rules +above. They resolve, and the audit checks them like any other. One environment note: in a worktree-isolated agent the shell guard refuses a loop that passes a runtime-computed argument to `bd`, so the recipe above