magento2-code-review · diff

git:20261005.6d59fc7 to git:20261027.b2b8f0b

30 added, 22 removed. Audit A to A.

---

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

+ 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

+ **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.

+ 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

+ 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" | GitHub MCP (`pull_request_read`, preferred for GitHub) / `glab mr diff` / `gh pr diff` — **text-pattern checks only**, see below |

+ | Remote fetch | "review MR !123 before I check it out" | GitHub MCP (`pull_request_read`, preferred for GitHub) / `glab mr diff` / `gh pr diff`, **text-pattern checks only**, see below |

> **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

+ 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

+ "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

+ 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

+ 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

+ 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

+ 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

+ 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

+ 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

+ 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

+ `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

+ actually listed below it, recount by hand if they don't match, the same

discipline `magento2-performance-audit` already requires of its own

report.

+ ## Findings report

+ Each trio skill (and the performance audit, when it ran) leaves a `findings.json` in the shared schema (`magento2-dev-core/references/findings-schema.md`). Merge them, then render the merged file:

+ ```bash

+ node <skills>/magento2-code-review/scripts/merge-findings.mjs --out findings.json linter/findings.json security/findings.json perf/findings.json

+ node <skills>/magento2-dev-core/scripts/report/render.mjs render --layout findings --audience dev --data findings.json --out <report-dir>

+ ```

+ `merge-findings.mjs` keeps one finding per code (or rule), file and line, at the highest severity any input gave it, and recomputes the summary, so the counts in the report always match the findings. Dev-core anti-pattern findings found by hand go into their own `findings.json` with `tool: "magento2-dev-core"` before the merge.

## Report template

```markdown

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

+ - **[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

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

```