git:20260901.36ba68b to git:20260902.6995fc4

- description: Generator-evaluator separation and review methodology — loaded by review agents to enforce fresh-context review discipline and gate verdicts; findings from the code, security, and docs reviewers are formatted per the conventional-comments skill. Trigger on "review this diff", "review these changes", "code review this", or "/code-review".

- Reviews must be performed by agents with fresh context. The generator (the

- agent that wrote the code) must never evaluate its own output.

- Write the prose this skill governs at a seventh-grade reading level, in

- STE-flavored mode. Full methodology: `writing-prose`. Call the Skill tool

- with `writing-prose` and apply its `## Self-lint` checklist before you

- finalize.

- conversation history this skill forbids. Do not review inline. Dispatch the

- `code-reviewer` agent (or, if unavailable, a fresh read-only subagent

- instructed to follow this skill) against the requested diff, then present

- its report in full, in the shape `## Report Format` pins — never a summary

- of it. Everything below is the methodology that dispatched reviewer

- applies.

-

- ## Generator-Evaluator Separation

-

- - Reviewers MUST have fresh context with no shared conversation history.

- - Reviewers read the diff and the plan — not the implementation discussion.

- - Reviewers form their own understanding of intent from artifacts, not from

- the implementer's explanation.

- - A reviewer needing clarification flags it as an open question. It never asks

- the implementer.

-

- The cross-gate canon lives at `skills/principle-generator-evaluator/SKILL.md`;

- this skill owns the code-review application.

-

- ## Veto Without Authorship

-

- Block the line, change nothing

- (`skills/principle-generator-evaluator/SKILL.md`).

-

- - **You hold no write tool.** Every reviewer agent has read-only tool grants

- and `permissionMode: plan`. Report the defect. Never fix it.

- The constraint is the withheld tool, not a request for restraint

- (`skills/principle-least-privilege/SKILL.md`).

- - **The veto holds until the finding is resolved.** Your verdict blocks the

- line for as many rounds as it takes, and a check that can never be satisfied

- grinds until a person stops the run. Report the finding you actually have —

- do not hold the line on one you cannot support with evidence.

-

- ## Conventional Comments

-

- Findings from the code, security, and docs reviewers use the Conventional

- Comments format in `skills/conventional-comments/SKILL.md`. The one exception

- is the ux-reviewer: its live-verification report uses its own

- Working/Broken/Could Improve format.

-

- ## Report Format

-

- One report shape binds every surface a code review crosses: the

- code-reviewer's final report, the report a subagent returns when it

- reviews a diff on a dispatcher's behalf, and the full output the top-level

- session presents after a direct invocation. A relay reproduces the report

- in full — never a paraphrase, never a subset. A reviewer that carries its

- own report template in its agent file (the security-reviewer, the

- ux-reviewer, the technical-writer, the verifier) keeps it; this shape

- governs the code review.

-

- ```markdown

- **Verdict: <✅ APPROVE | ❌ REQUEST CHANGES | 💬 COMMENT>**

-

- ### Summary

-

- <What was reviewed — the diff or range — and why the verdict. Two to

- five sentences.>

-

- ### Findings

-

- <One finding per entry, Blocking tier first. Exactly "No findings."

- when there are none.>

-

- ### Checks

-

- <Each done criterion, met or not met. The test-suite command and its

- result. Any other check run, with its result.>

-

- ### Refuted by verification

-

- <Findings the skeptic pass refuted. Exactly "Nothing refuted." when the

- pass ran and refuted none. Exactly "Not run: <reason>." when it did not

- run.>

-

- ### Cross-model disposition

-

- <The cross-model pass's per-round record, built per

- `skills/cross-model-review/SKILL.md`. Exactly "Not run: <reason>." when

- that pass did not run.>

- ```

-

- - **The verdict line comes first.** The orchestrator parses it. The

- tokens are the Code Reviewer list in `## Verdict Criteria` — no other

- token, no prose verdict. Each token carries its standard emoji prefix

- (✅ APPROVE, ❌ REQUEST CHANGES, 💬 COMMENT); the word token, not the

- emoji, is what the orchestrator matches on.

- - `### Findings` entries use the Conventional Comments format

