review-impl

name	review-impl
description	Review an implementation in scope, such as an uncommitted diff, a just-finished agent change, a commit, or named files, for missing or wrong behavior in phase.rs. Use when Codex needs a findings-only architecture and correctness review across engine, parser, frontend, multiplayer, AI, deck, build, or release changes.

Review Implementation

Review for gaps: things that are missing or wrong. Do not spend findings on style nits, CI-enforced formatting, or a diff recap.

Workflow

1. Identify the changed surface from the diff, commit, or named files.
2. Classify the surface area: engine logic, parser, frontend/UI, multiplayer/transport, AI heuristics, deck/format/feeds, build/CI/release, or docs.
3. Apply only the relevant lenses below.
4. If the scope is a PR, fetch whatever external review comments exist (CodeRabbit, human reviewers) and confirm-or-refute each against the current head with code evidence, folding confirmed findings into your own. Assume none exist by default — Gemini Code Assist has been sunset, so no bot is guaranteed to have pre-screened this PR. Your own lenses are the complete review, not a supplement to a bot's; do not under-invest expecting a backstop. Where an external finding does exist, silently omitting it — or returning a verdict less severe than an open, unrefuted finding from another reviewer — is itself a defect. Review comments, checks, and uploaded/sticky artifacts count only when their evidence identifies the current PR head SHA; otherwise report the evidence as missing rather than attributing it to the current diff.
5. If the scope is a PR touching engine/parser source, the parse-diff sticky comment (marker <!-- coverage-parse-diff -->) is required evidence: fetch its full body and confront the card-level diff against the PR's claimed scope. Unexplained gained/lost/changed cards are findings (unintended parser blast radius). A Baseline pending body means the diff is unavailable — flag it so the handler brings the branch current to regenerate it; an absent comment despite changed engine source, or a comment/artifact not bound to the current PR head SHA, means CI evidence is missing for the current head. This PR-head requirement does not apply to Engine-Implementer Checkpoint Mode: review the committed BASE_SHA..CANDIDATE_SHA diff instead.
6. Report findings only. Silence means LGTM.

When pr-contribution-handler explicitly requests the manual quality gate, add Quality Gate: PASS|FAIL before findings. PASS requires all three current-PR facts: (1) claimed parse-impact count equals the measured parse-diff count and the normalized card sets are identical, using the full artifact when the sticky comment truncates examples; (2) the change is at an existing authority/right seam and reuses its vocabulary; and (3) a production-pipeline test is demonstrated to fail when the production change is reverted. On PASS, return the applicable existing praise tokens (right-seam, scope-discipline, discriminating-runtime-test, parameterized-not-proliferated) for the ordinary review/enqueue event. Never infer quality from Tier or standing and never create a quality_recommended event.

Skip checks CI already enforces:

• scripts/check-parser-combinators.sh
• cargo fmt --check and cargo clippy --all-targets -- -D warnings
• scripts/coverage-regression-check.sh --fail-on-engine
• TypeScript pnpm type-check and pnpm lint

Engine-Implementer Checkpoint Mode

Default review output is findings-only. Exception: when /engine-implementer invokes this skill against a checkpointed candidate, it supplies BASE_SHA, CANDIDATE_SHA, the in-scope paths, the named START_SHA, what the completion checks were and how they came out, and the existing maintainer-simulation matrix. Confirm the first round has START_SHA == BASE_SHA and each fix round starts from the prior reviewed candidate.

In this mode, emit these lines before findings:

Review Head: <CANDIDATE_SHA>
Completion Gate: PASS|FAIL
Maintainer-Simulation Gate: PASS|FAIL

Review Head must be the supplied CANDIDATE_SHA, and the reviewed diff must reproduce from exactly BASE_SHA..CANDIDATE_SHA; otherwise report a blocking finding.

Completion Gate passes when every check the changed surface calls for was run against the committed candidate and passed: formatting for implementation changes, the Rust/engine/parser block for Rust paths, the frontend block for frontend paths, the parser gate for parser paths. Check the set against the candidate diff rather than against the plan, and re-run anything you doubt — you have the candidate SHA and a shell. A check run against uncommitted edits, or against a different tree, does not count. Markdown-only policy work needs scope and diff checks only.

Phase Mode (chartered runs)

Activated when the spawn inputs include a phase charter and one phase's deferral allowlist — supplied by /engine-implementer for per-phase reviews (composing with checkpoint mode above) or by /implement-task for per-step reviews. The allowlist scopes this file's lenses and gates as stated here — this section, not the spawn prompt, is the authority:

• Deferred ≠ gap; foreclosure = gap. An item on the phase's deferral list is chartered to a named later phase: do not report it as missing, dropped, or incomplete. DO report any code that forecloses or contradicts a deferred item.
• Lens scoping: the new-field-threading, sibling-coverage, claim-to-test, and serialized-surface lenses evaluate consumers against the charter's phase attribution — a consumer attributed to a later phase is deferred, not silently dropped. The class-vs-single-case lens is satisfied from the charter's class attribution — an infrastructure phase's class coverage lives in the charter and lands with its consumer phase.
• Maintainer-Simulation Gate carve-out: a matrix row whose consuming function or hostile fixture is deferral-listed to a named later phase is recorded DEFERRED(phase n) and does not FAIL the gate; a row the phase's own code forecloses still FAILs. "FAIL when any row is missing" is otherwise unchanged.
• Checkpoint mode's gates are untouched — phase mode scopes what counts as a finding, never what counts as a valid check. The supplied BASE_SHA is the phase base, and the reviewed span is that phase's BASE_SHA..CANDIDATE_SHA.

Integration Mode (chartered runs, final review)

Activated when the spawn inputs include a phase charter and the run-span diff with no single-phase allowlist. Scope is exactly:

1. Cross-phase seams — files touched by two or more phases, deferral handoff points, and the surfaces the charter's seam notes name.
2. Charter completeness — every deferral-list item either landed in its attributed phase or is reported unresolved.

The universal-lens full sweep is NOT re-run over the cumulative diff — each phase's review already ran it phase-scoped, and re-running it over the whole span would recreate the oversized-review non-convergence this machinery exists to eliminate. Chain integrity is NOT this review's job — the orchestrator verifies it mechanically at run-level acceptance. This mode is findings-only.

Universal Lenses

Two gates lead every review; apply them before the rest.

1. Correct seam / location: Is the change at the architecturally correct location — the layer/module/function the codebase's design says owns this responsibility — or a symptom-patch at the wrong seam that merely makes a test pass? A wrong-location fix is technical debt even when it works: it ossifies a dead or duplicate path and leaves the real seam (and the rest of the card class) untouched. This is the highest-priority check; a wrong seam is disqualifying no matter how clean the code looks. Name the correct seam in the finding.
2. Most idiomatic change at the seam: Given the right seam, is this the implementation a principal engineer steeped in this repository would write — the established building block reused rather than re-implemented, an existing typed enum parameterized rather than a new bool or sibling variant, nom combinators composed rather than string dispatch? "Works and is in the right place" is not enough when a cleaner house idiom exists; a correct-but-unidiomatic change is a finding, not a style nit. Three structural checks make this concrete: a reference enum (HandSize, LifeTotal) must not carry a Fixed/constant payload that belongs one level up in an expression wrapper (mixed abstraction layers); a new sibling on an X/OpponentX/TargetX cluster should be one parameterized variant — but only when the axis stays within a single CR rule section (don't unify across sections, e.g. life CR 119 vs power/toughness CR 208/209); and before flagging or accepting any new enum variant, grep data/engine-inventory.json (gitignored — run cargo engine-inventory to (re)generate it locally first) to confirm it doesn't already exist and to surface the cluster smell.

• Class vs single case: Does the change cover a reusable class? Name at least three examples in that class. If there is only one, flag a special-case smell.
• Sibling coverage: If one site in a class changed, name siblings that needed the same treatment and verify they were handled or intentionally unaffected.
• Test adequacy: Ensure tests exercise the failure path and the building block, not only one card or a constructor shortcut that bypasses production wiring.
• Vacuous negative assertions: For every negative assertion (!detector(...), "does not parse to X", "counter NOT applied"), verify the input actually reached the code under test. An upstream short-circuit makes the negative pass for the wrong reason — canonical instance: check_swallowed_clauses returns early when any parsed ability contains Effect::Unimplemented, so !has_swallowed_clause(...) passes on a card that failed to parse at all. Require a paired positive reach-guard in the same test (parse succeeded, zero Effect::Unimplemented, expected positive shape) or a runtime assertion that flips on revert. This is the highest-frequency contributor-PR finding — check it on every test the diff adds.
• New-field threading: When the diff adds a field to an existing enum variant or struct, grep every construction and consumption site of that variant across the workspace. Flag any site that silently defaults or drops the field — resume/continuation paths, single-pick vs multi-pick branches, batch handlers, and WASM/adapter/serialization payload constructors are the recurring drop points. Each site must thread the field or carry an explicit reason why the default is correct there.
• Claim-to-test map: For every behavior the PR claims to support, identify the changed seam/function, production entry point, and test that reaches it. Flag any behavior tested only through a helper, constructor, parsed AST, or direct resolver call when production reaches it through apply(), WaitingFor, GameAction, stack/casting, combat declaration, replacement handling, or the scenario runner. Parser shape tests do not satisfy runtime semantics or coverage-support claims; they are acceptable for parser-only work only when unsupported semantics remain red/honest.
• Fixture path-divergence: A test can drive the real production entry point and still miss the bug if its fixtures are shaped so simply that they take a different internal branch than production inputs. Technique: trace the fix's entry through its first input-shape dispatches — is_none()/, /, variant , and "has-X" guards (e.g. ). For each such branch the fix can reach, map every test fixture to the arm it triggers, and flag any production-reachable arm with fixture. Smell: every fixture is degenerate in the same way (no ability/effect, no targets, empty or single-element collection, default/ field), so only the trivial shortcut arm runs while the arm real data takes ships untested. Name the unexercised arm and the minimal fixture change that would reach it.

Surface-Specific Lenses

Engine Logic

• Verify every new or moved // CR <rule> by checking docs/MagicCompRules.txt; the cited rule must actually describe the code.
• Compound CR annotations are the project's documented convention, not format violations: CR X + CR Y for interacting rules, CR X / CR Y for alternatives, and range/subpart forms like CR 702.45a/b (see CLAUDE.md "MTG Comprehensive Rules Annotations"). Do not flag the + / / / range forms as malformed or as regex violations — external reviewers routinely raise these and they should be refuted, not echoed. Only flag a CR citation whose base number does not resolve in docs/MagicCompRules.txt, or one that does not describe the annotated code.
• Check reuse of building blocks in parser/oracle_nom/, parser/oracle_util.rs, game/filter.rs, game/quantity.rs, game/ability_utils.rs, game/keywords.rs, game/zones.rs, and game/targeting.rs.
• Keep game logic in the engine. If player-visible state was added, verify multiplayer filtering.
• For non-battlefield zones, player-scoped queries usually use owner, not controller.
• Zone changes should route through replacement-aware pipelines rather than direct moves when replacements can apply.

Parser

• Reject verbatim full-string Oracle matches and ad hoc dispatch.
• Verify plural, possessive, opponent, non-X, another, and sibling phrase variants for new parser arms — including article/quantity word-forms (a/an/one/two/N/X/each) when dispatch splits on a count or article boundary.
• When one parser arm rejects a value to defer to another (e.g. a multi-count arm returns None for count==1 so the single-count arm handles it), trace the receiving arm and prove it actually accepts that form — do not trust the comment. A deferral to a path that doesn't handle the form silently drops the card to Unimplemented (precedent: "roll one d6" — the multi-die arm rejected count==1 and the single-die arm only matched "a "/"an ", so the word "one" parsed nowhere despite a comment claiming otherwise).
• Prefer composable nom axes over cartesian lists of full tag() strings.

Frontend / UI

• The frontend renders engine-provided state; it must not infer game rules or hidden data.
• Check React effect dependencies, unmount cleanup, touch equivalents, mobile scroll containment, and empty/loading/error states.
• Type-check passing is not proof of feature correctness; say when browser verification was not performed.
• i18n: Flag frontend-authored user-facing text (titles, labels, buttons, tooltips, placeholders, log templates) hardcoded in JSX instead of routed through t(). Conversely, flag engine/card pass-through (card names, Oracle text, interpolated enum strings) that was wrongly wrapped in t() — it belongs to the content pipeline, not chrome. Boundary rule: a string gets t() iff the frontend authored it (client/src/i18n/README.md). Also flag hand-rolled pluralization (count === 1 ? …) that should use key_one/key_other, and any direct i18n.changeLanguage call (the preferences store owns language).

Multiplayer / Transport

• Verify hidden-information filtering.
• Round-trip new fields across WASM, WebSocket, Tauri, and P2P adapters where applicable.
• Check reconnect and 3+ player behavior when touched.

AI

• Classifiers must cover the full enum/category, including untargeted board wipes and non-target effects.
• Deadline-bail branches must score candidates consistently with the no-bail path.
• Cache keys must include all inputs that alter decisions.
• Combination generators should short-circuit infeasible cases before enumerating.

Deck / Format / Feeds

• Format checks should use semantic identity, such as Basic supertype, not brittle name allowlists.
• Feed code must not overwrite cached state with empty or zero-deck responses.

Output

Use this exact finding shape:

**[HIGH/MED/LOW]** <short summary>. Evidence: <path:line>. Why it matters: <one sentence>. Suggested fix: <one line>.

Severity calibration: a latent bug — one not reachable today because a guard or parser blocks it — is still a finding. Rate it by what happens when the form is reached or the guard removed, not by today's reachability: if it then produces a wrong result for a card-class the feature appears to cover, it is at least MED, and "unreachable today" belongs in the evidence, not as a reason to downgrade to LOW. A feature that ships silently incorrect for a sub-class it looks like it handles (e.g. a multi-die roll that overwrites the stored result each iteration instead of supporting the aggregate) is a MED the maintainer must see — never a NIT.

Findings first. No praise, no diff recap.

Exception: in Engine-Implementer Checkpoint Mode, the Review Head, Completion Gate, and Maintainer-Simulation Gate lines precede findings.