aboutsummaryrefslogtreecommitdiff
path: root/review-code
diff options
context:
space:
mode:
Diffstat (limited to 'review-code')
-rw-r--r--review-code/SKILL.md53
1 files changed, 40 insertions, 13 deletions
diff --git a/review-code/SKILL.md b/review-code/SKILL.md
index ec08a9d..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
@@ -261,7 +285,7 @@ Do **not** flag any of these as issues:
- **Issues explicitly silenced in code** (e.g., `# type: ignore[...]` with a reason, lint ignore comments) unless the silencing is unjustified
- **Intentional changes** in functionality clearly related to the PR's stated goal
- **Changes in unmodified lines** (real issues in files the PR touches but on lines it doesn't change)
-- **Framework behavior being tested** — see `testing.md` anti-patterns
+- **Framework behavior being tested** — see the `testing-standards` skill, anti-patterns
### Severity Categorization
@@ -273,9 +297,9 @@ Remaining issues get tagged:
## Phase 5 — Output
-**Terminal display — plain text only.** Everything this skill echoes to Craig in the chat terminal — the structured report, the per-criterion table, the verdict, and any draft summary or inline awaiting approval — must be rendered as plain text. No bold (`**`), no backtick code spans, no markdown tables, no headings-as-markup. They render as reverse video in his terminal and are hard to read. Write `file.py:42` as bare text, identifiers and `test:`-style prefixes unquoted, the criterion audit as a plain dashed list rather than a table, severity tiers as plain labels. This applies only to the terminal echo. The review actually posted to GitHub (the `gh api` body and inline blocks) keeps normal markdown — GitHub renders it correctly, so draft the posted artifact in markdown and strip the markup only when mirroring it into the chat. (Same rule as `interaction.md`'s no-reverse-video constraint; repeated here because the violation happens at exactly this print step.)
+**Terminal display — plain text only.** Everything this skill echoes to Craig in the chat terminal — the structured report, the per-criterion table, the verdict, the draft summary, and the full prose of every inline pin awaiting approval (the exact words, never a description of them) — must be rendered as plain text. No bold (`**`), no backtick code spans, no markdown tables, no headings-as-markup. They render as reverse video in his terminal and are hard to read. Write `file.py:42` as bare text, identifiers and `test:`-style prefixes unquoted, the criterion audit as a plain dashed list rather than a table, severity tiers as plain labels. This applies only to the terminal echo. The review actually posted to GitHub (the `gh api` body and inline blocks) keeps normal markdown — GitHub renders it correctly, so draft the posted artifact in markdown and strip the markup only when mirroring it into the chat. (Same rule as `interaction.md`'s no-reverse-video constraint; repeated here because the violation happens at exactly this print step.)
-Before printing any approve/request-changes summary for posting, run the praise/correction gate (see Posted Summary Voice, and `/voice` personal #40): scan the summary and cut every clause that describes or justifies a good change — keep praise plus verdict only. Then confirm each finding and change-request states its why, gently and briefly. This is a mechanical pass, not a judgment call.
+Before posting, print the full summary body AND every inline comment exactly as it will post — never the summary alone with an inline merely described ("I'd pair it with one inline on..."). Craig approves the exact words that post under his name, so the exact words must be on screen. Then run the praise/correction gate (see Posted Summary Voice, and `/voice` personal #40): scan the summary and cut every praise clause and every clause that describes or justifies a good change — an approve summary is the substantive pointer plus the verdict, no praise, not even a bare positive. Then confirm each finding and change-request states its why, gently and briefly. This is a mechanical pass, not a judgment call.
```markdown
# Code Review — <PR title / branch name / SHA range>
@@ -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
@@ -446,21 +470,24 @@ None.
The summary body and the inline pins work as a pair: scannable verdict on top, full coaching conversation in the pins. Read this section paired with Inline Comment Voice below — the summary is terse precisely because the inlines carry the teaching weight.
-The structured report above stays local. When the verdict is posted as a GitHub review (per `commits.md` Step 2 Shape 1), keep the summary body terse — one long sentence or a few short ones is plenty. Vary the phrasing run-to-run so consecutive reviews don't read templated. Voice: an encouraging senior dev who doesn't like to talk; positive feedback is short, blunt, and lands cleanly.
+The structured report above stays local. When the verdict is posted as a GitHub review (per the `publish` skill, Step 2 Shape 1), keep the summary body terse — one long sentence or a few short ones is plenty. Vary the phrasing run-to-run so consecutive reviews don't read templated. Voice: an encouraging senior dev who doesn't like to talk. Lead with the substantive pointer — the design note or blocker that's pinned inline — and close with the verdict; the summary carries no praise clause.
-Name the good thing and stop: do not explain *why* it's good. The author made the change and already knows the rationale, so justifying the praise reads as sycophantic. "Clean migration off the window globals, tests cover the right edges" lands; appending "...because there are no stragglers and the provider, mocks, and Normal/Boundary/Error cases are all covered" turns a compliment into padding. Elaboration is for findings (something is wrong, here's the failure mode and the fix), never for compliments.
+The summary body carries no praise — not a named good thing, not a bare positive. The author made the change and already knows its merits, so a compliment in a terse summary reads as filler or sycophancy. If a genuine positive is worth surfacing, it goes as a single inline pin on the relevant line (see below), never the summary body. Elaboration in the summary is for the substantive pointer and for findings — what's wrong, the failure mode, the fix — never for compliments.
-This holds for re-review approvals too. A re-review confirming requested changes is just "Approving." Mechanical rule: an approve summary is the verdict plus at most a bare positive ("Clean.", "Solid fix."). It must contain no clause that says what the change does or why it works. "The hoist to App fixes the crash, and the new test locks it in" is the banned pattern — it describes and justifies the change on an approve. If a clause references the code's behavior, cut it.
+This holds for re-review approvals too. A re-review confirming requested changes is just "Approving." Mechanical rule: an approve summary is the substantive pointer (the inline design note, if any) followed by the verdict — no praise, not even a bare positive. Lead with the pointer, close with the verdict: "One design note inline, not a blocker. Approving." An approve with nothing to flag is just "Approving." No clause may say what the change does or why it works, and none may compliment it — "Clean.", "Solid fix.", "The hoist to App fixes the crash and the new test locks it in" are all cut. If a clause references the code's behavior or praises it, cut it.
-The asymmetry: praise gets no why, but a finding, change-request, or inline coaching note *always* gets the why. Behavior only changes when the reason lands, so a correction that just says what to fix without saying why teaches nothing. Deliver that why gently and briefly, the way a kind coach would, never as a verdict from on high. The praise-strips / correction-explains split is enforced as `/voice` personal pattern #40, which every posted review summary passes through.
+The asymmetry: the summary drops praise entirely, but a finding, change-request, or inline coaching note *always* gets the why. Behavior only changes when the reason lands, so a correction that just says what to fix without saying why teaches nothing. Deliver that why gently and briefly, the way a kind coach would, never as a verdict from on high. The drop-praise / correction-explains split is enforced as `/voice` personal pattern #40, which every posted review summary passes through.
Good:
-- "Nice, clean, good coverage. One small naming point inline. Approving."
-- "Clean shape, tests cover the right edges. Approving."
-- "Solid. One blocker inline — see the auth gap. Request changes."
+- "One small naming point inline, not a blocker. Approving."
+- "One edge-case gap noted inline, minor. Approving."
+- "One blocker inline — see the auth gap. Request changes."
+- "Approving." (nothing to flag)
-Bad (chatty, padded, marketing-adjacent):
+Bad (chatty, padded, or any praise on an approve):
- "Great work overall! This is a really clean addition. The OneToOne relationship behaves as expected, the migration is correctly dependent on 0028, CI is green across all backend/frontend checks, and the tests cover Normal/Boundary/Error cases. One small naming nit inline — fine to roll into a follow-up."
+- "Clean, good coverage. One naming point inline. Approving." — the leading praise is now cut; lead with the pointer instead.
+- "Solid fix. Approving." — a bare positive is still praise; drop it.
If specific praise lands somewhere, surface it as a single inline comment on the relevant line, not in the summary body. The summary stays scannable; the inline pins carry the specifics.