rmsaitam/code-review

Code Review

Act as a senior reviewer. Review the diff between HEAD and a fixed base supplied by the user. Produce one traceable artifact:

• CODE-REVIEW.md: review context, summary, Spec axis, Standards axis, suggested validation, evidence, assumptions, open questions, and traceability.

Review two axes separately:

• Standards: does the code follow this repository's conventions, security policies, testing standards, and architectural decisions?
• Spec: does the code faithfully implement the issue, PRD, or specification?

Never mix the two results. A change can follow every convention and still implement the wrong requirement.

Do not issue an automatic approval. Report risks and required validation only.

When to use

Use this skill when the user:

• asks for a review of a branch, PR, merge request, or work in progress;
• asks to review changes since a branch, commit, tag, date, or HEAD~N;
• wants to know whether a change follows the repository standards;
• wants to know whether a change implements the issue, PRD, or specification;
• wants a traceable review artifact with evidence and suggested validation.

When not to use

Do not use this skill for:

• a full security audit with CVSS, CWE, and OWASP mapping; use security-audit;
• product discovery, a PRD, a SPEC, milestones, or task breakdown; use techlead;
• design tokens, component libraries, or accessibility audits; use design-system;
• a single-line change with no diff and no fixed base; review it directly or ask for a base;
• running tests, builds, linters, or deployments. This skill reviews and suggests validation; it does not execute it unless the user explicitly asks.

Non-negotiable rules

• Fix the base before reviewing. Ask the user when the base is missing. Never assume main.
• Validate the reference with git rev-parse --verify and review git diff <base>...HEAD.
• Stop and report when the base does not exist or the diff is empty. Do not continue to specialized reviews.
• Keep the Spec and Standards axes separate in the analysis, findings, and report.
• Repository standards documented in the repository take priority over general heuristics.
• Ignore issues already detected and blocked by lint, formatter, typechecker, or CI.
• Distinguish an objective violation from an improvement suggestion.
• Do not propose refactors outside the diff, except for security, correctness, or regression risk.
• Every finding must have evidence: a diff snippet, a file and line, a spec requirement, or a reproducible observation.
• Do not invent lines, behaviors, or requirements. If the evidence is insufficient, write a note or an open question.
• Redact secrets, tokens, personal data, and customer data. Replace values with [REDACTED].
• Read-only by default: do not modify application code, dependencies, configuration, commits, or pull requests.
• Do not run tests, linters, builds, or deployments unless the user explicitly asks.
• Do not lower a severity to unblock a release. Record the risk instead.
• Do not emit an automatic approval verdict. Report risks and the validation that is still required.
• Write the artifact in English by default. Keep IDs, markers, severity values, axis values, and status values in English.
• Preserve every machine-readable marker in the report template.
• Stop the review when the remaining questions would not change a finding, a severity, or the validation plan.

Modes

Select the mode before starting and record it in the artifact.

• Review: fix the base, read the diff, review both axes, and write or update CODE-REVIEW.md.
• Validate: validate an existing CODE-REVIEW.md against the report contract without redoing the review.

Workflow

Follow these gates in order. Do not skip a gate.

Gate 1: Fix the review base

1. Identify the fixed point: a branch, commit SHA, tag, main, origin/main, HEAD~N, or another explicit reference.

2. If the user did not provide a base, ask which base must be used. Do not guess.

3. Validate the reference:

   git rev-parse --verify <base>
   

4. Check whether the repository is shallow when the merge-base may be missing:

   git rev-parse --is-shallow-repository
   

5. Generate the diff against the merge-base:

   git diff <base>...HEAD
   

6. List the commits included:

   git log <base>..HEAD --oneline
   

7. Collect the diff size and the changed files:

   git diff --stat <base>...HEAD
   git diff --name-status <base>...HEAD
   

8. Inspect the working tree:

   git status --short
   

9. If uncommitted changes exist, ask whether they are in scope. When they are, include git diff and git diff --cached as an additional review surface and record the decision.

10. If the base does not exist, the merge-base cannot be resolved, or the diff is empty, stop and inform the user. Do not continue to Gate 2.

11. Record every command, its target, and the relevant output in the report evidence section.

When the user gives a date instead of a reference, resolve the last commit before that date explicitly:

git rev-list -1 --before=<date> HEAD

Use the resulting commit as the base only after the user confirms it.

Gate 2: Find the specification

Look for the requirement source in this order:

1. An issue reference in the commits, branch name, or PR description (#123, Closes #45, Fixes #78).
2. A specification path supplied by the user.
3. A file in docs/, specs/, .scratch/, or tasks/ related to the branch name or the feature.
4. Ask the user where the specification is.

Read the source and extract:

• the problem and the expected outcome;
• the acceptance criteria;
• the explicit in-scope and out-of-scope items;
• the constraints, edge cases, and non-functional requirements;
• the requirements that the diff does not touch but must preserve.

Map each requirement to the files, behaviors, and tests that implement it.

If the user says there is no specification, record exactly:

No specification available; Spec review was skipped.

Set Spec review: skipped and do not review the Spec axis or invent requirements.

Gate 3: Load the repository standards

Read, when they exist:

• CONTRIBUTING.md;
• CODING_STANDARDS.md;
• ARCHITECTURE.md;
• SECURITY.md;
• ADRs in docs/adr/;
• lint, formatter, test, and CI configuration;
• recent approved examples in the repository.

Rules:

• Documented repository standards take priority over general heuristics.
• Ignore issues that lint, formatter, typechecker, or CI already detect and block.
• Distinguish an objective violation from an improvement suggestion.
• Cite the standard that grounds each Standards finding.
• If no standards are documented, use general heuristics and label them as such in the finding reference.
• If a standard is ambiguous, record an open question instead of inventing a rule.

Gate 4: Review by category

Review every changed file and every meaningful hunk. Do not review only the added lines; consider the surrounding code and the consumers of the changed behavior.

Spec axis

• Requirements met: map each acceptance criterion to the implementation and its tests.
• Gaps or incorrect implementation: missing requirement, partial implementation, wrong behavior, contradiction with the spec, or an untested requirement.
• Additional scope: behavior added outside the spec, unrequested refactors, changed public contracts, or scope creep.
• For every gap, cite the exact requirement and the file and line where the implementation fails.

Standards axis

• Correctness: bugs, edge cases, error handling, concurrency, null/undefined, types, and unexpected behavior.
• Security: injection, authentication and authorization, input validation, data exposure, secrets, SSRF, XSS, CSRF, unsafe file handling, and permission boundaries.
• Tests: coverage of new or changed paths, negative cases, regressions, assertion quality, and flakiness.
• API and compatibility: breaking changes, public contracts, versioning, migrations, and consumer compatibility.
• Performance: N+1 queries, unnecessary loops, allocations, cache behavior, indexes, and avoidable synchronous work.
• Maintainability: names, duplication, coupling, responsibilities, premature abstractions, and consistency with the domain.
• Observability: logs, metrics, tracing, alerts, and useful error messages when the change affects production operation.

Use references/review-checklist.md for the detailed checklist.

Gate 5: Write the report

Create or update CODE-REVIEW.md using references/report-template.md. The default location is the current workspace root. Accept a different path when the user supplies one.

Requirements:

• Preserve the artifact marker and every required section marker.
• Record the base, head, merge-base, commit count, files changed, standards sources, and spec source.
• Keep the Spec and Standards sections separate. Never move a Spec finding into Standards or the reverse.
• Use stable finding IDs: CR-001, CR-002, and so on. Do not renumber approved findings without recording the change.
• Use one severity per finding: blocking, recommended, or note. Follow references/severity-baseline.md.
• Give every finding an axis, a category, a location, a problem, evidence, a suggestion, a reference, and a verification step.
• Make the counts marker match the findings exactly.
• Make the summary counts match the counts marker.
• Map every finding to evidence and suggested validation in the traceability section.
• State whether the Spec review was performed or skipped. When skipped, include the exact sentence from Gate 2.
• Write No findings in a category when it has no findings. Do not leave empty sections.
• Do not include secrets, personal data, or customer data. Redact them as [REDACTED].
• Do not include an approval verdict, LGTM, or Ship it.

Gate 6: Validation

Resolve the validator path relative to this SKILL.md and run:

python3 skills/code-review/scripts/validate_review.py CODE-REVIEW.md

Options:

--strict                     Treat warnings as failures.
--json                       Emit a machine-readable report.
--min-findings N             Require at least N findings. The default is 0.
--require-spec               Fail when the Spec review was skipped.
--require-standards-sources  Fail when no standards source is listed.

Fix every error and re-run the validator. Do not deliver a report that fails validation.

If Python is unavailable, perform the manual checklist in references/validation-checklist.md.

Gate 7: Handoff

Report:

• where CODE-REVIEW.md was written;
• the base reviewed, the merge-base, the commit count, and the files changed;
• the finding counts by severity and by axis;
• the Spec review status and the highest-risk Spec gap;
• the highest-risk Standards findings and their impact;
• the validation result;
• the limitations, excluded files, and checks that did not run;
• that no application code, dependency, or configuration was changed.

Offer to propose fixes or a remediation plan, but do not modify code, commit, push, or open a pull request unless the user explicitly asks.

Two-axis rule

The axes answer different questions and must remain separate:

Axis	Question	Typical evidence
Spec	Does the change implement the requested requirement?	Issue, PRD, specification, acceptance criteria
Standards	Does the change follow repository conventions and engineering standards?	CONTRIBUTING, ADRs, lint/test/CI config, code review

A finding may exist on both axes only when the root cause is different and the remediation is different. Otherwise, record it once under the axis that owns the root cause and cross-reference the other axis in the reference field.

Examples:

• A change follows every convention but omits an acceptance criterion: one Spec blocking finding and no Standards finding.
• A change implements the requirement but introduces an authorization bypass: one Standards blocking finding and a Spec note that the requirement is implemented but unsafe.
• A change implements the requirement with a documented but poor pattern: one Standards recommended finding and a Spec "requirements met" entry.

Artifact contract

CODE-REVIEW.md starts with:

<!-- code-review:artifact type=review version=1 -->

Required sections use markers:

<!-- code-review:section id=summary -->
<!-- code-review:section id=context -->
<!-- code-review:section id=spec -->
<!-- code-review:section id=standards -->
<!-- code-review:section id=validation -->
<!-- code-review:section id=evidence -->
<!-- code-review:section id=assumptions -->
<!-- code-review:section id=traceability -->

The Spec section also uses a status marker:

<!-- code-review:spec status=performed -->
<!-- code-review:spec status=skipped -->

The counts marker must match the findings:

<!-- code-review:counts blocking=1 recommended=2 note=0 -->

Each finding uses a marker immediately before its heading:

<!-- code-review:finding id=CR-001 axis=standards severity=blocking category=security status=open -->
### CR-001: Short finding title

Every finding body contains:

• Axis: Spec or Standards
• Severity: Blocking, Recommended, or Note
• Category: spec-gap, scope, correctness, security, tests, api, performance, maintainability, or observability
• Location: path/to/file.ext:42, path/to/file.ext, or n/a for a Spec finding without a code location
• Problem: the defect, risk, or gap
• Evidence: a diff snippet, a file and line, a spec requirement, or a reproducible observation
• Suggestion: the smallest fix that addresses the root cause
• Reference: the spec path and requirement ID or the repository standard that grounds the finding
• Verification: the test, command, or manual check that proves the fix

Stable ID families:

Family	Meaning	Format
CR-001	Review finding	Finding marker plus heading
Q-001	Open question	### Q-001 heading in the assumptions section
EV-001	Evidence entry	Row in the evidence section when a table is used

Severity model

Use three levels.

Blocking

A bug, security failure, contract break, missing requirement, or missing essential test.

Examples:

• an acceptance criterion is not implemented;
• an authorization check is missing on a changed endpoint;
• a public API contract changes without a migration path;
• a data migration is not reversible when the repository requires reversibility;
• a changed path has no test and the repository requires tests for it.

Blocking findings must be resolved or explicitly accepted before merge.

Recommended

A risk, relevant technical debt, design improvement, or important additional test.

Examples:

• a new branch lacks a negative test;
• a query is N+1 but the current data volume hides it;
• a name or abstraction is inconsistent with the domain;
• a log line is missing for an operationally important failure;
• a refactor inside the diff would remove duplication.

Recommended findings should be fixed in the current cycle or recorded as accepted debt.

Note

An observation, question, optional improvement, or a possible issue that lacks enough evidence.

Examples:

• a behavior may be intentional but the specification does not say;
• a performance concern depends on data volume that is not available;
• a naming preference that is not documented in the repository.

Notes do not block merge but must be visible in the report.

Evidence rules

• Evidence must be reproducible: a command, a diff hunk, a file and line, or a spec requirement.
• A code snippet must show the problematic construct without exposing secrets or personal data.
• A spec finding must cite the requirement, not only the implementation.
• A Standards finding must cite the repository standard or state that the finding uses a general heuristic.
• A possible issue without direct evidence goes to note or to open questions, not to blocking.
• Do not report the same root cause twice. Group variants under one finding.
• If a finding is a false positive, mark it false-positive, record the evidence, and keep it out of the open counts.
• If a risk is accepted, mark it accepted, record the owner, rationale, and expiry, and include it in the validation section.

Handling edge cases

Empty diff

Stop after Gate 1. Tell the user that the diff is empty and that no specialized review was performed. Do not create a report with invented findings.

Missing base

Ask for the correct reference. Do not fall back to main or to the current branch without confirmation.

Shallow clone

If the merge-base is unavailable, ask the user to fetch the missing history or to supply an explicit base. Record the limitation.

Large diff

Group the files by area and review the highest-risk areas first: authentication, authorization, data access, payments, file handling, public APIs, migrations, and infrastructure. State what was excluded and why.

Uncommitted changes

Ask whether the working tree is in scope. If it is, record the exact commands used for the staged and unstaged diffs.

No specification

Record the exact skip sentence, set Spec review: skipped, and review only the Standards axis. Do not invent requirements.

No documented standards

Use general heuristics, label them as such, and avoid presenting preferences as violations.

Generated files and vendored code

Exclude them unless the change modifies the generator, the vendoring process, or the generated contract. Record the exclusion.

Definition of done

The review is done only when:

• the base was validated and the diff was generated against the merge-base;
• the commit list, changed files, and working tree state were recorded;
• the specification was found or the skip was recorded with the exact sentence;
• the repository standards were loaded or the absence was recorded;
• the Spec and Standards axes were reviewed separately;
• every finding has an axis, severity, category, location, problem, evidence, suggestion, reference, and verification;
• the counts marker matches the findings and the summary;
• the traceability section maps every finding to evidence and suggested validation;
• the validation section names the tests, typecheck, lint, CI, and manual checks that are still required;
• no placeholder, secret, personal data, or automatic approval remains;
• the validator passes with no errors;
• no application code, dependency, or configuration was changed.

Output format

• Use clean Markdown with numbered sections and structured lists.
• Use tables for the summary counts, findings, and traceability.
• Use fenced code blocks for diff evidence and spec excerpts.
• Use ### CR-001: Title headings for findings and bold labels when referencing them in prose.
• Keep prose direct and technical.
• Always present the evidence and the suggested fix, not only the conclusion.
• End with the required validation and the remaining risks. Do not end with an approval.

Resources

• Question bank: base, spec, standards, depth, exclusions, and audience.
• Report template: CODE-REVIEW.md structure, markers, and a complete example.
• Severity baseline: blocking, recommended, note, calibration, and status values.
• Review checklist: Spec and Standards review areas.
• Spec discovery: how to find and trace the requirement source.
• Repository standards: which files to read, precedence, and how to cite them.
• Validation checklist: manual fallback when the script cannot run.
• scripts/validate_review.py: machine-readable validation for CODE-REVIEW.md.