Add Captain Marvel, Apex Avenger - #6938
Conversation
github-actions
Bot
added
the
needs-maintainer
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds ChangesCounter reproduction engine
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/parser/oracle_trigger.rs (1)
15801-15811: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale batching comment.
This comment states that "Tier-3 per-kind reproduction stays non-batched" after the
lower_trigger_irre-gate. The re-gate at lines 1777-1781 now clearsbatchedonly forEffect::Unimplemented, so reproduction effects stay batched. Align this comment with the new rule to avoid a wrong maintainer assumption.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_trigger.rs` around lines 15801 - 15811, The batching comment above the counter-filter check is stale: update it to state that the lower_trigger_ir re-gate clears batched only for Effect::Unimplemented, so Tier-3 reproduction effects remain batched. Preserve the existing explanation of batched-by-phrasing and kind-filtered behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/game/ability_scan.rs`:
- Line 5373: Move ReproduceEventCounters out of the SnapshotOrEvent and
BoundedOrNoPopulation classifications, and add it to the LiveBoardCensus and
corresponding census partition in effect_census_role. Keep both classification
partitions byte-identical so census_partition_agrees_with_effect_target_ctx
remains valid.
In `@crates/engine/src/game/effects/mod.rs`:
- Around line 4154-4156: Add local CR annotations to the
Effect::ReproduceEventCounters dispatch arm and the grouped event-bookkeeping
arms near the corresponding reproduction handlers, using only verified CR
references already supported by the codebase. Match each citation to that arm’s
counter placement and batched-trigger behavior, and do not invent or reuse
unrelated CR numbers.
In `@crates/engine/src/game/engine_payment_choices.rs`:
- Around line 78-83: Add a production-pipeline regression test near the trigger
handling covered by the changed restoration logic, using a non-empty plural
CounterAdded batch. Pause the plural optional trigger, accept it through the
normal pipeline, and assert that Effect::ReproduceEventCounters observes every
event; do not rely on empty trigger_events fixtures.
In `@crates/engine/src/parser/oracle_effect/imperative.rs`:
- Around line 12743-12756: Preserve the `_multi_target` value returned by
`try_parse_reproduce_event_counters` when constructing
`ZoneCounterImperativeAst::ReproduceEventCounters`. Add or reuse a
`MultiTargetSpec` field on that AST variant and thread it through the normal
imperative-lowering path so `resolve_counter_placement_target` retains
zero-target and target-cardinality semantics without widening the parsed target
filter.
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 16136-16151: Preserve the multi-target cardinality returned by
try_parse_reproduce_event_counters when building
ZoneCounterImperativeAst::ReproduceEventCounters in try_parse_verb_and_target.
Propagate _multi_target through the targeted proxy or update
try_split_targeted_compound to recover and apply it, ensuring “up to one” and
“up to N” reproduction targets remain optional rather than mandatory.
In `@crates/engine/src/parser/oracle_trigger.rs`:
- Around line 5570-5586: Update the subtype-condition handling around
parse_event_object_subtype_intervening_if so zone-change triggers evaluate the
original ZoneChangeRecord object, using ZoneChangeObjectMatchesFilter or an
equivalent incarnation-aware matcher instead of EventObjectMatchesFilter.
Preserve current behavior for non-zone-change triggers and add a regression test
covering an object leaving and re-entering before resolution.
- Around line 9254-9268: The shared ParseContext allows a stale
relative_player_scope from counter-effect parsing to affect later general
subjects in parse_single_subject. Clear or isolate relative_player_scope before
general trigger-subject parsing, while preserving the scope only for the
intended counter-placement actor path and keeping parse_type_phrase_with_ctx
behavior there.
In `@crates/phase-ai/src/policies/effect_classify.rs`:
- Around line 79-84: Update the Effect::ReproduceEventCounters arm in the effect
polarity classifier to return EffectPolarity::Contextual instead of Beneficial.
Preserve event-derived counter and target handling without assuming self-buffs,
and add coverage for non-SelfRef targets, harmful counter kinds, and
EventCounterReproductionCount::PerKind.
---
Outside diff comments:
In `@crates/engine/src/parser/oracle_trigger.rs`:
- Around line 15801-15811: The batching comment above the counter-filter check
is stale: update it to state that the lower_trigger_ir re-gate clears batched
only for Effect::Unimplemented, so Tier-3 reproduction effects remain batched.
Preserve the existing explanation of batched-by-phrasing and kind-filtered
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d24a4e74-63e9-43bf-a7a3-763cfce43460
📒 Files selected for processing (37)
crates/engine/data/known-tokens.tomlcrates/engine/data/mtgjson-vintagecrates/engine/src/analysis/ability_graph.rscrates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/coverage.rscrates/engine/src/game/effects/counters.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/engine_payment_choices.rscrates/engine/src/game/engine_phase_trigger_regression_tests.rscrates/engine/src/game/log.rscrates/engine/src/game/printed_cards.rscrates/engine/src/game/sba.rscrates/engine/src/game/targeting.rscrates/engine/src/game/trigger_index.rscrates/engine/src/game/trigger_matchers.rscrates/engine/src/game/triggers.rscrates/engine/src/parser/oracle_effect/counter.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/src/parser/oracle_ir/ast.rscrates/engine/src/parser/oracle_ir/doc.rscrates/engine/src/parser/oracle_static/mod.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/src/types/ability.rscrates/engine/src/types/events.rscrates/engine/src/types/resolution.rscrates/engine/tests/integration/captain_marvel_apex_avenger.rscrates/engine/tests/integration/cr733_resolved_frame_transition.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/optional_effect_remember_legal_actions.rscrates/phase-ai/src/policies/effect_classify.rscrates/phase-ai/src/policies/redundancy_avoidance.rscrates/phase-ai/src/policies/sacrifice_value.rscrates/phase-ai/src/policies/tests/effect_classify_snapshot.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Remove the unrelated generated token-registry refresh from this card-mechanic PR. Evidence: crates/engine/data/known-tokens.toml:381 begins additions such as Bejeweled Warg; the file contributes 3,007 added lines, and crates/engine/data/mtgjson-vintage:1 advances the source snapshot. Why it matters: these unrelated corpus updates enlarge the review and regression surface of a 37-file engine/parser change without being required by Captain Marvel's counter-reproduction mechanic. Suggested fix: restore both generated-data paths to the PR base and land any registry refresh separately with its own generation and validation evidence.
[MED] Do not request merge approval until the current-head parser-impact artifact is available. Evidence: current-head packet for be6b73673a8f65d0ac2884a0f6786825db18ce6c reports parse_diff.present=false / state=absent, while crates/engine/src/parser/oracle_effect/counter.rs:638 and crates/engine/src/parser/oracle_trigger.rs:2390 change parser behavior. Why it matters: the required card-level diff is the only evidence that this broad parser change has no unintended coverage blast radius. Suggested fix: let/regenerate CI's <!-- coverage-parse-diff --> artifact for this exact head and reconcile every gained, lost, or changed card before resubmission.
|
Generated for head Parse changes introduced by this PR · 26 card(s), 11 signature(s) (baseline: main
|
|
Update: the required head-bound parse artifact arrived immediately after the review was submitted. It reports 27 changed cards / 13 signatures for |
…R (CR review [HIGH]) Restore known-tokens.toml and the mtgjson-vintage stamp to the PR base. The +3,007-line token-registry refresh and the 2026-07-30 -> 2026-08-02 snapshot bump are a broad multi-set catalog regeneration, unrelated to Captain Marvel's counter-reproduction mechanic. The card corpus (client/public/card-data.json) is generated at build time, so this does not remove any released card; the registry refresh should land separately with its own generation/validation. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…dition (CR 603.4)
The subtype intervening-if recognizer scanned the whole effect string via
scan_preceded, so it also matched a TRAILING resolution conditional
("... return that card ... if it's a Samurai card" -- Oathkeeper, Takeno's
Daisho) and wrongly promoted it to a CounterAdded/ChangesZone trigger
condition. That changes whether the ability triggers and goes on the stack,
rather than gating only the effect at resolution.
An intervening-if (CR 603.4) is defined by position: it sits at the head of
the effect text. Restrict the recognizer to a leading match (before empty,
modulo whitespace); a trailing conditional now falls through to the existing
effect-level ChangeZone gate as before. Leading cases (Captain Marvel's "if
it's not a Kree", Otherworldly Escort's "if it's not a Spirit") are unchanged.
Adds a regression test asserting the trailing form is not lifted.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
|
Thanks @matthewevans — both points addressed in [HIGH] Unrelated generated-data churn — reverted. Restored [MED] Parser blast-radius — reconciled. Walked all 13 signatures against real Oracle text:
Note the parse-diff "affected" lists join names on commas, so a few entries ( |
|
Follow-up on the [HIGH] item: the token/subtype catalog refresh is already on |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Preserve the optional target cardinality for counter reproduction. Evidence: crates/engine/src/parser/oracle_effect/imperative.rs:12743-12755 and crates/engine/src/parser/oracle_effect/mod.rs:16136-16151 bind the MultiTargetSpec returned by try_parse_reproduce_event_counters as _multi_target; ZoneCounterImperativeAst::ReproduceEventCounters at crates/engine/src/parser/oracle_ir/ast.rs:1730-1733 and its lowerer at imperative.rs:13113-13119 consequently have no place to retain it. try_split_targeted_compound only recovers this metadata for PutCounter (oracle_effect/mod.rs:16638-16650). Why it matters: Aragorn, Company Leader's printed “up to one other target creature” is parsed but then loses its zero-or-one bound, so the resulting ability is not rules-correctly optional. The present Aragorn checks assert only effect shape / a supplied target (captain_marvel_apex_avenger.rs:688-726, :734-767), and therefore do not prove that the parsed ability carries min=0,max=1 or that the production choice path can decline it.
Please thread the MultiTargetSpec through the AST and both imperative/targeted-proxy lowering routes (or centralize recovery for this effect alongside the existing PutCounter recovery), then add a regression that parses Aragorn and proves the resulting ability's optional cardinality plus a real cast/trigger path covering both zero and one selected reproduction target. Re-run the relevant Tilt resources before re-requesting review.
…ction (CR 115.1d)
Aragorn, Company Leader reproduces counters "on up to one other target
creature" (min=0, max=1). `try_parse_reproduce_event_counters` computes the
correct MultiTargetSpec::up_to(1), but both binding sites discard it and the
ZoneCounterImperativeAst carries no channel for it, so the AST->Effect lowering
dropped the bound and the reproduction target bound as MANDATORY.
PutCounter and ReproduceEventCounters share the identical target-side grammar
("...counter(s) on up to N target ..."), so recover the reproduction effect's
cardinality through the same dedicated `extract_put_counter_multi_target`
post-parse fixup in `lower_imperative_clause` -- the centralized-recovery route
alongside the existing PutCounter recovery.
Tests: adds a parse-level assertion that the reproduction ability stamps
MultiTargetSpec::up_to(1) (the mandatory-vs-optional discriminator the prior
runtime-only zero-target test could not prove); the existing 0- and 1-target
cast/trigger runtime paths still pass. Full captain_marvel_apex_avenger module
green (19/19); Tilt was down so verified via direct targeted test runs.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the single-clause recovery is a good step, but compound counter-reproduction still loses the optional target bound.
🔴 Blocker
crates/engine/src/parser/oracle_effect/mod.rs:15332 returns from try_split_targeted_compound before the later direct-clause recovery at :15375-15399. On that compound route, try_parse_verb_and_target binds the reproduction parser's MultiTargetSpec as _multi_target and lowers only the target/effect (:16145-16160); then the splitter's primary-cardinality recovery at :16647-16660 reparses only Effect::PutCounter. A compound such as Put one of each of those kinds of counters on up to one other target creature and draw a card. therefore retains the reproduction effect but returns no MultiTargetSpec, turning the printed zero-or-one target into a mandatory target.
The new Aragorn checks exercise the non-compound sentence (captain_marvel_apex_avenger.rs:679-844), so they do not reach this earlier compound return. Please carry the parsed cardinality through the reproduction AST/proxy path, or centralize the recovery so ReproduceEventCounters uses the same primary-clause path as PutCounter. Add a parser regression for the compound form plus production-pipeline zero-target and one-target regressions.
✅ Clean
The current head's direct-clause recovery and the Aragorn zero/one coverage address the original non-compound case; this request is limited to the compound route above.
Recommendation: request changes for the compound-cardinality path, then re-request review on a new head with the current-head parse-diff artifact.
… reproduction (CR 115.1d)
Follow-up to the single-clause fix. When the reproduction is the PRIMARY clause
of a compound ("put one of each of those kinds of counters on up to one other
target creature AND draw a card"), the clause returns from
try_split_targeted_compound before the direct-clause lower_imperative_clause
fixup ever runs. That splitter's own primary-cardinality recovery only reparsed
Effect::PutCounter, so a compound reproduction primary dropped its "up to one"
bound and bound the target as mandatory.
Extend the splitter's primary-cardinality recovery to ReproduceEventCounters,
re-deriving the MultiTargetSpec from the consumed primary-clause prefix via
try_parse_reproduce_event_counters -- the same building block that produced the
effect, mirroring the existing PutCounter recovery exactly.
Tests: adds a compound parser regression (primary retains MultiTargetSpec::up_to(1)
and the trailing conjunct survives as a sub-ability) plus production-pipeline
zero-target and one-target runtime regressions proving the optional slot while the
mandatory second conjunct still draws. Module green (22/22); Tilt down, verified
via direct targeted runs.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
|
Both addressed — single-clause in Single-clause: confirmed the gap with a parse-level assertion ( Compound: extended Added a compound parser regression plus 0- and 1-target production-pipeline runtime tests; the reproduction module is green (22/22). Tilt was down locally so I verified with direct targeted runs; CI on the prior head was green on all non-AI gates. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] ReproduceEventCounters is not intrinsically beneficial. Evidence: crates/phase-ai/src/policies/effect_classify.rs:79-84 returns Beneficial for every variant, while crates/engine/src/parser/oracle_effect/counter.rs:648-675 intentionally preserves event-derived counter kinds and target shapes including Aragorn's up to one other target creature; crates/phase-ai/src/policies/anti_self_harm.rs:319-385 consumes this classification when evaluating creature-targeted effects. Why it matters: the triggering event can carry a harmful counter and the reproduced effect can target a non-self creature, so treating the whole effect class as a self-buff can make target-selection policy reason in the wrong direction. Suggested fix: classify ReproduceEventCounters as Contextual until the policy has both the selected target and triggering counter multiset, and replace the self-only snapshot expectation with discriminating coverage for SelfRef/another-target and SameNumber/PerKind forms.
…l (CR 122.1)
The reproduced counter KIND is event-derived at resolution (the triggering event
can carry a harmful kind, e.g. -1/-1), and the TARGET is not necessarily self --
Aragorn, Company Leader reproduces onto "up to one other target creature". So the
effect's polarity depends on the selected target and the triggering counter
multiset, neither of which is known at classify time. Treating the whole class as
a static self-buff let anti_self_harm reason in the wrong direction on
creature-targeted reproductions.
Reclassify effect_polarity(ReproduceEventCounters) as Contextual so the call site
inspects the concrete target and multiset instead of assuming a self-buff.
Tests: replace the single self-only Beneficial snapshot expectation with
discriminating coverage across both axes -- target {SelfRef, other} x per-kind
{SameNumber, PerKind} -- all asserting Contextual (phase-ai effect_classify
suite green, 18/18).
Co-Authored-By: Claude Opus 4.8 <[email protected]>
|
Maintainer hold — current head The correction at Required Rust lint and test shards are still in progress, and the only |
…napshot evaluator (CR 603.4 + CR 603.10)
The "if it's [not] a <subtype>" recognizer emitted EventObjectMatchesFilter
unconditionally. That matcher is live-first (current state, then LKI fallback),
which is correct for a non-zone event (Captain Marvel's CounterAdded "if it's
not a Kree"), but wrong for a ZONE-CHANGE trigger: for a dies/leaves trigger
(Otherworldly Escort, "when this dies, if it's not a Spirit") the subject "it"
must be judged from the zone-change event snapshot, or a same-ID incarnation
that re-enters before the recheck would be judged instead of the object that
left.
Split the recognizer to return the typed filter + negation, and let the caller
choose the evaluator by trigger kind via build_event_object_subtype_condition:
ZoneChangeObjectMatchesFilter { origin, destination, filter } when
trigger_zone_change is present, EventObjectMatchesFilter otherwise. The zones
come from trigger_head_dies_zone_change (Battlefield -> Graveyard), matching the
dies_lookback_condition convention.
Tests: subtype_intervening_if_dispatches_by_trigger_kind proves the routing
(dies -> ZoneChangeObjectMatchesFilter, CounterAdded -> EventObjectMatchesFilter).
The destination evaluator's event-snapshot / re-entry-LKI authority is already
covered by zone_change_object_condition_uses_original_exit_lki_after_leave_and_
reentry and zone_change_object_condition_checks_dead_object_snapshot.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
…storation (CR 603.2c + CR 608.2) The OptionalEffectFrame restoration in engine_payment_choices.rs restores the PLURAL trigger_events batch (not just the singular event) when a suspended optional ReproduceEventCounters resumes, but the frame unit tests supply trigger_events: Vec::new() and pass even if that restoration is removed. Add optional_frame_restores_full_multi_event_batch_on_accept: a multi-kind placement (a +1/+1 count-2 event AND a shield count-1 event) suspends Captain Marvel's "may" reproduction and accepts it, then asserts BOTH kinds reproduce onto Captain Marvel. Dropping the plural restoration leaves the resumed resolution with only the singular event, so at most one kind survives and the test fails. Co-Authored-By: Claude Opus 4.8 <[email protected]>
|
Maintainer hold — head e77ae43. The prior substantive review findings are resolved. Required CI is green; approval/enqueue remains on hold while the Paired-seed AI gate and Decision-cost perf gate are pending. Once both settle successfully, we will recheck the live head and resume approval/enqueue. No author action is requested at this time. |
Maintainer port for current main; preserve ReproduceEventCounters read/census/randomness handling while accepting main’s live-board resolution-prompt authority. Co-authored-by: Jacob Woodson <[email protected]>
Port the new effect through main’s fail-closed resolution-prompt classifier. The branch predates that classifier; CI exposed the exhaustive-match omission after the maintainer merge. Co-authored-by: Jacob Woodson <[email protected]>
|
Maintainer hold — current head The maintainer port resolved the conflict with current |
Current main added this OptionalEffectFrame fixture after the contributor introduced its plural trigger_events field. The fixture has no trigger context, so an empty batch preserves its intended scenario. Co-authored-by: Jacob Woodson <[email protected]>
|
Maintainer hold — current head The maintainer port added the empty |
matthewevans
removed
the
needs-maintainer
# Conflicts: # crates/engine/src/game/printed_cards.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested for head 1a9a085971ae9cf5623de0527ed7ac8f14e738da.
[HIGH] Effect::ReproduceEventCounters is missing from two intentionally exhaustive Effect matches, which leaves the engine uncompilable. CI run 31300471390 reports E0004 at both sites:
crates/engine/src/types/ability.rs:15795,moves_card_to_or_from_library: classify this effect in the ordinaryfalsearm—it does not move a card to or from a library.crates/engine/src/types/ability_visit.rs:576,visit_effect_scoped: addReproduceEventCountersto the leaf arm so the visitor remains exhaustively complete without inventing a nested carrier.
The Paired-seed AI and Decision-cost perf gates failed because code generation could not compile past these E0004s, not because they measured an AI or performance regression. After the exhaustive-match fixes, please provide current-head Rust/AI/perf results and a fresh parse/coverage artifact.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head 5dfebc7: the visitor exhaustiveness fix completes the existing counter-reproduction path, feedback is addressed, and CI is green.
|
Maintainer hold — current head The prior exhaustive-match compile blockers are resolved. This branch is behind current |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Re-reviewed current main-refreshed head 41d6f48: counter-reproduction fixes and merged-base validation are green.
2 participants
Summary
Adds engine support for Captain Marvel, Apex Avenger.
Files changed
CR references
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Tier: Frontier
Verification
cargo fmt --all— pass./scripts/check-parser-combinators.sh (Gate A)— passcargo clippy-strict— passcargo test -p phase-engine— pass./scripts/gen-card-data.sh— passcargo coverage— passcargo semantic-audit— passScope Expansion
Beyond the plan: fixed a latent bug where the "you may" optional-trigger pause dropped the plural current_trigger_events batch (added OptionalEffectFrame.trigger_events + save/restore in handle_optional_effect_choice) — required for the reproduction resolver and benefiting all batched "you may" triggers; and added a ZoneCounterImperativeAst::ReproduceEventCounters AST variant for idiomatic parser plumbing.
Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit