Menu
Review a pull request and provide constructive feedback with structured verdict. Used by awinogradov/code-review-action
Install with Codex or Claude Copy this prompt, paste it into Codex, Claude, or another assistant, and let it review the skill page and install it for you.
Based on SOC occupation classification
Perform deep analysis of the codebase, recent changes, and the requested task. Create a validated, expert-reviewed implementation plan
Plan, implement, commit, create PR, and monitor until approved
Analyze staged changes and create conventional commits with intelligent grouping. Use when creating commits, or when invoked from other skills.
Create a GitHub issue with a structured body (Context, What, Why, Scope, Solution) and curated labels via the gh CLI. Use when filing new issues, or when invoked from other skills.
Create a Linear issue with a structured body (Context, What, Why, Scope, Solution) and wizard-selected status, label, and assignee via the Linear MCP. Use when filing a Linear ticket on a linear-tracked project.
Answer a user comment on a PR review and update review state if needed
| name | pr:review |
| description | Review a pull request and provide constructive feedback with structured verdict. Used by awinogradov/code-review-action |
| argument-hint | REPO: <owner/repo> PR_NUMBER: <number> REVIEWER: <bot-login> PR_AUTHOR: <author-login> RULES_DOC_URL: <url> (all but RULES_DOC_URL fall back to gh when omitted) |
| allowed-tools | ["Read","Glob","Grep","Agent","Bash(gh *)","Bash(echo *)","MCP(github:*)","MCP(repomix:*)","MCP(context7:*)","MCP(Ref:*)","MCP(exa:*)","MCP(perplexity:*)"] |
Arguments: $ARGUMENTS
Expected form (typically supplied by awinogradov/code-review-action):
REPO: <owner/repo> PR_NUMBER: <number> REVIEWER: <bot-login> PR_AUTHOR: <author-login> RULES_DOC_URL: <url>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.Do NOT prompt the user. Return structured output with an explicit error if inputs cannot be resolved.
$ARGUMENTS
You review the whole PR yourself in a single pass: load context, evaluate the diff against every check in Phase 2, then emit one structured verdict. There are no review sub-agents — Phase 2 is the complete rubric.
Fetch PR metadata and the diff:
gh pr view <PR_NUMBER> -R <REPO> --json title,body,files,commits,reviews,latestReviews,comments,reviewDecision,headRefOid,baseRefOid
gh pr diff <PR_NUMBER> -R <REPO>
Fetch the diff exactly once and review it in-model. Never embed the diff more than once.
This gh pr view output is the authoritative source for the PR title/body/diff 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 read-only gh api call the fetch-pr-reviews agent makes in §1.2 (the review action now permits gh api GETs; only write forms are blocked). 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) plus the inline threads loaded by fetch-pr-reviews (§1.2) as the record of past findings: the review skill writes a self-contained summary body for every non-empty review (see reviewComment Format), 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.
Extract the linked issue ID from PR metadata. Check in order, stop at first match:
Issues: section — lines starting with Closes or Related to followed by a ticket ID; the id may be bare (#12, ENG-123) or inside a tracker issue URL (https://linear.app/<workspace>/issue/ENG-123/<slug>) — extract the #N / KEY-N token either way[a-z]+-[0-9]+ segment, convert to UPPERCASELoad the remaining context in parallel — the codebase snapshot, the prior inline review threads, and (when an issue is linked) the linked-issue context plus the related TODOs / issue references in the codebase. Prior-review verdicts and summary bodies already come from the §1.1 gh pr view output; the fetch-pr-reviews agent adds the per-line inline annotations via read-only gh api, returning a categorized summary (raw API output stays out of this context).
Acquire codebase snapshot (prefer the committed pack to avoid re-packing):
Check whether `.repomix/pack.xml` exists at the repository root.
- If it exists, call `mcp__repomix__attach_packed_output` with:
- `path`: [repository root absolute path]/.repomix/pack.xml
- If it is absent (or the attach fails), fall back to `mcp__repomix__pack_codebase` with:
- `directory`: [repository root absolute path]
- `compress`: true
- `includePatterns`: ".claude/**, **.md, **.yml, .github/**"
Agent (fetch-pr-reviews):
Use the Agent tool with:
- `subagent_type`: "autopilot:fetch-pr-reviews"
- `prompt`: "Fetch reviews for PR #[PR_NUMBER]. Repo: <REPO>. Author: <PR_AUTHOR>."
- `description`: "Fetch PR review threads"
Agent (resolve-issue-context) — only if linked issue found:
Use the Agent tool with:
- `subagent_type`: "autopilot:resolve-issue-context"
- `prompt`: "Fetch issue context. Issue number: [N]. Repository: <REPO>."
- `description`: "Resolve issue context"
Agent (search-codebase-todos) — only if linked issue found:
Use the Agent tool with:
- `subagent_type`: "autopilot:search-codebase-todos"
- `prompt`: "Search for TODOs. Issue number: [N]."
- `description`: "Search codebase TODOs and issue references"
If no issue number found, output: "No linked issue — skipping issue comparison" and skip the issue-context agent.
If a gh call fails (auth/network error) inside an agent, continue with whatever context loaded — never treat a failed fetch-pr-reviews as "no prior findings", and skip issue comparison only when resolve-issue-context itself found no issue.
After all calls complete, store the outputId from the snapshot acquisition (attach or pack) response, the categorized review threads from fetch-pr-reviews, the issue context from resolve-issue-context, and the TODOs / issue references from search-codebase-todos. Use these plus the prior-review verdicts from §1.1 for the round handling below.
Read the pack, don't dump it. The snapshot exists so you can pull targeted context on demand — use grep_repomix_output (regex + contextLines) and read_repomix_output with a specific startLine/endLine slice. NEVER read_repomix_output over the whole range (that loads the entire codebase into context). When the diff is self-contained and needs no cross-file lookup (the common case), don't read the pack at all — pull cross-file context only for checks that need it (e.g. architecture reuse, duplicated logic).
First review (no previous reviews by REVIEWER):
reviewComment, no body text at all.Follow-up review (previous review by REVIEWER exists):
reviews/latestReviews bodies (§1.1) and the per-line inline threads from fetch-pr-reviews (§1.2)reviewComment (no body text)When skipping, output only: Review skipped: no new findings since last review
Do NOT produce the structured JSON output.
Consecutive approval (previous review by REVIEWER was APPROVED):
Review skipped: already approved, no new commitsreviewComment (no body text)Read the project's own conventions before judging the diff — you enforce them, so you must load them first (mirrors the plan skill's Phase 1):
CLAUDE.md; map each changed line to the rule it must satisfy.docs/* (project conventions) — read the root README.md and the docs it links; treat docs/ as the source of truth for project-specific conventions. When the root README carries no docs index, fall back to docs/README.md, then to the Glob docs/*.md file names.rfc/ (versioned standards) — if rfc/ exists at the repository root, build a standards inventory and read the diff-relevant standards; the Repository Standards checks enforce them:
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.gh api repos/<REPO>/contents/<path>?ref=<baseRefOid> (from §1.1) — 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 enforces them. The folder is root-only, and its entries carry no status frontmatter — a principle is prose, never a blocking source.Phase 1 is the single context-gathering pass. Record a compact map; Phase 2 reasons over it without re-fetching the diff or re-reading the pack:
resolve-issue-context (§1.2), or "no linked issue".#<issue> references in the codebase from search-codebase-todos (§1.2): 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.fetch-pr-reviews (§1.2); empty on first review.docs/* points that bear on the diff (§1.4).principles/ values selected in §1.4: 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 folders are absent. This map is the audit log of what was loaded and why.grep hits pulled for cross-file checks; "none" when the diff is self-contained.agents.rules value (drives §2 thresholds), or unknown.Review the diff against all checks below in a single pass and collect findings, reasoning over the §1.5 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).
Use the Stack already recorded in the §1.5 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.
These rules are mandatory. Apply them exactly as written. Exceptions are only those enumerated here or named by a check's own text.
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.+ or -). Skip checks that do not apply to the diff.Each check below carries an HTML anchor so this skill can link its CHECK- code back to this file (see §2.5). Keep each <a id="..."> immediately above its rule.
CHECK-BUG-001: Wrong variable referenced — Severity: blocker
A variable from an outer scope, a similarly-named variable, or a copy-paste leftover is used instead of the intended one.
requestConfig but the body reads a config from an outer scope; a loop variable shadowing an outer item.CHECK-BUG-002: Shared mutable state across async tasks — Severity: blocker
Multiple async tasks reading/writing the same mutable object (array, object, or instance field) without synchronization; interleaved awaits can cause inconsistent state even in single-threaded async runtimes.
CHECK-BUG-004: Incorrect serialization/deserialization — Severity: blocker
Data lost or corrupted during JSON serialization — missing fields, wrong types, enum value mismatch between sender and receiver.
JSON.stringify drops undefined fields the consumer expects as null.CHECK-PERF-001: Repeated I/O or query inside a loop (N+1) — Severity: suggestion
A network call, database query, or filesystem read issued once per item in a loop where a single batched call would do.
for (const id of ids) { await db.user(id) } instead of one db.users(ids) batch.CHECK-PERF-002: Quadratic or unbounded per-item work — Severity: suggestion
An operation whose cost grows super-linearly with input — a nested scan (Array.find/includes inside a loop over a large collection) where a Map/Set would give O(1) lookup.
items.filter((a) => others.find((b) => b.id === a.id)) with a large others.CHECK-SEC-001: Hardcoded secret or credential — Severity: blocker
An API key, token, password, private key, or connection string with embedded credentials committed in source instead of read from a secret store or environment variable.
const apiKey = "sk-live-..."; a database URL with inline user:password@host."xxx", "<your-token>", process.env.X references).CHECK-SEC-002: Injection via unsanitized input — Severity: blocker
Untrusted input concatenated into a SQL query, shell command, file path, or HTML sink without parameterization, escaping, or validation.
db.query("SELECT * FROM users WHERE id = " + req.params.id); user input in a filesystem path without normalization (path traversal).CHECK-SEC-003: Missing or broken access control — Severity: blocker
A privileged action, route, or resource accessed without verifying authentication or the caller's authorization; a check present but trivially bypassed.
isAdmin flag read from client-supplied input.CHECK-SEC-004: Weak or misused cryptography — Severity: blocker
A broken algorithm (MD5/SHA1 for security), a non-constant-time secret comparison, a hardcoded/static IV or salt, or Math.random() for security-sensitive values.
=== instead of crypto.timingSafeEqual.CHECK-SEC-005: Unsafe deserialization or dynamic evaluation of untrusted input — Severity: blocker
eval/Function, dynamic import()/require() with a user-controlled path, or deserializing attacker-controlled data into executable structures.
CHECK-SEC-006: Secrets or PII written to logs or responses — Severity: suggestion
Tokens, passwords, full request bodies with credentials, or personal data logged or returned in an API/error response.
console.log("auth", req.headers.authorization); an error handler returning a stack trace with a connection string to the client.CHECK-SEC-007: External input crosses a trust boundary without validation — Severity: suggestion
Data from outside the program — a request body/params, an external API response, a webhook payload, env vars, or file contents — consumed without validating its shape at the boundary before use. Distinct from CHECK-SEC-002 (injection sinks) and CHECK-PLAT-003 (which validation library): this fires when external input is trusted with no validation at all.
const { amount } = await res.json() used directly in a calculation without parsing the response against a schema.CHECK-TEST-001: Testing mock behavior, not real behavior — Severity: blocker
Test configures a mock to return X, then asserts the code got X. This tests the mock, not the code.
mockService.get.mockReturnValue(42); expect(handler()).toBe(42).CHECK-TEST-002: Business logic duplicated in test — Severity: blocker
Test reimplements the same calculation/logic as production to compute the expected value instead of using known input/output pairs. If production is wrong, the test is wrong the same way.
const expected = sum(items) * taxRate + shipping; expect(calculateTotal(items)).toBe(expected).CHECK-TEST-003: Mock without verifying call arguments — Severity: suggestion
Test creates a mock but never checks what arguments it was called with, only that the return value flowed through.
mockDb.save.mockReturnValue(true) but no expect(mockDb.save).toHaveBeenCalledWith(expectedRecord).CHECK-TEST-004: Error path untested — Severity: suggestion
Only the happy path is tested. Error conditions (invalid input, timeout, connection failure, empty result) have no coverage.
fetchUser only cover successful fetch, never user-not-found or network error.CHECK-TEST-005: Edge cases of modified function not tested — Severity: suggestion
A function is modified (new parameter, changed boundary) but existing tests don't cover the new behavior.
offset parameter to a pagination function but no test exercises non-zero offset.CHECK-TEST-006: Test fixtures duplicated across files — Severity: suggestion
Same test data or setup copy-pasted in multiple test files instead of shared fixtures.
mockGrpcChannel with identical setup.CHECK-TEST-007: Test asset (fixture data) inlined as giant string — Severity: suggestion
Large JSON blobs, XML payloads, or byte strings hardcoded in test files instead of loaded from tests/assets/ or tests/fixtures/.
CHECK-TEST-008: New public function without test — Severity: suggestion
A new public function, method, or endpoint added with zero test coverage. Every non-trivial public interface needs at least a happy-path test.
parseConfig with no test file changes in the diff.CHECK-TEST-009: Flaky test indicator — sleep or retry in test — Severity: suggestion
Tests using setTimeout, fixed delays, or retry loops to wait for conditions — indicates a timing-dependent test.
await new Promise(r => setTimeout(r, 500)); expect(queue).toBeEmpty().CHECK-CPLX-001: Function exceeds 100 lines — Severity: blocker
Any function or method longer than 100 lines (all stacks).
CHECK-CPLX-002: Nesting depth too deep
Control flow nested beyond the stack-specific threshold. Prefer early returns over nested conditionals.
max-depth: 2 per CLAUDE.md).CHECK-CPLX-003: Cyclomatic complexity exceeds 15 — Severity: suggestion
Function has more than 15 independent code paths (branches, loops, exception handlers).
CHECK-CPLX-004: File exceeds 1000 lines — Severity: blocker
Any code file longer than 1000 lines. Long files must be split.
CHECK-CPLX-005: Misleading function/variable name — Severity: blocker
Name implies different behavior than the code does. get* that mutates state, is* that returns non-boolean, validate* that also transforms.
CHECK-CPLX-006: Inconsistent naming within module — Severity: suggestion
Same concept named differently in the same file or closely related files — user_id, uid, userId.
CHECK-CPLX-007: Magic numbers or magic strings — Severity: suggestion
Numeric or string literals used in logic without a named constant explaining their meaning.
if (buffer.length > 8192) without explaining what 8192 represents.CHECK-CPLX-008: Long parameter list (>9 total or >6 positional) — Severity: suggestion
Function accepts more than 9 total parameters or more than 6 positional, indicating it should accept a config/options object instead.
CHECK-CPLX-009: Comment explains "what" instead of "why" — Severity: suggestion
Comments describing what the code does (obvious from the code) instead of why.
CHECK-PLAT-001: No issue IDs in commit messages — Severity: blocker
GitHub issue references (#123, Closes #123) must NOT appear in commit messages. The PR description handles issue linking via magic words.
commitlint.config.mjs custom rule no-issue-id.CHECK-PLAT-002: Lint or type suppression comment (@ts-ignore / @ts-expect-error / eslint-disable) — Severity: blocker
Zero tolerance for lint/type suppression comments. Any @ts-ignore, @ts-expect-error, @ts-nocheck, eslint-disable, eslint-disable-next-line is a blocker.
CHECK-PLAT-003: Wrong validation library — Severity: suggestion
Data validation must use the stack-appropriate library, not manual validation or plain classes.
CHECK-ARCH-001: Shared library utility not used — Severity: suggestion
Code reimplements functionality already available in a shared library (logging, telemetry, pooling, database client, metrics). Check shared libraries before writing new utilities. Use Grep to confirm an existing implementation when in doubt.
CHECK-ARCH-002: Reinventing stdlib or well-known library — Severity: suggestion
Custom implementation of functionality available in the language stdlib or an approved dependency.
p-retry is already a dependency.CHECK-ARCH-003: Copy-paste from another service without abstraction — Severity: suggestion
Large code blocks copied from another repo/service instead of extracting to a shared library.
CHECK-ARCH-004: New dependency for trivial functionality — Severity: suggestion
Adding a package for something doable in <20 lines with stdlib. Each dependency adds supply-chain risk.
dotenv to read 2 environment variables when process.env suffices.CHECK-DEP-001: Deprecated or unmaintained dependency added — Severity: suggestion
A newly added dependency is deprecated, archived, or visibly unmaintained, or pulls a heavy/duplicate transitive tree for a small need.
request (deprecated) instead of the built-in fetch.CHECK-DEP-002: Dependency with incompatible or missing license — Severity: suggestion
A new dependency carries a license incompatible with the project (e.g. GPL into a permissively-licensed project) or has no discernible license.
CHECK-ARCH-007: Inconsistent error handling pattern — Severity: suggestion
New code uses a different error-handling pattern than existing code in the same module (some methods raise, some return null).
CHECK-ARCH-008: Inconsistent async pattern — Severity: suggestion
Mixing sync and async code in the same layer. If the module is async, new code should be async too.
CHECK-ARCH-010: Duplicated logic across files — Severity: suggestion
Same or near-identical logic (>5 lines) appearing in multiple places. Should be extracted to a shared utility.
CHECK-AI-001: Unnecessary abstraction layer — Severity: suggestion
Interface/protocol/base class with exactly one implementation and no plan for others.
AudioConverterProtocol with only WavConverter implementing it.CHECK-AI-002: Output parameters (mutable args used for returning data) — Severity: blocker
Function mutates a passed-in object to "return" data through it instead of using actual return values. A C-ism with no place in TypeScript.
function getStatus(result) { result.status = "active"; result.code = 200; } — should return a value.CHECK-AI-003: Unnecessary async wrapping — Severity: suggestion
Function marked async with no await — synchronous code wearing an async costume.
CHECK-AI-004: Logging every line of execution — Severity: suggestion
Debug logging at entry, exit, and every intermediate step. Logs should capture decisions and state changes, not trace every line.
CHECK-AI-005: Excessive type annotations on obvious code — Severity: suggestion
Type annotations on every local variable, including trivially obvious ones, adding noise without aiding understanding.
CHECK-AI-006: Placeholder implementation left in production code — Severity: blocker
An empty stub body, a throw new Error("Not implemented"), or a // TODO placeholder in code that should be fully implemented.
CHECK-DEAD-001: Dead code introduced by the diff — Severity: suggestion
Commented-out code blocks, or unused imports / variables / private functions / exports, added or left behind by this change. Ship live code only — recover history from version control instead of parking it in comments.
import added but never referenced.CHECK-CS-001: Constant value is clearly wrong — Severity: blocker
A constant whose value doesn't match what it represents — too large, too small, wrong units, or nonsensical for the domain.
TIMEOUT_MS = 1 (1ms is too short for most network calls).CHECK-CS-002: Timeout too short or too long — Severity: suggestion
Timeout values dangerously short (false failures) or too long (blocking resources). Compare against the expected operation duration.
const requestTimeout = 0.5 for a call involving ML inference; const sessionTimeout = 86400 * 30.CHECK-CS-003: Unbounded growth — no limits on collections — Severity: suggestion
A data structure that grows without bound (cache, in-memory queue, log buffer) without eviction policy or size limit.
this.history = [] that pushes every request but never trims.CHECK-CS-004: Error message doesn't help debugging — Severity: suggestion
An error message lacking enough context to diagnose — missing which value failed, what was expected, or what operation was attempted.
CHECK-CS-005: Log message at wrong level — Severity: suggestion
Expected/handled conditions logged as errors (noisy), or critical failures logged as warnings (hidden).
logger.error("user not found") for a normal 404 flow.CHECK-CS-006: Feature flag or environment variable undocumented — Severity: suggestion
A new environment variable or feature flag added without documenting it in README, config template, or deployment docs.
CHECK-BUG-005: Unreachable code after early return — Severity: suggestion
Code placed after an unconditional return, raise, break, or continue that can never execute.
CHECK-BUG-006: Timezone-naive datetime operations — Severity: suggestion
Mixing timezone-aware and timezone-naive datetimes, or assuming local time when UTC is required.
new Date() without explicit UTC handling when the codebase standardizes on a UTC helper.CHECK-BUG-007: Incorrect exception handling — catching too broadly — Severity: suggestion
Bare catch (e) { ... } that swallows errors without rethrowing — especially where an AbortError or a programmer error should propagate.
catch (e) { logger.error("failed"); } swallowing an AbortError from a cancelled fetch.CHECK-BUG-008: Return type mismatch with type annotation — Severity: suggestion
A function's actual return value doesn't match its type annotation on some code paths.
function getName(): string with an implicit return undefined on cache miss.CHECK-CS-007: Filename too broad for its contents — Severity: suggestion
File named generically (utils.ts, helpers.ts, common.ts) when it contains code for a specific domain and sits among 10+ other files.
maintenance.ts containing only queue maintenance routines should be queueMaintenance.ts.CHECK-CS-008: Inconsistent naming scheme across related files — Severity: suggestion
Related files follow different naming patterns — some _client, others _service, mixing conventions.
CHECK-CS-009: New file in wrong directory — Severity: suggestion
File placed in a directory that doesn't match its purpose per the project's directory-structure conventions.
src/api/ instead of src/services/.Stack is not relevant for PR hygiene — these apply universally.
CHECK-PR-010: Task ↔ solution ↔ result alignment — Severity: suggestion
Compare three artifacts and flag divergence in each leg. Skip a leg only when its source is absent or vague (no linked issue, or empty PR body).
Legs:
CHECK-PR-001: Diff matches PR title/description — Severity: blocker
The actual changes must match what the PR title and description claim. No hidden changes, no scope creep, no "while I was here" additions. (Scope-creep claims and unaddressed requirements are CHECK-PR-010; this is the blocker for undescribed changes present in the diff.)
CHECK-PR-002: PR is atomic — single concern — Severity: suggestion
PR addresses one logical change. Bug fixes shouldn't include refactoring; features shouldn't include unrelated cleanup.
CHECK-PR-003: PR is reviewable size (<1000 lines of meaningful diff) — Severity: suggestion
Exclude generated files, lockfiles, and config, but the meaningful code diff should be reviewable in one sitting.
CHECK-PR-004: No merge commits in feature branch — Severity: suggestion
Feature branches should be rebased on main, not merged. Merge commits clutter history.
CHECK-PR-005: No "fix review" or "address feedback" commits — Severity: suggestion
Review feedback should be squashed into the relevant original commit, not added as separate commits.
CHECK-PR-006: No unrelated file changes — Severity: suggestion
Files modified that have nothing to do with the PR's purpose — whitespace, import reordering, formatting in unrelated files.
CHECK-PR-007: Description explains "why", not just "what" — Severity: suggestion
The PR description should explain motivation and context, not just list changed files.
CHECK-PR-008: Breaking changes called out — Severity: blocker
Breaking changes (API changes, config format changes, removed features) must be explicitly listed in the PR description with migration steps.
CHECK-PR-009: Release notes section present for user-facing changes — Severity: suggestion
Feature/fix PRs affecting users should include a **Release notes:** section in the PR description.
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: Dynamic value interpolated into a log message — Severity: suggestion
The log message must be a static string so aggregators group it across occurrences. IDs, counts, durations, hosts, and user input belong in structured context fields, not in the message via interpolation or concatenation.
logger.info("Request " + id + " took " + ms + "ms") → logger.info({ request_id: id, duration_ms: ms }, "Request processed.").CHECK-LOG-002: Log level mismatched to the message pattern — Severity: suggestion
Progressive ("-ing", about-to-act) messages must be debug; completed business events are info in past tense; recoverable failures are warning; unrecoverable failures are error with a reason. A progressive message at info, or a bare error with no reason, is a mismatch.
logger.info("Processing payment.") → logger.info("Payment processed."), or keep the wording and drop to debug.CHECK-LOG-003: Non-static error or exception message — Severity: suggestion
The string passed to an error constructor or throw must be static; put dynamic context as error properties (or structured fields) so error trackers group it as one issue instead of thousands.
throw new Error("Couldn't connect to " + host) → a static message with host carried as an error property.CHECK-LOG-004: Asynchronous or fire-and-forget logging — Severity: suggestion
Log calls must be synchronous. Wrapping them in setImmediate, process.nextTick, a Promise callback, or await-ing them solely to defer risks dropping records on shutdown and makes ordering non-deterministic.
CHECK-LOG-005: Logging an error at the throw site — Severity: suggestion
A function that throws should not also log the same failure — the error carries the context and the handler logs it once. Logging at both the raise and the catch sites double-reports the same incident.
CHECK-LOG-006: Large or binary payload logged in full — Severity: suggestion
Binary or oversized data (audio, images, encoded blobs, whole buffers) logged in full. Log its byte length (and an optional bounded preview), not the content. Useful text such as model prompts/completions may be logged in context fields when the pipeline can handle the volume.
Applies to repositories carrying a docs/ folder and README.md. Skip when the diff changes neither documented behavior nor documentation.
CHECK-DOC-001: Docs not updated in the same PR as the code — Severity: suggestion
Documentation ships with the code that changes it. A diff that adds or alters a documented behavior, endpoint, or area without updating the corresponding docs/*.md (or README.md) in the same PR is incomplete.
CHECK-DOC-002: New or renamed doc missing from the README index — Severity: suggestion
The root README.md Documentation section is the single index — every .md under docs/ (at any depth) must be listed there. A new or renamed docs/*.md not added to the index is a finding.
CHECK-DOC-003: Doc filename not kebab-case or not self-descriptive — Severity: nitpick
docs/*.md files must be kebab-case, self-descriptive, and share a domain prefix with their siblings (e.g. tts-google.md, tts-elevenlabs.md, not elevenlabs.md). A subfolder must not carry its own readme.md; the root README is the only index.
CHECK-DOC-004: Doc file too large or covering multiple areas — Severity: nitpick
A docs/*.md file exceeds ~5000 characters or documents more than one area. Split it; keep code examples minimal (link to source, explain the "why" in prose).
CHECK-DOC-005: Diff contradicts a documented project convention — Severity: suggestion
A change violates a convention documented in the repository's docs/* or README — the §1.5 Applicable standards map lists the conventions in play. Docs are unversioned prose, so this is capped at suggestion; a convention that must block belongs in an Accepted RFC. Quote the contradicted sentence verbatim (≤2 lines) in the finding detail and cite the doc section as a <pr-blob-url> link (defined in reviewComment Format).
docs/*.md API chapter prescribes cursor pagination and the diff adds an offset-paginated endpoint.Applies to repositories carrying an rfc/ folder (§1.4 builds the inventory). Skip CHECK-RFC-001/002 when the §1.5 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 <pr-blob-url> link (e.g. [RFC-0003](<pr-blob-url>/rfc/0003-service-logging-standard.md); <pr-blob-url> is defined in reviewComment Format).
CHECK-RFC-001: Diff violates an Accepted repository RFC — Severity: blocker
A change contradicts a clause of an RFC whose status is Accepted — the repository's ratified standard, immutable except through a version bump.
console.log to a service.CHECK-RFC-002: Diff conflicts with a Draft repository RFC — Severity: suggestion
A change contradicts a clause of a Draft RFC (including standards whose status was defaulted to Draft). Draft standards are proposals — advisory only, never a blocker; name the Draft status in the finding.
/v1/ route prefixes and the diff adds an unversioned route.CHECK-RFC-003: Accepted RFC edited without a version bump — Severity: blocker
The diff changes the content of an Accepted RFC without incrementing its version frontmatter and adding a Changelog entry (and updating updated when the format carries it). An Accepted RFC is immutable except through an explicit, recorded version bump.
version: stays unchanged.CHECK-RFC-004: RFC file hygiene — Severity: suggestion
A new or renamed RFC is missing from the rfc/README.md index, its filename does not follow NNNN-short-slug.md, or its frontmatter is missing or malformed (no parseable status).
rfc/0008-*.md standard without an index row; an RFC whose frontmatter lacks status.rfc/README.md index at all — the Glob fallback is the index there; flag only frontmatter problems.Applies to repositories carrying a principles/ folder (§1.4 builds the inventory). Skip when the §1.5 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: Diff conflicts with a stated repository principle — Severity: suggestion
A change works against a value stated in principles/ — the repository's long-lived design values, which its standards and reviews appeal to. Quote the principle verbatim (≤2 lines) in the finding detail and cite it as a <pr-blob-url> link (defined in reviewComment Format), matching CHECK-RFC-001/002; a finding that only paraphrases the value is not reportable.
docs/* convention already covers the same ground — report that check once and cite the principle in its detail; the diff itself amends the principle.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: New or changed HTTP API without an OpenAPI schema — Severity: suggestion
A new or changed public HTTP endpoint must have a matching OpenAPI/JSON schema, and a breaking change must be versioned with backward compatibility rather than altering an existing version in place.
CHECK-SVC-002: Service entrypoint without health checks — Severity: suggestion
A new long-running service must expose liveness, readiness, and startup health checks for orchestration. Flag a service entrypoint that wires none.
CHECK-SVC-003: Unstructured service logging — Severity: suggestion
Service logs must be structured JSON carrying a correlation/trace ID for cross-service tracing (the Logging checks then govern their quality). Plain console.log or free-form string logs in a service are a finding.
CHECK-SVC-004: Runtime or language version below the supported floor — Severity: nitpick
A new service must target the supported runtimes (e.g. Node.js 22+ LTS with TypeScript 5.8+, or Bun 1.2.19+ with TypeScript 5.8+). A manifest pinning an older floor is a finding.
{ severity, file, line, rule, title, detail }.(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.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:
[CHECK-BUG-002](<RULES_DOC_URL>#check-bug-002).[[CHECK-BUG-002](<RULES_DOC_URL>#check-bug-002), [CHECK-AI-002](<RULES_DOC_URL>#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 <a id> 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:
CHECK-BUG-002.CHECK-BUG-002, CHECK-AI-002.In both modes, append nothing when a finding has no rule code (do not emit [UNSPECIFIED]).
Map severity to its emoji when rendering in Phase 3: blocker → 🚧, suggestion → 🙋♂️, nitpick → 💡. The emoji stays first so downstream severity filters keep working.
STRICT RULES - No exceptions:
verdict: "requestChanges"verdict: "approve" (suggestions are non-blocking)verdict: "approve", reviewComment: ""FORBIDDEN:
{
"verdict": "approve" | "requestChanges" | "comment",
"reviewComment": "...",
"inlineComments": [
{"path": "src/file.ts", "line": 42, "body": "🚧 Issue description"},
{"path": "src/other.ts", "line": 15, "body": "🙋♂️ Suggestion here"},
{"path": "src/calc.ts", "line": 8, "startLine": 7, "body": "🚧 Off-by-one in the running sum [CHECK-BUG-001](<RULES_DOC_URL>#check-bug-001)", "suggestion": " for (let i = 0; i < n; i++)\n total += items[i];"}
]
}
startLine (first line of a multi-line range) and suggestion (verbatim replacement for the anchored line(s)) are optional per-comment fields — emit them only for concrete, mechanical fixes (see Code suggestions).
CRITICAL: Use these EXACT section names. "Observations", "Positive Notes", or similar variations are NOT allowed.
SKIP empty sections entirely. Do NOT write "None" or "N/A" - just omit the section.
Ticket references: when the body cites the linked ticket, cite it as a markdown link built from the §1.2 resolve-issue-context url (e.g. [ENG-123](https://linear.app/<workspace>/issue/ENG-123)) — a bare tracker id GitHub does not auto-link is dead text; fall back to the bare id only when no URL is resolvable. GitHub issue numbers stay bare (#42 auto-links).
File and doc links: define <pr-blob-url> = https://github.com/<REPO>/blob/<headRefOid> (headRefOid from §1.1; valid for fork PRs too — PR head commits stay reachable in the base repo via refs/pull/N/head). Percent-encode path characters that break markdown links (spaces → %20, parentheses → %28/%29, : → %3A). Then:
inlineComments bodies. With a known line: [<path>:<NN>](<pr-blob-url>/<path>#L<NN>) (ranges #L<start>-L<end>). With no line — a plain prose mention of a repo file/doc — drop the fragment and still link the path: [<path>](<pr-blob-url>/<path>). A bare RFC-NNNN links to its doc via the §1.4 standards inventory even when a trailing §X section anchor is unresolvable (link the document; omit the anchor).files list or one the §1.2 snapshot / Glob / gh lookup confirmed; a standard's id resolves to its path via the §1.4 standards inventory. An id with no resolved path, or a path with no blob at head (deleted, or a renamed-from old path), is NEVER linked by guess — keep it backticked/bare; a fabricated 404 is worse than no link. A token that is not a real target — a glob or pattern (*.steps.ts), a bare directory, a config key (agents.trackers), or an illustrative name that resolves to no repo blob — is a code specimen, not a reference: keep it backticked and never link it..md target inserts ?plain=1 before #L so the anchor lands (e.g. [docs/setup.md:96](<pr-blob-url>/docs/setup.md?plain=1#L96)); a section cite uses the rendered heading-anchor form <pr-blob-url>/<path>#<heading-anchor>, no ?plain=1; never combine the two.reviewComment (including the summary sentence, not just finding lines) and every inlineComments body: outside code spans/fences, any resolvable repo-relative file or doc path — with OR without a line number — a bare RFC-NNNN id, or a bare 7–40-char hex token is a violation unless it is one of the two allowed forms above (inline own anchor; unresolvable/no-blob-at-head path). Link paths per these rules and SHAs as https://github.com/<REPO>/commit/<sha>. A path that resolves to no repo blob (a glob, pattern, config key, or illustrative name) is NOT a violation — leave it backticked.Empty vs non-empty reviewComment follows the canonical Verdict Decision Rules: use empty "" for an approval with no findings (rule 3) — the verdict field drives the GitHub event, so no body text is needed; use a non-empty body for any review with findings or a requestChanges verdict; and when there is nothing new to report, emit no structured output at all (rule 0).
If reviewComment is non-empty, use these verdict headers at the END:
verdict: "requestChanges" → ### ⛔ Request Changesverdict: "approve" (with suggestions/nitpicks) → ### 👍 Approveverdict: "comment" → ### 💬 CommentExample: approve with no findings (most common case)
{
"verdict": "approve",
"reviewComment": "",
"inlineComments": []
}
Example: requestChanges with blockers
{
"verdict": "requestChanges",
"reviewComment": "Adds retry logic to the payment webhook handler.\n\n### 🚧 Blockers\n\n1. **Missing idempotency check** - [src/webhooks/payment.ts:45](<pr-blob-url>/src/webhooks/payment.ts#L45) - Retries can cause duplicate charges [CHECK-BUG-002](<RULES_DOC_URL>#check-bug-002)\n\n### ⛔ Request Changes\n\nAdd idempotency key validation before processing payment.",
"inlineComments": [
{
"path": "src/webhooks/payment.ts",
"line": 45,
"body": "🚧 No idempotency check — retries will duplicate charges [CHECK-BUG-002](<RULES_DOC_URL>#check-bug-002)"
}
]
}
Example: approve with suggestions (non-blocking)
{
"verdict": "approve",
"reviewComment": "Points the retry policy in [docs/webhooks.md](<pr-blob-url>/docs/webhooks.md) at the new handler.\n\n### 🙋♂️ Suggestions\n\n- [src/webhooks/payment.ts:62](<pr-blob-url>/src/webhooks/payment.ts#L62) - Consider exponential backoff; the `retry*` helpers in [src/webhooks/config.ts](<pr-blob-url>/src/webhooks/config.ts) still assume single-attempt delivery [CHECK-ARCH-002](<RULES_DOC_URL>#check-arch-002)\n- [docs/webhooks.md:12](<pr-blob-url>/docs/webhooks.md?plain=1#L12) - Retry policy chapter still documents single-attempt delivery [CHECK-DOC-001](<RULES_DOC_URL>#check-doc-001)\n\n### 👍 Approve",
"inlineComments": [
{
"path": "src/webhooks/payment.ts",
"line": 62,
"body": "🙋♂️ Consider exponential backoff for retries [CHECK-ARCH-002](<RULES_DOC_URL>#check-arch-002)"
}
]
}
reviewComment body template (ONLY when there are findings):
Every blocker, suggestion, and nitpick line ends with the rule code rendered per §2.5 (e.g. [CHECK-BUG-002](<RULES_DOC_URL>#check-bug-002)). If two checks flagged the same (path, line), render the merged form [[CHECK-BUG-002](<RULES_DOC_URL>#check-bug-002), [CHECK-AI-002](<RULES_DOC_URL>#check-ai-002)]. Build the links yourself from RULES_DOC_URL. When a finding has no rule code, omit the suffix entirely.
[1 factual sentence: what this PR changes — no quality judgment]
### 🚧 Blockers
1. **[Title]** - [src/path/to/file.ts:NN](<pr-blob-url>/src/path/to/file.ts#LNN) - [Problem in 1 line] [CHECK-BUG-XXX](<RULES_DOC_URL>#check-bug-xxx)
### 🙋♂️ Suggestions
- [src/path/to/file.ts:NN](<pr-blob-url>/src/path/to/file.ts#LNN) - [Recommendation in 1 line] [CHECK-AI-XXX](<RULES_DOC_URL>#check-ai-xxx)
### 💡 Nitpicks
- [src/path/to/file.ts:NN](<pr-blob-url>/src/path/to/file.ts#LNN) - [Optional fix in 1 line] [CHECK-CPLX-XXX](<RULES_DOC_URL>#check-cplx-xxx)
### ⛔ Request Changes / ### 👍 Approve
[1 sentence: what must change — ONLY for requestChanges. Omit for approve.]
Add inline comments for issues with specific code locations:
Each inline comment: 1-2 sentences, start with severity emoji, end with the rule code rendered per §2.5 (e.g. 🚧 No idempotency check — retries will duplicate charges [CHECK-BUG-002](<RULES_DOC_URL>#check-bug-002)).
Add an optional suggestion to an inline comment when the fix is concrete and mechanical — a rename, a guard clause, a corrected operator — and you can write it as exact replacement text. The action renders it as a one-click GitHub suggestion block ("Commit suggestion").
suggestion REPLACES the anchored line(s). Reproduce the original line(s) verbatim except for your change, including leading indentation — GitHub applies the text as-is, so a stray space silently reindents the file.```suggestion fence, no +/- diff markers, no prose (the action wraps it).line only. Multi-line fix: set startLine (first line) and line (last line) over a contiguous range fully inside the diff. If the fix touches lines outside the diff, describe it in prose and omit suggestion.suggestion only when confident it applies cleanly; otherwise keep the prose finding alone.[src/services/payment/processor.ts:66](<pr-blob-url>/src/services/payment/processor.ts#L66), NOT processor.ts:66)[<CODE>](<RULES_DOC_URL>#<code>) with the fragment lowercased, or merged [[<CODE1>](<RULES_DOC_URL>#<code1>), [<CODE2>](<RULES_DOC_URL>#<code2>)] for a shared location) on every finding line (blocker, suggestion, nitpick) and every inlineComments.body; omit the suffix entirely when no rule code is availablesrc/services/payment/processor.ts:66); apply the linking rules to the review-body prose and to any cross-file or out-of-diff reference inside inline bodiessuggestion field instead (see Code suggestions); it renders as a one-click GitHub suggestion blockThese rules govern references — when you point the reader at a real file, standard, section, commit, or issue. (A token named only as an example, with no real target, is a code specimen in backticks, like any code identifier.) Every reference must resolve: render it as a real link whose target exists, and prefer the most stable link form so it does not rot. Render the same kind of reference the same way everywhere:
buildReviewComments, reviewOutput.ts. A backticked token names a thing as an example; it is not a reference and carries no link.[release field spec](<repo-blob-url>/docs/06-release-field.md). Use a repo-relative path in repository files and the absolute <repo-blob-url> form in generated output posted outside the repo (PR/issue bodies, review comments, release notes), where relative paths do not resolve. Any prose mention of a file or path that exists in the repo is such a reference — link it so it resolves on the default branch at writing time; a path that does not exist yet (a file the text proposes to create) or one shown inside a command or fenced block is a code specimen, not a reference.[RFC-0001](<repo-blob-url>/rfc/0001-reference-formatting.md); an Accepted RFC is immutable except through an explicit version bump, so the link never rots.[title](url) to the canonical source, taking the title from the source (or the site name). Use only a URL present in your input or context — never produce one from memory; a source with no known URL stays plain prose. When several sources back one document, they may be gathered into a short references list.#anchor, e.g. [Phase 6](#phase-6-reply-to-review-threads). Another document: path#anchor — a repo-relative path in repository files, the absolute <repo-blob-url>/path#anchor form in generated output. A GitHub anchor is the heading lower-cased, spaces turned to hyphens, punctuation dropped.[0328a61](<repo-commit-url>/0328a61); a commit is immutable. If you cannot build the URL, leave the bare SHA un-backticked.ENG-123) is dead text when bare: in prose, ALWAYS render it as a markdown link, e.g. [ENG-123](https://linear.app/<workspace>/issue/ENG-123) — a slug-less issue URL resolves. On a magic-word line (Closes/Fixes/Related to in a PR body's **Issues:** section) use plain forms only: bare #N for GitHub, the plain issue URL for other trackers — never a markdown-bracket link, which breaks the close-parsers.Backticks suppress GitHub autolinking: a commit SHA or issue/PR number inside a code span renders as dead text — that is why a backticked SHA was un-clickable in a prior review. Never wrap a SHA or issue/PR number in backticks; link it, or leave it bare so GitHub auto-links it.
Write the most helpful, readable output you can: plain, direct prose; every reference resolvable; explain the "why", not the obvious "what".