magento2-code-review

name	magento2-code-review
description	This skill should be used when the user asks to "review this PR/MR", "review this merge request", "review this module", "audit this module before merge", "review this theme", "audit this theme PR", or wants a "full review before release". Orchestrates a PR/MR, module, theme, or full-project code review by running the QA trio (magento2-linter, magento2-security-scan, magento2-performance-audit) and magento2-dev-core's anti-pattern checks at the right scope, then merges their findings into one report using a shared severity scale and stable finding codes. DEPENDENT on magento2-dev-core; invokes magento2-linter, magento2-security-scan, and magento2-performance-audit as needed for the chosen scope.
compatibility	claude, codex, opencode, copilot, dsh
depends	["magento2-dev-core"]
metadata	{"audience":"tech leads, reviewers","workflow":"magento","requires":["magento2-linter","magento2-security-scan","magento2-performance-audit"]}

Magento 2 Code Review

Orchestrates a review at one of four scopes — PR/MR, module, theme, or whole project — by calling the existing QA trio and magento2-dev-core at the right scope, then merging their output into one report with one severity scale. This skill does not reimplement any check the trio already owns.

Related Skills

REQUIRED BACKGROUND: Load magento2-dev-core first — its references/severity-and-codes.md defines the severity scale and finding codes this skill's report uses.

Invokes magento2-linter (style/static analysis), magento2-security-scan (vulnerability scanning), and magento2-performance-audit (runtime/ infrastructure) at whichever scope is in play — see "Scope modes" below. Theme scope additionally cross-references magento2-hyva-dev (CSP) and magento2-frontend-dev (Luma build/verify); it does not duplicate their content. Together with the trio, this forms the "QA quartet" — the four skills expected to run before a release.

Scope modes

Mode	Trigger phrase example	File list source
Full path	"review this module/project"	module/theme/app/code path, as today
Explicit file list	caller already knows which files	passed straight through
Local git diff	"review this PR", branch already checked out	git diff against a base ref
Remote fetch	"review MR !123 before I check it out"	On DSH: gitlab_get_mr_diff (+ gitlab_list_own_review_threads, gitlab_post_inline_comment) in one call. Elsewhere: GitHub MCP (pull_request_read, preferred for GitHub) / glab mr diff / gh pr diff — text-pattern checks only, see below

On DSH: call maestro_review_scope_split {diffStats:{files,addedLinesPerFile}, mode} → {split:{quick,deep},reason,estimatedSavingsTokens}. Run quick checks only on quick files, full deep only on deep; inject reason into prompt. Otherwise: apply same extension/path table by hand and note split in review header.

Full mechanics, exact commands, and the remote-fetch limitation: references/scope-modes.md.

Govard audit for PR/MR: prefer govard audit run --checks lint --mode project --scope diff --base auto --format json (auto-detects base via git merge-base HEAD origin/HEAD || gh pr view --json baseRefName); --provider is an alias for --lint-provider, and --allow-xdebug is required when stack.features.xdebug: true.

Workflow

1. Determine scope (ask if ambiguous — a bare "review this" with no target and no diff in the working tree is not enough to guess from).
2. Resolve the scope to a file list per references/scope-modes.md.
3. Run magento2-security-scan against that file list (see its own "Scoping" section for how it accepts a list vs. a path) — its text-pattern greps are genuinely file-scoped. magento2-linter's native gate is not: the file list does not flow into govard audit run — that command has no file-list or changed-lines argument, and its finest granularity is a whole module (module_in_project) or the whole project (project); govard audit diff --base <ref> records the base ref but still analyzes the full target (effective_scope: project). So at PR/MR scope, invoke magento2-linter's native run at whatever target mode it resolves to and pick the PHP matrix/provider per its "Govard-Native Lint Audit Is the Real Gate" section — don't restate that policy here — preserve the govard audit run session and run IDs it returns, and then split its findings into diff-introduced vs. pre-existing per references/scope-modes.md's "Local git diff" guidance: report a pre-existing violation in a touched file separately, and don't block the PR/MR on legacy debt the diff didn't create. The same file list still feeds magento2-linter's bare-tool fast pre-check (its own "Scoping" section) — that's a local sanity check, not a substitute for the native run's findings. For a repeat review of the same scope (e.g. re-checking after fixes), rerun the exact prior session (govard audit rerun --session <session-id>, per that skill's "Caching, rerun identity, and read-only source") before comparing findings — a fresh govard audit run starts an unrelated session, not a comparable one. magento2-security-scan's Authentication & Authorization, Data Exposure, and CSP Configuration checks are environment-level, not file-scoped — see its "Environment-level checks — scope boundary": they run once per audit at project/module/theme scope (never per file) and are skipped entirely at PR/MR scope (no live environment to query).
4. Run the performance/theme checks applicable to scope — PR/MR scope is not exempt, it always gets the static/file-scoped subset (never zero performance/theme coverage); project/module scope additionally gets the live/infra steps; theme scope runs the full routing in references/theme-audit-checks.md instead of the 9-step audit. Full split of which checks are static vs. live: references/scope-modes.md's "Performance/theme checks by scope".

Report template

## Code Review Report

**Scope**: [PR #123 / app/code/Vendor/Module / Vendor/theme / full project]
**Mode**: [full path / file list / local git diff / remote fetch]

### Summary

| Severity | Count |
|---|---|
| Critical | N |
| High | N |
| Medium | N |
| Low | N |

### Findings

- **[Code]** [severity] — `file:line` — [one-line description] — fix: [what to change]

### Coverage note

[If mode=remote fetch: state explicitly that PHPStan/PHPMD did not run —
text-pattern checks only. If scope is PR/MR, list which live/infra
performance/theme steps from `references/scope-modes.md`'s "Performance/
theme checks by scope" did not run and why, and confirm
`magento2-security-scan`'s environment-level checks (Authentication &
Authorization, Data Exposure, CSP Configuration) were skipped for the same
reason. If any trio member's step was skipped, say `Skipped: <reason>` here
rather than omitting it.]