fix(engine): Jeweled Amulet notes and reproduces spent mana type (#6504) - #6812
Conversation
|
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:
📝 WalkthroughWalkthroughAdds ChangesNoted mana support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant CastingEngine
participant GameObject
participant ResolvedAbility
participant NoteManaSpent
participant ManaResolver
OracleParser->>CastingEngine: lower note and NotedType abilities
CastingEngine->>GameObject: record spent mana
CastingEngine->>ResolvedAbility: snapshot payment and source incarnation
ResolvedAbility->>NoteManaSpent: provide captured payment
NoteManaSpent->>GameObject: validate incarnation and store noted mana
ManaResolver->>GameObject: read noted mana type
ManaResolver->>CastingEngine: produce noted mana
Possibly related PRs
Suggested labels: 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 |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the declared noted-mana class overreaches Ice Cauldron and loses source incarnation.
🔴 Blocker
parser/oracle_effect/imperative.rs:10121-10136 deliberately accepts “note the type and amount of mana,” including Ice Cauldron, but game/effects/mana.rs:544-559 and :852-865 retain only the first noted type and repeat the printed count. The producer parser at parser/oracle_effect/mana.rs:1407-1420 recognizes only “mana of ~'s last noted type.” Ice Cauldron’s current Oracle text is: “Add this artifact's last noted type and amount of mana.” Its X payment must determine the later production amount; this implementation cannot parse or reproduce that full instruction while claiming the class supports it.
Either narrow the change and its claims to singular-type wording, keeping Ice Cauldron strict-unimplemented, or model the noted amount as typed state and add parser plus end-to-end runtime coverage for Ice Cauldron’s activation, stored amount, later production, and spend restriction.
🔴 Blocker
game/effects/note_mana_spent.rs:29-41 reads and writes the latch through raw ability.source_id, while types/ability.rs:22697-22706 provides source_incarnation specifically to prevent rebinding a departed source and game/zones.rs:161-164 keeps an ObjectId as storage identity across zone changes. The new mana_spent_to_activate field at game_object.rs:1134-1149 has no zone-exit cleanup. A bounced or flickered source can therefore receive an old activation’s note on its new incarnation. Capture paid mana and source incarnation in the resolving context, write only if the captured source remains that incarnation, and add a stack-level bounce/flicker regression.
🟡 Required evidence
This engine/parser PR has no current <!-- coverage-parse-diff --> sticky despite a completed Card data job. Publish and reconcile current-head parse-diff evidence. The Rust lint job is also terminal red; resolve its reported gate before re-review.
Recommendation: make the supported class honest or implement the missing type-and-amount and source-incarnation semantics with discriminating end-to-end tests, then regenerate the parser evidence.
|
Generated for head Parse changes introduced by this PR · 1 card(s), 4 signature(s) (baseline: main
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 4964: Update the ManaProduction::NotedType arm in the ability scan
classification to set sibling: true while preserving its existing count scan.
Add a regression test covering fixed-count NotedType behavior when another
resolution changes the noted mana type, ensuring LoopFirewall detects the
dependency.
In `@crates/engine/src/game/casting.rs`:
- Around line 14725-14739: Move the spent-mana snapshot out of the source-global
latch and bind it to each activated ability/stack entry together with the source
incarnation. In crates/engine/src/game/casting.rs:14725-14739, capture the
payment snapshot during activation; in
crates/engine/src/game/game_object.rs:1134-1149, clear or remove transient
source-level payment state on object changes; and in
crates/engine/src/game/effects/note_mana_spent.rs:29-41, require the matching
incarnation and consume that activation’s snapshot before recording the note.
Add regression coverage for repeated activations across untap and source
re-entry before resolution.
In `@crates/engine/src/game/effects/mana.rs`:
- Around line 544-560: Update the NotedType representation and its handling in
the ManaProduction path to preserve and later reproduce the full mana
composition recorded by the noting activation, including both type and amount
for Ice Cauldron. Replace the first-type repetition and current count resolution
in the NotedType branch with consumption of the durable noted mana value, and
update related support paths around noted_mana_type_for so the reusable
abstraction is not limited to Jeweled Amulet.
In `@crates/engine/tests/integration/issue_6504_jeweled_amulet_noted_mana.rs`:
- Line 77: Update the test around runner.activate and resolve to separate
activation from resolution, then assert that no durable noted mana type is
usable while the activated ability remains unresolved. Ensure the regression
reaches the production mana-payment pipeline and verifies the noted type becomes
available only after NoteManaSpent resolves.
- Around line 127-155: Update jeweled_amulet_tracks_a_different_noted_color to
use the same amulet for two complete note/consume cycles: first note red and
consume the resulting ability, then note green and consume it again through the
existing runner production pipeline. Assert the second cycle produces green and
no red, ensuring the stored noted type is replaced rather than appended and read
stale.
🪄 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: 1ae99cad-6b87-4655-9932-5ab54062d9d3
📒 Files selected for processing (29)
crates/engine/src/analysis/ability_graph.rscrates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/coverage.rscrates/engine/src/game/effects/mana.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/effects/note_mana_spent.rscrates/engine/src/game/game_object.rscrates/engine/src/game/mana_sources.rscrates/engine/src/game/printed_cards.rscrates/engine/src/game/trigger_index.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/mana.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_trigger.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/issue_6504_jeweled_amulet_noted_mana.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/oracle_parser.rscrates/mtgish-import/src/convert/action.rscrates/phase-ai/src/features/devotion.rscrates/phase-ai/src/mana_colors.rscrates/phase-ai/src/policies/effect_classify.rscrates/phase-ai/src/policies/redundancy_avoidance.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — supplemental current-head blockers.
🔴 Blocker
game/ability_scan.rs:4959-4968 treats ManaProduction::NotedType as count-only and never asserts sibling: true, but the producer reads mutable per-source chosen_attributes through effects/mana.rs:856-865. The scanner’s own axis contract at ability_scan.rs:109-116 requires sibling-sensitive state to participate in loop-firewall classification. Add the required sibling axis/coverage so sibling activations cannot be treated as independent when their noted-mana state is shared/mutable.
🟡 Required tests and gate
issue_6504_jeweled_amulet_noted_mana.rs:77 immediately resolves the activation, so it cannot prove the note is absent before resolution; :127-155 uses a new amulet, so it cannot detect overwrite/append behavior on one object. Add reachability tests for both semantics alongside the already requested bounce/flicker case. The current Rust lint failure is the parser-combinator gate rejecting the new string dispatch at parser/oracle_effect/imperative.rs:10128; rewrite it with the repository’s required nom dispatch convention.
The current parse-diff is now present and confirms the earlier semantic concern: it adds NoteManaSpent for Ice Cauldron while adding Mana(NotedType) only for Jeweled Amulet. The earlier type-and-amount and source-incarnation blockers remain unchanged.
Recommendation: address these supplemental authority/test/gate defects together with the existing current-head blockers, then request re-review.
…n staleness Two review findings on PR phase-rs#6812 (issue phase-rs#6504): - The parser accepted Ice Cauldron's "note the type AND AMOUNT of mana spent..." wording and routed it through the same Effect::NoteManaSpent as Jeweled Amulet's singular-type wording, but the resolver only ever stores/reproduces the first noted type — it doesn't model Ice Cauldron's exact stored multiset or its spend restriction. Narrowed the parser (and every doc comment) to Jeweled Amulet's exact "note the type of mana spent..." wording only; Ice Cauldron's text is now intentionally left unmatched and still reports Unimplemented. This also fixes a parser-combinator-gate violation: the narrowed recognizer moved out of the `match first_word { "note" => .. }` string-literal dispatch (flagged as new bare-string-match-arm code) into an anchored nom `all_consuming` guard, mirroring the existing "end the turn" precedent in the same function. - `Effect::NoteManaSpent` read/wrote its source purely via `ability. source_id`, with no check that the object at that storage id was still the same incarnation that made the payment (CR 400.7). A source bounced or flickered while its OWN "note" ability sat unresolved on the stack would have the OLD incarnation's payment silently promoted onto the NEW incarnation's `chosen_attributes`. `GameObject::mana_spent_to_activate` is now paired with the incarnation captured at payment time (`mana_spent_to_activate_incarnation`); the resolver refuses to write unless the object's live incarnation still matches, mirroring the engine's existing incarnation-pairing idiom used by `ResolvedAbility::source_is_current` / `TargetFilter::SelfRef` resolution. Added a stack-level bounce regression test, verified revert-failing.
|
Addressed both blockers: Ice Cauldron overreach — narrowed the parser to Jeweled Amulet's exact singular-type wording only ("note the type of mana spent to pay this activation cost"). Ice Cauldron's "note the type AND AMOUNT..." is now intentionally left unmatched and still reports Source incarnation — The CI lint-gate failure was specifically the parser-combinator-gate violation above; re-ran |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/casting.rs`:
- Around line 14725-14744: Bind each activation’s mana-payment snapshot to its
individual pending activation or stack entry, including the source incarnation,
instead of storing it only in GameObject::mana_spent_to_activate. Update the
payment flow around the shown source-mutation logic to attach the snapshot to
the activation record, and update NoteManaSpent resolution to consume that exact
record while rejecting incarnation mismatches; preserve support for both
direct-activation and PendingCast paths without relying on tap-cost behavior.
In `@crates/engine/tests/integration/issue_6504_jeweled_amulet_noted_mana.rs`:
- Around line 211-280: Prove ability 0’s payment was actually recorded before
the bounce by querying P0’s mana pool immediately after ActivateAbility and
asserting the single red mana unit was drained. Add this reach-guard before
capturing incarnation_before or invoking bounce::resolve, using the existing
mana-pool query helper in the test file; preserve the current bounce and final
noted_mana_spent assertions.
🪄 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: 466b644a-a929-4edb-b725-be04df019bd9
📒 Files selected for processing (7)
crates/engine/src/game/casting.rscrates/engine/src/game/effects/note_mana_spent.rscrates/engine/src/game/game_object.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_ir/ast.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/issue_6504_jeweled_amulet_noted_mana.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/types/ability.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — current-head paid-mana attribution remains unsafe.
🔴 Blocker
game/casting.rs:14725-14743 writes every activation’s paid mana onto the mutable source-global GameObject::mana_spent_to_activate, explicitly overwriting it on the next activation. effects/note_mana_spent.rs:42-52 later reads that mutable latch and verifies only the object’s current incarnation, not the resolving ability’s own payment. If an unresolved Jeweled Amulet activation is untapped and activated again with a different color, both note effects can read the later payment. Capture the spent mana and source incarnation in the activated stack/ResolvedAbility context, then consume the resolving ability’s snapshot; add a LIFO stacked-activation regression.
🔴 Blocker
game/ability_scan.rs:4959-4968 still treats ManaProduction::NotedType as count-only, even though effects/mana.rs:840-853 reads mutable per-source ChosenAttribute::NotedManaSpent. The scan contract at ability_scan.rs:109-116 classifies that as sibling-mutable state. Assert sibling: true for this producer and add the corresponding loop-firewall regression.
🟡 Required evidence
issue_6504_jeweled_amulet_noted_mana.rs:67-84 immediately resolves the helper activation, so it cannot prove no durable note before resolution/countering; its red and green checks use separate objects (:90-151), so they do not establish replace-versus-append behavior. Add those discriminating cases. The only parse-diff sticky predates this head and still lists Ice Cauldron as supported; regenerate and reconcile current-head evidence for the intentional narrowing.
Recommendation: retain the good narrowing and bounce fix, but make paid-mana provenance per activation, correct the scan axis, add the missing tests, and regenerate parse evidence before re-review.
Second round of review on PR phase-rs#6812 (issue phase-rs#6504): - game/casting.rs / game/casting_costs.rs / effects/note_mana_spent.rs: GameObject::mana_spent_to_activate was a per-source mutable latch that the NEXT activation's cost payment silently overwrote. Since the incarnation guard added in the prior round only protects against a bounced/flickered SOURCE, not a same-incarnation permanent untapped and reactivated while an earlier "note" activation still sits unresolved on the stack (legal: "Activate only if no charge counters" is checked at activation, and no counter exists yet while the first note ability is still unresolved), two stacked activations could both observe the LATER payment. `push_ability_entry` — the single authority where an activated ability reaches the stack — now drains that latch synchronously into THIS activation's own `ResolvedAbility:: noted_mana_payment` snapshot (a new `NotedManaPayment { types, source_incarnation }`, propagated through `sub_ability`/`else_ability` via a new `set_noted_mana_payment_recursive`, since `Effect:: NoteManaSpent` is chained as a sub-ability with its own separate ResolvedAbility node) immediately after cost payment completes, before any later activation of the same permanent can occur. `Effect::NoteManaSpent` now reads that per-activation snapshot instead of the shared object field. Added a LIFO stacked-activation regression (`jeweled_amulet_lifo_stacked_activations_each_note_their_own_payment`) that activates twice with different colors before either resolves and asserts each resolution notes its own payment. - game/ability_scan.rs: `ManaProduction::NotedType` read `ChosenAttribute::NotedManaSpent`, a per-object value a SIBLING copy of `Effect::NoteManaSpent` can mutate before this production resolves — exactly the race above — so it must self-assert `sibling: true` (CR 603.3b ordering-relevance) rather than sit in the count-only bucket alongside `ChosenColor` (whose as-enters choice is effectively fixed for the object's lifetime and has no such sibling-mutation risk). Added `noted_mana_type_self_asserts_sibling`. - issue_6504_jeweled_amulet_noted_mana.rs: added `jeweled_amulet_notes_nothing_before_resolution` (CR 608.2c: no durable note before the ability actually resolves) and `jeweled_amulet_second_note_replaces_not_appends` (two full note cycles on the SAME object leave exactly one entry, not two) — the prior round's red/green tests used separate objects and an always-resolved helper, so neither claim was actually exercised. `noted_mana_payment` is a new `ResolvedAbility` field; every exhaustive match/destructure/literal across the crate (batch-candidate proofs in stack.rs, inert-trigger equality, several no-target `ResolvedAbility` literals in effects/*.rs) is updated in lockstep, including two new "must not batch a per-activation payment snapshot" gates mirroring the existing `cost_paid_object` ones.
|
Addressed both blockers: Per-activation paid-mana attribution — One wrinkle this surfaced mid-fix: Added Sibling classification — Test evidence — added
Local verification on the pushed commit: |
There was a problem hiding this comment.
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/types/ability.rs (1)
1471-1483: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale doc: "transient
mana_spent_to_activatelatch" contradicts the per-activation snapshot design.Both
ChosenAttribute::NotedManaSpent's doc andEffect::NoteManaSpent's doc describe the noted-mana data as read from "the source's transientmana_spent_to_activatepayment latch" at resolution time. ButResolvedAbility::noted_mana_payment(and its accompanying doc, plus the commit history: "refined the implementation from per-object activation payment storage to per-activation snapshots") makes clear the actual mechanism captures payment once per activation on theResolvedAbilityat stack-push time — specifically to prevent stacked activations of the same permanent from clobbering each other's payment before either resolves.As written, these two comments describe the exact bug ("a per-object mutable latch") that the per-activation snapshot was introduced to fix, which will mislead anyone maintaining this code later. Please update both doc comments to say
Effect::NoteManaSpentreadsability.noted_mana_payment(the per-activation snapshot stamped bypush_ability_entry/set_noted_mana_payment_recursive), not a live per-object latch.Also applies to: 12589-12603
🤖 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/types/ability.rs` around lines 1471 - 1483, Update the documentation for ChosenAttribute::NotedManaSpent and Effect::NoteManaSpent to describe reading ability.noted_mana_payment, the per-activation snapshot stamped by push_ability_entry/set_noted_mana_payment_recursive. Remove references to the source's transient mana_spent_to_activate latch while preserving the existing zone-clearing and replacement behavior documentation.
🤖 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.
Outside diff comments:
In `@crates/engine/src/types/ability.rs`:
- Around line 1471-1483: Update the documentation for
ChosenAttribute::NotedManaSpent and Effect::NoteManaSpent to describe reading
ability.noted_mana_payment, the per-activation snapshot stamped by
push_ability_entry/set_noted_mana_payment_recursive. Remove references to the
source's transient mana_spent_to_activate latch while preserving the existing
zone-clearing and replacement behavior documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: aad4c36a-5f42-4ffd-ad9b-52caa869e72c
📒 Files selected for processing (19)
crates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/effects/additional_phase.rscrates/engine/src/game/effects/double.rscrates/engine/src/game/effects/extra_turn.rscrates/engine/src/game/effects/grant_extra_loyalty_activations.rscrates/engine/src/game/effects/note_mana_spent.rscrates/engine/src/game/effects/player_counter.rscrates/engine/src/game/effects/reverse_turn_order.rscrates/engine/src/game/effects/skip_next_step.rscrates/engine/src/game/effects/skip_next_turn.rscrates/engine/src/game/effects/vote.rscrates/engine/src/game/game_object.rscrates/engine/src/game/stack.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/issue_6504_jeweled_amulet_noted_mana.rscrates/engine/tests/integration/the_chain_veil_loyalty_grants.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/game/ability_rw.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
🔴 Blocker — ability copies retain an activation-only payment snapshot
copy_spell.rs:109-130 clones the entire StackEntryKind, then the activated-ability arm only preserves the original source. That leaves ResolvedAbility::noted_mana_payment (including nested sub_ability/else_ability nodes) intact. note_mana_spent.rs:46-70 consumes that snapshot when the copied ability resolves, so a copied Jeweled Amulet activation can write the original activation's paid mana colors even though the copy did not pay an activation cost.
CR 707.10 says a copied activated ability is not activated. The original's announced decisions and objects used to pay costs are copyable where relevant, but this engine's noted_mana_payment is an activation-payment observation for NoteManaSpent, not a decision or cost object that the copy paid. Preserving it produces a false durable note on resolution.
Please clear noted_mana_payment recursively when copying activated abilities (while retaining the existing CR 707.10b source preservation), and add a runtime regression that copies an activated Jeweled Amulet ability after a colored original payment and proves the copied ability cannot note those original colors. The regression should exercise the copy stack path and resolution rather than only inspecting a constructor.
The existing source-incarnation and per-activation provenance fixes are valuable, but they do not distinguish an original activation from a copy of that activation.
Third round of review on PR phase-rs#6812 (issue phase-rs#6504): copy_spell.rs's ActivatedAbility/TriggeredAbility copy arm only called preserve_ability_copy_source_recursive to re-stamp source_id; the rest of the cloned ResolvedAbility chain, including the new noted_mana_payment field, rode along unchanged. CR 707.10: a copy of an activated ability is not itself activated, so it never paid a mana cost — but a copied Jeweled Amulet activation still carried the ORIGINAL's captured payment, and Effect::NoteManaSpent resolving on the copy would falsely note colors the copy never spent. preserve_ability_copy_source_recursive now also calls a new ResolvedAbility::clear_noted_mana_payment_recursive (mirrors set_noted_mana_payment_recursive's recursion shape in reverse) so the copy's whole chain — including any sub_ability/else_ability nodes — loses the inherited snapshot. Added jeweled_amulet_copied_activation_does_not_note_original_payment, which exercises the real copy pipeline (copy_spell::resolve targeting the original activation's own stack-entry id, the same stack-entry lookup and LIFO resolution a real "copy target activated or triggered ability" card like Lithoform Engine drives) rather than only unit -testing the clearing method: activates paying red, copies it, resolves the copy first (asserting no note) and the original second (asserting it still notes red — the paired positive reach-guard proving the fix clears only the copy's snapshot). Verified revert-failing.
|
Addressed: CR 707.10 — copies of Jeweled Amulet's activation don't pay a cost, so they must not inherit the original's noted-mana payment.
Added Local verification on the pushed commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/integration/issue_6504_jeweled_amulet_noted_mana.rs`:
- Around line 487-595: Add an assertion immediately before the
copy_spell::resolve call that the original stack entry identified by
original_entry_id still carries its noted_mana_payment snapshot. Use the stack
entry’s resolved ability data and assert the snapshot is present, so the later
is_none() check specifically verifies that NoteManaSpent on the copied
activation does not inherit it.
🪄 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: 3b7b506e-a8fd-4945-ad77-beedadd02ed5
📒 Files selected for processing (3)
crates/engine/src/game/effects/copy_spell.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/issue_6504_jeweled_amulet_noted_mana.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the new effect is still reached through a verbatim, one-card Oracle-text match.
🔴 Blocker
crates/engine/src/parser/oracle_effect/imperative.rs:9765-9782 recognizes the entire normalized sentence "note the type of mana spent to pay this activation cost" in one all_consuming(terminated(tag(...))) branch. The confirmed Jeweled Amulet Oracle text is "Note the type of mana spent to pay this activation cost."; the current parser therefore hard-codes that card sentence rather than parsing the grammatical "note [the] type of mana spent to pay this [activation] cost" structure. This violates the parser's composable-building-block boundary and leaves even straightforward grammatical siblings as unrelated future special cases.
Please move this into a named nom grammar parser that composes the instruction prefix, the noted-mana subject, and the activation-cost referent (with explicit, typed boundaries), then add building-block tests for the accepted structural variants and a negative boundary. Keep Ice Cauldron unsupported unless its distinct amount-and-spend-restriction semantics are modeled end-to-end.
✅ Clean
The current head's per-activation ResolvedAbility::noted_mana_payment snapshot and recursive clearing on copied ability chains address the prior aliasing and copied-ability findings. The copy regression's final original-resolution assertion is also a sufficient positive reach guard for the original snapshot: it would fail if the clone cleanup erased or never preserved it.
Recommendation: request changes for the parser seam, then re-run the current-head parser and runtime evidence.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/parser/oracle_effect/imperative.rs`:
- Around line 13511-13560: The tests only exercise parse_note_mana_spent_clause,
so add end-to-end production parser assertions for both cards. Verify Jeweled
Amulet’s complete wording lowers to Effect::NoteManaSpent, and verify Ice
Cauldron’s type-and-amount wording reaches Effect::Unimplemented; include the
positive reach guard before the negative assertion as required by the parser
test conventions.
🪄 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: 171c54de-2992-4d65-9425-9edbab4c1c01
📒 Files selected for processing (2)
crates/engine/src/parser/oracle_effect/imperative.rscrates/engine/tests/integration/oracle_parser.rs
|
Maintainer hold — current head I pushed the narrow documentation correction: both relevant No approval or enqueue yet. The current-head CI run is still queued/in progress (AI contributor labels, paired-seed AI gate, decision-cost perf gate, Superagent Security Scan, and CodeRabbit), and the only Next step: wait for those checks to settle and for a parse-diff artifact generated from this head, then resume the final review/enqueue decision. |
|
Current-head maintainer hold — performance evidence is terminal red. Head |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
🔴 Blocker — current head does not compile
Reviewed commit 56fc5b9e8c18e608b4a12da20966c386df226b66.
Current merge CI is terminal red: Rust lint, Rust tests shard 1, Rust tests shard 2, WASM, Card data, paired-seed AI, and decision-cost perf.
The terminal Rust compiler failures require explicit handling of the new data:
ResolvedAbility::noted_mana_paymentis omitted from the explicit destructures incrates/engine/src/game/ability_scan.rs:209andcrates/engine/src/game/resolution_prompt.rs:523.Effect::NoteManaSpentis omitted from the exhaustive effect classifications incrates/engine/src/game/ability_scan.rs:408,:5218, and:5933;crates/engine/src/game/resolution_prompt.rs:210; andcrates/engine/src/types/ability_visit.rs:276.
Please classify both additions deliberately at every affected seam—whether each is carried, scanned, visited, or intentionally ignored. Do not resolve this with wildcard match arms or destructure ..; these sites represent distinct semantic classifications and must stay explicit. Add or update focused tests for those classifications.
Afterward, provide fresh green CI and a current-head parse-diff. The existing parse-diff predates this head, so it is not evidence for 56fc5b9e8c18e608b4a12da20966c386df226b66.
…n staleness Two review findings on PR phase-rs#6812 (issue phase-rs#6504): - The parser accepted Ice Cauldron's "note the type AND AMOUNT of mana spent..." wording and routed it through the same Effect::NoteManaSpent as Jeweled Amulet's singular-type wording, but the resolver only ever stores/reproduces the first noted type — it doesn't model Ice Cauldron's exact stored multiset or its spend restriction. Narrowed the parser (and every doc comment) to Jeweled Amulet's exact "note the type of mana spent..." wording only; Ice Cauldron's text is now intentionally left unmatched and still reports Unimplemented. This also fixes a parser-combinator-gate violation: the narrowed recognizer moved out of the `match first_word { "note" => .. }` string-literal dispatch (flagged as new bare-string-match-arm code) into an anchored nom `all_consuming` guard, mirroring the existing "end the turn" precedent in the same function. - `Effect::NoteManaSpent` read/wrote its source purely via `ability. source_id`, with no check that the object at that storage id was still the same incarnation that made the payment (CR 400.7). A source bounced or flickered while its OWN "note" ability sat unresolved on the stack would have the OLD incarnation's payment silently promoted onto the NEW incarnation's `chosen_attributes`. `GameObject::mana_spent_to_activate` is now paired with the incarnation captured at payment time (`mana_spent_to_activate_incarnation`); the resolver refuses to write unless the object's live incarnation still matches, mirroring the engine's existing incarnation-pairing idiom used by `ResolvedAbility::source_is_current` / `TargetFilter::SelfRef` resolution. Added a stack-level bounce regression test, verified revert-failing.
Second round of review on PR phase-rs#6812 (issue phase-rs#6504): - game/casting.rs / game/casting_costs.rs / effects/note_mana_spent.rs: GameObject::mana_spent_to_activate was a per-source mutable latch that the NEXT activation's cost payment silently overwrote. Since the incarnation guard added in the prior round only protects against a bounced/flickered SOURCE, not a same-incarnation permanent untapped and reactivated while an earlier "note" activation still sits unresolved on the stack (legal: "Activate only if no charge counters" is checked at activation, and no counter exists yet while the first note ability is still unresolved), two stacked activations could both observe the LATER payment. `push_ability_entry` — the single authority where an activated ability reaches the stack — now drains that latch synchronously into THIS activation's own `ResolvedAbility:: noted_mana_payment` snapshot (a new `NotedManaPayment { types, source_incarnation }`, propagated through `sub_ability`/`else_ability` via a new `set_noted_mana_payment_recursive`, since `Effect:: NoteManaSpent` is chained as a sub-ability with its own separate ResolvedAbility node) immediately after cost payment completes, before any later activation of the same permanent can occur. `Effect::NoteManaSpent` now reads that per-activation snapshot instead of the shared object field. Added a LIFO stacked-activation regression (`jeweled_amulet_lifo_stacked_activations_each_note_their_own_payment`) that activates twice with different colors before either resolves and asserts each resolution notes its own payment. - game/ability_scan.rs: `ManaProduction::NotedType` read `ChosenAttribute::NotedManaSpent`, a per-object value a SIBLING copy of `Effect::NoteManaSpent` can mutate before this production resolves — exactly the race above — so it must self-assert `sibling: true` (CR 603.3b ordering-relevance) rather than sit in the count-only bucket alongside `ChosenColor` (whose as-enters choice is effectively fixed for the object's lifetime and has no such sibling-mutation risk). Added `noted_mana_type_self_asserts_sibling`. - issue_6504_jeweled_amulet_noted_mana.rs: added `jeweled_amulet_notes_nothing_before_resolution` (CR 608.2c: no durable note before the ability actually resolves) and `jeweled_amulet_second_note_replaces_not_appends` (two full note cycles on the SAME object leave exactly one entry, not two) — the prior round's red/green tests used separate objects and an always-resolved helper, so neither claim was actually exercised. `noted_mana_payment` is a new `ResolvedAbility` field; every exhaustive match/destructure/literal across the crate (batch-candidate proofs in stack.rs, inert-trigger equality, several no-target `ResolvedAbility` literals in effects/*.rs) is updated in lockstep, including two new "must not batch a per-activation payment snapshot" gates mirroring the existing `cost_paid_object` ones.
Third round of review on PR phase-rs#6812 (issue phase-rs#6504): copy_spell.rs's ActivatedAbility/TriggeredAbility copy arm only called preserve_ability_copy_source_recursive to re-stamp source_id; the rest of the cloned ResolvedAbility chain, including the new noted_mana_payment field, rode along unchanged. CR 707.10: a copy of an activated ability is not itself activated, so it never paid a mana cost — but a copied Jeweled Amulet activation still carried the ORIGINAL's captured payment, and Effect::NoteManaSpent resolving on the copy would falsely note colors the copy never spent. preserve_ability_copy_source_recursive now also calls a new ResolvedAbility::clear_noted_mana_payment_recursive (mirrors set_noted_mana_payment_recursive's recursion shape in reverse) so the copy's whole chain — including any sub_ability/else_ability nodes — loses the inherited snapshot. Added jeweled_amulet_copied_activation_does_not_note_original_payment, which exercises the real copy pipeline (copy_spell::resolve targeting the original activation's own stack-entry id, the same stack-entry lookup and LIFO resolution a real "copy target activated or triggered ability" card like Lithoform Engine drives) rather than only unit -testing the clearing method: activates paying red, copies it, resolves the copy first (asserting no note) and the original second (asserting it still notes red — the paired positive reach-guard proving the fix clears only the copy's snapshot). Verified revert-failing.
8dd5053 to
0fc5c36
Compare
…se-rs#6504) Jeweled Amulet's first ability placed a charge counter but never noted the mana type spent to pay its own activation cost, so the second ability ("Add one mana of this artifact's last noted type") always produced zero mana. Both clauses fell through to Effect::Unimplemented. Adds the noted-mana-type building block shared by this small card class (Jeweled Amulet, Ice Cauldron): a transient GameObject payment latch stamped at ability-mana-cost payment, a ChosenAttribute::NotedManaSpent slot written by a new Effect::NoteManaSpent at resolution (so a countered ability never notes anything), and a ManaProduction::NotedType variant that reads it back at CR 106.5-correct empty-set behavior.
…n staleness Two review findings on PR phase-rs#6812 (issue phase-rs#6504): - The parser accepted Ice Cauldron's "note the type AND AMOUNT of mana spent..." wording and routed it through the same Effect::NoteManaSpent as Jeweled Amulet's singular-type wording, but the resolver only ever stores/reproduces the first noted type — it doesn't model Ice Cauldron's exact stored multiset or its spend restriction. Narrowed the parser (and every doc comment) to Jeweled Amulet's exact "note the type of mana spent..." wording only; Ice Cauldron's text is now intentionally left unmatched and still reports Unimplemented. This also fixes a parser-combinator-gate violation: the narrowed recognizer moved out of the `match first_word { "note" => .. }` string-literal dispatch (flagged as new bare-string-match-arm code) into an anchored nom `all_consuming` guard, mirroring the existing "end the turn" precedent in the same function. - `Effect::NoteManaSpent` read/wrote its source purely via `ability. source_id`, with no check that the object at that storage id was still the same incarnation that made the payment (CR 400.7). A source bounced or flickered while its OWN "note" ability sat unresolved on the stack would have the OLD incarnation's payment silently promoted onto the NEW incarnation's `chosen_attributes`. `GameObject::mana_spent_to_activate` is now paired with the incarnation captured at payment time (`mana_spent_to_activate_incarnation`); the resolver refuses to write unless the object's live incarnation still matches, mirroring the engine's existing incarnation-pairing idiom used by `ResolvedAbility::source_is_current` / `TargetFilter::SelfRef` resolution. Added a stack-level bounce regression test, verified revert-failing.
Second round of review on PR phase-rs#6812 (issue phase-rs#6504): - game/casting.rs / game/casting_costs.rs / effects/note_mana_spent.rs: GameObject::mana_spent_to_activate was a per-source mutable latch that the NEXT activation's cost payment silently overwrote. Since the incarnation guard added in the prior round only protects against a bounced/flickered SOURCE, not a same-incarnation permanent untapped and reactivated while an earlier "note" activation still sits unresolved on the stack (legal: "Activate only if no charge counters" is checked at activation, and no counter exists yet while the first note ability is still unresolved), two stacked activations could both observe the LATER payment. `push_ability_entry` — the single authority where an activated ability reaches the stack — now drains that latch synchronously into THIS activation's own `ResolvedAbility:: noted_mana_payment` snapshot (a new `NotedManaPayment { types, source_incarnation }`, propagated through `sub_ability`/`else_ability` via a new `set_noted_mana_payment_recursive`, since `Effect:: NoteManaSpent` is chained as a sub-ability with its own separate ResolvedAbility node) immediately after cost payment completes, before any later activation of the same permanent can occur. `Effect::NoteManaSpent` now reads that per-activation snapshot instead of the shared object field. Added a LIFO stacked-activation regression (`jeweled_amulet_lifo_stacked_activations_each_note_their_own_payment`) that activates twice with different colors before either resolves and asserts each resolution notes its own payment. - game/ability_scan.rs: `ManaProduction::NotedType` read `ChosenAttribute::NotedManaSpent`, a per-object value a SIBLING copy of `Effect::NoteManaSpent` can mutate before this production resolves — exactly the race above — so it must self-assert `sibling: true` (CR 603.3b ordering-relevance) rather than sit in the count-only bucket alongside `ChosenColor` (whose as-enters choice is effectively fixed for the object's lifetime and has no such sibling-mutation risk). Added `noted_mana_type_self_asserts_sibling`. - issue_6504_jeweled_amulet_noted_mana.rs: added `jeweled_amulet_notes_nothing_before_resolution` (CR 608.2c: no durable note before the ability actually resolves) and `jeweled_amulet_second_note_replaces_not_appends` (two full note cycles on the SAME object leave exactly one entry, not two) — the prior round's red/green tests used separate objects and an always-resolved helper, so neither claim was actually exercised. `noted_mana_payment` is a new `ResolvedAbility` field; every exhaustive match/destructure/literal across the crate (batch-candidate proofs in stack.rs, inert-trigger equality, several no-target `ResolvedAbility` literals in effects/*.rs) is updated in lockstep, including two new "must not batch a per-activation payment snapshot" gates mirroring the existing `cost_paid_object` ones.
Third round of review on PR phase-rs#6812 (issue phase-rs#6504): copy_spell.rs's ActivatedAbility/TriggeredAbility copy arm only called preserve_ability_copy_source_recursive to re-stamp source_id; the rest of the cloned ResolvedAbility chain, including the new noted_mana_payment field, rode along unchanged. CR 707.10: a copy of an activated ability is not itself activated, so it never paid a mana cost — but a copied Jeweled Amulet activation still carried the ORIGINAL's captured payment, and Effect::NoteManaSpent resolving on the copy would falsely note colors the copy never spent. preserve_ability_copy_source_recursive now also calls a new ResolvedAbility::clear_noted_mana_payment_recursive (mirrors set_noted_mana_payment_recursive's recursion shape in reverse) so the copy's whole chain — including any sub_ability/else_ability nodes — loses the inherited snapshot. Added jeweled_amulet_copied_activation_does_not_note_original_payment, which exercises the real copy pipeline (copy_spell::resolve targeting the original activation's own stack-entry id, the same stack-entry lookup and LIFO resolution a real "copy target activated or triggered ability" card like Lithoform Engine drives) rather than only unit -testing the clearing method: activates paying red, copies it, resolves the copy first (asserting no note) and the original second (asserting it still notes red — the paired positive reach-guard proving the fix clears only the copy's snapshot). Verified revert-failing.
…oduction parser
Round-5 review: the intentionally unsupported "type and amount" clause
(Ice Cauldron) was only exercised by parse_note_mana_spent_clause's own
unit tests and a fabricated end-to-end sibling ("Amber Amulet") — neither
path parses Ice Cauldron's actual printed Oracle text, so its Unimplemented
status wasn't protected against an upstream routing/fallback change that
could start treating it as supported.
Added ice_cauldron_note_type_and_amount_stays_unimplemented, parsing Ice
Cauldron's full two-ability Oracle text (verified against MTGJSON) through
the production parser. It walks the exile -> cast -> counter -> note
sub-ability chain to confirm the parser actually reaches the "type and
amount" clause (not failing earlier for an unrelated reason) before
asserting it still lands on Effect::Unimplemented, and separately asserts
the second ability's "last noted type and amount of mana" is not matched
by ManaProduction::NotedType's singular-type pattern. Paired with the
existing jeweled_amulet_notes_and_reads_back_mana_type positive assertion,
this distinguishes the intentional boundary from a parser that simply
fails before reaching either clause.
0fc5c36 to
dd95f1c
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
🔴 Blockers on current head
Reviewed dd95f1c809ed1a9e82663b39c250319e3661ba8b. This is a new review; prior review evidence does not apply to this head.
-
Remove the prohibited dormant-importer mirror.
crates/mtgish-import/src/convert/action.rs:304-319addsManaProduction::NotedTypetorewrite_bound_x_in_mana_production.docs/AI-CONTRIBUTOR.md:91-95explicitly prohibits modifying mtgish paths or mirroring engine variants there. This hunk must be removed or explicitly reworked under maintainer resolution; “consistency” is not a permitted reason to change a dormant importer.I also checked the supplied ancestry claim:
2b204dff59c1e928ab67d4683831deb8b8ff9c15is an ancestor of4555043a1d9c5a1f44235fd95b79588d9e12032f, but it does not contain thisNotedTypearm.git log -Sidentifies4555043a…as the introducing commit. That provenance cannot justify retaining the policy-prohibited hunk. -
Bind source incarnation at payment capture, not later stack construction.
casting.rs:15591-15596records only mana types, whilecasting_costs.rs:5423-5429reads the mutable object's incarnation after cost processing. The residual self-return path atcasting_costs.rs:3181-3266can move the source after automatic payment and before that late read, allowing the payment snapshot to bind to the post-zone-change incarnation.Capture
{ types, source_incarnation }atomically at the payment authority, carry that immutable snapshot through every pause/resume and stack continuation, and add a discriminating self-return-cost regression that proves the old payment cannot bind to the new incarnation. -
The parse-diff sticky is stale. The only
<!-- coverage-parse-diff -->evidence says it was generated for0fc5c36bfd20bf4ce40918d93b8012e31efce1eb, not this head. Current CI is still pending. After it settles, publish and reconcile a fresh parse-diff artifact bound todd95f1c809ed1a9e82663b39c250319e3661ba8bbefore re-review.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer correction
This formal follow-up corrects the prior requested-changes review for unchanged head dd95f1c809ed1a9e82663b39c250319e3661ba8b.
Withdrawn item only: parse_diff_stale_for_current_head is withdrawn. The current parse-diff artifact is bound to this exact head, compares against main 1833435c0d59, and reports exactly one affected card and four signatures for Jeweled Amulet—matching the declared scope.
The requested-changes disposition remains in force for the two substantive blockers: the prohibited dormant mtgish-import mirror and late source-incarnation capture after cost processing. No approval or enqueue is implied by this correction.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved for merge queue.
Evidence: final head 2243c0834e has all required checks green; current-head review threads are empty; and the fresh parse-diff artifact reports the intended Jeweled Amulet change only (noted-mana production plus NoteManaSpent). The decision-cost baseline is refreshed from the exact five-sample CI report after the paired-seed gate passed.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
🔴 Current-head review — approval overridden
Reviewed 2243c0834e0307f4f9c204c73cbb5f580e106e57. CI and the current parse-diff are clean, but they do not clear these material blockers:
-
Prohibited dormant mirror remains.
crates/mtgish-import/src/convert/action.rs:304-319still addsManaProduction::NotedTypeto the mtgish X-rewrite.docs/AI-CONTRIBUTOR.md:91-95forbids modifying dormant mtgish paths or mirroring new engine variants there. Remove this hunk or obtain explicit maintainer resolution for a genuinely required exception. -
Source incarnation is still captured after payment and can rebind across a zone change.
casting.rs:15591-15596stores only paid types;casting_costs.rs:5423-5429later readsobj.incarnation. The self-return residual-cost flow atcasting_costs.rs:3181-3266can move the source between those points, binding the earlier payment to the post-move incarnation.Capture
{ types, source_incarnation }atomically at the payment authority, propagate that snapshot through every pause/resume and stack continuation, and add a discriminating self-return-cost regression proving an old payment cannot bind to a new incarnation.
This changes-requested review supersedes the stale approval. No approval or enqueue pending these fixes.
2 participants
Summary
Fixes #6504. Jeweled Amulet's first ability placed a charge counter but never noted the mana type spent to pay its own activation cost, so the second ability ("Add one mana of this artifact's last noted type") always produced zero mana. Both clauses were falling through to
Effect::Unimplemented.Built as a reusable primitive rather than a card-specific special case, scoped to Jeweled Amulet's exact "note the type of mana spent to pay this activation cost" / "one mana of this artifact's last noted type" wording:
GameObject::mana_spent_to_activate— a transient per-object latch stamped at ability-mana-cost payment time (mirrors the existingcolors_spent_to_castcast-side idiom).ChosenAttribute::NotedManaSpent(Vec<ManaType>)— the durable, engine-set (never player-prompted) noted value, following the same precedent asChosenAttribute::Card/TributeOutcome.Effect::NoteManaSpent— writes the noted value at resolution, not at payment time, so a countered ability never notes anything (CR 608.2c).ManaProduction::NotedType— reads it back at a companion mana ability's resolution, with CR 106.5-correct "no noted type → no mana" behavior.Ice Cauldron's sibling wording ("note the type AND AMOUNT of mana spent...") is intentionally NOT matched. It needs an exact stored multiset plus a spend restriction ("spend this mana only to cast the last card exiled with this artifact"), which this primitive doesn't model — that card's Oracle text still falls through to
Effect::Unimplemented, unchanged from before this PR.CR 400.7 note
Effect::NoteManaSpentrefuses to write unless the source's live incarnation still matches the incarnation captured when its cost was paid, so a source bounced/flickered while its own "note" ability sits unresolved on the stack can't have the departed incarnation's payment silently promoted onto the new one. Mirrors the engine's existing incarnation-pairing idiom (ResolvedAbility::source_is_current,TargetFilter::SelfRefresolution).Test plan
oracle_parser.rs) asserts Jeweled Amulet's Oracle text parses toEffect::NoteManaSpent/ManaProduction::NotedTypewith zeroUnimplementedresidue.issue_6504_jeweled_amulet_noted_mana.rs) drive the full activation → resolution → mana-ability pipeline:cargo fmt --all,cargo clippy --all-targets --workspace -- -D warnings,./scripts/check-parser-combinators.sh,cargo test -p phase-engine(18k+ lib tests, 4.2k+ integration tests),cargo test -p phase-aiall green.Summary by CodeRabbit
New Features
Bug Fixes
Tests