- (`## Conventional Comments` above), each with its `file:line`

- reference.

- - **The output format is not a choice.** Emit all five headings, in the

- order the template gives them, on every report. Invent no section,

- rename none, move none, and drop none. Two reports of the same diff

- differ in what their sections say and never in which sections they

- have.

- - **A section with nothing to report says so on its own line** — the way

- `### Findings` reads "No findings." when there are none. The last two

- sections record the two optional passes, the skeptic pass and then the

- cross-model pass, and a pass that did not run says `Not run: <reason>.`

- in its section. This is where "skip loudly" lands in the report.

- What did not happen is reported as visibly as what did (`skills/principle-skip-loudly/SKILL.md`).

-

- ## Gate Types and Severity Tiers

-

- Call the Skill tool with `review-severity-tiers`. It owns how each reviewer's

- verdict gates the pipeline: the gate-type table, the Blocking,

- Major, and Minor tiers with the auto-fix boundary, the consult guard, and the

- verdict-aggregation rules.

-

- ## Verdict Criteria

-

- ### Security Reviewer

-

- - **PASS:** No CRITICAL or HIGH findings. MEDIUM/LOW findings are reported but

- do not block.

- - **FAIL:** Any CRITICAL or HIGH finding. The pipeline MUST loop back to

- IMPLEMENT. No override.

-

- ### Verifier

-

- - **PASS:** All detected checks (format, lint, typecheck, build, test) pass.

- - **FAIL:** Any check fails. The pipeline loops back to IMPLEMENT.

-

- ### Code Reviewer

-

- - **✅ APPROVE:** All done criteria met, no blocking issues, tests pass.

- - **❌ REQUEST CHANGES:** Blocking issues found. The pipeline MUST loop back to

- IMPLEMENT. No override.

- - **💬 COMMENT:** Non-blocking suggestions only. Implementation is correct.

-

- **Test-quality flags.** Test files are part of the diff. Walk every changed

- `*test*` / `*spec*` / `__tests__/*` file against the rules in `test-style` —

- call the Skill tool with `test-style`.

- These are `suggestion:` individually and `issue:` when they appear across

- multiple tests:

-

- - Change-detector tests — assertions on which collaborator methods were

- called without verifying observable state

- - Mock-everything / mock chains where a real or fake equivalent exists

- - Full-equality assertions on complex objects when one field carries the

- contract

- - Logic in tests (`if`, loops, string-building) that can carry the same bug as

- the code

- - Tests named after methods (`testProcessOrder_2`) rather than behaviors

- (`refundsCardOnPartialFailure`)

- - DRY helpers that hide the asserted value

-

- **Flaky-test red flags (always blocking).** Distinct from the style flags

- above. Any test in the diff whose *outcome depends on* a nondeterministic

- input is `issue (blocking)` on **first** occurrence, routing to the Blocking

- tier and auto-looping the implementer. A single time-bomb ships a guaranteed

- future CI failure, and flakiness erodes the "green means safe" signal. The

- rule keys to outcome-dependence, not token presence: a `Date.now()` in a log

- line does not flag; one feeding an assertion does. Outcome-dependence covers

- the whole suite — state or resources left behind flag because a *later* test's

- outcome depends on them. The full catalog lives in

- `skills/test-style/SKILL.md` ("Flaky-test red flags (reviewer checklist)").

-

- **Comment red flags.** Check in-source comments in every changed file against

- the Code Comments rules in `engineering-standards` — call the Skill tool with

- `engineering-standards`. Findings

- cite the checklist item by name and carry the tier's decoration — a

- blocking-regime hit reads `issue (blocking): Comment Discipline — ...`. Two

- regimes apply:

-

- - **Blocking on first occurrence** — ticket/issue IDs, plan/slice/phase

- markers, and doc-section references in code comments, plus TODO/FIXME

- comments the diff introduces. These checks are mechanical and

- judgment-free, and the references rot.

- - **Style escalation** — comments restating WHAT the code does, wordy or

- narrating comments, commented-out code, process narration, comments far

- from the code they explain, vague language ("handle edge case"),

- speculation, duplication of what types/tests/names/docs already carry,

- fragile positional references, style diverging from the repo convention,

- doc comments restating a signature, and a stale comment the diff leaves

- contradicting the changed code. `suggestion:` for a single occurrence,

- `issue:` when repeated. A single what-comment never blocks a round.

- Discriminant for a stale-comment mismatch: when the changed code meets the

- plan's done criteria, the stale comment is the finding; when the code

- diverges from them, raise Correctness instead.

- - **Not violations:** upstream-bug links where the link IS the why.

- Ticket-like tokens outside comment syntax — string literals, log messages,

- fixture data (the check reads comments only). Doc comments on

- exported/public interfaces. A pre-existing TODO the diff does not touch. A

- diff with zero comments passes trivially — never manufacture a finding.

-

- A **missing-why** finding is separate and narrow. Raise it only when the

- diff introduces or rewrites code shaped by a constraint in the

- "Document non-obvious constraints" list *and* you can name the exact

- constraint and the consequence of removing the code. It is

- `suggestion (non-blocking): Comment Discipline`, never blocking, never

- escalating on repetition. Absence of comments is never by itself evidence.

-

- ### UX Reviewer

-

- - **APPROVE:** API/UX is intuitive, consistent with existing patterns.

- - **REQUEST CHANGES:** Usability issues found. Treated as a *major* —

- auto-fixed in the loop, not surfaced to the user.

- - **COMMENT:** Minor ergonomic suggestions (minor-and-below — recorded in the

- PR body's `## Review notes`, never presented mid-run).

-

- ### Technical Writer

-

- - **PASS:** Documentation is adequate for the changes made.

- - **GAPS:** Documentation gaps identified. Recorded for future work.

-

- ## Code Reviewer Inspection Contract

-

- Your input is the diff on the current branch (`git diff HEAD~1`, or the

- range the orchestrator names; `git log --oneline -10` when the scope is

- unclear) and the done criteria in whatever plan file, issue references, or

- commit messages the branch carries. When no criteria exist, review on

- general correctness and quality. Order the work however you judge best.

- Three obligations are non-negotiable:

-

- - **Verify every done criterion is met.** Flag any that are missing or

- incomplete.

- - **Run the project's test suite.** Report the command used and the result.

- - **Check each rule the diff introduces reaches every surface it must.**

- When the changed code or prose has more than one way in — two entry modes,

- a path documented as usable on its own, a split across turns or processes —

- a new rule added to one is not added to the others by implication. Take

- each rule the diff adds and name where it now holds. A rule present in one

- surface and silently absent from a sibling is a finding; a stated reason

- for the absence answers it. Read a self-contained path **alone**, the way

- its callers arrive at it.

-

- **Coverage checklist** — every changed file is checked against every item;

- no order implied:

-

- - **Correctness** — off-by-one errors, missing null checks, broken edge

- cases. Does the logic do what it claims?

- - **Maintainability** — intention-revealing names, obvious control flow.

- - **Error handling** — errors caught, surfaced, and handled at the right

- level; failures loud rather than silent.

- - **Comment discipline** — per the Comment red flags above; cite the

- `Comment Discipline` checklist item.

- - **Unnecessary complexity** — abstraction serving no current need.

- - **System fit** — does a sibling implementation now diverge? Does a caller

- outside the diff need updating? Does the change follow conventions

- established elsewhere (cite the convention)? Findings cite the

- `System Fit` checklist item. When the diff removes or weakens

- long-standing behavior — a guard, a threshold, a workaround that looks

- deliberate — check its rationale before flagging or approving the

- removal: call the Skill tool with `why`. A Chesterton's-fence deletion

- whose motivating constraint still holds is a finding; one whose

- constraint provably evaporated is not.

- - **SOLID violations** — per `skills/solid/SKILL.md`.

- - **Test files** — per both severity regimes above and

- `skills/test-style/SKILL.md`.

- ## Security Review

- The security reviewer's process lives in `skills/reviewing-security/SKILL.md`

- — attack-surface identification, the OWASP Top 10 checks, the extra

- vulnerability checks, and the CRITICAL/HIGH/MEDIUM/LOW severity ladder. The

- PASS/FAIL verdict rule stays here (Verdict Criteria above): any CRITICAL or

- HIGH finding is FAIL, no override.