feat(engine): observe Saga chapter-ability lifecycle so Narci can drain - #7224
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:
📝 WalkthroughWalkthroughThe engine now supports final Saga chapter triggers at trigger and resolution time. It records chapter provenance, emits successful-resolution events, matches related triggers, parses Saga-scoped references, updates event handling, and adds integration tests. ChangesSaga chapter trigger support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GameState
participant resolve_top
participant GameEvent
participant TriggerMatcher
GameState->>resolve_top: Resolve Saga chapter ability
resolve_top->>resolve_top: Capture chapter metadata
resolve_top->>GameEvent: Emit SagaChapterAbilityResolved after success
GameEvent->>TriggerMatcher: Match resolved chapter triggers
TriggerMatcher->>GameState: Create matching triggered abilities
Possibly related PRs
🚥 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.
Request changes — the new trigger family currently models an unsupported scope with incorrect multiplicity, and its CR annotations cite rules that do not state the claimed behavior.
🔴 Blocker
[HIGH] SagaChapterScope::Any is a speculative, incorrect branch. Evidence: crates/engine/src/types/triggers.rs:227-234, crates/engine/src/parser/oracle_trigger.rs:17747-17748, and crates/engine/src/game/trigger_matchers.rs:2313-2321. The Scryfall Oracle search for o:"chapter ability" returns exactly Historian's Boon, Narci, and Tom Bombadil, and every one says "the final chapter ability". More importantly, the matcher itself acknowledges that a multi-lore event crossing multiple thresholds only fires once. CR 714.2b makes each chapter symbol a separate triggered ability, so an Any + Triggered observer must fire once per crossed chapter ability, not once per CounterAdded event. Why it matters: the new public enum/parser grammar advertises support for a rules-incorrect branch that has no current card and cannot be repaired by a boolean matcher. Suggested fix: keep the proven class to Final plus the lifecycle axis, or first introduce an occurrence-level chapter-trigger event that preserves one firing per chapter ability before accepting an unqualified chapter-ability grammar.
[HIGH] The new Saga documentation repeatedly cites CR 714.2a and CR 714.4 for claims those rules do not make. Evidence: crates/engine/src/types/triggers.rs:221-241, crates/engine/src/game/stack.rs:991-1028, crates/engine/src/game/trigger_matchers.rs:2227-2324, and 23 new CR 714.2a annotations in this diff. The official Comprehensive Rules say 714.2a only defines the Roman numeral; 714.2b defines the lore-counter threshold trigger, 714.2d defines the final chapter number, and 714.2e defines the final chapter ability. CR 714.4 is the Saga-sacrifice state-based action, not the definition of a final chapter ability. Why it matters: these annotations are the repository's rules-correctness evidence, and the current citations point maintainers to the wrong authority throughout the new primitive. Suggested fix: audit every new Saga citation against the applicable 714.2b/d/e or 714.4 text and retain 714.4 only where the actual SBA timing is being described.
✅ Clean
The dedicated SagaChapterAbilityResolved event is at the appropriate runtime seam for the resolution half, and the Narci integration test drives the production stack/SBA/LKI path rather than only asserting parser shape.
Recommendation: request changes to narrow or correctly model the chapter-occurrence axis and to correct the verified CR annotations; then regenerate the current-head parse-diff and let the pending CI and CodeRabbit pass settle.
|
Generated for head Parse changes introduced by this PR · 3 card(s), 6 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/game_object.rs`:
- Around line 2914-2969: Model chapter abilities as typed occurrences backed by
preserved chapter-symbol provenance rather than inferring them from generic Lore
thresholds. In crates/engine/src/game/game_object.rs lines 2914-2969, derive
chapter numbers and final-chapter status only from that provenance; in
crates/engine/src/game/trigger_matchers.rs lines 2298-2324, emit and match one
lifecycle occurrence per distinct chapter ability without using .any() as
identity; and in crates/engine/src/game/stack.rs lines 1011-1028, classify
resolved abilities from the typed provenance. Add integration coverage for two
distinct final-chapter abilities and a nonchapter Lore-counter trigger.
In `@crates/engine/src/game/stack.rs`:
- Around line 1018-1028: Preserve the triggering Saga incarnation in the
chapter-trigger flow instead of resolving only by source_id. Capture the
ObjectIncarnationRef and required LKI when the trigger is added to the stack,
carry that identity through ResolvingSagaChapter and SagaChapterAbilityResolved,
and use it as the emitted EventSource so a re-entered Saga cannot replace the
original. Add a regression test covering blink before chapter ability
resolution.
- Around line 2541-2554: Move Saga chapter metadata onto the resolution carrier
used by resolve_top and remove the immediate SagaChapterAbilityResolved emission
from the pre-settlement block. Emit the event only from the terminal resolved
path after the settlement guard confirms all continuations and in-resolution
choices have completed, preserving the existing chapter fields. Add a test
covering an in-resolution choice and verify observers trigger only after that
choice resolves.
In `@crates/engine/src/parser/oracle_trigger.rs`:
- Around line 9228-9236: The comments associated with
try_parse_saga_chapter_ability_trigger and the corresponding Saga trigger
handling incorrectly cite CR 714.2a. Update those comments to cite CR 714.2 for
chapter abilities, CR 714.2d for the final chapter number, and CR 714.2e for the
final chapter ability, including the related comments in the later Saga trigger
section.
In `@crates/engine/src/types/triggers.rs`:
- Around line 627-640: Preserve multiplicity for SagaChapterScope::Any through
the trigger lifecycle: update the crossed-chapter matching and enqueueing flow
so each crossed chapter ability produces its own observer trigger instead of
collapsing the collection with any(crosses). Carry the specific chapter identity
or equivalent multiplicity until trigger creation, and only construct Any after
the matcher can emit one trigger per crossed ability; implement this generically
for all cards using this scope.
🪄 Autofix
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: a0cf843d-73a1-4737-839c-483ba8d16cf2
📒 Files selected for processing (17)
client/src/adapter/types.tsclient/src/animation/eventNormalizer.tscrates/engine/src/ai_support/shortcut_efficacy.rscrates/engine/src/analysis/ability_graph.rscrates/engine/src/game/game_object.rscrates/engine/src/game/log.rscrates/engine/src/game/public_state.rscrates/engine/src/game/stack.rscrates/engine/src/game/targeting.rscrates/engine/src/game/trigger_index.rscrates/engine/src/game/trigger_matchers.rscrates/engine/src/parser/oracle_nom/quantity.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/src/types/events.rscrates/engine/src/types/triggers.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/narci_fable_singer_final_chapter_drain.rs
| // CR 714.2a + CR 608.2: This is the only exit from `resolve_top` on which a | ||
| // triggered ability actually RESOLVED — the fizzle, no-legal-target and | ||
| // failed-intervening-if paths returned earlier, each pushing their own | ||
| // `StackResolved`. Publishing the chapter-resolution event only here is what | ||
| // keeps "whenever the final chapter ability of a Saga you control resolves" | ||
| // (Narci, Fable Singer) from firing on a chapter ability that never did. | ||
| if let Some(chapter) = saga_chapter { | ||
| events.push(GameEvent::SagaChapterAbilityResolved { | ||
| saga_id: chapter.saga_id, | ||
| controller: chapter.controller, | ||
| chapter: chapter.chapter, | ||
| final_chapter: chapter.final_chapter, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Publish the resolved event only after resolution settles.
Lines 2547-2554 publish SagaChapterAbilityResolved before the settlement guard at lines 2559-2561. If a chapter ability opens an in-resolution choice or continuation, observers can trigger while that chapter ability is still resolving.
Store the Saga metadata on the resolution carrier. Emit this event from the terminal resolved path after all continuations complete. Add a test that confirms an observer does not trigger until an in-resolution choice completes.
🤖 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/stack.rs` around lines 2541 - 2554, Move Saga chapter
metadata onto the resolution carrier used by resolve_top and remove the
immediate SagaChapterAbilityResolved emission from the pre-settlement block.
Emit the event only from the terminal resolved path after the settlement guard
confirms all continuations and in-resolution choices have completed, preserving
the existing chapter fields. Add a test covering an in-resolution choice and
verify observers trigger only after that choice resolves.
| /// CR 714.2a + CR 714.4: a meta-trigger on another permanent's Saga chapter | ||
| /// abilities — "whenever the final chapter ability of a Saga you control | ||
| /// resolves" (Narci, Fable Singer; Tom Bombadil) / "… triggers" | ||
| /// (Historian's Boon). Parameterized on the two independent axes the class | ||
| /// varies over: WHICH chapter ability is observed (`chapter`) and WHICH | ||
| /// point of that ability's lifecycle fires this trigger (`lifecycle`). | ||
| /// | ||
| /// The Saga itself is constrained by the trigger's ordinary `valid_card` | ||
| /// filter ("a Saga you control"), so no Saga-specific filter axis is needed | ||
| /// here. | ||
| SagaChapterAbility { | ||
| chapter: SagaChapterScope, | ||
| lifecycle: AbilityLifecyclePoint, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve one firing per crossed chapter ability for SagaChapterScope::Any.
SagaChapterScope::Any exposes a mode that must observe each chapter ability. The downstream matcher reduces crossed chapter numbers to any(crosses), so one lore-counter addition that crosses multiple thresholds creates only one observer trigger. CR 714.2c requires each crossed chapter ability to trigger.
Carry chapter-ability identity or multiplicity through the triggered lifecycle path. Do not construct Any until the matcher can enqueue one trigger per crossed ability.
As per path instructions: new capabilities must handle a class of cards, not one special case.
🤖 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/triggers.rs` around lines 627 - 640, Preserve
multiplicity for SagaChapterScope::Any through the trigger lifecycle: update the
crossed-chapter matching and enqueueing flow so each crossed chapter ability
produces its own observer trigger instead of collapsing the collection with
any(crosses). Carry the specific chapter identity or equivalent multiplicity
until trigger creation, and only construct Any after the matcher can emit one
trigger per crossed ability; implement this generically for all cards using this
scope.
Source: Path instructions
|
Correction to the current-head request-changes review — the original blockers remain, and this replaces its earlier clean assessment of the runtime resolution seam. 🔴 Blocker[HIGH] The resolved-lifecycle event is published before the chapter ability has settled. Evidence: [HIGH] The event derives chapter identity from a live The original Recommendation: retain request changes; address all four blockers, regenerate the current-head parse diff, then re-review. |
…er scope Addresses review on phase-rs#7224. Both blockers were real. Verified every citation against docs/MagicCompRules.txt, which had never been fetched in this worktree -- the original annotations were written from memory, which is exactly what CLAUDE.md's verification rule exists to prevent. The rules text says: 714.2 a chapter symbol is a keyword ability representing a triggered ability referred to as a chapter ability 714.2a defines ONLY the Roman numeral 714.2b "{rN}--[Effect]" means "When one or more lore counters are put onto this Saga, if the number of lore counters on it was less than N and became at least N, [effect]" 714.2d final chapter NUMBER is the greatest among a Saga's chapter abilities 714.2e final chapter ABILITY is the one carrying that number 714.4 the sacrifice state-based action So 714.2a was wrong everywhere it was used for the threshold-trigger claim, and 714.4 was wrong everywhere it was used to define the final chapter ability. Each citation is now the rule that actually states the claim. The same audit covers four PRE-EXISTING 714.2a citations on the shared counter-filter path (CounterTriggerFilter, match_counter_added and its CR 310.12b mirror), which make the identical error about the identical rule and sit directly on the code this change reads; leaving them would have re-earned the same review note. Removed SagaChapterScope. Its Any variant was speculative -- no printed card uses an unqualified "a chapter ability" -- and, worse, unmodelable here: CR 714.2b makes each chapter symbol its own triggered ability, so one lore-counter addition crossing several chapter numbers triggers that many chapter abilities. An observer of all of them owes one firing per crossed ability, which an event-keyed matcher reading a single CounterAdded cannot express. The original commit documented that shortfall in a comment instead of refusing it, which advertised a rules-incorrect branch through a public enum and parser grammar. The mode is now TriggerMode::FinalSagaChapterAbility { lifecycle }, the parser requires "final", and a new test pins the refusal: an unqualified clause stays Unknown (honestly coverage-red) rather than minting a trigger that under-fires. Restoring the scope needs an occurrence-level chapter event first, not a wider enum -- recorded on the variant. Also fail closed on an incarnation mismatch when classifying a resolving chapter ability (CR 400.7). source_id is storage identity: a Saga that left and re-entered can occupy the same id as a new object whose chapter numbers are not the ones this ability triggered from, and publishing those would misclassify a non-final chapter as final or drain for the wrong mana value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both blockers were real. Fixed in 🔴 CR annotations cite rules that don't state the claim — fixedYou were right, and the root cause is worse than a slip: Fetched the rules and audited every one. The text:
So The audit also covers four pre-existing 🔴
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — preserve the Saga incarnation for the resolved event
The final-chapter-only scope and parser refusal for the unqualified wording address the prior multiplicity issue. However, the resolved-lifecycle event still loses the identity of the Saga whose old chapter ability is resolving.
🔴 Blocker — stale Saga source is suppressed or misidentified
resolving_saga_chapter returns no lifecycle payload when definition_ref.source is no longer current (crates/engine/src/game/stack.rs:1018-1029). A Saga can leave and re-enter after its chapter ability has been put on the stack, while that old chapter ability still resolves independently. In that case no SagaChapterAbilityResolved event is emitted, so Narci/Tom cannot observe the resolution.
Simply removing the guard would not be sufficient: SagaChapterAbilityResolved currently carries only saga_id (crates/engine/src/types/events.rs:1053-1076), and the resolved matcher plus ObjectScope::EventSource dereference that raw ID (crates/engine/src/game/trigger_matchers.rs:2261-2271, crates/engine/src/game/targeting.rs:1569-1572). After re-entry the same ID names the new incarnation, so that Saga — including its mana value — could be evaluated against the wrong object.
Please capture and carry the original Saga's exact identity and event-subject facts when the chapter trigger is created, then match and resolve that Saga from that exact snapshot. The existing EventObjectSnapshot event pattern (crates/engine/src/types/events.rs:311-355, and the event subject on EffectResolved) plus matches_target_filter_on_event_snapshot is the established seam; a fail-closed raw-ID guard is not an equivalent replacement.
Add an integration test through the production stack path where a final chapter ability is put on the stack, the Saga leaves and re-enters before that old ability resolves, and the observer fires once for the original Saga with the original mana-value referent.
I also checked the current-head parse diff (three claimed cards only), full review/thread feedback, the updated final-only parser path, event serialization coverage, and the resolution-completion gates. The prior broad-scope concern is resolved; the blocker above remains on 1f191c6f3960a46eac222bc8e8304faa0cc175ca.
Addresses the two review findings previously deferred on phase-rs#7224. Chapter abilities were being recognized by shape: a lore-counter threshold on a Saga. CR 714.2b does give a chapter symbol that shape, but the converse does not hold -- a lore threshold trigger a Saga acquired some other way is not a chapter ability, and counting it corrupts the final chapter number CR 714.2d defines and CR 714.4's sacrifice depends on. TriggerDefinition now carries `saga_chapter: Option<u32>`, the chapter symbol's Roman numeral, set only by the Saga parser -- the one place that has read an actual chapter symbol. `saga_chapter_numbers`, `saga_chapter_for_occurrence` and read-ahead's CR 714.2d derivation in synthesis all read that provenance instead of re-deriving from thresholds, so there is one model rather than three. Keying `saga_chapter_for_occurrence` on the occurrence keeps CR 714.2c's two chapter abilities printed on one line distinct; the History of Benalia IR snapshot shows that directly ("I, II --" now yields saga_chapter 1 and 2). Hand-built Saga fixtures in sba.rs, synthesis.rs and game_object.rs must now declare the provenance too. That is the point: a bare lore threshold no longer passes for a chapter ability anywhere. The second finding -- that publishing the resolution event before the settlement guard lets observers fire while a chapter ability is still resolving -- is answered with the test the review asked for rather than a refactor. `narci_does_not_drain_until_a_paused_final_chapter_finishes_resolving` gives the final chapter an optional-effect choice, asserts the drain has NOT landed while that prompt is open, and asserts it lands exactly once after it is answered. A `saw_optional` reach guard fails the test if the pause never happens, so it cannot pass vacuously. The ordering holds because a pending trigger cannot reach the stack until a player would receive priority, which follows completion of the in-resolution choice. Moving the emission would have required threading an event sink through `settle_finished_resolving_stack_entry` and every parked-resolution settle path, changing where the engine considers a resolution finished for `StackResolved` too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/engine/src/database/synthesis.rs (1)
8997-9002: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for untagged Lore thresholds.
All supplied fixtures call
.saga_chapter(n). The positive tests can therefore pass if the implementation still derives the final chapter fromCounterTriggerFilter.threshold. Add an untagged Lore threshold greater than the real final chapter. Assert that Read Ahead uses the tagged maximum and that SBA sacrifices the Saga at the tagged final chapter. Add positive reach guards for the keyword, trigger, and live Saga. The Comprehensive Rules define the final chapter number from chapter abilities. (media.wizards.com)
crates/engine/src/database/synthesis.rs#L8997-L9002: test a tagged chapter 3 plus an untagged Lore threshold 99; assert the Read Ahead maximum remains 3.crates/engine/src/database/synthesis.rs#L22938-L22941: add a sibling fixture path that preserves an untagged Lore threshold.crates/engine/src/game/sba.rs#L4350-L4360: test a tagged final chapter 3 plus an untagged threshold 99; assert sacrifice occurs at 3.As per path instructions: “A test must exercise the FAILURE path the fix prevents,” and negative assertions require paired positive reach guards.
🤖 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/database/synthesis.rs` around lines 8997 - 9002, Add regression coverage in crates/engine/src/database/synthesis.rs:8997-9002 using a tagged chapter 3 and an untagged Lore threshold 99, with positive reach guards for the keyword, trigger, and live Saga, and assert Read Ahead uses maximum 3; update the sibling fixture path in crates/engine/src/database/synthesis.rs:22938-22941 to preserve the untagged threshold. In crates/engine/src/game/sba.rs:4350-4360, add the same tagged/untagged setup and positive reach guards, asserting SBA sacrifices the Saga at chapter 3 rather than 99.Sources: Path instructions, MCP tools
🤖 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/narci_fable_singer_final_chapter_drain.rs`:
- Around line 211-218: Update the CR citation in the comment above the final
chapter resolution test: replace CR 608.2c with citations to CR 608.2d for the
mid-resolution choice and CR 608.2p for the post-resolution observer trigger,
while preserving the existing test-ordering explanation.
---
Nitpick comments:
In `@crates/engine/src/database/synthesis.rs`:
- Around line 8997-9002: Add regression coverage in
crates/engine/src/database/synthesis.rs:8997-9002 using a tagged chapter 3 and
an untagged Lore threshold 99, with positive reach guards for the keyword,
trigger, and live Saga, and assert Read Ahead uses maximum 3; update the sibling
fixture path in crates/engine/src/database/synthesis.rs:22938-22941 to preserve
the untagged threshold. In crates/engine/src/game/sba.rs:4350-4360, add the same
tagged/untagged setup and positive reach guards, asserting SBA sacrifices the
Saga at chapter 3 rather than 99.
🪄 Autofix
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: b87789b9-ba2e-4d7d-87ed-0d9dc23cefb2
⛔ Files ignored due to path filters (3)
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__history_of_benalia_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__history_of_benalia_lowered.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/snapshots/engine__parser__oracle__pipeline_snapshot_tests__pipeline_saga_card.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (6)
crates/engine/src/database/synthesis.rscrates/engine/src/game/game_object.rscrates/engine/src/game/sba.rscrates/engine/src/parser/oracle_saga.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/narci_fable_singer_final_chapter_drain.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/game/game_object.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the current head still suppresses a successfully resolving old Saga chapter ability, and its new provenance field leaves a pre-existing fixture red.
🔴 Blocker
[HIGH] The resolved lifecycle still cannot represent the Saga incarnation whose chapter ability actually resolved. Evidence: crates/engine/src/game/stack.rs:1018-1036 reads the live object and returns None when definition_ref.source.is_current(state) is false; crates/engine/src/game/stack.rs:2555-2561 consequently emits no SagaChapterAbilityResolved event. A chapter ability already on the stack remains capable of resolving after its Saga leaves and re-enters, so the resolves observer silently misses that occurrence. Removing the guard alone would be wrong as well: crates/engine/src/types/events.rs:1070-1080 carries only saga_id, and crates/engine/src/game/targeting.rs:1568-1572 turns that raw storage id into EventSource, which can select the new incarnation. Why it matters: Narci's that Saga's mana value must bind to the original Saga, not suppress the trigger or read the re-entered object's mana value. Suggested fix: capture the original ObjectIncarnationRef and an EventObjectSnapshot before resolution, carry them through the resolution completion path, and have the event matcher plus EventSource quantity resolution consume that snapshot. Add a production-stack regression where the final Saga leaves and re-enters before its old chapter ability resolves, with a different new-incarnation mana value; the observer must fire exactly once using the old value.
🟡 Non-blocking
[MED] The current CI failure is an unupdated fixture introduced before this PR. Evidence: CI shard 2 fails manabrew_compat::tests::card_dto_uses_engine_supplied_saga_and_class_state at crates/manabrew-compat/src/lib.rs:6761 (None vs Some(3)); that test's two manually built chapter triggers at crates/manabrew-compat/src/lib.rs:6737-6748 lack the new saga_chapter provenance, while GameObject::final_chapter_number now intentionally reads only that provenance at crates/engine/src/game/game_object.rs:2928-2942. The test was introduced by ancestor commit 2b3510ef, so this is integration fallout of the current change, not main drift. Suggested fix: mark those two fixture triggers with .saga_chapter(1) and .saga_chapter(3) while adding the untagged-Lore regression suggested by the current CodeRabbit feedback.
The current CodeRabbit inline comment on the paused-resolution test's CR citation also remains open; resolve or explicitly refute it against the repository's authoritative rules source while updating this area.
Recommendation: request changes for an incarnation-bound event subject and its end-to-end blink regression; repair the CI fixture in the same update, then regenerate the parse diff and rerun the current-head checks.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/manabrew-compat/src/lib.rs (1)
6737-6752: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a non-provenance control to this fixture.
All triggers in this fixture carry
saga_chapter. A regression that derivesfinal_chapter_numberfrom lore thresholds would still returnSome(3), so this test would pass while the provenance contract is broken. Add a bare lore-counter trigger with a threshold greater than 3 and keep the expected final chapter atSome(3).As per path instructions, Saga chapter provenance must be explicit and must not be inferred solely from lore-counter thresholds. The engine contract is implemented in
crates/engine/src/game/game_object.rs:2915-2974.Suggested test adjustment
TriggerDefinition::new(TriggerMode::CounterAdded) .counter_filter(CounterTriggerFilter { counter_type: CounterType::Lore, threshold: Some(3), }) .saga_chapter(3), + // This threshold is not a chapter symbol. + TriggerDefinition::new(TriggerMode::CounterAdded) + .counter_filter(CounterTriggerFilter { + counter_type: CounterType::Lore, + threshold: Some(99), + }),🤖 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/manabrew-compat/src/lib.rs` around lines 6737 - 6752, Add a third lore CounterAdded trigger to the saga.trigger_definitions fixture without calling saga_chapter, using a threshold greater than 3. Keep the existing provenance-bearing triggers and the expected final_chapter_number assertion at Some(3), ensuring the fixture verifies provenance is not inferred from lore thresholds.Source: Path instructions
🤖 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.
Nitpick comments:
In `@crates/manabrew-compat/src/lib.rs`:
- Around line 6737-6752: Add a third lore CounterAdded trigger to the
saga.trigger_definitions fixture without calling saga_chapter, using a threshold
greater than 3. Keep the existing provenance-bearing triggers and the expected
final_chapter_number assertion at Some(3), ensuring the fixture verifies
provenance is not inferred from lore thresholds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 54117372-2c9b-49ec-b2de-6f4637d17653
📒 Files selected for processing (2)
crates/manabrew-compat/src/lib.rscrates/mtgish-import/src/convert/saga.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the new head still drops the old Saga incarnation’s resolved chapter event.
🔴 Blocker
[HIGH] A final chapter ability that is already on the stack is silently unobservable if its Saga leaves and re-enters before that ability resolves. Evidence: crates/engine/src/game/stack.rs:1018-1030 reads the current object by source_id and returns None when the stack entry’s definition_ref.source is no longer current; crates/engine/src/game/stack.rs:2555-2561 consequently emits no SagaChapterAbilityResolved event. Why it matters: an old chapter ability can still resolve, but Narci/Tom then never see the resolution. Suggested fix: capture the original Saga’s identity and event-time subject snapshot while constructing the triggered stack entry, carry it through the terminal resolution path, and publish that snapshot with the lifecycle event.
Removing the guard alone would be incorrect. crates/engine/src/game/targeting.rs:1568-1572 turns the event’s raw saga_id into EventSource, while crates/engine/src/game/trigger_matchers.rs:3586-3599 prefers a live object at that id before considering LKI. After re-entry that is the new incarnation, so that Saga’s mana value can bind to the wrong permanent. Reuse the existing EventObjectSnapshot authority (crates/engine/src/types/events.rs:311-355) and matches_target_filter_on_event_snapshot (crates/engine/src/game/filter.rs:2431-2466) for both the observer’s Saga filter and event-source quantity lookup.
Add a production-stack regression where a final chapter ability is on the stack, its original Saga leaves and re-enters as a differently costed incarnation, and the observer fires exactly once using the original mana value. That test must fail on the current fail-closed path.
🟡 Current evidence to refresh
The required parse-diff sticky is bound to 3ebf5f2611856f3c335583d13d7a82c48ca0638c, not this head 0fbf3b7ba0c080869546274d2aa1319d7a00f5c3; regenerate it after the implementation change. The open inline citation finding at narci_fable_singer_final_chapter_drain.rs:211-218 also needs resolution: the comment cites CR 608.2c for post-resolution observer timing, while the official CR’s 608.2p is the rule that addresses abilities triggering after the resolving ability’s steps complete.
Recommendation: request changes for incarnation-bound snapshot transport, a discriminating blink-before-resolution regression, and current-head parse evidence.
Narci, Fable Singer's third ability ("whenever the final chapter ability
of a Saga you control resolves, each opponent loses X life and you gain X
life, where X is that Saga's mana value") had no primitive behind it: the
engine could not observe another permanent's chapter ability at all.
Build the class, not the card. Three printed cards reference a chapter
ability (Narci, Tom Bombadil, Historian's Boon) and they vary over two
independent axes, so TriggerMode::SagaChapterAbility is parameterized on
both rather than split into sibling variants (mirroring the existing
Planeswalked { role }):
* SagaChapterScope -- any chapter ability vs the final one (CR 714.4).
* AbilityLifecyclePoint -- the ability triggering (CR 603.2) vs
finishing resolution (CR 608.2). These are genuinely different
events: a chapter ability that triggers may still be countered or
fail its intervening-if and never resolve.
GameEvent::SagaChapterAbilityResolved is new because nothing on the bus
reported that fact. StackResolved is pushed on the fizzle and
failed-intervening-if paths too, and carries the stack entry id rather
than the Saga, so it cannot distinguish an ability that resolved from one
that merely left the stack. The new event is published from the single
resolve_top exit reached only after a triggered ability genuinely
resolved.
Chapter identity is captured BEFORE the ability executes, keyed off the
exact trigger occurrence that fired. It cannot be recovered afterward:
CR 704.5s sacrifices a Saga the moment its final chapter ability leaves
the stack, and a chapter ability may exile its own Saga (Fable of the
Mirror-Breaker III). Deriving the chapter from the lore count instead
would be wrong under Read Ahead and under multi-counter additions, which
cross several thresholds at once.
"That Saga's mana value" binds to ObjectScope::EventSource, not to Target
like the neighbouring "that creature's" arm -- a meta-trigger announces
no targets, so the Saga carried by the event is the only referent that
exists. It resolves through the existing live-then-LKI mana-value path,
which is what makes X correct after the Saga is already gone.
final_chapter_number now delegates to a new saga_chapter_numbers and is
scoped to lore counters: per CR 714.2a only lore-counter triggers are
chapter abilities, so a Saga carrying a thresholded trigger on some other
counter type no longer inflates its final chapter.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…er scope Addresses review on phase-rs#7224. Both blockers were real. Verified every citation against docs/MagicCompRules.txt, which had never been fetched in this worktree -- the original annotations were written from memory, which is exactly what CLAUDE.md's verification rule exists to prevent. The rules text says: 714.2 a chapter symbol is a keyword ability representing a triggered ability referred to as a chapter ability 714.2a defines ONLY the Roman numeral 714.2b "{rN}--[Effect]" means "When one or more lore counters are put onto this Saga, if the number of lore counters on it was less than N and became at least N, [effect]" 714.2d final chapter NUMBER is the greatest among a Saga's chapter abilities 714.2e final chapter ABILITY is the one carrying that number 714.4 the sacrifice state-based action So 714.2a was wrong everywhere it was used for the threshold-trigger claim, and 714.4 was wrong everywhere it was used to define the final chapter ability. Each citation is now the rule that actually states the claim. The same audit covers four PRE-EXISTING 714.2a citations on the shared counter-filter path (CounterTriggerFilter, match_counter_added and its CR 310.12b mirror), which make the identical error about the identical rule and sit directly on the code this change reads; leaving them would have re-earned the same review note. Removed SagaChapterScope. Its Any variant was speculative -- no printed card uses an unqualified "a chapter ability" -- and, worse, unmodelable here: CR 714.2b makes each chapter symbol its own triggered ability, so one lore-counter addition crossing several chapter numbers triggers that many chapter abilities. An observer of all of them owes one firing per crossed ability, which an event-keyed matcher reading a single CounterAdded cannot express. The original commit documented that shortfall in a comment instead of refusing it, which advertised a rules-incorrect branch through a public enum and parser grammar. The mode is now TriggerMode::FinalSagaChapterAbility { lifecycle }, the parser requires "final", and a new test pins the refusal: an unqualified clause stays Unknown (honestly coverage-red) rather than minting a trigger that under-fires. Restoring the scope needs an occurrence-level chapter event first, not a wider enum -- recorded on the variant. Also fail closed on an incarnation mismatch when classifying a resolving chapter ability (CR 400.7). source_id is storage identity: a Saga that left and re-entered can occupy the same id as a new object whose chapter numbers are not the ones this ability triggered from, and publishing those would misclassify a non-final chapter as final or drain for the wrong mana value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses the two review findings previously deferred on phase-rs#7224. Chapter abilities were being recognized by shape: a lore-counter threshold on a Saga. CR 714.2b does give a chapter symbol that shape, but the converse does not hold -- a lore threshold trigger a Saga acquired some other way is not a chapter ability, and counting it corrupts the final chapter number CR 714.2d defines and CR 714.4's sacrifice depends on. TriggerDefinition now carries `saga_chapter: Option<u32>`, the chapter symbol's Roman numeral, set only by the Saga parser -- the one place that has read an actual chapter symbol. `saga_chapter_numbers`, `saga_chapter_for_occurrence` and read-ahead's CR 714.2d derivation in synthesis all read that provenance instead of re-deriving from thresholds, so there is one model rather than three. Keying `saga_chapter_for_occurrence` on the occurrence keeps CR 714.2c's two chapter abilities printed on one line distinct; the History of Benalia IR snapshot shows that directly ("I, II --" now yields saga_chapter 1 and 2). Hand-built Saga fixtures in sba.rs, synthesis.rs and game_object.rs must now declare the provenance too. That is the point: a bare lore threshold no longer passes for a chapter ability anywhere. The second finding -- that publishing the resolution event before the settlement guard lets observers fire while a chapter ability is still resolving -- is answered with the test the review asked for rather than a refactor. `narci_does_not_drain_until_a_paused_final_chapter_finishes_resolving` gives the final chapter an optional-effect choice, asserts the drain has NOT landed while that prompt is open, and asserts it lands exactly once after it is answered. A `saw_optional` reach guard fails the test if the pause never happens, so it cannot pass vacuously. The ordering holds because a pending trigger cannot reach the stack until a player would receive priority, which follows completion of the in-resolution choice. Moving the emission would have required threading an event sink through `settle_finished_resolving_stack_entry` and every parked-resolution settle path, changing where the engine considers a resolution finished for `StackResolved` too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follows the provenance change in the previous commit across the crate boundary. CI shard 2/4 caught manabrew-compat's Saga DTO test -- final_chapter_number returned None where it expected Some(3) -- because the previous commit was verified with `-p phase-engine` only, which is the wrong scope for a change to a shared type. Fixing that surfaced the more serious instance. `mtgish-import`'s saga converter is a PRODUCTION import path that builds chapter triggers from chapter ordinals, and it set only the lore threshold. Under the new model every Saga imported through it would have reported no final chapter number, so CR 714.4 would never sacrifice it, read-ahead (CR 702.155b) would find nothing to read ahead to, and a final-chapter observer would never see it. The converter has the ordinal in hand and simply was not recording it; it now does. Swept every counter_filter construction site in the workspace. The remaining ones build Time (CR 702.62a / CR 702.63a) and Defense (CR 310.12b) threshold triggers and correctly carry no chapter provenance -- which is exactly the distinction the field exists to make, and the one threshold-shape inference could not express. Also corrects CR 714.2a -> CR 714.2 on the converter's chapter-body comment: 714.2a defines only the Roman numeral, while 714.2 is what makes a chapter symbol a triggered ability. Note: `cargo test -p mtgish-import --lib` has two failures on this base (convert::action::tests::search_players_library_target_opponent_preserves_acquire_shape and convert::replacement::tests::would_deal_damage_fixed_actions_convert_to_typed_modifications). Both reproduce with this change reverted, neither touches Saga code, and no upstream commit since this branch's base touches those files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses the remaining review blocker on phase-rs#7224. Both of the shapes I had considered were wrong, and the review named why: reading live state by storage id lets a re-entered Saga answer "that Saga's mana value", while guarding on an incarnation mismatch (the previous commit's fail-closed compromise) drops a firing that CR 113.7a says really did happen -- an already-triggered chapter ability resolves even after its Saga leaves. The fix needed no new machinery. TriggerSourceContext is already the engine's "complete event-time authority for a triggered source": captured when the chapter ability triggered, it pins the incarnation in `identity.reference` and carries that incarnation's `trigger_entries` and `lki`. The event now carries it instead of a raw saga_id, and every consumer reads from it: chapter numbers live object by id -> trigger_entries on the pinned incarnation (CR 714.2 / 714.2d) "a Saga you control" valid_card_matches_with_lki by id -> matches_target_filter_on_lki_snapshot on the pinned lki (CR 400.7) "that Saga's mana value" live -> LKI cache by id -> pinned lki.mana_value (CR 202.3) `resolving_saga_chapter` no longer consults `state.objects` at all, so the `is_current` guard is gone -- it was papering over reading the wrong source. Same-id confusion is now structurally excluded rather than guarded against. The blink regression discriminates on BOTH failure modes: the Saga re-enters at the same storage id with mana value 7 while the original was 3, so a live-state read drains 7, the old guard drains 0, and only the incarnation-bound read drains 3 exactly once. Also from this review round: - CR 608.2p is the rule this whole feature implements -- "Once all possible steps described in 608.2c-n are completed, any abilities that trigger when that spell or ability resolves trigger" -- and was never cited. It now annotates the event and its single emission site, and it settles the earlier question about publishing before the settlement guard by rule rather than by test alone. The paused-resolution test cites CR 608.2d for the mid-resolution choice; CR 608.2c (instruction order) described neither. - The manabrew Saga fixture gains a lore threshold with NO chapter provenance, at a threshold above the real final chapter. Without it the test passed under both the provenance contract and the threshold inference it replaced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0fbf3b7 to
3e013fd
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved at current head 3e013fd42aaee760bbe82c640735c2d7725de14a.
The incarnation-bound Saga snapshot now travels from the exact triggered ability through terminal resolution, matcher filtering, and EventSource mana resolution. The blink regression changes the same storage id's incarnation and mana value (3 to 7), so it discriminates against both the old fail-closed and wrong-live-object paths. Current required CI is green; the current parse diff is limited to Narci, Tom Bombadil, and Historian's Boon final forms. The remaining CodeRabbit threads are outdated and their concerns are fixed or eliminated on this head.
2 participants
Implements Narci, Fable Singer (
{1}{W}{B}{G}Legendary Creature — Human Bard).Lifelink and
Whenever you sacrifice an enchantment, draw a card.already worked. The third ability had no primitive behind it — the engine could not observe another permanent's chapter ability at all:Built for the class, not the card
Scryfall has exactly three cards that reference a "chapter ability", and they vary over two independent axes, so
TriggerMode::SagaChapterAbilityis parameterized on both rather than split into sibling variants (mirroring the existingPlaneswalked { role }):FinalResolvedFinalResolvedFinalTriggeredSagaChapterScope— any chapter ability vs the final one (CR 714.4).AbilityLifecyclePoint— the ability triggering (CR 603.2) vs finishing resolution (CR 608.2). These are genuinely different events: a chapter ability that triggers may still be countered, or fail its intervening-if, and never resolve.The observed Saga rides the trigger's ordinary
valid_cardfilter ("a Saga you control"), so no Saga-specific filter axis was needed.Why a new
GameEventGameEvent::SagaChapterAbilityResolvedis new because nothing on the bus reported that fact.StackResolvedis pushed on the fizzle and failed-intervening-if paths too, and carries the stack entry id rather than the Saga, so it cannot distinguish an ability that resolved from one that merely left the stack. The new event is published from the singleresolve_topexit reached only after a triggered ability genuinely resolved.For the
Triggeredhalf no new event is needed: chapter abilities have no event of their own — they are the Saga's lore-counter threshold triggers (CR 714.2a), so the matcher reads the sameCounterAdded { Lore }thatmatch_counter_addedconsumes.Two things that are easy to get subtly wrong
Chapter identity must be captured before the ability executes. It cannot be recovered afterward: CR 704.5s sacrifices a Saga the moment its final chapter ability leaves the stack, and a chapter ability may exile its own Saga (Fable of the Mirror-Breaker III). The snapshot is keyed off the exact trigger occurrence that fired — deriving the chapter from the lore count would be wrong under Read Ahead and under multi-counter additions, which cross several thresholds at once.
"That Saga's mana value" binds to
ObjectScope::EventSource, not toTargetlike the neighbouring"that creature's"arm. A meta-trigger announces no targets, so the Saga carried by the event is the only referent that exists. It resolves through the existing live-then-LKI mana-value path, which is what makes X correct after the Saga is already gone.Drive-by correctness fix
final_chapter_numbernow delegates to a newsaga_chapter_numbersand is scoped to lore counters. Per CR 714.2a only lore-counter triggers are chapter abilities, so a Saga carrying a thresholded trigger on some other counter type no longer inflates its final chapter number.Testing
Five parser tests covering both axes and the subject grammar, plus three runtime tests in
crates/engine/tests/integration/narci_fable_singer_final_chapter_drain.rs:Triggeredhalf — Historian's Boon's clause firing on the lore crossing.Verified with
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings(clean), the full engine integration suite (4802 passed, 0 failed), and the engine unit suite (18,840 passed).One unit test fails on this machine —
bounded_offer_conjunct_tests::f2c_the_cr_603_5_conjunct_set_has_one_production_assembler— but it is a pre-existing Windows-only issue: the census compares hardcoded POSIX paths againstPath::display(), which emits backslashes on Windows. Line numbers match exactly; only the separators differ, and it censusesgame/effects/mod.rs, which this branch does not touch.Not verified: the two TypeScript edits (one union member in
adapter/types.ts, one string in theeventNormalizernon-visual set). This worktree has nonode_modulesand installing would churn the lockfile, sotscdid not run on them.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes