Code Review

Code Review

Purpose

Perform structured code review focused on:

• correctness
• architecture
• regression risks
• security and performance issues

---

Constraints

• Apply @rules/php/core-standards.mdc
• Apply @rules/api/general.mdc — when the diff adds or modifies an HTTP API surface (routes, controllers / __invoke request handlers, API Resources / DTOs serialized into responses, FormRequests, status-code / response() / abort() calls, Idempotency-Key handling), walk it against the API contract pillars. The dedicated walk lives in @skills/api-review/SKILL.md (Specialized Reviews → Always run); severities follow that rule's CR Severity Rules section.
• Apply @rules/code-review/general.mdc
• Apply @rules/refactoring/general.mdc — use the shared refactoring definition when assessing refactoring changes or when proposing refactoring; reject big-bang rewrites and prefer incremental migration.
• Apply @rules/php/dependency-selection.mdc — when the PR diff adds a new require / require-dev entry to composer.json, walk the Activity + Compatibility gates from that rule against the PR description / commit body. A missing selection note is a Critical finding; an adopted archived / abandoned / branch-pinned package is a Critical finding on the spot; a single-maintainer adoption without bus-factor flag is a Moderate finding.
• If the current project uses Laravel, also apply @rules/laravel/laravel.mdc, @rules/laravel/architecture.mdc, @rules/laravel/filament.mdc, and @rules/laravel/livewire.mdc
• Output findings only (no praise)
• Read-only skill — never modify code, never stage / commit / push changes, and never run any git write operation (git add, git commit, git push, git reset, git checkout -- …, etc.). Checking out the relevant branch and git pull to read the latest code are required (the mandatory Branch checkout gate below); mutating the working tree or pushing to the remote is not. Output is the review markdown only.
• Apply @rules/reports/general.mdc — the review markdown handed to code-review-github / code-review-jira for publishing on the GitHub PR stays in canonical English per the rule's Exception — technical CR findings on the GitHub PR (severity labels, structured field labels, rule references, and code identifiers are all in English). The non-technical mirror that the wrappers delegate to @skills/pr-summary/SKILL.md follows the language of the source assignment — that is the wrapper's responsibility, not this skill's.
• Do not duplicate findings the project's fixers already auto-correct (Pint, PHPCS, Rector — pure whitespace, import ordering, unused-use, single-line vs multi-line argument splits). Those are caught by the build. Do flag every rule violation a fixer does not cover — architectural breaches, structural rules, missing return types, untyped DTO boundaries, naming bound to a domain rule, testing-pattern violations, etc.

---

Execution

• Branch checkout gate (mandatory, always). Before any analysis step, check out the branch that contains the changes and pull the latest commits — git fetch, git checkout , and git pull when the branch tracks a remote (skip the pull for a local-only branch that has no upstream, e.g. the read-only fallback review of a branch that maps to no PR) — so the review always runs against the actual current codebase on disk (the checked-out working tree), never against a remote diff in isolation. Confirm local HEAD matches the change branch's head commit. If the branch cannot be checked out (missing ref, detached HEAD, or local changes that would be overwritten), stop and report it instead of reviewing from a diff. Every subsequent step reads the checked-out files so findings reflect the real state of the code.
• Identify changes vs main branch.
• Deduplicate previous findings.

Cross-run history

The CR wrappers publish the review through an always-new comment per CR run (both GitHub and JIRA — see @skills/code-review-github/SKILL.md and @skills/code-review-jira/SKILL.md). Every run POSTs a fresh comment so the chronological sequence of comments is the audit trail; history never lives in a tracker's edit history. Do not load prior CR findings from PR comments and do not author a Previous CR Status section in the output — the always-new-comment convention makes it redundant.

Issue Context Analysis

Before reviewing code, load and analyze the full issue context:

1. Load the complete issue or task (description, all comments, and attachments) from the linked tracker (GitHub, JIRA, Bugsnag). For JIRA issues, call skills/code-review-jira/scripts/load-issue.sh and read all fields off the resulting JSON document — never call acli directly. Fall back to the JIRA MCP server only when the script is unavailable or for data outside its scope (changelog, available transitions, friendly custom-field names). For Bugsnag errors, call skills/code-review-bugsnag/scripts/load-issue.sh (requires BUGSNAG_TOKEN) and read the error class, message, context, latestEvent.stacktrace, comments[], and linkedIssues[] off the JSON — never call api.bugsnag.com directly. Fall back to a Bugsnag MCP server only when the script is unavailable.
2. Extract from the issue:

• Requirements and acceptance criteria — what the code must do
• Expected behavior — how the feature or fix should work
• Edge cases and constraints — mentioned by the reporter or in comments
• Test data — any sample inputs, payloads, or scenarios provided in the issue

1. Use this context to evaluate whether the implementation fully satisfies the issue — not just whether the code is technically correct.
2. If the issue contains test data or test scenarios, verify they are covered by existing or new tests. Flag missing test coverage as a finding.

Assignment Conformance Gate (mandatory)

Every CR run must explicitly verify both directions of the relationship between the diff and the linked assignment, then surface a single conformance verdict. When no tracker is linked (closingIssues[] empty for a GitHub PR, no JIRA / Bugsnag reference), skip the gate and state assignment conformance: no linked issue on the summary line.

1. Requirements → changes (completeness). Every requirement, acceptance criterion, expected behavior, edge case, and sample test scenario extracted under Issue Context Analysis must map to a concrete change in the diff that implements it — including the testing logic: tests added or modified by the diff must themselves assert the correct, assignment-required behavior and must not assert a stale, incorrect, or reduced version of a requirement. Verify that test assertions (expected values, exception types, event payloads, response shapes) are consistent with the current assignment requirements — not with an older version of the spec. This direction is already executed by the always-run @skills/assignment-compliance-check/SKILL.md and @skills/analyze-problem/SKILL.md (assignment-conformance scope) in Specialized Reviews; do not re-derive or duplicate their findings here — consume their result. Any unmet requirement (in production code or in test logic) is already a Critical finding raised there.
2. Changes → requirements (traceability, no scope creep). This is the direction those two skills do not cover and the gate adds: walk every changed code block in the diff (added or modified production lines, per file) and trace each one back to a specific requirement, acceptance criterion, or expected behavior from the assignment. Classify each block:

• In scope — directly implements a stated requirement. Cite the requirement.
• Allowed support — does not implement a requirement on its own but is necessary to deliver one: tests for the changed behavior, a refactor/extraction the requirement forces, a migration/config the requirement needs, a fixup of code the change touches. Cite the requirement it supports.
• Out of scope (finding) — traces to no requirement and is not allowed support: an unrequested feature, an opportunistic refactor of untouched concerns, a drive-by behavior change, a config/dependency change the assignment never asked for. Raise one finding per out-of-scope block: file:line, the change in one sentence, and "no assignment requirement traces to this change". Severity: Moderate by default; escalate to Critical when the untraceable change alters observable behavior, touches a security / payment / auth surface, or adds a dependency. The Suggested Fix is to remove the change from this PR and, when it has independent value, move it to its own issue / PR. Do not duplicate a finding Simplicity First already raised for the same block — keep it here as the assignment-traceability finding and cite Simplicity First instead of emitting a second entry. The two lenses are not the same: Simplicity First owns unrequested complexity, while this gate owns traceability — a change can be perfectly simple yet still trace to no requirement (e.g. a one-line drive-by rename), and that case is this gate's to raise, not Simplicity First's.

1. Verdict. Record an explicit one-line verdict on the review summary line: assignment conformance: conformant (every requirement implemented and every change traces to the assignment), or assignment conformance: N gap(s). The verdict is computed at Output assembly — after the Specialized Reviews have produced their results and after the Critical Findings Verification (issue #537) step has dropped any refuted Criticals — so it counts only gaps that survive into the published review and never contradicts the Counts line. N is the count of Critical assignment gaps (unmet requirements from step 1) plus out-of-scope findings (step 2), regardless of which surface publishes each one — step-1 gaps may appear on the linked-tracker compliance block while step-2 findings sit in the PR comment's severity buckets, but both count toward N. The verdict is always rendered on the summary line so a reader sees the conformance result without scanning the body; the individual findings live in their normal severity buckets.

Third-Party API & Service Analysis

Run this section only when the diff integrates with, modifies, or depends on a third-party API or external service (HTTP clients, vendor SDK calls, webhooks, OAuth flows, payload schemas, queue/event consumers backed by external systems).

1. Identify every affected API or service from the diff and list the concrete endpoints, SDK methods, webhook events, or message contracts that changed.
2. Locate the official public reference for each one — vendor documentation, OpenAPI/Swagger spec, SDK reference, or webhook contract. Prefer URLs cited in the issue or PR; otherwise look up the vendor's current published documentation for the version in use.
3. Compare the implementation against the public contract:

• endpoints, HTTP methods, and required vs optional parameters
• request and response schemas, status codes, and error envelopes
• authentication, scopes, rate limits, idempotency keys, and retry semantics
• pagination, filtering, sorting, webhook signatures, and timeouts

1. Cross-check the implementation against the issue assignment — verify the chosen endpoints, parameters, and behaviors satisfy what the issue actually asked for. Flag any divergence (missing endpoint, wrong verb, ignored field, fabricated parameter) as a finding.
2. Confirm coverage of every API use case that is in scope for the issue — documented filters, status branches, error states, and edge inputs the issue explicitly or implicitly requires. Missing in-scope use cases are findings. Do not propose adopting API features that current scope does not require (YAGNI per @rules/php/core-standards.mdc); only when the diff exposes an out-of-scope structural shortcoming in how the project consumes the API (e.g. missing webhook signature verification across other consumers) raise it under Refactoring Proposals.
3. If the public reference cannot be located, accessed, or matched to the version in use, raise this as a Moderate finding instead of silently assuming the contract.

Core Analysis

• Regression risk (shared logic, dependencies)
• Architecture and design quality
• Business logic correctness
• Missing or incorrect behavior
• Type safety and error handling
• **Full Core Analysis walk-through (canonical detail in @rules/code-review/general.mdc Core Analysis Walk-through). Apply every bullet there to the diff and raise one finding per violation at the severity it declares: Reuse of existing logic, Action scope, Speculative interfaces, Simplicity First, method-parameter-count (>4 → DTO), public-method raw-array-vs-DTO, new static-analysis / linter suppression, Strict rule compliance (mandatory walk-through), Architecture conformance (Laravel) (issue #530), Test organization (issue #528), per-row DB operations in loops, variable ordering / lazy evaluation, object caching, new storage reuse analysis, SQL index reuse / performance non-regression, refactoring quality + test-coverage contract, data-validation encapsulation, pass-through Action, repository scope, inline Eloquent / read-write layer separation, Action-returns-HTTP-response, inline data mapping → Data Builder, inline validation guards / throw_if / throw_unless / enum-mode match() → Data Validator, only-Laravel-and-arch-layers class inventory, Request → DTO transformation belongs in the FormRequest, not the controller, Data Modification (DRY), and Entry-point error handling for known failures (Laravel)**.

Highest-Priority Fast Track

Apply this subsection only when the source issue is flagged as highest priority, so the bug fix can deploy as fast as possible without sacrificing the Critical / Moderate gate.

1. Detect highest priority from the issue context already loaded under Issue Context Analysis:

• GitHub: any label whose name matches (case-insensitively) priority: highest, priority/highest, priority-highest, p0, urgent, or blocker.
• JIRA: the native priority field equals Highest or Blocker.
• Bugsnag: the linked GitHub issue carries one of the GitHub labels above.

If no signal matches, skip the rest of this subsection and run the review normally.

1. Narrow the review scope to whatever directly affects the bug fix and its safe deployment. Out-of-scope improvements that the diff merely happens to sit near must be moved to Refactoring Proposals as follow-up items, never blockers.
2. Keep the resolution gate at Critical and Moderate. No widening, no narrowing — those two severities still block the merge, exactly as in the default flow. State this explicitly in the review header so the caller does not have to infer it.
3. Demote non-blocking sections to follow-up only. Still emit them so nothing is lost, but mark each entry as follow-up; does not block merge:

• Minor findings (naming, dead code, wording nits without a binding rule).
• Refactoring & Tech Debt (DRY) Analysis entries that propose changes beyond the literal bug fix.
• Refactoring Proposals drafted for separate issues.

Critical and Moderate findings, the Strict rule compliance walk-through, the Coverage gate, the Database Analysis section, and every Specialized Review that the diff triggers stay mandatory and blocking — fast-track never skips them.

1. Record the fast-track decision in the review output: the matched signal (label name or JIRA priority value), the deferred sections, and a one-line reminder that the gate remained Critical + Moderate.

Named Arguments Review

• Would positional arguments be ambiguous?
• Are there boolean, null, array, or repeated scalar values?
• Would a DTO or value obje

…

Source & license

This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code.

• Author: pekral
• Source: pekral/cursor-rules
• License: MIT
• Homepage: https://pekral.cz

Install and usage instructions live in the source repository linked above.