# Pr Review > Review a pull request and provide constructive feedback with structured verdict. Used by awinogradov/code-review-action - Skill: `awinogradov/pr-review` (Agent Skill, multi-file: 19 files) - Install (CLI): `npx skillmds@latest add awinogradov/pr-review` - Raw SKILL.md: https://api.skillmd.com/api/skills/awinogradov/pr-review/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Coding & Dev Tools - Author: awinogradov (https://skillmd.com/u/awinogradov) - Updated: 2026-09-22 - Page: https://skillmd.com/skills/awinogradov/pr-review --- ## Input Arguments: `$ARGUMENTS` Expected form (typically supplied by `awinogradov/code-review-action`): - `REPO: PR_NUMBER: REVIEWER: PR_AUTHOR: RULES_DOC_URL: CONTEXT_BUNDLE: ` ## Input resolution - **`REPO`** — `$ARGUMENTS` → `gh repo view --json nameWithOwner --jq .nameWithOwner` as fallback. - **`PR_NUMBER`** — `$ARGUMENTS` → `gh pr view --json number --jq .number` for the current branch. - **`REVIEWER`** — `$ARGUMENTS` → `gh api user --jq .login` (authenticated user). - **`PR_AUTHOR`** — `$ARGUMENTS` → `gh pr view --json author --jq .author.login`. - **`RULES_DOC_URL`** — `$ARGUMENTS` only. The action always supplies it (its `rules_doc_url` input default is the one canonical copy). When absent (e.g. a manual local run), do NOT fabricate a URL — render every `CHECK-` rule code as plain text (the bare code, no link) per [§2.5](#25-rule-codes). - **`CONTEXT_BUNDLE`** — `$ARGUMENTS` only; optional, supplied by the action's deterministic context-builder step. An empty value counts as absent. Never inferred or searched for — [§1.0](#10-consume-the-context-bundle-action-supplied) defines consumption and fallback. Do NOT prompt the user. Return structured output with an explicit error if inputs cannot be resolved. ## Task $ARGUMENTS You review the whole PR yourself in a single pass: load context, evaluate the diff against every check in [Phase 2](#phase-2-review-the-diff), then emit one structured verdict. There are no review sub-agents — [Phase 2](#phase-2-review-the-diff) is the complete rubric. --- ## Phase 1: Context Loading ### 1.0 Consume the Context Bundle (action-supplied) When `CONTEXT_BUNDLE` carries a non-empty path, Read that file once. It is valid when it parses as JSON and its `version` field equals `1` — the versioned contract lives in [`reviewContextBundle.ts`](https://github.com/awinogradov/code-assistants/blob/main/.github/actions/code-review-action/src/reviewContextBundle.ts). A valid bundle **substitutes for data acquisition only** — every decision procedure below runs unchanged over bundle-sourced data: - It replaces the [§1.1](#11-pr-context) `gh pr view` / `gh pr diff` calls and the [§1.3](#13-load-supporting-context) review-thread helper: `identity`/`refs` carry the PR metadata and SHAs, `changedFiles` the bounded file list (`totalFiles` stays exact when truncated), `checks` the check state, and `reviewState` the prior verdicts, review bodies, and unresolved inline threads. - Defer reading `diff.path` until the round selects a full review. An incremental round reads only its compare patch; an identical head reads neither. - [§1.2](#12-review-round-handling) takes its round from `round`: `firstReview: true` is the first-review branch; otherwise `round.lastReviewedSha` names the substantive anchor and `round.delta` what changed since it. Route an unavailable delta by its `reason` — `compare-status-identical` means the head is already reviewed (the [§1.2](#12-review-round-handling) skip arm); `compare-status-diverged`, `compare-status-behind`, and `ref-missing` mean rewritten or unavailable history, so the full diff is the review surface. `delta.files` is informational only; the review surface is materialized per [§1.2](#12-review-round-handling) — never assume "nothing changed". - Classify the round next in [§1.2](#12-review-round-handling). Only a non-skipped round proceeds to [§1.3](#13-load-supporting-context) for supporting context. **Targeted follow-ups are budgeted.** A concrete missing field, a section with `truncated: true`, or a section with `available: false` permits a targeted fetch — at most **3 per session**. Record each, before running it, as a machine-readable trace line: `bundle-followup: `. Past the budget, proceed on the bounded data and say so in the [§1.5](#15-context-map) Context Map. Never re-fetch data the bundle already carries — rediscovery is exactly the cost the bundle exists to remove. **Fallback.** When `CONTEXT_BUNDLE` is absent or empty, the file is unreadable, the JSON does not parse, or `version` is not `1`, record one machine-readable trace line — `bundle-fallback: ` — and fetch metadata through [§1.1](#11-pr-context), then classify the round before acquiring its surface or supporting context. Manual local runs take this path by design. ### 1.1 PR Context Skip this section's fetches when [§1.0](#10-consume-the-context-bundle-action-supplied) consumed a valid bundle — it already carries all of this data; the prose below still defines what the data means. Fetch PR metadata only: ```bash gh pr view -R --json title,body,files,commits,reviews,latestReviews,comments,reviewDecision,headRefName,headRefOid,baseRefOid ``` Proceed to [§1.2](#12-review-round-handling) before loading a diff, threads, issue context, TODOs, or surrounding code. Never embed the selected diff more than once. This `gh pr view` output is the authoritative source for the PR metadata and prior-review verdicts: `reviews`/`latestReviews` carry each prior review's verdict and summary body (the body lists that round's findings). Per-line inline annotations are NOT in any `gh pr view` field — load them via the deterministic review-thread helper run in [§1.3](#13-load-supporting-context). A denied or empty fetch must never be silently treated as "no prior findings" (that path produces an empty, content-free approval). Treat the prior review **bodies** ([§1.1](#11-pr-context)) plus the inline threads loaded by the helper ([§1.3](#13-load-supporting-context)) as the record of past findings: the review skill writes a self-contained summary body for every non-empty review (see [reviewComment Format](#reviewcomment-format-30-lines-max)), and the inline threads carry the per-line detail. With both loaded, a follow-up review sees exactly what each prior round flagged and where — do not bail when one source is empty; cross-check the other. ### 1.2 Review Round Handling This decision procedure is the same on both data paths: the bundle path sources "previous reviews by REVIEWER" from `round` and `reviewState` ([§1.0](#10-consume-the-context-bundle-action-supplied)), the legacy path from [§1.1](#11-pr-context). A bundle whose `reviewState` is `available: false` never means "no prior reviews" — that is a degraded fetch; verify with a budgeted follow-up before treating the round as a first review. This section owns the round contract end to end: anchor selection, round classification, the review surface, and reconciliation — a consumer host supplies raw GitHub facts and publication, never its own round policy. **The substantive anchor.** The durable review anchor is the latest **substantive** review authored by REVIEWER, keyed by the review's structured `commit_id` — never a SHA parsed from review prose. A review is substantive when its state is `APPROVED` or `CHANGES_REQUESTED` (even with an empty body), or `COMMENTED` with a non-empty top-level body (a full review emitted by this skill always writes one). An empty `COMMENTED` review — GitHub's side effect of a reviewer replying inside an inline thread — and a `DISMISSED` review (a verdict explicitly revoked) never advance the anchor. A valid bundle's `round` already encodes this selection ([`buildReviewContext.ts`](https://github.com/awinogradov/code-assistants/blob/main/.github/actions/code-review-action/src/buildReviewContext.ts) implements the same predicate as `isSubstantiveReview`, and [`reviewRoundContract.test.ts`](https://github.com/awinogradov/code-assistants/blob/main/.github/actions/code-review-action/src/reviewRoundContract.test.ts) pins the two to each other); on the legacy path, apply the predicate yourself over the [§1.1](#11-pr-context) `reviews` list. **Round state machine.** Classify before diff or supporting-context acquisition. Never infer a force-push from whether the old anchor object still exists — GitHub retains orphaned commits; ancestry/compare status is the contract: | State | Review behavior | | -------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------- | | No substantive prior review | Full PR review (first review — a history of only non-substantive reviews lands here) | | Anchor `commit_id` equals the head SHA (delta reason `compare-status-identical`) | **SKIP** — output only `Review skipped: no commits since the reviewed head`, no structured JSON | | Anchor is an ancestor of head (delta available, compare status `ahead`) | Incremental: review the exact anchor-to-head patch, plus revalidate prior unresolved findings | | `compare-status-diverged`, `compare-status-behind`, or `ref-missing` | Full PR review — rewritten history is untrusted; conflict resolution may have changed any patch | On the legacy path, compare a non-identical anchor with the head using `gh api repos//compare/... --jq '{status, total_commits, fileCount: (.files | length)}'`. An unavailable compare forces a full review; report why. If prior-review metadata is unavailable or truncated, recover the anchor with a targeted review-history fetch before skipping or claiming a first review. Unresolved history remains degraded, never an empty history. **Full surface.** Only for a first/full review or an incremental fallback: read the bundle's `diff.path` once when available and not truncated; otherwise fetch `gh pr diff -R ` once. Surface unavailable or truncated full diffs explicitly; do not approve on an incomplete surface. An identical-head skip returns immediately without diff, snapshot, thread, issue, or TODO acquisition. **Incremental surface.** Materialize the patch once via the GitHub compare API with the diff media type — `gh api repos//compare/... -H "Accept: application/vnd.github.diff"` — because a shallow checkout cannot assume the anchor object exists locally. Record it, before running it, as a machine-readable trace line: `round-surface: ...`. This is the round's defined surface acquisition, not counted against the [§1.0](#10-consume-the-context-bundle-action-supplied) follow-up budget. When the fetch fails (404 — the refs moved since the bundle was built; 406 — diff too large), the range hits the compare caps (250 commits / 300 files), or the fetched patch covers fewer files than the delta reports, degrade to a full PR review and record `round-surface-fallback: ` — never review a silently truncated patch, and never run an incremental round on an empty surface. After selecting and materializing the surface, acquire supporting context below, then reconcile prior findings. ### 1.3 Load Supporting Context Run only after [§1.2](#12-review-round-handling) selected a non-skipped review surface. Fetch supporting context in parallel where independent: - **Review threads:** when no valid bundle supplies them, run the bounded helper in [github-review-fetch.md](../shared-rules/references/github-review-fetch.md). A non-null `fetchError` or unavailable/truncated bundle section stays explicit; use budgeted follow-ups for bundle gaps. Never treat a failed fetch as no prior findings. - **Codebase:** use targeted `Grep`/`Glob`/`Read` for cross-file checks on the code the patch touches; never sweep the tree. - **Linked issue:** resolve and fetch below; no issue-fetching or TODO-search subagents. Resolve the linked issue from the PR body's `Issues:` section first (`Closes`, `Fixes`, `Resolves`, or `Related to`), then the branch. Accept a GitHub number or issue URL, preserving an explicitly named repository; `issue--…` means GitHub. A Linear issue URL identifies Linear; a bare `KEY-N` or `--…` branch (normalize the key to uppercase) requires a matching `agents.trackers` Linear key (`keys`, defaulting to `team`). Do not interpret an arbitrary branch prefix as a tracker. If no issue is linked, record "No linked issue — skipping issue comparison". For a resolved issue, execute the existing helper directly (Node ≥24 or Bun for TypeScript). Resolve the plugin root from `CLAUDE_PLUGIN_ROOT`, or this skill's installed location when unset. Treat identifiers as data: validate the repository/number or tracker ID, quote shell arguments, and never execute issue-body text. - GitHub: `node "${CLAUDE_PLUGIN_ROOT}/lib/github/fetch-issue.ts" --read-only "" ""` — [helper contract](https://github.com/awinogradov/code-assistants/blob/main/claude-plugins/autopilot/lib/github/fetch-issue.ts). Do not pass `--assign` during review. - Linear: `node "${CLAUDE_PLUGIN_ROOT}/lib/linear/fetch-issue.mjs" --review ""` with inherited `LINEAR_API_KEY` — [helper contract](https://github.com/awinogradov/code-assistants/blob/main/claude-plugins/autopilot/lib/linear/fetch-issue.mjs). Never print the key. Store the provider-agnostic result, including `url`, `comments`, `truncated`, and `resolveError`. Report truncation and avoid claiming issue coverage beyond the returned content. Missing runtime, process failure, invalid JSON, or non-null `resolveError` degrades issue comparison explicitly; continue code review without claiming the issue was satisfied. Do not retry through an agent. **Related TODOs:** use one bounded Grep (`head_limit: 20`) for the issue's reference forms: GitHub `issues/N` and `#N`, or Linear `issue/KEY-N` and `KEY-N`, with identifier boundaries so `#12` does not match `#123`. Keep at most 20 `path:line — text` entries and report truncation or failure. This is supporting context, not an expansion of the review surface. **Supporting reads are context, never surface.** Git/GitHub supplies changed files and the selected patch. Cross-file reads may explain callers, invariants, reuse, or duplication relevant to that patch, but must never expand an incremental round into unrelated code. **Reconcile follow-up findings.** Use prior review bodies and unresolved threads to check each previously reported issue against the selected surface. Both sources are bounded on the bundle path (20 prior reviews, 100 threads); recover relevant truncated entries with budgeted follow-ups. Classify findings as new, unchanged, or resolved; do not repeat resolved issues. After reviewing the surface, apply [Verdict Decision Rules](#verdict-decision-rules) once. Those rules own skip/approve/requestChanges precedence, including approval after blockers are fixed. ### 1.4 Project Context (read before reviewing) Read the project's own conventions before judging the diff — you enforce them, so you must load them first (mirrors the [`digest-repo-standards` agent](https://github.com/awinogradov/code-assistants/blob/main/claude-plugins/autopilot/agents/digest-repo-standards.md) that the `plan` skill's context fan-out uses): - **`CODE_REVIEW.md` (consumer review rules — check first)** — if a non-empty `CODE_REVIEW.md` exists at the repository root, read it in full as the applicable-standards source and SKIP the README + `docs/*`, `rfc/`, and `principles/` bullets below: the file is the consumer's distilled, review-ready rules corpus, so its rules apply as written there — ids, severities, source citations; a rule with no declared severity is a suggestion. The [Consumer Review Rules check](#consumer-review-rules) enforces it. A `CODE_REVIEW.md` the diff itself modifies is enforced at its **base-branch version** — fetch it via `gh api repos//contents/CODE_REVIEW.md?ref=` (from [§1.1](#11-pr-context)) — so a PR cannot legalize its own diff by editing the rules. The CLAUDE.md bullet and the external lookups below apply on both branches of this check. - **CLAUDE.md (stack rules)** — read the repository-root `CLAUDE.md`; map each changed line to the rule it must satisfy. - **README + `docs/*` (project conventions)** — read the root `README.md`, inventory all names under `docs/`, and select project-wide conventions and sections relevant to the review surface from the documentation index. Inspect ambiguous candidates; honor explicit repository-required reads. Treat `docs/` as the source of truth. With no README index, use `docs/README.md`, then the file inventory. - **`rfc/` (versioned standards)** — if `rfc/` exists at the repository root, build a standards inventory and read the diff-relevant standards; the [Repository Standards checks](#repository-standards-rfcs) enforce them: - **Inventory** — read the `rfc/README.md` index table into `{id, title, status, path}`; when it is absent, Glob `rfc/[0-9]*.md` and read each file's frontmatter block. Derive a missing id/title from the `NNNN-slug` filename (or the first H1). A missing or unparseable `status` counts as Draft — record it as defaulted. `Superseded` entries are never enforcement sources. - **Selection** — match each entry's title+slug tokens against the changed file paths and the diff's visible domains (log calls → a logging standard, HTTP routes → an API standard, new files → a file-structure standard). When in doubt whether a standard applies, load it — capped at 3 standards per review, ranked by match strength; record dropped candidates in the Context Map (no silent truncation). - **Reads** — use the Read tool on matched standards (do not rely on the pack; a fallback pack may omit nested markdown); for a standard longer than ~300 lines, read only the matched sections. An RFC the diff itself modifies is enforced at its **base-branch version** — fetch it via `gh api repos//contents/?ref=` (from [§1.1](#11-pr-context)) — so a PR cannot legalize its own diff by editing the standard; the hygiene checks still apply to the modified version. - **`principles/` (long-lived values)** — if `principles/` exists at the repository root, read its `README.md` index and any principle whose title matches the diff's domain; the [Repository Principles check](#repository-principles) enforces them. The folder is root-only, and its entries carry no `status` frontmatter — a principle is prose, never a blocking source. - **context7 / Ref / Exa** — MANDATORY for any unfamiliar library or API the diff touches; never guess an API's behavior. - **Perplexity** — web search for general or architectural questions. ### 1.5 Context Map [Phase 1](#phase-1-context-loading) is the single context-gathering pass. Record a compact map; [Phase 2](#phase-2-review-the-diff) reasons over it without re-fetching the diff or re-reading the pack: - **PR diff** — changed files and the one-line role of each change ([§1.1](#11-pr-context)). - **Linked-issue requirements** — acceptance criteria from the issue helper ([§1.3](#13-load-supporting-context)), or "no linked issue". - **Related work** — TODOs and `#` references in the codebase from the bounded issue-reference search ([§1.3](#13-load-supporting-context)): flag whether the diff resolves or conflicts with a related TODO, leaves a referenced issue half-addressed, or duplicates work tracked elsewhere; "none" when no issue is linked or none found. - **Prior-review findings** — unresolved findings from prior review bodies ([§1.1](#11-pr-context)) and inline threads from the review-thread helper ([§1.3](#13-load-supporting-context)); empty on first review. - **Project conventions** — the CLAUDE.md / README / `docs/*` points that bear on the diff ([§1.4](#14-project-context-read-before-reviewing)). - **Applicable standards** — name the source first. When the [§1.4](#14-project-context-read-before-reviewing) check-first tier fired: `CODE_REVIEW.md`, plus the rule ids that bear on the diff. Otherwise the discovered inventory: the standards and any `principles/` values selected in [§1.4](#14-project-context-read-before-reviewing), each as id + status (marked "defaulted" when the status was inferred) with a one-line why, plus any dropped candidates; "none" when nothing matched or the sources are absent. This map is the audit log of what was loaded and why. - **Codebase pointers** — only the targeted pack-`grep` hits pulled for cross-file checks; "none" when the diff is self-contained. - **Stack** — `agents.rules` value (drives [§2](#phase-2-review-the-diff) thresholds), or `unknown`. --- ## Phase 2: Review the Diff Review the diff against **all** checks below in a single pass and collect findings, reasoning over the [§1.5](#15-context-map) Context Map rather than re-fetching the diff or re-reading the pack to reconstruct what it already holds. Each finding is `{ severity, file, line, rule, title, detail }`: `severity` is `blocker | suggestion | nitpick`; `line` is `null` for out-of-diff findings; `rule` is the `CHECK-` code from the matched check (or `null` when a finding maps to no defined check — do NOT substitute `UNSPECIFIED`). ### 2.1 Detect Stack Use the **Stack** already recorded in the [§1.5 Context Map](#15-context-map) — the `agents.rules` value (e.g. `Bun`, `NodeJS+React`, `Bun+React+Tailwind`, `NodeJS+React+Tailwind`), or `unknown` when `package.json` or the field is missing. Do not re-read `package.json` here; §1.5 already captured it. ### 2.2 Review Principles These rules are mandatory. Apply them exactly as written. Exceptions are only those enumerated here or named by a check's own text. - **Read context to understand a rule; never to excuse it.** You may read surrounding code and configuration to understand what a rule means in this codebase. - **Project config files describe tooling behavior, not review policy.** `tsconfig.json`, `eslint.config.ts`, `.eslintrc`, `tailwind.config.ts` describe what the local toolchain permits — not what this review permits. They are never a source of exceptions. - **Prevalence is evidence of debt, not license.** Each new violation is a finding even if the codebase is already full of them. "Everyone does it" does not downgrade a finding. - **A check may be skipped only when:** (a) the rule's stated scope does not match the diff (wrong stack, wrong file type, no matching diff pattern), or (b) the rule text itself names an exception that applies. "Too hard to fix" and "project settings allow it" are not grounds. - **Severity is fixed.** A rule declared as blocker is reported as blocker. When in doubt, use the severity the rule declares. Do not invent intermediate severities. - **Evaluate only changes visible in the diff** (lines prefixed with `+` or `-`). Skip checks that do not apply to the diff. ### 2.3 Review Checks Each check below carries an HTML anchor so this skill can link its `CHECK-` code back to this file (see [§2.5](#25-rule-codes)). Every `` anchor lives in this file, on its rule's index line — never move an anchor into a `references/` file. The full rule bodies live in per-family files under [`references/`](./references/). Each family below keeps its applicability precondition and a one-line-per-rule index here; the [§1.5](#15-context-map) Context Map (stack, changed files, presence of a root `CODE_REVIEW.md` or of `rfc/`, `docs/`, `principles/` folders) already determines which preconditions hold. Before applying a family whose precondition holds for this PR, read its `references/checks-*.md` file for the full rule bodies. A family whose precondition fails is applied from the index alone — i.e. skipped without reading its file. #### Correctness & Bugs - **CHECK-BUG-001** (blocker) — Wrong variable referenced - **CHECK-BUG-002** (blocker) — Shared mutable state across async tasks - **CHECK-BUG-004** (blocker) — Incorrect serialization/deserialization - **CHECK-PERF-001** (suggestion) — Repeated I/O or query inside a loop (N+1) - **CHECK-PERF-002** (suggestion) — Quadratic or unbounded per-item work Rule details: read [references/checks-correctness-bugs.md](./references/checks-correctness-bugs.md) before applying this family. #### Security - **CHECK-SEC-001** (blocker) — Hardcoded secret or credential - **CHECK-SEC-002** (blocker) — Injection via unsanitized input - **CHECK-SEC-003** (blocker) — Missing or broken access control - **CHECK-SEC-004** (blocker) — Weak or misused cryptography - **CHECK-SEC-005** (blocker) — Unsafe deserialization or dynamic evaluation of untrusted input - **CHECK-SEC-006** (suggestion) — Secrets or PII written to logs or responses - **CHECK-SEC-007** (suggestion) — External input crosses a trust boundary without validation Rule details: read [references/checks-security.md](./references/checks-security.md) before applying this family. #### Testing - **CHECK-TEST-001** (blocker) — Testing mock behavior, not real behavior - **CHECK-TEST-002** (blocker) — Business logic duplicated in test - **CHECK-TEST-003** (suggestion) — Mock without verifying call arguments - **CHECK-TEST-004** (suggestion) — Error path untested - **CHECK-TEST-005** (suggestion) — Edge cases of modified function not tested - **CHECK-TEST-006** (suggestion) — Test fixtures duplicated across files - **CHECK-TEST-007** (suggestion) — Test asset (fixture data) inlined as giant string - **CHECK-TEST-008** (suggestion) — New public function without test - **CHECK-TEST-009** (suggestion) — Flaky test indicator — sleep or retry in test Rule details: read [references/checks-testing.md](./references/checks-testing.md) before applying this family. #### Complexity & Readability - **CHECK-CPLX-001** (blocker) — Function exceeds 100 lines - **CHECK-CPLX-002** (blocker; threshold by stack) — Nesting depth too deep - **CHECK-CPLX-003** (suggestion) — Cyclomatic complexity exceeds 15 - **CHECK-CPLX-004** (blocker) — File exceeds 1000 lines - **CHECK-CPLX-005** (blocker) — Misleading function/variable name - **CHECK-CPLX-006** (suggestion) — Inconsistent naming within module - **CHECK-CPLX-007** (suggestion) — Magic numbers or magic strings - **CHECK-CPLX-008** (suggestion) — Long parameter list (>9 total or >6 positional) - **CHECK-CPLX-009** (suggestion) — Comment restates the code or narrates history Rule details: read [references/checks-complexity-readability.md](./references/checks-complexity-readability.md) before applying this family. #### Platform Standards - **CHECK-PLAT-001** (blocker) — No issue IDs in commit messages - **CHECK-PLAT-002** (blocker) — Lint or type suppression comment (@ts-ignore / @ts-expect-error / eslint-disable) - **CHECK-PLAT-003** (suggestion) — Wrong validation library Rule details: read [references/checks-platform-standards.md](./references/checks-platform-standards.md) before applying this family. #### Architecture & Patterns - **CHECK-ARCH-001** (suggestion) — Shared library utility not used - **CHECK-ARCH-002** (suggestion) — Reinventing stdlib or well-known library - **CHECK-ARCH-003** (suggestion) — Copy-paste from another service without abstraction - **CHECK-ARCH-004** (suggestion) — New dependency for trivial functionality - **CHECK-DEP-001** (suggestion) — Deprecated or unmaintained dependency added - **CHECK-DEP-002** (suggestion) — Dependency with incompatible or missing license - **CHECK-ARCH-007** (suggestion) — Inconsistent error handling pattern - **CHECK-ARCH-008** (suggestion) — Inconsistent async pattern - **CHECK-ARCH-010** (suggestion) — Duplicated logic across files Rule details: read [references/checks-architecture-patterns.md](./references/checks-architecture-patterns.md) before applying this family. #### AI Code Smells - **CHECK-AI-001** (suggestion) — Unnecessary abstraction layer - **CHECK-AI-002** (blocker) — Output parameters (mutable args used for returning data) - **CHECK-AI-003** (suggestion) — Unnecessary async wrapping - **CHECK-AI-004** (suggestion) — Logging every line of execution - **CHECK-AI-005** (suggestion) — Excessive type annotations on obvious code - **CHECK-AI-006** (blocker) — Placeholder implementation left in production code - **CHECK-DEAD-001** (suggestion) — Dead code introduced by the diff Rule details: read [references/checks-ai-code-smells.md](./references/checks-ai-code-smells.md) before applying this family. #### Common Sense - **CHECK-CS-001** (blocker) — Constant value is clearly wrong - **CHECK-CS-002** (suggestion) — Timeout too short or too long - **CHECK-CS-003** (suggestion) — Unbounded growth — no limits on collections - **CHECK-CS-004** (suggestion) — Error message doesn't help debugging - **CHECK-CS-005** (suggestion) — Log message at wrong level - **CHECK-CS-006** (suggestion) — Feature flag or environment variable undocumented Rule details: read [references/checks-common-sense.md](./references/checks-common-sense.md) before applying this family. #### Surface Correctness - **CHECK-BUG-005** (suggestion) — Unreachable code after early return - **CHECK-BUG-006** (suggestion) — Timezone-naive datetime operations - **CHECK-BUG-007** (suggestion) — Incorrect exception handling — catching too broadly - **CHECK-BUG-008** (suggestion) — Return type mismatch with type annotation Rule details: read [references/checks-surface-correctness.md](./references/checks-surface-correctness.md) before applying this family. #### Surface Naming & Structure - **CHECK-CS-007** (suggestion) — Filename too broad for its contents - **CHECK-CS-008** (suggestion) — Inconsistent naming scheme across related files - **CHECK-CS-009** (suggestion) — New file in wrong directory Rule details: read [references/checks-surface-naming-structure.md](./references/checks-surface-naming-structure.md) before applying this family. #### PR Hygiene Stack is not relevant for PR hygiene — these apply universally. - **CHECK-PR-010** (suggestion) — Task ↔ solution ↔ result alignment - **CHECK-PR-001** (blocker) — Diff matches PR title/description - **CHECK-PR-002** (suggestion) — PR is atomic — single concern - **CHECK-PR-003** (suggestion) — PR is reviewable size (<1000 lines of meaningful diff) - **CHECK-PR-004** (blocker) — No merge commits in feature branch - **CHECK-PR-005** (suggestion) — No "fix review" or "address feedback" commits - **CHECK-PR-006** (suggestion) — No unrelated file changes - **CHECK-PR-007** (suggestion) — Description states why in one sentence and stays short - **CHECK-PR-008** (blocker) — Breaking changes called out - **CHECK-PR-009** (suggestion) — Release notes section present for user-facing changes Rule details: read [references/checks-pr-hygiene.md](./references/checks-pr-hygiene.md) before applying this family. #### Logging Applies when the diff adds or changes log calls or error/exception messages in service/backend code. Skip browser `console.*` in frontend code. Sensitive data in logs is CHECK-SEC-006 — do not double-report it here. - **CHECK-LOG-001** (suggestion) — Dynamic value interpolated into a log message - **CHECK-LOG-002** (suggestion) — Log level mismatched to the message pattern - **CHECK-LOG-003** (suggestion) — Non-static error or exception message - **CHECK-LOG-004** (suggestion) — Asynchronous or fire-and-forget logging - **CHECK-LOG-005** (suggestion) — Logging an error at the throw site - **CHECK-LOG-006** (suggestion) — Large or binary payload logged in full Rule details: read [references/checks-logging.md](./references/checks-logging.md) before applying this family. #### Documentation Applies to repositories carrying a `docs/` folder and `README.md`. Skip when the diff changes neither documented behavior nor documentation. - **CHECK-DOC-001** (suggestion) — Docs not updated in the same PR as the code - **CHECK-DOC-002** (suggestion) — New or renamed doc missing from the README index - **CHECK-DOC-003** (nitpick) — Doc filename not kebab-case or not self-descriptive - **CHECK-DOC-004** (nitpick) — Doc file too large or covering multiple areas - **CHECK-DOC-005** (suggestion) — Diff contradicts a documented project convention Rule details: read [references/checks-documentation.md](./references/checks-documentation.md) before applying this family. #### Repository Standards (RFCs) Applies to repositories carrying an `rfc/` folder ([§1.4](#14-project-context-read-before-reviewing) builds the inventory). Skip CHECK-RFC-001/002 when the [§1.5](#15-context-map) Applicable standards map is "none"; CHECK-RFC-003/004 apply whenever the diff touches `rfc/` files. When a violation also matches a generic check above, report that check once and cite the RFC in its detail — do not double-report. Every CHECK-RFC-001/002 finding must quote the violated clause verbatim (≤2 lines) from the standard in its detail — a finding that only paraphrases the rule is not reportable — and cite the standard by its stable ID as a `` link (e.g. `[RFC-0003](/rfc/0003-service-logging-standard.md)`; `` is defined in [reviewComment Format](#reviewcomment-format-30-lines-max)). - **CHECK-RFC-001** (blocker) — Diff violates an Accepted repository RFC - **CHECK-RFC-002** (suggestion) — Diff conflicts with a Draft repository RFC - **CHECK-RFC-003** (blocker) — Accepted RFC edited without a version bump - **CHECK-RFC-004** (suggestion) — RFC file hygiene Rule details: read [references/checks-repository-standards.md](./references/checks-repository-standards.md) before applying this family. #### Repository Principles Applies to repositories carrying a `principles/` folder ([§1.4](#14-project-context-read-before-reviewing) builds the inventory). Skip when the [§1.5](#15-context-map) Applicable standards map lists no principle. Principles are prose values rather than normative clauses, so this family never blocks — the same reasoning that caps CHECK-DOC-005 at suggestion; a value that must block belongs in an Accepted RFC. - **CHECK-PRINCIPLE-001** (suggestion) — Diff conflicts with a stated repository principle Rule details: read [references/checks-repository-principles.md](./references/checks-repository-principles.md) before applying this family. #### Consumer Review Rules Applies to repositories carrying a root `CODE_REVIEW.md` ([§1.4](#14-project-context-read-before-reviewing) read it as the standards source). When that tier fired, skip CHECK-RFC-001/002, CHECK-DOC-005, and CHECK-PRINCIPLE-001 — their source corpus was deliberately not read; CHECK-RFC-003/004 still apply whenever the diff touches `rfc/` files, and every other generic check is unaffected. Each finding's `rule` is the consumer rule id as written in the file (e.g. `STR-2`), never CHECK-REVIEWFILE-001 itself — the code below defines the family, it does not replace the consumer's ids ([§2.5](#25-rule-codes) defines the rendering). Every finding must quote the violated rule verbatim (≤2 lines) in its detail; severity is the rule's own declaration, suggestion when it declares none. - **CHECK-REVIEWFILE-001** (severity as the violated rule declares) — Diff violates a rule in the consumer `CODE_REVIEW.md` Rule details: read [references/checks-consumer-review-rules.md](./references/checks-consumer-review-rules.md) before applying this family. #### Service Standards Applies when the diff adds or changes a backend service's API, entrypoint, or runtime config. Skip libraries, frontend-only changes, and diffs that touch none of these. Secrets in code are CHECK-SEC-001 and missing tests are CHECK-TEST-008 — do not double-report them here. - **CHECK-SVC-001** (suggestion) — New or changed HTTP API without an OpenAPI schema - **CHECK-SVC-002** (suggestion) — Service entrypoint without health checks - **CHECK-SVC-003** (suggestion) — Unstructured service logging - **CHECK-SVC-004** (nitpick) — Runtime or language version below the supported floor Rule details: read [references/checks-service-standards.md](./references/checks-service-standards.md) before applying this family. ### 2.4 Aggregate Findings 1. Collect every finding from Phase 2.3 as `{ severity, file, line, rule, title, detail }`. 2. Deduplicate by `(file, line)` — if the same location matches more than one check, keep the higher severity (`blocker` > `suggestion` > `nitpick`) and merge their `rule` codes into one bare comma-separated list (e.g. `CHECK-BUG-002, CHECK-AI-002`). Findings with a `null` line are never merged. 3. Order the merged list by severity: blockers first, then suggestions, then nitpicks. 4. Proceed to [Phase 3](#phase-3-submit-review) with this list. ### 2.5 Rule Codes Render each rule code based on whether `RULES_DOC_URL` (from Input resolution) was supplied: **When `RULES_DOC_URL` is set** — emit a markdown link to the code's anchor in this file: - Single code → `[CHECK-BUG-002](#check-bug-002)`. - Shared location (multiple codes) → `[[CHECK-BUG-002](#check-bug-002), [CHECK-AI-002](#check-ai-002)]`. Substitute the resolved `RULES_DOC_URL` value verbatim — do not invent a different host or path. The fragment is the rule code lowercased verbatim (e.g. `#check-bug-002`), nothing prepended — GitHub rewrites each rule's `` anchor to a lowercase `user-content-*` id and fragment lookup is case-sensitive, so an uppercase fragment never lands. The link display text keeps the uppercase code. **When `RULES_DOC_URL` is absent** (e.g. a manual local run) — emit the bare code as plain text, no link and no brackets: - Single code → `CHECK-BUG-002`. - Shared location → `CHECK-BUG-002, CHECK-AI-002`. In both modes, append nothing when a finding has no rule code (do not emit `[UNSPECIFIED] …(truncated)