Magento2 Code Review

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

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".
5. If the file list touches di.xml (including a <preference> addition), events.xml, a Plugin/ class, or an Observer/ class, run the conflict check in references/plugin-observer-conflict-check.md — it now also covers preference conflicts (M2-ARCH-008), not just plugin/observer ones.
6. Merge every finding into the report template below, using magento2-dev-core/references/severity-and-codes.md — map to an existing code before minting a new one.
7. Self-verification gate (mandatory, before presenting the report): the Summary table's per-severity counts must equal the number of findings actually listed below it — recount by hand if they don't match, the same discipline magento2-performance-audit already requires of its own report.

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.]