pekral/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/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.). Switching to the relevant branch and git pull to read the latest diff are allowed; 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

• Before starting, ensure you are on the branch that contains the changes to review. If not, switch to it.
• Identify changes vs main branch.
• Deduplicate previous findings.

Cross-run history

The CR wrappers publish the review through a single-comment upsert keyed by the current actor identity (see @skills/code-review-github/SKILL.md and @skills/code-review-jira/SKILL.md). Follow-up runs edit that one comment in place, so the per-run audit trail lives in the 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 upsert 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 <KEY|URL> 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).
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
3. Use this context to evaluate whether the implementation fully satisfies the issue — not just whether the code is technically correct.
4. 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.

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
4. 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.
5. 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.
6. 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
• Reuse of existing logic — for every block of newly added or modified logic in the diff, search the codebase for an existing implementation that already does the same thing (helper, Service, Action, Data Builder, DTO, trait, ModelManager, Repository, scope, etc.). If equivalent logic already exists, flag the change and require reusing it instead of introducing a parallel implementation. The goal is unified logic across the application; parallel implementations of the same behavior are a finding (see @rules/code-review/general.mdc Reuse Existing Logic section).
• Speculative interfaces — flag any new project-owned interface that has neither at least two non-test consumers nor at least two non-test implementations. Test doubles, mocks, and fakes do not count toward either threshold. Implementing a framework or vendor interface (e.g. ShouldQueue, HasLabel, Arrayable) is always allowed. Require collapsing single-implementation, single-consumer interfaces back into the concrete class unless the PR documents an architectural reason — a published package API surface or a plugin extension point with a written contract — see @rules/php/core-standards.mdc Design Principles.
• Simplicity First — flag any added or modified code on the diff that introduces complexity beyond what the issue actually asked for. Concrete patterns: new abstractions (helper / Service / wrapper / facade / trait / config flag) for code with a single call site or single use case; unrequested "flexibility" / configurability / extension points (mode switches, strategy hooks, optional knobs) not justified by an existing caller or the linked issue; error handling for impossible scenarios (catching exceptions the call surface cannot throw, defensive if guards on internal values the caller already validates, fallbacks for unreachable branches); oversized method or class bodies where the same behavior fits in roughly a quarter of the lines without losing clarity ("if you write 200 lines and it could be 50, rewrite it"). The senior-engineer heuristic applies: when a reader would call the changed block overcomplicated, that is the finding. Severity: Moderate by default; escalate to Critical when the speculative abstraction creates a new business-logic home outside the seven allowed Laravel layers (see Architecture conformance). Pre-existing complexity the diff does not touch is out of scope — only added or modified lines are flagged. The YAGNI rules for speculative method parameters and speculative project-owned interfaces are already covered by @rules/php/core-standards.mdc Design Principles and the Speculative interfaces bullet above — do not duplicate findings; this bullet covers the remaining surfaces.
• Method parameter count (>4 → DTO required, strict): flag every method, function, closure, constructor, __invoke(), or other callable whose signature declares more than 4 parameters on any line added or modified by the diff, per @rules/php/core-standards.mdc Structure section (parameter counting rules, exemption list, and required fix are defined there). Severity: Critical (structural / required-pattern violation; the Strict rule compliance default applies and may not be silently downgraded). Existing methods that the diff does not touch are out of scope; a pre-existing method becomes in scope the moment the diff adds, removes, renames, or re-types any parameter of that method.
• Strict rule compliance (mandatory walk-through) — for every rule file applied via Constraints (@rules/php/core-standards.mdc, @rules/code-review/general.mdc, @rules/refactoring/general.mdc, @rules/code-testing/general.mdc, and on Laravel projects @rules/laravel/laravel.mdc, @rules/laravel/architecture.mdc, @rules/laravel/filament.mdc, @rules/laravel/livewire.mdc, plus any project-specific @rules/**/*.mdc), scan the diff for any pattern that matches a numbered or bulleted rule from those files and raise one finding per matched violation. The standard is "every applicable rule must hold on every changed line", but the review process is pattern-matching the diff against the rule set, not asserting each rule against each line individually. Each finding cites file:line and the rule reference (e.g. @rules/php/core-standards.mdc#PHP Practices or @rules/laravel/architecture.mdc#Business Logic Layers). Severity defaults — apply the severity declared in the rule file's CR Severity Rules section if present; otherwise: architectural / structural / required-pattern violations are Critical, PHP-practice violations a fixer doesn't catch (missing return types, raw arrays across boundaries when DTOs exist, magic numbers, unsuppressed errors, generic exceptions, untyped iterables) are Moderate, naming or wording nits without a binding rule are Minor. Do not summarize "all rules apply" as a single finding — each violation needs its own line so the reviewer can verify the citation.
• Architecture conformance (Laravel) — mandatory standalone walk-through (issue #530). This is a section-by-section deep-dive for @rules/laravel/architecture.mdc that is independent of Strict rule compliance and runs as a separate mandatory block on every Laravel project — detected by laravel/framework in composer.json require. Walk every section of that file against the current diff regardless of which files the diff touches (helpers, routes, configs, migrations, seeders, tests, or even a docs-only commit). The motivation is that architecture is also a verdict on where new code should have lived, not just on what the touched files do; a diff that ignores the business-logic layers entirely is itself an architecture signal worth raising. Walk every section: Architecture, Business Logic Layers (seven allowed homes including the Eloquent-model carve-out — verify each newly added or modified business-logic block sits in one of: Actions / Model Services / Repositories / ModelManagers / Data Validators / Data Builders / Eloquent models; flag every block landing in a controller, middleware, Blade view, Livewire component, job, listener, event handler, or helper with a concrete suggestion where it belongs), Actions (orchestration-only, single __invoke(), final readonly, constructor injection), Action Rules (no inline Eloquent, no DB::, no inline persistence — assessed on every Action file touched by the diff), Model Services (BaseModelService extension, no inline queries), Repositories and ModelManagers (read/write separation, basic queries only in Repositories, batch-first writes in ModelManagers — assessed on every Service / Repository / Manager file touched by the diff), DTOs (typed boundaries, mapping attributes, no raw arrays across layers), Data Modification (DRY), Data Builders (multi-method, no DB), Shared Concerns (Traits) (globally shared, domain-agnostic, reusable-as-is logic only — flag domain-specific code parked under app/Concerns/ and reusable trait logic scattered outside app/Concerns/), Validation Rules (Traits), Data Validators (DataValidator trait when pekral/arch-app-services is installed), Controllers and Other Entry Points (slim, every request-consuming controller method — resource action, single-action __invoke(), or custom method — must type-hint a project-owned FormRequest subclass and never Illuminate\Http\Request / untyped / a generic Request with $request->* reads; one dedicated FormRequest per request shape; no rules() branching on $this->method(); no inline validate() / Validator::make()), Resource Controllers (CRUD-only), Single-Action Controllers, Livewire (entry point, boot() injection, no business logic), Custom Helpers (global app/helpers.php functions, no static-method wrappers), and the CR Severity Rules subsection. Inherit the severity declared in CR Severity Rules; absent that, default to Critical for any orchestration / persistence / query bypass and any new code outside the seven allowed business-logic layers. arch-app-services examples (when installed): if vendor/pekral/arch-app-services exists or composer.json requires pekral/arch-app-services, additionally cross-check the diff against the package's published README.md examples at https://github.com/pekral/arch-app-services/blob/master/README.md — especially the worked examples — so the walk matches the canonical Action / Service / Repository / ModelManager / Data Validator shapes shipped with the package. When the package is not installed, ignore this README cross-check (do not adopt the package and do not invent examples). Report contract: the architecture walk runs on every Laravel CR run, but the published CR comment carries a ## Architecture section only when the walk produces at least one finding. When findings exist, list them with file:line, the cited subsection of @rules/laravel/architecture.mdc, and the standard reproducer fields (Faulty Example / Expected Behavior / Test Hint / Suggested Fix). When the walk produces zero findings, omit the ## Architecture heading entirely — never render a "walked, 0 findings" status line, a "clean" placeholder, or any other confirmation that the walk ran; the absence of the section is the clean signal. The walk itself is still mandatory; only the user-visible section is conditional. On non-Laravel projects (no laravel/framework in composer.json require), skip the walk entirely and omit the ## Architecture section from the CR comment — the section is Laravel-only by design (out of scope per issue #530: no framework-agnostic architecture rule).
• Test organization (issue #528) — for every test file added or moved by the diff (any new *.php under the project's test root, or any rename that changes the test file's path or base name), verify three things per @rules/code-testing/general.mdc Test Organization:
  1. Placement mirrors the SUT namespace. Resolve the production class the test targets (Pest uses(...), the first new <Class>(...) / <Class>:: reference, or the file base name without the Test suffix) and confirm the test file sits under the parallel directory tree (src/Service/Billing/InvoiceCalculator.php → tests/Service/Billing/InvoiceCalculatorTest.php). Cross-cutting tests that intentionally target no single production class sit under an intent-named directory (tests/Feature/<flow>, tests/Contract/<vendor>, tests/Integration/<area>). Tests parked next to an unrelated class to satisfy a parallel path are a finding.
  2. File name matches the SUT. Primary test files use {ClassName}Test.php; extracted scenario files use {ClassName}{Scenario}Test.php and stay in the same directory. Generic file names (MiscTest.php, TestsTest.php) and missing Test suffix are findings.
  3. it() / test() description matches the asserted scenario. For every it('…') / test('…') block introduced or modified by the diff, read the assertions in the body and confirm the description states that scenario in plain language. Findings: generic placeholders (it('it works'), test('test1'), test('happy path')), method-named descriptions (test('calculate'), it('handles getUser')), descriptions that contradict the assertions (it('adds user') over a body asserting removal), and stale descriptions left unchanged after the assertions were rewritten. Severity: Moderate by default (organization / readability gate; a reviewer must be able to navigate the test tree by walking the parallel folder). Escalate to Critical when the misplacement hides regressions because the test never runs under the test runner's discovery glob, or when the description so misrepresents the assertions that future maintainers would change the wrong test. Suggested Fix must use one of these literal templates so @skills/process-code-review/SKILL.md extracts it deterministically:
  • Placement / file name fix — Move \<current/path/FooTest.php>` to `<tests/<MirroredNamespace>/FooTest.php>` (rename base name to `<FooTest.php>` if the file name itself is wrong).`
  • Description fix — In \<path/FooTest.php>`, replace `it('<current description>')` with `it('<plain-language scenario the body asserts>')`. Never emit a vague "rename it". When the discovery fallback chain (Pestuses(...)→ firstnew <Class>(...)/<Class>::reference → file base name without theTest suffix) cannot resolve a single SUT — typical for legacy single-file test suites covering many classes — degrade to checking that the file sits under an intent-named directory (tests/Feature/<flow>, tests/Contract/<vendor>, tests/Integration/<area>`, or the project test root for an installer-style cross-cutting suite) and skip the per-SUT path check rather than flagging the pre-existing layout as a regression.
• Per-row DB operations in loops — flag any loop that issues per-row update(), create(), delete(), or single-row read; require batching via ModelManager batchUpdate / batchInsert, whereIn(...)->delete(), or a single bulk read keyed in memory (see @rules/sql/optimalize.mdc "Batch over per-row operations"). Allowed only when an explicit code comment or PR note justifies an unavoidable per-row side-effect dependency.
• SQL query reuse of existing indexes — for every new or modified SQL / Eloquent / query-builder code in the diff, locate the current DB schema (migrations under database/migrations/, model $table metadata, and live SHOW INDEX output when DB access is available) and verify the query is shaped to hit an existing index. Flag queries that bypass an existing covering index — WHERE / JOIN / ORDER BY column order does not match the left-to-right composite order, functions wrap indexed columns (non-SARGable), or SELECT pulls columns outside the covering index. The Suggested Fix must rewrite the query (column re-ordering, SARGable rewrite, covering projection) — propose a new index only when no existing index can cover the query and the schema gap is justified by EXPLAIN. Severity: Moderate, escalated to Critical when the un-indexed query runs on a hot path or large table (see @rules/sql/optimalize.mdc "Reuse existing indexes first").
• Third-party API/service contract — when changes touch external APIs or services, verify the implementation matches the public API documentation, satisfies the issue assignment, and covers all relevant in-scope API use cases (see Third-Party API & Service Analysis section)
• Refactoring quality — when changes are refactoring in nature, validate them against @rules/refactoring/general.mdc: behavior must be preserved, migration must be incremental (no big-bang rewrites), and entry points / responsibilities / DRY / concurrency must follow the recommended process. In Laravel projects, combine with @rules/laravel/architecture.mdc.
• Refactoring test-coverage contract (issue #493) — when the diff is refactoring, enforce @rules/refactoring/general.mdc Test Coverage Contract:
  • Walk the PR commit history and verify the refactor commit is preceded by a dedicated test commit that brings the refactored lines to 100% coverage (commit message in the test(scope): cover <area> before refactor form per @rules/git/general.mdc). Missing pre-refactor coverage commit → Critical finding.
  • Verify the refactor commit modifies no pre-existing test file (allowed exemption: mechanical renames forced by the refactor itself, but the commit body must document them). Any other test edit inside the refactor commit invalidates the behavior-preservation proof → Critical finding.
  • Verify the coverage of the refactor commit alone using the project's available coverage tooling (see the Coverage gate below) — every changed line, branch, and condition must report 100% coverage. Sub-100% coverage on the refactored lines → Critical finding.
• Data validation encapsulation — verify that all validation logic is in dedicated Data Validator classes or FormRequests (using validation rules from reusable traits in app/Concerns/), not inline in Actions, controllers, jobs, commands, listeners, or Livewire components (see @rules/laravel/architecture.mdc Data Validators section)
• Repository scope — verify Repositories expose only basic, reusable queries (find, findBy{Attribute}, all, simple where lookups, pagination of a base scope). Feature-specific or use-case–specific query methods in Repositories are a finding; specialization belongs in a Service (single-model) or Action (cross-model / cross-feature) composing basic Repository methods (see @rules/laravel/architecture.mdc Repositories and ModelManagers section)
• Data Modification (DRY) — enumerate every place in the changes that modifies data before it is saved or passed downstream (DTO mapping, payload shaping, key renaming, default fallbacks, format normalization, business-driven derivation). Cross-check against existing entry points; if the same shaping appears in more than one Action / Service / controller / job / listener / Livewire component / command, flag it and require consolidation into the canonical layer (Data Builder, DTO named constructor, Data Validator, ModelManager, Repository — see @rules/laravel/architecture.mdc Data Modification (DRY) section). Output the list of modification places explicitly in the review so the duplication picture is visible.

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.
2. 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.
3. 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.
4. 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.
5. 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 object be a better design?
• Is this a public API where parameter names must remain stable?
• Are arguments still listed in the original method signature order?

Specialized Reviews

• Always run:

  • @skills/prepare-issue-context/SKILL.md with MODE=cr as a pre-flight before any other specialized review — audits whether the dev database currently holds the fixture data the assignment scenarios need, and surfaces every scenario the diff should cover but the codebase / dev DB cannot reproduce. The CR uses its gap report to ground the rest of the review in real data; an empty gap report means later findings (assignment compliance, security, refactoring) are made against scenarios that genuinely exist, not against guesses. Surface the gap count in the PR comment summary line and treat any behavioral gap the skill reports as a Critical CR finding (the diff cannot satisfy a scenario the codebase does not support).
  • @skills/assignment-compliance-check/SKILL.md — verifies the PR implementation satisfies the business requirements stated in the linked issue / task. The skill does not publish anywhere itself — it returns either the assembled ## Assignment Compliance markdown block (only when at least one Critical gap exists), the status no critical gaps — assignment compliance block omitted (when the implementation satisfies every stated requirement), or the status no linked issue — assignment compliance skipped (when no linked tracker exists). The CR wrapper passes the returned block as an embedded block to @skills/pr-summary/SKILL.md only when a block is returned so each linked tracker (GitHub issue or JIRA ticket) receives one consolidated comment per CR run (per issue #498); on either skip status the wrapper embeds nothing and surfaces the status on the PR comment summary line. Do not embed the block into the PR comment — that comment carries technical findings only.
  • @skills/security-review/SKILL.md
  • @skills/class-refactoring/SKILL.md with MODE=cr — read-only refactoring lens scoped to the PR diff. Use it to surface concrete tech-debt-reducing changes (DRY duplication, single-responsibility breaches, oversized methods) that apply to lines actually touched by the PR. MODE=cr guarantees the lens never modifies code, writes tests, commits, runs fixers, or chains another review — it returns proposals only. Do not propose changes that fall outside the diff.

• Run conditionally:

  • Diff is a refactoring (behavior-preserving structural change per @rules/refactoring/general.mdc) → run the full refactoring skill set read-only. Detect this when the PR restructures existing code without adding a feature or changing observable behavior (extracted methods / classes, moved orchestration, renamed-for-clarity, dedup, layer reshuffles). When it fires, additionally invoke @skills/refactor-entry-point-to-action/SKILL.md with MODE=cr (and run the class-refactoring lens above at full depth) to surface the entry-point → Action proposals. Both skills run read-only — no code changes, no commits, no fixers, no review chaining — MODE=cr enforces this. Fold their output into the Refactoring (DRY / tech debt) section (in-scope items) and Refactoring proposals section (out-of-scope items) of the review; this skill never modifies code.
  • Database operations detected in the diff → @skills/mysql-problem-solver/SKILL.md is mandatory. Trigger this skill whenever the diff touches any of: raw SQL strings, Eloquent / query-builder calls (DB::, ->where(, ->join(, ->whereHas(, ->withCount(, ->orderBy(, ->groupBy(, ->chunk(, ->cursor(, paginate(, simplePaginate(), Eloquent relationship definitions, with( / load( eager loads, model scopes, ModelManager / Repository methods, database migrations (Schema::, up() / down()), seeders, factories that materialise rows, DynamoDB / NoSQL access. Pass the diff scope to mysql-problem-solver and capture its findings — they must appear in the published CR review under the dedicated ## Database Analysis section described in Output Rules, never silently absorbed into the generic Critical / Moderate / Minor buckets.
  • Shared state / concurrency → @skills/race-condition-review/SKILL.md
  • I/O or external calls → I/O review

Refactoring & Tech Debt (DRY) Analysis

Run this section over the PR diff only — never over untouched code.

1. List every block of changed lines (added or modified) per file.
2. For each block, evaluate against @skills/class-refactoring/SKILL.md (run with MODE=cr — read-only):
   • duplicated logic that already exists elsewhere in the codebase — diff-scoped pass of Core Analysis "Reuse of existing logic" (DRY); do not restate the rule, raise the finding once
   • data-shaping repeated across Actions/Services/controllers/jobs/listeners/Livewire/commands
   • oversized methods, deep nesting, mixed responsibilities introduced or amplified by the change
   • per-row DB operations in loops (link to the Core Analysis rule above)
   • Livewire / Blade layout splitting (Laravel projects). Walk every *.blade.php file added or modified by the diff against the triggers in @rules/laravel/livewire.mdc HTML / Blade Layout Splitting. Raise one Refactoring (DRY / tech debt) entry per match — repeated structural HTML block (same wrapper + same inner skeleton appearing 2+ times in the same view, or once in 2+ views), Blade view exceeding 150 lines of actual markup with no splitting attempt and no documented exemption, a self-contained wire:model / wire:click / wire:submit interaction cluster left inline in the parent component, a @foreach body longer than ~10 lines of per-iteration markup, a region owning its own wire:loading / @empty / error state inline, or a clearly named UI concern (a noun phrase a designer would use) still inline. Each entry cites file:line, the matched trigger, the proposed concern name, the Component-type decision (Livewire iff state / lifecycle / server interaction; Blade otherwise — never a Livewire wrapper around a stateless presentational block), and the target folder (app/Livewire/<Domain>/ + resources/views/livewire/<domain>/ or resources/views/components/<domain>/). Severity is the refactoring-section default — these are tech-debt proposals, not Critical / Moderate blockers. The Critical / Moderate / Minor entries from the rule's CR Severity Rules (HTML Layout Splitting) subsection are picked up separately by the Strict rule compliance walk-through in Core Analysis and surface there at their declared severity, so this DRY entry does not duplicate them.
   • Conditional query composition via ->when() (Laravel projects). Flag every block on the diff that gates a query-builder / Eloquent clause on a nullable, optional, or boolean filter through an imperative if and then mutates $query inside the body — including but not limited to: if ($filters->x !== null) { $query->where*(...); }, if (! empty($filters->x)) { $query->where*(...); }, if (isset($filters->x)) { $query->where*(...); }, if ($filters->flag) { $query->orderBy(...); }, if ($search !== '') { $query->where('name', 'like', ...); }. The canonical Laravel form is $query->when($filters->x, fn ($q, $value) => $q->whereIn('campaign_type', $value)) — when() takes the filter value as the truth test, passes it as the second closure argument, and short-circuits when the value is falsy / null / empty. Raise one entry per matched if block with file:line, a one-sentence statement of the imperative conditional being rewritten, and a Suggested Fix that rewrites the if as a ->when(...) call while preserving the original predicate semantics byte-for-byte. Laravel's when() short-circuits on every falsy value (null, false, 0, '0', '', []); whenever the original if predicate would still fire for one of those values, the Suggested Fix must use the explicit truth-test form so behavior does not silently change. Concrete templates the reviewer must pick from:
     • Original if ($filters->x !== null) over a value the caller may legitimately set to 0 / '0' / false / '' / [] (collection / array filters, "clear all chips" → [], boolean false, numeric 0): rewrite as ->when($filters->x !== null, fn ($q) => $q->whereIn('campaign_type', $filters->x)). The truth test is the literal !== null and the closure references $filters->x directly — do not drop to ->when($filters->x, …), which would skip the clause for [] / 0 / false and change the result set (e.g. whereIn('campaign_type', []) produces 0 = 1 / empty result, while a skipped when() returns the unfiltered base set).
     • Original if ($filters->x) (already truthy-gated — caller intentionally skips falsy values) or if (! empty($filters->x)): the short form ->when($filters->x, fn ($q, $value) => $q->whereIn('campaign_type', $value)) preserves semantics and the closure receives the value as $value.
     • Original if (isset($filters->x)): rewrite as ->when($filters->x !== null, …) — isset rejects only null, matching !== null and not the broader falsy set when() short-circuits on. Do not raise the entry when the body does more than one query mutation that genuinely cannot be expressed as a single closure (multi-statement orchestration, branching on the value, side effects outside $query); in that case the imperative if stays. Severity is the refactoring-section default — these are tech-debt proposals listed under Refactoring (DRY / tech debt), not Critical / Moderate blockers.
   • when the PR is itself a refactoring (see the conditional trigger under Specialized Reviews), also fold in the entry-point → Action proposals from @skills/refactor-entry-point-to-action/SKILL.md run with MODE=cr
3. Output the result in the Refactoring (DRY / Tech Debt Reduction) section of the review template:
   • file:line of the offending change
   • the duplicated/structural problem in one sentence
   • a concrete refactoring that reduces tech debt (consolidate into Data Builder / DTO / Service / Action / Repository per @rules/laravel/architecture.mdc)
4. Refactoring proposals here are improvements that fit inside the PR scope. Out-of-scope structural problems still belong in Refactoring Proposals (separate issue draft).

Validation

• Verify acceptance criteria
• Coverage gate (mandatory, scoped to changed files only): every line, branch, and condition added or modified in the current PR diff must be covered by tests. Verify the coverage of the changed files only — do not gate code review on a project-wide coverage percentage.
  • Use the coverage tooling the project already provides. Discover the available coverage command (a Phing coverage target, a Composer test:coverage / coverage script, or a direct vendor/bin/pest --coverage-clover=<file> / PHPUnit --coverage-clover invocation) and run it to produce a coverage report. Do not assume a default and do not add a new bespoke coverage script to the consuming project.
  • Restrict the assessment to the changed source files. When the runner can limit the report to specific paths (--coverage-clover plus a path filter, PCOV pcov.directory scoped to the changed directories), narrow it so the run stays fast; otherwise generate the report and read off only the changed files.
  • Map the report to the diff and list any uncovered added/changed lines as Critical findings.
  • Delete the generated coverage report file (the --coverage-clover output or any other auto-generated coverage artifact) as soon as it has been read, so it is never accidentally committed. Keep such artifacts in the project's .gitignore as a second line of defence.
  • If the project ships no coverage tooling at all, or it cannot be executed (missing driver, missing test binary, no source directory), raise that as a Critical finding instead of skipping the check.
  • Coverage reporting is short by default. Run the gate on every review, but render the ## Coverage section on the published comment only when there is something the reader must act on — uncovered changed lines (Critical findings) or unavailable / non-runnable coverage tooling (Critical finding with the reason). When every changed line is at 100% coverage and the tool ran successfully, omit the ## Coverage section entirely, omit the Coverage: header line, and omit the coverage … slot from the final summary line. The Counts line carries the clean signal; the omission is the report. Never emit 100% / clean / n/a placeholders for the section, the header line, or the summary slot.
• Identify missing test scenarios beyond raw coverage (edge cases, error paths, regressions).

Critical Findings Verification (issue #537)

Run this step after every preceding analysis step has produced its findings (Core Analysis, Highest-Priority Fast Track, Named Arguments Review, Specialized Reviews, Refactoring & Tech Debt, Validation incl. the Coverage gate) and before the Output assembly. Walk every Critical finding aggregated within this skill's run through @skills/analyze-problem/SKILL.md to confirm the finding reflects a real problem in the diff before it blocks the PR.

1. For each Critical finding, invoke @skills/analyze-problem/SKILL.md inline in this skill's context (do not dispatch as a subagent) and pass:
   • the finding's file:line, risk/impact line, Faulty Example, Expected Behavior, Test Hint, and Suggested Fix
   • the surrounding code context — at minimum the enclosing method / class, plus any helper / Service / Repository the Suggested Fix points at — so the analysis can separate facts from assumptions
   • the issue context already loaded under Issue Context Analysis (requirements, acceptance criteria, test data, edge cases)
2. Read the returned analysis (template at @skills/analyze-problem/templates/analysis-report.md):
   • Verified Facts must support the finding's claim
   • Probable Root Cause must align with the finding's risk / impact
3. Outcome — binary, no middle ground:
   • Confirmed — Verified Facts and Probable Root Cause back the finding → keep the Critical finding verbatim in the report (reproducer fields, severity, and Suggested Fix unchanged)
   • Refuted — Verified Facts contradict the finding, or Probable Root Cause names a different cause whose mitigation already exists in the diff → drop the finding from the report entirely
4. Never silently downgrade a Critical to Moderate or Minor on the basis of this verification — the gate is binary (kept or dropped). A finding that survives stays Critical; a finding that is refuted is removed. Use the regular Strict-rule-compliance stratification (not this verification) for any genuine severity reassessment.
5. Moderate and Minor findings are not subject to this verification — they pass through to Output assembly unchanged. The verification exists to eliminate false-positive Critical blockers, not to second-guess lower-severity items.
6. The verification runs on every CR run; the published comment carries only verified Critical findings, so reviewers never spend time on a Critical that the analysis itself could not stand up.

---

Output Rules

• Output only findings
• No praise, no summaries of what was checked
• Omit empty sections entirely. Only the header block (Status / Counts / Last updated / tracker-status line) and the final Summary line are always rendered. The Coverage: header line, the ## Coverage section, and the coverage … slot in the summary line are all conditional — render them only when the coverage gate produced something to report (uncovered changed lines or unavailable / non-runnable tooling, both Critical findings). When every changed line is at 100% coverage and the tool ran successfully, drop all three coverage surfaces; the Counts line is the clean signal. Every other section — Findings (including each severity sub-heading), Refactoring (DRY / tech debt), Refactoring proposals, Database Analysis, and any specialized-review sub-section — appears only when it has at least one item. Never emit None. / Not applicable. / n/a / 100% placeholders for empty sections or omitted coverage surfaces; drop the whole heading and body instead. History across CR runs is preserved by the tracker's edit history on the upserted comment — never re-create a Previous CR Status section in the body.
• ## Architecture section (issue #530). On Laravel projects the architecture walk-through described in Core Analysis runs on every CR run, but the ## Architecture heading is rendered only when the walk produces at least one finding. When findings exist, render them under the heading with the standard Critical / Moderate / Minor severity grouping and the reproducer fields. When the walk produces zero findings, omit the heading entirely — never render a walked, 0 findings status line, a clean placeholder, or any other "the check ran" confirmation. The principle is the same as for every other section: report only items that still need action; an empty section is dropped. On non-Laravel projects (laravel/framework not in composer.json require), the ## Architecture section is omitted entirely — the section is Laravel-only by design.
• Use severity levels:
  • Critical
  • Moderate
  • Minor
• Group findings by severity
• Each finding must include:
  • location
  • risk/impact
  • concrete fix
• Each Critical and Moderate finding must additionally include:
  • Faulty Example — minimal code snippet or input payload that reproduces the issue (redact secrets/PII)
  • Expected Behavior — single assertable statement (return value, exception, persisted state, emitted event)
  • Test Hint — one sentence pointing at the test layer (unit, integration, feature) and entry point
  • Suggested Fix — minimal corrected code snippet that resolves the finding. Must comply with @rules/php/core-standards.mdc and, for Laravel projects, @rules/laravel/architecture.mdc. Use n/a — <reason> only when a snippet adds no value over the one-line Fix description (e.g. naming-only changes, dead-code removal, pointers to an existing helper whose name already says enough).
• These four fields exist so @skills/process-code-review/SKILL.md can convert each finding into a reproducer test and apply the fix without re-deriving context.
• Minor findings may omit these fields when no behavior change is implied (naming, dead code, etc.).
• This skill is read-only and does not publish anywhere itself. The wrapper skills that consume its output (@skills/code-review-github/SKILL.md, @skills/code-review-jira/SKILL.md) must delegate the single consolidated comment on every linked issue in the originating tracker (GitHub issue, JIRA ticket, or both) to @skills/pr-summary/SKILL.md — the CR wrappers never author their own non-technical template. pr-summary produces a uniform "Summary of changes + How to test" comment understandable by non-technical project managers, rendered as GitHub Markdown for GitHub issues and JIRA Wiki Markup for JIRA tickets per @rules/jira/general.mdc. When @skills/assignment-compliance-check/SKILL.md returns a markdown block (i.e. at least one Critical gap was detected), the CR wrapper passes it as an embedded block to pr-summary, which appends it after How to test so the linked-tracker audience reads exactly one comment per CR run (per issue #498). When the compliance check returns a skip status (clean or no linked tracker), the wrapper embeds nothing and the consolidated comment carries only the change summary — clean compliance is reported by the absence of the block. Technical findings still go directly on the PR comment.
• Safe validation & error texts (issue #540): for every user-facing string the diff adds or modifies — FormRequest::messages() / attributes(), custom validation messages on rules, exception messages surfaced to end users (abort(), throw new RuntimeException(...) reaching a JSON / Inertia / Blade response), Notification subject and body, Mailable bodies, flash messages, API error envelopes, Filament notifications, __() / trans() / Lang::get() / @lang / t() / i18next.t() calls in every locale shipped by the project (every key under lang/ / resources/lang/ / translations/ / *.json locale files), and every literal string rendered into Blade / Livewire / Filament / Vue / React templates — walk each string against @rules/security/backend.md Safe Validation & Error Messages and (for frontend / mobile surfaces) @rules/security/frontend.md / @rules/security/mobile.md Safe Validation & Error Messages, and raise one finding per match against any of these patterns:
  • Identity / account enumeration on auth, password-reset, sign-up, change-email, or account-lookup flows — distinct wording for email not found vs wrong password vs account locked vs email not verified vs 2FA required vs user disabled, or a different response shape / status / latency between those branches.
  • Authorization granularity leak — "You do not have permission to access invoice #42", "Project not found in your workspace", or any wording that confirms a resource exists to a caller who is not authorized to read it (the generic 404 Not Found envelope must cover both missing and forbidden).
  • Internal implementation detail in the response body — stack traces, fully-qualified class names, file paths, framework / library versions, database table or column or index names, raw SQL fragments, ORM error envelopes, queue / cache driver identifiers, feature-flag keys.
  • Verbatim echo of attacker input — string interpolation of the rejected value ("Email '$email' is invalid") into the validation message or error body; the rule is to reference the field by name only.
  • Password / token policy leak beyond the stated rule — proximity hints ("one character short", "almost matches the breach list"), missing-character-class enumeration when several classes are missing, or password-fragment echo.
  • Translation drift — the default-locale key carries the safe wording but a translated locale reintroduces an enumeration / authorization / introspection leak (e.g. en.json says "Invalid credentials." but cs.json says "Účet neexistuje."). Walk every locale shipped by the project, not just the default. This is independent of the Translation completeness check — the wording finding fires even when the key is present in every locale. Severity: Critical when the unsafe wording sits on an auth / password-reset / sign-up / authorization surface (the leak is directly exploitable for enumeration); Moderate on every other user-facing surface. The Suggested Fix rewrites the string to a generic, non-enumerating equivalent — "Invalid credentials.", "If the account exists, we sent the reset link.", "The email address is invalid." — routed through the project's translation function in every shipped locale (any locale the agent cannot translate confidently keeps the TODO(<locale>): translate "<source>" placeholder used by the Translation completeness check above). Specific, non-enumerating validation messages — "The age must be at least 18.", "Phone number is required.", "File must be a PDF under 5 MB." — are not findings; the gate fires on enumeration / authorization / introspection wording only.
• Translation completeness (mandatory when the project ships translations): detect whether the project uses translations by looking for any of: a lang/ directory (Laravel), a resources/lang/ directory (older Laravel layout), a translations/ directory (Symfony), *.json / *.po / *.mo locale files under a recognised locale directory, calls to __() / trans() / Lang::get() / @lang / t() / i18next.t() / useTranslation() / $t( / $tc( anywhere in the diff or in the wider project. If none of those signals match, skip this section entirely. When translations are in use, for every user-facing string introduced or modified by the diff (including labels in Blade / Livewire / Filament / Vue / React templates, validation messages, Notification subject and body, Mailable / Markdown views, FormRequest custom messages(), enum getLabel(), Filament resource / form field labels, log messages exposed to humans, exception messages surfaced to users) walk every detected locale and raise one finding per missing key — that is, the diff added a translation key in the default locale but did not add (or updated but did not synchronise) the same key in every other locale shipped with the project, or the diff hard-codes a literal string in a user-facing surface even though the surrounding code uses __() / t() for analogous strings. Severity: Moderate; escalate to Critical when the missing key is on a primary-flow surface (login, checkout, primary CRUD form, transactional email) or when the project's CI / build asserts translation parity and the diff would break that gate. Each finding cites the missing locale and key, and the Suggested Fix is the literal translation entry the locale file is missing (translated into the locale's language; for languages the agent cannot translate confidently, leave the value as TODO(<locale>): translate "<source>" and flag the gap explicitly so a human translator picks it up).
• Database Analysis section (mandatory when the diff touches DB operations): when the conditional @skills/mysql-problem-solver/SKILL.md trigger fires (see Specialized Reviews for the trigger pattern list), append a dedicated ## Database Analysis section to the published review before the ## Coverage section. The section reports only the findings — each with severity on the Critical / Moderate / Minor scale and the proposed query rewrite / index reuse / batching fix per @rules/sql/optimalize.mdc. Do not include the trigger decision, the inspected file:line list, or the EXPLAIN / static-analysis summary; those belong to the internal mysql-problem-solver investigation, not to the published review. When no DB operations are present in the diff, omit the section entirely; never replace it with a generic "no DB changes" placeholder absorbed into Coverage or the summary line.
• Default severity for rule violations: every unexcused violation of an Apply'd rule on a line touched by the diff defaults to the severity declared in that rule file's CR Severity Rules subsection if present. Otherwise apply the Strict rule compliance stratification from Core Analysis: architectural / structural / required-pattern violations are Critical; PHP-practice violations a fixer doesn't catch are Moderate; naming / wording nits without a binding rule are Minor. Reviewers may not silently downgrade further than that stratification; if a rule's spirit is satisfied by an alternative the diff documents in code or PR description, cite that exemption explicitly in the finding instead of suppressing it.

---

Output Format

Use the template defined in templates/review-output.md.

Principles

• Focus on risks, not style
• Prefer impact over quantity
• Avoid duplication of findings
• Prioritize regression detection
• Be precise and actionable

---

After Completion

• Do not auto-invoke @skills/test-like-human/SKILL.md. The user-perspective testing skill runs on demand only (via /test-like-human or an explicit follow-up); CR-track skills must never chain into it.