From 80624606b8a60d97d8c7600882ddc35f5225469f Mon Sep 17 00:00:00 2001 From: Craig Jennings Date: Tue, 28 Jul 2026 21:28:29 -0500 Subject: 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. --- review-code/SKILL.md | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) (limited to 'review-code/SKILL.md') diff --git a/review-code/SKILL.md b/review-code/SKILL.md index 05bc6af..559cee8 100644 --- a/review-code/SKILL.md +++ b/review-code/SKILL.md @@ -33,7 +33,31 @@ When intent context is given, the review grades "does this match what was asked? ## Execution Model -For substantive reviews on large diffs: **dispatch the perspective passes as parallel sub-agents** via the Agent tool. Each sub-agent starts with a clean context window — the reviewer shouldn't inherit the implementer's mental model. For small single-commit tweaks, run inline. +Two levels of dispatch, and conflating them is the mistake to avoid. + +**Level one — who runs this skill.** When invoked from the `publish` flow's Step 1 gate, this skill is already running inside an isolated reviewer subagent that the publish flow spawned. That isolation is not optional and not this skill's call: the author never reviews their own change. If you are reading this in the same context that wrote the diff, the publish flow was not followed. + +**Level two — how the perspectives run inside it.** For substantive reviews on large diffs, dispatch the Phase 2 perspective passes as parallel sub-agents. For small single-commit tweaks, run them inline. This is a cost decision about fan-out *within* the review and never a licence to skip level one. + +### The adversarial contract (publish-flow reviews) + +When running as the publish flow's reviewer, operate under these terms: + +- **You have three inputs, and the boundary is deliberate.** The diff; a one-line claim from the author about what it does; and, when one exists, the requirement source it was built against (ticket, plan, ADR, task body). You were *not* given the conversation, the author's reasoning, or the alternatives they rejected, because those transmit the author's model of the change and your value is in not holding it. Do not ask for them. +- **The requirement source is not leakage — use it.** It was written before the work and usually by someone else, so it is the one input that can contradict the author's claim about their own diff. Without it, the claim is self-certifying and you can only check the diff against a sentence the author wrote. It is what makes the Intent-vs-Delivery criterion below live rather than skipped, and scope creep and missing requirements are exactly what it catches. +- **Review the diff against the claim.** Does it do what the claim says? What does it do that the claim does not mention? What breaks that the claim assumes is fine? +- **Try to refute the change, not to bless it.** Assume there is something wrong and go looking. The default posture is skepticism. +- **A finding you cannot substantiate against the diff is not a finding.** Drop it. An agent told to attack will manufacture findings to satisfy the instruction, and a manufactured finding costs the author a round of work and teaches them to discount the next review. The confidence filter and false-positive filter in Phase 4 are what hold this line: adversarial raises how hard you look, never how weak a claim you will ship. +- **Verify the premise before judging the fix.** When the diff claims to fix a bug, confirm the bug is real and reproduces in the pre-change code. A fix for a misdiagnosed problem passes a diff-shaped review and is still wrong. + +### Re-review mode (continued rounds) + +The publish flow sends the updated diff back to the *same* reviewer rather than spawning a fresh one, so a continued round has your prior findings in context. On each round: + +1. **Re-verify every prior blocking finding against the new diff.** Not against the author's description of the fix. "I fixed it" is a claim like any other, and accepting it is how a re-review becomes a rubber stamp. +2. **Review the fix as a new change.** A fix written under review pressure is prime ground for a regression, so it earns the same scrutiny as the original diff, including on lines the fix touched incidentally. +3. **Say when a prior finding recurs.** If something reported fixed is back, name it as a recurrence rather than filing it fresh. The publish flow treats recurrence as oscillation and stops the loop, which is the right outcome — another round will not converge. +4. **Do not invent new findings to justify another round.** If the blocking findings are addressed and nothing new is substantiated, approve. Approval is the expected end state, not a failure to find something. ## Phase 0 — Eligibility Gate @@ -437,7 +461,7 @@ None. ## Hand-Off -- **Critical** → must be addressed before merge; author fixes, re-review via `/review-code` on the updated SHA +- **Critical** → must be addressed before merge; author fixes, then the updated diff comes back to *this* reviewer for another round (publish flow, Step 1), not to a fresh one and not to the author's own judgment - **Important** → fix, or deliberately defer with an ADR (run `/arch-decide`) - **Minor** → follow-up issues or a cleanup PR - **Intent-vs-Delivery gaps** → either file tickets for the missing pieces or update the plan to reflect reality -- cgit v1.2.3