fix(engine): Jeweled Amulet notes and reproduces spent mana type (#6504) - #6812
fix(engine): Jeweled Amulet notes and reproduces spent mana type (#6504)#6812jsdevninja wants to merge 6 commits into
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)
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.
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
| // CR 106.1b + CR 602.2b + CR 400.7: stamp the mana type(s) just spent onto | ||
| // this ability's own source, mirroring `colors_spent_to_cast`'s cast-side | ||
| // stamp-then-read idiom. This is the single authority where an activated | ||
| // ability's mana sub-cost is paid (both the direct-activation and | ||
| // interactive/PendingCast routes funnel through here), so it is where a | ||
| // companion "note the type of mana spent to pay this activation cost" | ||
| // effect (Jeweled Amulet) reads back what was spent — at that effect's | ||
| // OWN resolution, not here. Never cleared: overwritten by this source's | ||
| // next ability-mana-cost payment (see `GameObject::mana_spent_to_activate`). | ||
| // The source's CURRENT incarnation is captured alongside the spent types | ||
| // so the reader can refuse a stale payment from a since-departed | ||
| // incarnation (bounce/flicker while this same ability sits unresolved on | ||
| // the stack) — see `GameObject::mana_spent_to_activate_incarnation`. | ||
| let spent_units = match &payment { | ||
| ManaCostPayment::Paid(units) | ManaCostPayment::Paused { value: units, .. } => units, | ||
| }; | ||
| if let Some(obj) = state.objects.get_mut(&source_id) { | ||
| obj.mana_spent_to_activate = spent_units.iter().map(|unit| unit.color).collect(); | ||
| obj.mana_spent_to_activate_incarnation = obj.incarnation; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
Bind the payment snapshot to the individual activation.
mana_spent_to_activate is overwritten by every payment, while NoteManaSpent later reads it using only the source ID and incarnation. An untap effect can enable a second activation before the first resolves in the same incarnation; the second payment then gets recorded for both abilities. Store the snapshot on the pending activation/stack entry (including its source incarnation) and consume that exact snapshot during resolution.
As per path instructions, reusable engine state must be scoped to the actual activation rather than relying on a card-specific tap-cost 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/game/casting.rs` around lines 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.
Source: Path instructions
| #[test] | ||
| fn jeweled_amulet_bounced_mid_stack_does_not_note_on_new_incarnation() { | ||
| let mut scenario = GameScenario::new(); | ||
| scenario.at_phase(Phase::PreCombatMain); | ||
| let amulet = scenario | ||
| .add_creature_from_oracle(P0, "Jeweled Amulet", 0, 0, JEWELED_AMULET_ORACLE) | ||
| .as_artifact() | ||
| .id(); | ||
| scenario.with_mana_pool( | ||
| P0, | ||
| vec![ManaUnit::new(ManaType::Red, ObjectId(0), false, vec![])], | ||
| ); | ||
| let mut runner = scenario.build(); | ||
|
|
||
| // (1) Activate ability 0. The {1} cost auto-pays from the single floating | ||
| // red unit with no ambiguity, so one action step lands the ability on the | ||
| // stack, unresolved, at the post-announcement Priority window. | ||
| runner | ||
| .act(GameAction::ActivateAbility { | ||
| source_id: amulet, | ||
| ability_index: 0, | ||
| }) | ||
| .expect("activating ability 0 must succeed"); | ||
| assert_eq!( | ||
| runner.state().stack.len(), | ||
| 1, | ||
| "reach-guard: the note ability must be sitting on the stack, unresolved" | ||
| ); | ||
| let incarnation_before = runner.state().objects[&amulet].incarnation; | ||
|
|
||
| // (2) Interject: bounce the amulet through the real bounce resolver | ||
| // (not a raw zone-field flip) before its own ability resolves. | ||
| let bounce_ability = ResolvedAbility::new( | ||
| Effect::Bounce { | ||
| target: TargetFilter::Any, | ||
| destination: None, | ||
| selection: BounceSelection::Targeted, | ||
| }, | ||
| vec![TargetRef::Object(amulet)], | ||
| ObjectId(999), | ||
| P0, | ||
| ); | ||
| let mut events = Vec::new(); | ||
| bounce::resolve(runner.state_mut(), &bounce_ability, &mut events) | ||
| .expect("bouncing the amulet must succeed"); | ||
| assert_eq!( | ||
| runner.state().objects[&amulet].zone, | ||
| Zone::Hand, | ||
| "reach-guard: the amulet must actually be in hand now" | ||
| ); | ||
| assert!( | ||
| runner.state().objects[&amulet].incarnation > incarnation_before, | ||
| "reach-guard: the zone change must have bumped the object's incarnation" | ||
| ); | ||
|
|
||
| // (3) CR 112.7a: the ability is independent of its departed source and | ||
| // still resolves. | ||
| runner.resolve_top(); | ||
| assert!( | ||
| runner.state().stack.is_empty(), | ||
| "the note ability must have resolved off the stack" | ||
| ); | ||
|
|
||
| // (4) The new incarnation, sitting in hand, must have nothing noted. | ||
| assert!( | ||
| runner.state().objects[&amulet].noted_mana_spent().is_none(), | ||
| "a bounced-and-returned amulet must not inherit the departed \ | ||
| incarnation's payment as a noted type" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing pre-bounce reach-guard: prove payment was actually recorded before asserting it wasn't noted.
The final assertion (noted_mana_spent().is_none()) is a negative assertion. The reach-guards present (stack has 1 entry, zone becomes Hand, incarnation increases, stack empties) correctly rule out the "source missing" early-return in note_mana_spent::resolve, leaving the incarnation mismatch as the only remaining explanation — that part is solid. But nothing here proves ability 0's activation actually stamped mana_spent_to_activate/mana_spent_to_activate_incarnation on incarnation_before in the first place. If payment recording silently broke for an unrelated reason, this test would still pass for the wrong reason.
Since a mana-pool query helper already exists in this file (per the added helpers), assert the pool was actually drained by the {1} cost right after activating ability 0 and before the bounce, to prove payment genuinely happened on this activation prior to the incarnation change.
As per path instructions, "For every negative assertion (!detector(...), 'not applied', 'does not parse to X'), require a paired positive reach-guard proving the input actually reached the code under test... An upstream short-circuit makes a negative pass for the wrong reason."
🧪 Suggested reach-guard addition
assert_eq!(
runner.state().stack.len(),
1,
"reach-guard: the note ability must be sitting on the stack, unresolved"
);
+ assert_eq!(
+ mana_pool_total(&runner, P0),
+ 0,
+ "reach-guard: the {{1}} cost must have actually consumed the red mana \
+ before the bounce, proving this activation really paid"
+ );
let incarnation_before = runner.state().objects[&amulet].incarnation;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn jeweled_amulet_bounced_mid_stack_does_not_note_on_new_incarnation() { | |
| let mut scenario = GameScenario::new(); | |
| scenario.at_phase(Phase::PreCombatMain); | |
| let amulet = scenario | |
| .add_creature_from_oracle(P0, "Jeweled Amulet", 0, 0, JEWELED_AMULET_ORACLE) | |
| .as_artifact() | |
| .id(); | |
| scenario.with_mana_pool( | |
| P0, | |
| vec![ManaUnit::new(ManaType::Red, ObjectId(0), false, vec![])], | |
| ); | |
| let mut runner = scenario.build(); | |
| // (1) Activate ability 0. The {1} cost auto-pays from the single floating | |
| // red unit with no ambiguity, so one action step lands the ability on the | |
| // stack, unresolved, at the post-announcement Priority window. | |
| runner | |
| .act(GameAction::ActivateAbility { | |
| source_id: amulet, | |
| ability_index: 0, | |
| }) | |
| .expect("activating ability 0 must succeed"); | |
| assert_eq!( | |
| runner.state().stack.len(), | |
| 1, | |
| "reach-guard: the note ability must be sitting on the stack, unresolved" | |
| ); | |
| let incarnation_before = runner.state().objects[&amulet].incarnation; | |
| // (2) Interject: bounce the amulet through the real bounce resolver | |
| // (not a raw zone-field flip) before its own ability resolves. | |
| let bounce_ability = ResolvedAbility::new( | |
| Effect::Bounce { | |
| target: TargetFilter::Any, | |
| destination: None, | |
| selection: BounceSelection::Targeted, | |
| }, | |
| vec![TargetRef::Object(amulet)], | |
| ObjectId(999), | |
| P0, | |
| ); | |
| let mut events = Vec::new(); | |
| bounce::resolve(runner.state_mut(), &bounce_ability, &mut events) | |
| .expect("bouncing the amulet must succeed"); | |
| assert_eq!( | |
| runner.state().objects[&amulet].zone, | |
| Zone::Hand, | |
| "reach-guard: the amulet must actually be in hand now" | |
| ); | |
| assert!( | |
| runner.state().objects[&amulet].incarnation > incarnation_before, | |
| "reach-guard: the zone change must have bumped the object's incarnation" | |
| ); | |
| // (3) CR 112.7a: the ability is independent of its departed source and | |
| // still resolves. | |
| runner.resolve_top(); | |
| assert!( | |
| runner.state().stack.is_empty(), | |
| "the note ability must have resolved off the stack" | |
| ); | |
| // (4) The new incarnation, sitting in hand, must have nothing noted. | |
| assert!( | |
| runner.state().objects[&amulet].noted_mana_spent().is_none(), | |
| "a bounced-and-returned amulet must not inherit the departed \ | |
| incarnation's payment as a noted type" | |
| ); | |
| } | |
| #[test] | |
| fn jeweled_amulet_bounced_mid_stack_does_not_note_on_new_incarnation() { | |
| let mut scenario = GameScenario::new(); | |
| scenario.at_phase(Phase::PreCombatMain); | |
| let amulet = scenario | |
| .add_creature_from_oracle(P0, "Jeweled Amulet", 0, 0, JEWELED_AMULET_ORACLE) | |
| .as_artifact() | |
| .id(); | |
| scenario.with_mana_pool( | |
| P0, | |
| vec![ManaUnit::new(ManaType::Red, ObjectId(0), false, vec![])], | |
| ); | |
| let mut runner = scenario.build(); | |
| // (1) Activate ability 0. The {1} cost auto-pays from the single floating | |
| // red unit with no ambiguity, so one action step lands the ability on the | |
| // stack, unresolved, at the post-announcement Priority window. | |
| runner | |
| .act(GameAction::ActivateAbility { | |
| source_id: amulet, | |
| ability_index: 0, | |
| }) | |
| .expect("activating ability 0 must succeed"); | |
| assert_eq!( | |
| runner.state().stack.len(), | |
| 1, | |
| "reach-guard: the note ability must be sitting on the stack, unresolved" | |
| ); | |
| assert_eq!( | |
| mana_pool_total(&runner, P0), | |
| 0, | |
| "reach-guard: the {1} cost must have actually consumed the red mana \ | |
| before the bounce, proving this activation really paid" | |
| ); | |
| let incarnation_before = runner.state().objects[&amulet].incarnation; | |
| // (2) Interject: bounce the amulet through the real bounce resolver | |
| // (not a raw zone-field flip) before its own ability resolves. | |
| let bounce_ability = ResolvedAbility::new( | |
| Effect::Bounce { | |
| target: TargetFilter::Any, | |
| destination: None, | |
| selection: BounceSelection::Targeted, | |
| }, | |
| vec![TargetRef::Object(amulet)], | |
| ObjectId(999), | |
| P0, | |
| ); | |
| let mut events = Vec::new(); | |
| bounce::resolve(runner.state_mut(), &bounce_ability, &mut events) | |
| .expect("bouncing the amulet must succeed"); | |
| assert_eq!( | |
| runner.state().objects[&amulet].zone, | |
| Zone::Hand, | |
| "reach-guard: the amulet must actually be in hand now" | |
| ); | |
| assert!( | |
| runner.state().objects[&amulet].incarnation > incarnation_before, | |
| "reach-guard: the zone change must have bumped the object's incarnation" | |
| ); | |
| // (3) CR 112.7a: the ability is independent of its departed source and | |
| // still resolves. | |
| runner.resolve_top(); | |
| assert!( | |
| runner.state().stack.is_empty(), | |
| "the note ability must have resolved off the stack" | |
| ); | |
| // (4) The new incarnation, sitting in hand, must have nothing noted. | |
| assert!( | |
| runner.state().objects[&amulet].noted_mana_spent().is_none(), | |
| "a bounced-and-returned amulet must not inherit the departed \ | |
| incarnation's payment as a noted type" | |
| ); | |
| } |
🤖 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/tests/integration/issue_6504_jeweled_amulet_noted_mana.rs`
around lines 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.
Source: Path instructions
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
|
|
||
| /// CR 707.10: a copy of an activated ability is not itself activated, so it | ||
| /// never paid a mana cost. Copies Jeweled Amulet's first ability (through the | ||
| /// real `copy_spell::resolve` pipeline — same stack-entry lookup and LIFO | ||
| /// resolution a real "copy target activated or triggered ability" card like | ||
| /// Lithoform Engine drives) after the ORIGINAL paid red, and proves the copy | ||
| /// cannot note red (or anything) even though its `ResolvedAbility` chain was | ||
| /// cloned from an original that carried a live `noted_mana_payment` | ||
| /// snapshot. Both PutCounter placements still fire unconditionally (CR | ||
| /// 602.5/608.2c: "Activate only if..." gates ACTIVATION, never re-checked at | ||
| /// resolution, and a copy was never gated by it in the first place), so the | ||
| /// counter count alone can't distinguish correct from buggy behavior here — | ||
| /// only `noted_mana_spent()` can. | ||
| #[test] | ||
| fn jeweled_amulet_copied_activation_does_not_note_original_payment() { | ||
| let mut scenario = GameScenario::new(); | ||
| scenario.at_phase(Phase::PreCombatMain); | ||
| let amulet = scenario | ||
| .add_creature_from_oracle(P0, "Jeweled Amulet", 0, 0, JEWELED_AMULET_ORACLE) | ||
| .as_artifact() | ||
| .id(); | ||
| scenario.with_mana_pool( | ||
| P0, | ||
| vec![ManaUnit::new(ManaType::Red, ObjectId(0), false, vec![])], | ||
| ); | ||
| let mut runner = scenario.build(); | ||
|
|
||
| // (1) Activate ability 0 paying red. On the stack, unresolved. | ||
| runner | ||
| .act(GameAction::ActivateAbility { | ||
| source_id: amulet, | ||
| ability_index: 0, | ||
| }) | ||
| .expect("activating ability 0 must succeed"); | ||
| let original_entry_id = runner | ||
| .state() | ||
| .stack | ||
| .back() | ||
| .expect("reach-guard: the activation must be on the stack") | ||
| .id; | ||
|
|
||
| // (2) Copy it — mirrors what Lithoform Engine's "{2}, {T}: Copy target | ||
| // activated or triggered ability you control" drives through the real | ||
| // engine pipeline, minus the copying permanent's own stack presence | ||
| // (the mechanism under test, `copy_spell::resolve` and its | ||
| // `preserve_ability_copy_source_recursive` call, is identical either | ||
| // way — this is the same direct-resolver-call idiom already used above | ||
| // for `bounce::resolve`). | ||
| let copy_ability = ResolvedAbility::new( | ||
| Effect::CopySpell { | ||
| target: TargetFilter::StackAbility { | ||
| controller: None, | ||
| tag: None, | ||
| kind: None, | ||
| }, | ||
| retarget: CopyRetargetPermission::KeepOriginalTargets, | ||
| copier: None, | ||
| additional_modifications: Vec::new(), | ||
| starting_loyalty_from_casualty_sacrifice: false, | ||
| }, | ||
| vec![TargetRef::Object(original_entry_id)], | ||
| ObjectId(999), | ||
| P0, | ||
| ); | ||
| let mut events = Vec::new(); | ||
| copy_spell::resolve(runner.state_mut(), ©_ability, &mut events) | ||
| .expect("copying the activation must succeed"); | ||
| assert_eq!( | ||
| runner.state().stack.len(), | ||
| 2, | ||
| "reach-guard: original + copy must both be on the stack" | ||
| ); | ||
|
|
||
| // (3) LIFO: the copy resolves first. | ||
| runner.resolve_top(); | ||
| assert_eq!( | ||
| runner.state().stack.len(), | ||
| 1, | ||
| "reach-guard: exactly the original must remain" | ||
| ); | ||
| assert_eq!( | ||
| charge_counters(&runner, amulet), | ||
| 1, | ||
| "the copy's PutCounter still fires unconditionally" | ||
| ); | ||
| assert!( | ||
| runner.state().objects[&amulet].noted_mana_spent().is_none(), | ||
| "CR 707.10: the copy never paid a mana cost, so its NoteManaSpent \ | ||
| must not note the original's (red) payment" | ||
| ); | ||
|
|
||
| // (4) The original resolves second — it DID pay, so it must still note | ||
| // correctly. This is the paired positive reach-guard proving the fix | ||
| // clears only the COPY's snapshot, not the original's. | ||
| runner.resolve_top(); | ||
| assert!(runner.state().stack.is_empty()); | ||
| assert_eq!( | ||
| charge_counters(&runner, amulet), | ||
| 2, | ||
| "the original's PutCounter also fires (a second charge counter — \ | ||
| CR 602.5: the no-counters restriction gates activation, not \ | ||
| resolution, and was never re-checked for the copy either)" | ||
| ); | ||
| assert_eq!( | ||
| runner.state().objects[&amulet].noted_mana_spent(), | ||
| Some([ManaType::Red].as_slice()), | ||
| "the original activation must still note its own (red) payment" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# Map the test file and locate relevant symbols/usages.
ast-grep outline crates/engine/tests/integration/issue_6504_jeweled_amulet_noted_mana.rs --view expanded
rg -n "noted_mana_payment|noted_mana_spent|ResolvedAbility|StackEntry|ability\(\)|stack\.iter\(\)|stack\.back\(\)" crates/engine -g '!target'Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
# Read the surrounding section of the integration test with line numbers.
sed -n '487,595p' crates/engine/tests/integration/issue_6504_jeweled_amulet_noted_mana.rs
# Find the definitions of the relevant types and accessors.
rg -n "struct ResolvedAbility|enum StackEntry|fn ability\(|noted_mana_payment|noted_mana_spent" crates/engine/src crates/engine/tests -g '!target'Repository: phase-rs/phase
Length of output: 11542
Add a pre-copy reach-guard for the original noted-mana snapshot. Assert the original stack entry still has noted_mana_payment before copy_spell::resolve; otherwise the is_none() check can pass even if the copy never reaches NoteManaSpent.
🤖 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/tests/integration/issue_6504_jeweled_amulet_noted_mana.rs`
around lines 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.
Source: Path instructions
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 Would you please check this CI? Not sure if this is relaetd to this PR |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the intentionally unsupported Ice Cauldron boundary is not covered through the production Oracle parser.
🔴 Blocker
[MED] crates/engine/tests/integration/oracle_parser.rs:1009-1035 exercises a fabricated “Amber Amulet” grammar sibling, while crates/engine/src/parser/oracle_effect/imperative.rs:13546-13558 tests parse_note_mana_spent_clause directly. Neither path parses Ice Cauldron’s complete Oracle ability. The change deliberately excludes Ice Cauldron’s “type and amount” clause, but its unsupported status is therefore not protected against an upstream routing/fallback change that could make the coverage report treat it as supported or partially supported.
Please add a production-parser assertion for Ice Cauldron’s exact full Oracle text that positively reaches the relevant ability and asserts Effect::Unimplemented, alongside a positive full-Oracle Jeweled Amulet assertion that reaches Effect::NoteManaSpent. That paired guard distinguishes the intentional boundary from a parser that simply fails before reaching either clause.
Recommendation: request changes for the end-to-end coverage-honesty guard, then re-run the parser evidence on the new head.
|
Thanks for flagging this. The failed job did run against this PR's merge commit ( The concrete failure is This PR can still affect the AI duel trajectory through its engine/card-data changes, so the exact source of the static-gate increase is unresolved. Please rebase onto current |
…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.
Round-4 review: the "note the type of mana spent to pay this activation
cost" recognizer was a single sentence-shaped tag() — a verbatim match on
Jeweled Amulet's exact printed wording despite using a real nom combinator,
so any grammatical sibling (shorter "this cost", articleless "note type")
would need its own new whole-sentence arm.
Split into three independently typed grammar pieces composed with
parse_note_mana_spent_clause: parse_note_instruction_prefix (instruction +
optional article), parse_noted_mana_subject (scoped to the singular-type
wording; Ice Cauldron's "type AND AMOUNT" subject stays deliberately
unmatched), and parse_activation_cost_referent ("this activation cost" vs.
the grammatically shorter "this cost", both CR 602.2b's activation cost).
Added building-block tests for each accepted structural variant plus a
negative boundary (Ice Cauldron's type-and-amount subject correctly
rejected), and an end-to-end integration test exercising a hypothetical
sibling wording through the full Oracle-text pipeline to confirm the
grammar composes for real, not just at the unit level.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
3 verified findings block merge: activation-scoped mana provenance can be rebound after a source zone move, and the parser/test boundaries do not yet prove their failure paths.
🔴 Blocker
[MED] The noted-mana snapshot records the source incarnation after later activation costs, rather than atomically with the mana payment. Evidence: crates/engine/src/game/casting.rs:14769-14774 latches only mana colors, while crates/engine/src/game/casting_costs.rs:5358-5378 later builds NotedManaPayment with the object's then-current incarnation. Why it matters: a source-zone-changing residual cost can bump the object’s incarnation between those steps, letting Effect::NoteManaSpent accept the new incarnation instead of rejecting the departed payer under CR 400.7. Suggested fix: latch both types and source incarnation at the mana-payment authority, carry that snapshot through any pending activation continuation, and add a mana-plus-source-zone-changing-cost regression.
[MED] Ice Cauldron’s strict-failure boundary is tested only at the clause helper, not through the production Oracle pipeline. Evidence: crates/engine/src/parser/oracle_effect/imperative.rs:13618-13630 checks helper rejection, while crates/engine/tests/integration/oracle_parser.rs:939-1035 exercises complete Jeweled Amulet and a positive hypothetical sibling but not complete Ice Cauldron. Why it matters: a dispatcher or lowering regression could classify the unsupported real card as supported while the helper-only test remains green. Suggested fix: parse Ice Cauldron’s complete Oracle text and assert its type-and-amount clause remains Effect::unimplemented, retaining the positive Jeweled Amulet reach guard.
🟡 Non-blocking
[LOW] The bounce and copied-activation regressions do not prove that the original stack ability has a noted_mana_payment snapshot before their negative assertions. Evidence: crates/engine/tests/integration/issue_6504_jeweled_amulet_noted_mana.rs:227-281 and :514-575 assert stack/zone/copy outcomes but never inspect that snapshot before bounce or copy. Why it matters: either test can pass if the payment-to-stack bridge regresses. Suggested fix: assert the original activated stack entry carries the expected red snapshot immediately before the bounce/copy action.
Current parser evidence is stale: the parse-diff sticky names prior head 11112a640a69bc68b9b9cbec8143ef22f3b348cb, not this review head; current Rust/card-data checks are also pending. These are additional hold evidence, not substitutes for the findings above.
Recommendation: request changes. Please correct the provenance binding and add the production parser and snapshot reach-guards, then regenerate current-head parse-diff evidence and re-request review.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — current head 87960bf2667487a4a008324df0e5e7a521a9cee5 still has unresolved correctness and scope blockers.
[HIGH] The diff edits the dormant mtgish-import conversion path. Evidence: crates/mtgish-import/src/convert/action.rs:304-319 adds ManaProduction::NotedType solely to mirror the live engine enum. Why it matters: this repository’s contributor policy explicitly excludes mtgish/ and crates/mtgish-import/ from the runtime pipeline; mirroring live engine variants there creates a parallel, unsupported consumer. Suggested fix: remove this hunk; keep the live behavior in the MTGJSON → engine parser → card-data pipeline.
[MED] The snapshot’s source incarnation is still bound after payment, rather than atomically with the paid mana types. Evidence: crates/engine/src/game/casting.rs:14769-14774 latches only types; crates/engine/src/game/casting_costs.rs:5372-5378 later reads obj.incarnation to construct NotedManaPayment. Why it matters: a source-zone-changing residual activation cost between those steps can rebind the old payment to the new incarnation, defeating the CR 400.7 guard in effects/note_mana_spent.rs:52-60. Suggested fix: capture { types, source_incarnation } together at the mana-payment authority and thread that immutable value through the pending payment flow.
[MED] The intentionally unsupported Ice Cauldron boundary still lacks a production-parser regression. Evidence: crates/engine/src/parser/oracle_effect/imperative.rs:13618-13630 only rejects its clause helper; crates/engine/tests/integration/oracle_parser.rs:939-1035 parses Jeweled Amulet and a fabricated sibling, not Ice Cauldron’s full Oracle ability. Why it matters: a routing/lowering change could falsely mark the real type-and-amount card supported while these tests remain green. Suggested fix: parse Ice Cauldron’s complete Oracle text, positively reach its relevant ability, and assert an Effect::Unimplemented marker alongside the Jeweled Amulet positive guard.
[LOW] The bounce/copy negative tests do not prove the original ability captured a noted-mana snapshot before exercising the negative path. Evidence: crates/engine/tests/integration/issue_6504_jeweled_amulet_noted_mana.rs:214-282 and :487-575 lack an assertion over the original stack entry’s noted_mana_payment. Why it matters: the negative outcomes can pass if the payment-to-stack bridge regresses. Suggested fix: assert the expected original snapshot immediately before bounce/copy.
The parse-diff sticky comment predates this head (comment at 2026-07-30T10:48Z; current head commit at 2026-07-31T12:49-05:00), and the Paired-seed AI / Decision-cost perf gates are still in progress. Do not approve, enable auto-merge, or enqueue until a corrected head has fresh parser evidence and terminal required checks.
Recommendation: request changes.
…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.
87960bf to
af6766f
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head review — af6766f9663c1c0e4861b0df5c4ba94b894b2f02
[HIGH] Remove the mtgish-import mirror update. The added ManaProduction::NotedType arm in crates/mtgish-import/src/convert/action.rs:304-319 changes the dormant importer. Repository policy explicitly says that crate must not be modified or mirrored when extending engine variants; this creates a second, unsupported surface for the production ManaProduction model. Keep the implementation confined to the live engine/parser path.
[MED] Add the missing reach guards to the zone-change and copy regressions. The bounce test at crates/engine/tests/integration/issue_6504_jeweled_amulet_noted_mana.rs:214-282 never proves its red mana was actually spent before it performs the bounce, and the copy test at :501-595 never inspects the original stack entry to prove it carried the red noted_mana_payment snapshot before copying. Each test's final outcome can therefore pass without exercising the boundary it claims to protect. Assert the empty mana pool immediately after activation, and assert the original entry's snapshot before invoking copy_spell::resolve.
The new Ice Cauldron production-parser test is a good correction to the prior coverage gap; I found no remaining issue with that boundary.
Hold evidence
./scripts/check-parser-combinators.shpasses at this head (including Gate A).- The parse-diff sticky artifact is still for head
0f6bfbf23e59e5c7d09b0211281b65a6d52fcff7, not this head. - Required Rust/AI/card-data/performance checks are still in progress, and GitHub currently reports
BEHIND.
Please address the findings, rebase if required, and let the current-head checks plus a fresh parse-diff artifact settle before requesting re-review.
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