diff options
Diffstat (limited to 'publish')
| -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. |