diff options
| author | Craig Jennings <c@cjennings.net> | 2026-07-28 21:28:29 -0500 |
|---|---|---|
| committer | Craig Jennings <c@cjennings.net> | 2026-07-28 21:28:29 -0500 |
| commit | 80624606b8a60d97d8c7600882ddc35f5225469f (patch) | |
| tree | 4c4e56089c1293ab75863fe50bafefcf5084565e /publish/SKILL.md | |
| parent | 3a933a26c12b6350fa46faf89bf86e99bfab0f30 (diff) | |
| download | rulesets-80624606b8a60d97d8c7600882ddc35f5225469f.tar.gz rulesets-80624606b8a60d97d8c7600882ddc35f5225469f.zip | |
feat(publish): isolate the pre-commit review and loop it to approval
The review already ran before every commit. It ran in the context that wrote the diff, which is the part that didn't work. A self-review checks the change against the author's own model of it and can't check the model, so the errors that survive are the ones that were never in the diff: a scope inherited from whoever reported the problem, a blast radius estimated instead of measured.
So the reviewer is now a subagent, on every commit including a one-line one. It gets the diff, a one-line claim of what the change does, and the ticket where one exists. It doesn't get the conversation or my reasoning, because those are the model it exists not to hold. The ticket stays in for the opposite reason: it was written before the work and is the only thing that can contradict my claim about my own diff.
Findings loop back to the same reviewer until it approves, rather than to a fresh one that can't tell an addressed finding from one that never existed. Three rounds, or a finding that recurs after being reported fixed, and it stops for a human instead. The unattended callers park the task rather than wait for an answer nobody is awake to give.
I gave the stance a substantiation floor. An agent told to attack will manufacture findings to satisfy the instruction, and a manufactured finding costs a round and teaches the author to discount the next review.
subagents.md said three separate times not to dispatch work this small, so it now carries an Isolation Override: the size heuristics assume the main thread could do the job equally well, and they lapse when its own context is what makes its answer untrustworthy. Field 2 of the Prompt Contract inverts there. Paste your context in and you hand over the very model you spawned the agent to escape.
I wrote it by running it on itself: three rounds and thirteen findings. Two were things I'd have shipped. Withholding the ticket made my own claim self-certifying, and the override reaffirmed the contract field that would have destroyed the isolation.
Diffstat (limited to 'publish/SKILL.md')
| -rw-r--r-- | publish/SKILL.md | 164 |
1 files changed, 140 insertions, 24 deletions
diff --git a/publish/SKILL.md b/publish/SKILL.md index a93f3dd..d2b906c 100644 --- a/publish/SKILL.md +++ b/publish/SKILL.md @@ -222,28 +222,142 @@ Step 0. Upstream can advance during a long session, especially across machines or with teammates pushing in parallel. Run Step 0 every time the publish flow starts. -### Step 1: local code review (mandatory) - -Run the `review-code` skill against the change: - -- Before a commit: `/review-code --staged` -- Before a PR: `/review-code` (branch diff against `main` merge-base) -- Before commenting on someone else's PR: `/review-code <PR#>` - -Surface **all** findings to the user: Critical, Important, and Minor. - -**Default block:** any Critical or Important finding stops the flow. Fix the -issues and re-run `/review-code` until the diff is clean. Minor findings are -shown but do not block. - -**Override:** the user can bypass the block with an explicit "proceed anyway" -(or equivalent wording). Without the explicit override, do not proceed to -Step 2. - -The `review-code` skill already has a Phase 0 eligibility gate that handles -trivial and ineligible diffs (whitespace-only, revert with obvious -justification, already-reviewed SHA). Trust that gate; there is no "trivial -enough to skip review" exemption on top of it. +### Step 1: adversarial review by an isolated reviewer (mandatory) + +The review runs in a **subagent**, never inline, and it runs on **every** +commit. The author does not review their own work. + +**Why isolation, not just review.** A self-review checks the diff against the +author's own model of what the diff should do. It cannot check the model. The +errors that survive a self-review are the ones that were never visible in the +diff — a scope inherited from whoever reported the problem, a blast radius +estimated instead of measured, a fix that is correct for the case the author +had in mind and wrong for the one they never considered. Only a reviewer that +does not hold the author's model catches those, so the isolation is the point +and the adversarial stance is the method. + +**Dispatch contract.** Spawn the reviewer via the Agent tool and give it these +three things, the third whenever one exists: + +1. **The diff** — `git diff --cached` for a commit, the branch diff for a PR. +2. **The claim** — one line from the author stating what the change does. Write + it before dispatching. This is the thing under test: the reviewer's job is + to check the diff against the claim. +3. **The requirement source, when one exists** — the ticket, plan, ADR, or task + body the work was done against. Pass it verbatim. + +Withhold everything else: the conversation, the exploration, the dead ends, and +above all the author's reasoning for why the change is right. Those are what +transmit the author's model, which is what the reviewer exists to not have. A +reviewer given the rationale reviews the rationale. + +**Why the requirement source is not withheld.** A ticket or plan is not the +author's model of the change — it is the independent record of what was asked, +written before the work and usually by someone else. It is the only artifact +that can contradict the author's one-line claim. Withhold it and the claim +becomes self-certifying: the reviewer checks the diff against a sentence the +author wrote, which cannot surface scope creep or a missing requirement. That +also strands `review-code`'s Intent-vs-Delivery criterion, which is skipped +outright when no intent context is supplied and is the one criterion aimed at +the inherited-scope error this whole gate exists to catch. + +Invoke the review with `/review-code --staged` (commit), `/review-code` (branch +diff against the `main` merge-base), or `/review-code <PR#>` (someone else's +PR), and tell it to run its adversarial pass. + +**Adversarial, with substantiation.** The reviewer is prompted to *refute* the +change rather than to bless it. But an agent told to attack will manufacture +findings to satisfy the instruction, so the stance carries a floor: a finding +that cannot be substantiated against the diff is not a finding and must be +dropped. `review-code`'s confidence filter and false-positive filter are what +enforce that floor — adversarial raises the appetite for looking, never the +tolerance for a weak claim. + +**Scope is every commit; the reviewer decides triviality, not the author.** +There is no "trivial enough to skip" exemption. A floor written in terms of +"small" or "mechanical" puts the judgment back with the author, whose judgment +is the thing being checked. Dispatch always, and let `review-code`'s own Phase 0 +eligibility gate return fast on a whitespace-only diff, an obvious revert, or an +already-reviewed SHA. A cheap spawn on a trivial commit is the price of the +author never getting to rule on their own diff. + +**Verdict, and the re-review loop.** The reviewer returns one of four outcomes, +and all four are defined exits: + +- **Approve** — the gate is satisfied. Proceed to Step 2. +- **Skipped** — `review-code`'s Phase 0 found the diff ineligible (whitespace + only, an obvious revert, an already-reviewed SHA). This **satisfies the gate** + and the flow proceeds. A skip is a reviewer's ruling, which is the point; what + is forbidden is the *author* ruling their own diff too trivial to look at. +- **Request Changes** — blocking findings stand. Enter the loop below. +- **Needs Discussion** — the reviewer has a disagreement it cannot settle from + the diff: an architectural objection, a question about whether the change + should exist at all. This **stops immediately and goes to the user**; it does + not enter the loop. Routing it to the loop would answer "should we do this?" + with "fix these findings," which is the wrong question and burns rounds on a + disagreement no amount of editing resolves. Unattended callers park it exactly + as they park a bound hit. + +Approval is the reviewer's to give; the author never declares their own change +clean. + +That set is closed. `review-code` emits Approve, Request Changes, or Needs +Discussion, and its Phase 0 emits Skipped; every one has a defined exit above. A +verdict outside those four means the reviewer went off-contract — surface it +rather than interpreting it. + +**The loop turns on blocking findings, not on the verdict token.** It ends when +no Critical or Important finding stands. A Minor-only result is not grounds for +another round: fix it or don't, but do not spend a round on it, and do not +escalate to the user over one. A reviewer holding only Minor findings should +return Approve and say what it left. + +On Request Changes: + +1. Surface **all** findings — Critical, Important, Minor. Critical and Important + block; Minor is shown and does not block. +2. Fix the blocking findings. +3. **Re-review, and keep re-reviewing until the reviewer approves.** A fix is a + new change and gets the same scrutiny as the original. Fixing under review + pressure is exactly when a regression gets introduced, so an unreviewed fix + is the hole this loop closes. + +**Continue the same reviewer, don't spawn a fresh one.** Send the updated diff +back to the existing reviewer (`SendMessage` with its agent ID). It holds its own +findings, so it can confirm each one is actually addressed. A fresh reviewer each +round cannot tell "addressed" from "never existed", re-litigates settled points, +and drifts to a new set of findings every round, which never converges. + +The continued reviewer must **re-verify each finding against the new diff**, not +against the author's description of the fix. "I fixed it" is a claim, and taking +it at face value is how a review round becomes a rubber stamp. + +**Bounds, so the loop terminates.** Two conditions end it early and hand the +decision to the user: + +- **Three rounds without approval** (the initial review plus two re-reviews). + This matches the two-fix-attempts limit in `subagents.md`: past that, the + problem is usually the approach rather than the diff. +- **A finding recurs after being reported fixed.** That is oscillation — the + fix for one finding reintroducing another — and another round will not + resolve it. Stop on the first recurrence rather than spending the remaining + rounds. + +In both cases, stop and surface: the standing findings, what was tried, and the +decision needed. + +**Override.** The user can bypass the block with an explicit "proceed anyway" (or +equivalent). The user is also the adjudicator when the author believes a finding +is wrong: say so with the reasoning and let the user rule. Do not resolve a +disagreement with the reviewer by overruling it silently. Without an explicit +override, do not proceed to Step 2. + +**When the Agent tool is unavailable.** Per `subagents.md`, don't block: run the +review in the main thread, but hold it to the same contract — review against the +stated claim, refute rather than bless, substantiate every finding, loop on fixes +until clean, same bounds. State plainly that the review was not isolated, because +a self-review under an adversarial prompt is weaker evidence and the user should +know which one they got. ### Step 2: draft, review, publish @@ -307,8 +421,10 @@ this file — load it when the artifact is a PR. **Exception:** trivial one-liners the user dictated verbatim in the conversation (e.g. "commit this as `chore: bump version`", "reply just 'thanks for the review'") can skip the draft-file step in Step 2. -`/review-code` in Step 1 still runs when it applies; Phase 0 of that skill -handles trivial diffs, and acknowledgment-only replies don't need it at all. +Step 1's review still runs on every commit — this carve-out is about the +draft-file step, not the review. Phase 0 is what rules a trivial diff out, and +its Skipped result satisfies the gate. An acknowledgment-only PR reply commits +nothing, so there is no diff to review. **Single-skill gate.** Each of the three subflows above runs `/voice personal` before printing the draft — the full pattern walk covering AI-writing signs, universal good-writing rules, Craig's voice patterns, and the artifact-mechanics patterns (first-person rewrite, public-artifact scope flag, praise/correction asymmetry, finding stems). Publish artifacts (commits, PR titles + bodies, PR review comments) always use personal mode; the `.ai/`-tracking check at the top of Step 2 decides only whether the approval gate fires, not which patterns run. Running the skill is mandatory; the printed draft must have been through it. When the user asks mid-flow for "the voice pass" on an in-progress draft, that means re-run the full pattern walk — not a subset. Always state that the skill ran when announcing the printed draft (e.g. "/voice personal — full pattern walk"). Skipping the pass without flagging it is a defect. The terse/omit-needless-words cut (pattern #38) is the *last* thing the skill does before the draft is printed: read each sentence and cut it in half, keeping only what changes meaning. The draft the user first sees must already be terse — if they have to ask for an Orwell pass after seeing it, the pass was skipped. |
