Roast My Code

Take the diff between HEAD and a fixed point the user names, and interrogate it along three independent axes:

• Standards — does the code live up to this repo's documented coding standards?
• Spec — does the code faithfully implement the originating spec/ticket?
• Security — does the change open a vulnerability (injection, auth bypass, data exposure, unsafe deserialization, secrets, etc.)?

The three interrogations run as parallel subagents so no axis contaminates another's context; this skill then aggregates their findings.

Every finding gets a severity from this closed set: Critical (broken/security) → Required (must fix before merge) → Nit (cosmetic) → Optional (nice but not required) → FYI (informational). Closed set = the severity IS the categorical feature; tallying verdicts across subagents stays deterministic. No free-form severity prose.

Process

1. Fix the comparison point

Whatever the user said — a commit SHA, branch name, tag, main, HEAD~5. If they didn't specify one, ask.

Capture the diff command once: git diff <fixed-point>...HEAD (three-dot, against the merge-base). Also note the commit list via git log <fixed-point>..HEAD --oneline.

Before going further, confirm the fixed point resolves (git rev-parse <fixed-point>) and the diff is non-empty. A bad ref or empty diff should fail here — not inside two parallel subagents.

2. Locate the spec

Find the originating spec, in order:

1. Issue references in commit messages (#123, Closes #45, etc.).
2. A path handed over as an argument by the user.
3. A spec file under docs/, specs/, or .trust/ matching the branch name or feature.
4. If nothing is found, ask the user. If there isn't one, the Spec subagent is skipped and reported as "no spec available".

3. Locate the standards sources

Anything in the repo documenting how code should be written, such as CODING_STANDARDS.md or CONTRIBUTING.md.

Beyond whatever the repo documents, the Standards axis always carries the smell baseline below — a fixed set of Fowler code smells (Refactoring, ch.3) that applies even when a repo documents nothing. Two rules bind it:

• The repo wins. Where a documented standard endorses something the baseline would flag, the baseline stays silent.
• A smell is a suspicion, not a verdict. Each entry is a labelled heuristic ("possible Feature Envy"), never a hard violation — and, like any standard here, skip anything tooling already enforces.

Every smell reads what it is → the fix; match each against the diff:

• Mysterious Name — a name that doesn't reveal what it does or holds. → rename; if no honest name comes, the design's murky.
• Duplicated Code — the same logic shape in more than one hunk or file. → extract the shared shape, call it from both.
• Feature Envy — a method reaching into another object's data more than its own. → move the method onto the data it envies.
• Data Clumps — the same few fields/params always travelling together (a type wanting to be born). → bundle them into one type.
• Primitive Obsession — a primitive standing in for a domain concept deserving its own type. → give the concept its own small type.
• Repeated Switches — the same switch/if-cascade on the same type recurring. → polymorphism, or one shared map.
• Shotgun Surgery — one logical change forcing scattered edits across many files. → gather what changes together into one module.
• Divergent Change — one module edited for several unrelated reasons. → split so each module changes for one reason.
• Speculative Generality — abstraction/parameters/hooks for needs the spec doesn't have. → delete; inline until a real need shows.
• Message Chains — long a.b().c().d() navigation. → hide the walk behind one method on the first object.
• Middle Man — a class/function that mostly delegates onward. → cut it, call the real target direct.
• Refused Bequest — a subclass ignoring/overriding most of what it inherits. → drop the inheritance, use composition.

4. Spawn all three subagents in parallel

Send a single message with three parallel subagent calls.

Blind reviewers (never leak the parent's verdict). A reviewer that reads the orchestrator's conclusion first tends to audit that conclusion instead of the diff. Each subagent prompt carries ONLY: the diff command, the commit list, its axis brief, and the reference material its axis needs — never the orchestrator's own assessment, suspicions, or summary of the change. If a harness forces you to pass context, pass the inputs to verify, not the conclusions drawn. Findings raised independently by more than one axis are the highest-signal findings — say so in the aggregate.

Standards subagent prompt — include:

• The diff command and the commit list, complete.
• The list of standards-source files from step 3, plus the smell baseline from step 3 pasted in full — the subagent has no other access to it.
• The brief: "Report — per file/hunk where relevant — (a) every place the diff violates a documented standard: cite the standard (file + rule); and (b) any baseline smell you spot: name it and quote the hunk. Distinguish hard violations from judgement calls — documented-standard breaches can be hard, baseline smells are always judgement calls, and a documented repo standard overrides the baseline. Skip anything tooling enforces. Assign a severity to each finding. Under 400 words."

Spec subagent prompt — include:

• The diff command, plus the commit list.
• The spec itself: its path, or the fetched contents.
• The brief: "Report: (a) requirements the spec asked for that are missing or partial; (b) behaviour in the diff that wasn't asked for (scope creep); (c) requirements that look implemented but where the implementation looks wrong. Quote the spec line for each finding. Assign a severity. Under 400 words."
• Test-strategy check (part of this axis, no extra subagent): tests in the diff are the instruments that verify the spec, so examine them for two verified failure classes — (i) tautological assertions (assert.ok(true) placeholders, expectations recomputed from the code's own output like === (2+3) instead of intent-derived literals like === 5, tests that cannot fail by construction); (ii) test/impl contract mismatch (the test reads path A while the implementation writes path B, different function/flag/shape names — a suite that passes while testing nothing real). Both are at minimum Required: a green suite either way is a certificate with no exam. Live exhibits: C1 rounds on DSH/Codex (assert.ok(true) ×2 plus ledger.test.jsonl vs ledger.jsonl).

If the spec is missing, skip the Spec subagent and note this in the final report.

Serial fallback — if the harness cannot spawn parallel subagents (no delegation tool, or single-threaded delegation), run the same three prompts sequentially, treating each axis as its own fresh context: finish one axis's report before starting the next, and carry nothing but the axis briefs between them. The isolation goal — no axis contaminating another's context — matters more than the parallelism; aggregation (step 5) is unchanged.

Security subagent prompt — include:

• The diff command plus the commit list.
• The brief: "Report — per file/hunk where relevant — every place the diff introduces or widens a security vulnerability: injection (SQL, command, XSS), auth/authz bypass, sensitive data exposure, unsafe deserialization, hardcoded secrets, missing input validation at a trust boundary, insecure defaults. Distinguish hard vulnerabilities from hardening opportunities. Skip anything tooling already enforces (e.g. a secrets scanner). Assign a severity to each finding. Under 400 words."

5. Aggregate

Present the three reports under ## Standards, ## Spec, and ## Security headings, verbatim or lightly cleaned. Do not merge or rerank findings.

End with a one-line summary: total findings per axis, and the worst issue within each axis (if any). Don't pick a single winner across axes.

Review-loop contract

A review that finds something opens a loop: fix, re-review, new findings, fix again. Left unbounded, that loop never closes — an agent author and an agent reviewer can keep each other busy indefinitely, each round surfacing one more thing. So the loop declares its exit before round 1, and reaching the cap is a STOP with a report, never a silent continue. Same contract shape as make-it-so §Termination contract, applied to review: a loop with no declared exit is a stall wearing a review's clothes.

A round is one full three-axis pass over the change. Fixes made between rounds are not rounds.

1. Success condition — categorical, checkable in one step: every Critical and Required finding is either fixed or dismissed with a one-line reason, and the latest round raised no new Critical. Nit, Optional, and FYI never block the exit — they are recorded, not chased.
2. Hard round cap — 3 rounds, where round 1 is the initial full review. The cap is a number, not a suggestion. Raising it is a human decision, recorded in one line (Ruling: review cap raised to N — why), never a self-made extension.
3. Terminal action — on the final round, emit the aggregate report plus the findings still open, and hand the decision to the human. There is no "one more round".

Three rules keep the loop converging instead of oscillating:

• Round 2+ reviews the new diff only — git diff <last-reviewed-sha>...HEAD, not the whole branch. Re-reading unchanged hunks is how a reviewer keeps re-finding old bugs and calls them new ones.
• No repeats — a finding already posted is never posted again. If it is still present, mark it unresolved (the fix did not land); restating it inflates the tally and hides that the loop is stuck.
• Flag the class, not the instance — a finding that recurs across rounds, or repeats across several hunks, is a class: name the class and fix it at the source (a rule, a lint check, a test, one shared helper) rather than the single line that got flagged.

Report the round explicitly (Round 2/3) so the loop's position is a fact in the record, not a feeling.

Why three axes

A single-axis review gives every change one way to look acceptable, which means every change has a way to hide its failure:

• Code that meets every documented standard yet builds the wrong thing → Standards pass, Spec fail.
• Code that does exactly what the ticket asked but breaks the project's conventions → Spec pass, Standards fail.
• Code that is correct and well-styled but introduces a vulnerability → Standards + Spec pass, Security fail.

Reporting the axes separately is what keeps one axis's pass from laundering another axis's fail.