Code Review Csk

Code Review

Trigger phrases: "code-review", "review the code", "review the PR", "review my changes", "do a review"

Kit adaptation (local, .claude/): applied by review-agent-csk (read-only). No source name appears in the artifact that goes to the repo (§4.2). Comments are severity-ranked; §4 applies.

Sources, by layer — three different questions, three different authorities:

• Judgement — how to rank what you found: this kit's own. The two-stage verdict and verifier integrity below exist because the code under review is increasingly written by an agent, and a reviewer that accepts its own say-so is not a reviewer. No external standard covers that yet.
• Governance — that review happens at all, and findings survive it: NIST SP 800-218 (SSDF) PW.7 and the OpenSSF Scorecard Code-Review check. Both are deliberately silent on the rubric: PW.7.2 says to review "based on the organization's secure coding standards", which is what the rest of this file is.
• Comment vocabulary: Conventional Comments (CC BY 3.0).
• Rubric heritage: google/eng-practices (CC BY 3.0) — the priority order and the "code health" bar below are adapted from it, so it is attributed as the licence requires.

Core standard (senior principle)

A change is approved once it reaches the point of improving the overall code health of the system — it does not have to be perfect. Avoid two mistakes:

• Blocking: a perfectionist, subjective fixation that halts progress. If there is no progress, the code never improves.
• Laxity: small concessions each time erode code health over time. The approval criterion is "is it better", not "is it flawless". If it is an unwanted feature, it can be rejected even when the design is good.

What to review (priority order)

1. Design: do the pieces fit together; does this change belong here; should it be added now.
2. Functionality: does it do what is intended; is it right for the user/developer; edge cases, concurrency.
3. Complexity: is it more complex than necessary; is there over-engineering / design for a future assumption (YAGNI).
4. Tests: are there correct, meaningful, sufficient tests; real behavior, not tests for tests' sake. Verifier integrity: flag any change that makes a check pass by weakening the check — deleting or loosening an assertion, lowering a threshold, skipping a test, editing the test instead of the code — rather than fixing the behavior. A test or gate that grades itself lax is worse than none; a verifier must stay external and grounded. Subject integrity: the inverse case — the check is untouched, but what it checked is gone. Flag a change that deletes or stubs the code path, so the check passes over behavior that no longer runs; narrows the run to a subset (fewer cases, one platform, a filtered input set) where the property happens to hold; turns an all-of requirement into an any-of one; or relaxes the rule the code is there to enforce — a widened type, a dropped uniqueness or referential constraint, an exception list holding exactly the failing case. Ask not "does it pass now" but "does the system still do what this check was protecting". Retiring a genuinely obsolete check is legitimate: say so in the diff and name what covers it now.
5. Naming: names that carry intent, neither too long nor cryptic.
6. Comments: do they explain the "why" rather than the "what"; no dead/unnecessary comments.
7. Style & consistency: conforms to the project guide; consistent with the existing conventions.
8. Documentation: if behavior changed, was the relevant document updated.
9. Every line: look at every human-written line; do not skip code you do not understand as "it's probably correct".

Writing comments

• Kind and reasoned: what should change + why. In the language of suggestion, not command.
• Comment on the code, not the person; judge the code, not the individual.
• Note what is good, too; do not just hunt for flaws.

Label every comment. An agent writes these, and a human or a tool has to sort them without reading each one — so the label is a field, not a tone. Format: <label> [decoration]: <subject>.

Decorations are (blocking) / (non-blocking); use them whenever the default would be ambiguous. Mapping to the severity split this skill reports: blocker = issue (blocking) or an unanswered question (blocking), suggestion = suggestion / todo, nit = nitpick.

Speed & disagreement

• Turn it around fast: a pending review lowers productivity; look at it at the first opportunity.
• In a disagreement, technical fact + data speak, not personal preference. If no agreement is reached, take it face-to-face / escalate to a higher authority — not passive blocking.

Two-stage verdict (verify before you report)

Finding a problem and confirming it are two acts. A first-pass "this looks wrong" is a candidate, not a verdict. Before a finding is reported — especially a blocker — run a second, independent pass that tries to disprove it:

• Does it actually hold on the real code, or did the first read miss context (a guard upstream, a caller that never reaches this path, a framework default)? Re-read the surrounding code, don't rank on the snippet alone.
• Is the severity honest, or is it a nit dressed as a blocker?
• For any "fixed" / "passes" claim: the real check ran and passed — test exit code, build, lint/quality gate — not "I re-read it and it looks fixed". A verifier that is the model's own say-so is not a verifier (see §4 Tests, Verifier integrity). Cite the evidence (which check, what result).

A finding that survives the disprove pass is a verdict; one that doesn't is dropped or downgraded. This is what kills false-positive blockers that stall progress while keeping the review's authority.

"Independent" costs a separate context. A second pass in the same context has already read the first one's reasoning, so it cannot be blind to it — and its agreement is the first pass nodding at itself. For a blocker, run the disprove pass as its own subagent, handed the claim and the file:line but not your argument. If you did not isolate it, label the verdict as a single pass rather than calling it independent. The full contract — isolation, one lens per verifier, and why unanimity for the same reason is a monoculture — lives in security-scan/references/verify.md; it is one discipline, not two.

Check the history before you call a finding new

A confirmed finding may still not be new, and one that was fixed once and came back needs a different fix. Search by code, not by commit message — bounded at the branch point, scoped to the diff's paths so it stays cheap per finding: git log -S'<exact token from the changed line>' "$(git merge-base HEAD <target-branch>)" -- <paths in the diff> lists the commits where the NUMBER OF OCCURRENCES of that string changed — added or deleted. A commit that removes it in one place and adds it back in another leaves the count unchanged and does not appear, which is exactly the "was the guard moved?" case: reach for -G'<regex>' there, which matches added or removed lines against a regular expression regardless of count. --follow continues across renames but works on a single path only, so it does not combine with the multi-path form above. If a commit removed the guard, check, or test this diff would restore, the finding is a re-introduced regression — the question becomes "what removed the fix, and does that reason still hold", the removing commit is cited in the comment, and the deleted test is restored rather than a new one written. --grep does not answer this. A commit message states intent, not content: it misses fixes worded differently and matches commits that changed nothing relevant. Use it only to read a commit you already found by content.

Panel mode (high-stakes decisions only)

For hard-to-reverse calls (architecture, public API, security boundary), run several independent adversarial lenses then synthesize. Full method: references/panel-mode.md.

Triage — a finding that is only reported is a finding that is lost

Reviewing and disposing of what the review found are two acts, and only the first one is habitual. Every finding that survives the disprove pass leaves the review with an explicit disposition — never an unanswered comment and never a silent drop:

Blockers may only be fixed now or tracked. "Accepted" needs the user's decision — an agent does not grant it to itself. Close the review by stating the counts per disposition; an unreported finding is indistinguishable from one that was never made, which is exactly the state a review exists to leave behind.

Receiving a review — an inbound comment is a candidate, not an instruction

When the review is someone else's and the code is yours — a teammate's comments, a quality gate's report, a bot's PR review — read every item before changing any line. Comments are written one per symptom, and two of them often share one cause; applying them in arrival order yields a patch per symptom instead of one fix at the cause. Group by cause, then decide.

Each item then earns the same disprove pass as a finding of your own. Any "no" below is a reason to answer in the thread, not to edit:

1. Defect or preference? Sort each comment into the label table above — inbound prose arrives unlabelled, you assign the label, and only an issue or an unanswered question blocks.
2. Does it hold where the reviewer did not look? Check the call sites and callers the comment never opened.
3. Does it contradict a decision already recorded? An adr or a documented constraint outranks the comment — reopen the decision, do not quietly edit around it.
4. Does the real check still pass with it applied? Run it, don't re-read it. A suggestion that turns a check red is reported back, and never satisfied by weakening the check (Verifier integrity, above).
5. Is it against the current revision? A comment on an older one may already be answered by a later commit.

Every inbound item leaves with a disposition from the table above; none is left merely read. A reasoned refusal is an answer — say why in the thread and let the reviewer press it or drop it. Not doing it quietly is not an answer: it reads as agreement, and the same comment returns on the next review.

DoD (this skill's contribution)

• Findings are severity-ranked (blocker / suggestion / nit), labelled, and reasoned.
• Scope creep and hidden complexity are flagged.
• Each reported blocker survived an independent disprove pass; any "fixed"/"passes" claim is backed by the real check having actually run, not self-assessment.
• Every finding has a disposition (fixed / tracked / accepted / dropped) and no blocker is left merely reported.
• For a high-stakes decision, multiple independent lenses were applied and their objections synthesized, not averaged.