feat(engine): add storied and recruit - #7074
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe engine adds persistent Enduring Story designation state, Storied keyword support, Recruit discard-result handling, and discard-frame tracking. The client exposes localized Enduring Story badges. Tests cover rules, parsing, integration behavior, and serialization. ChangesEnduring Story and Recruit functionality
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Battlefield as Battlefield SBA
participant GameState
participant DiscardPipeline
participant ResolutionStack
participant ClientHUD
Battlefield->>GameState: add player to enduring_story
Battlefield->>GameState: emit EnduringStoryGained
DiscardPipeline->>ResolutionStack: record direct discard result
ResolutionStack->>GameState: resume Recruit continuation
GameState->>ClientHUD: expose enduring_story
ClientHUD->>ClientHUD: render EnduringStoryBadge
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Generated for head Parse changes introduced by this PR · 17 card(s), 8 signature(s) (baseline: main
|
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/engine/src/types/resolution.rs (1)
537-571: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
game_state_eqcomparesDiscardframes by their monotonicid, breaking CR 104.4b loop-equality.
MultiDrawframes get a dedicated arm here that ignores per-instance frame ids viadraw_sequences.loop_equal(...), specifically to keep state-equality checks (used for CR 104.4b mandatory-loop detection) working across states that differ only in a freshly minted frame identifier.Discardframes fall through to the generic(Some(left), Some(right)) if left == rightarm, which compares the fullDiscardFrame, includingid.Since
begin_discardallocates a newDiscardFrameIdper operation, two otherwise-identical repeating game states that each have a Recruit discard paused at the same decision point will compare unequal here, because theirDiscardframes carry differentidvalues. This can prevent loop detection for a repeating action that involves a paused discard.Add a
Discard-specific arm that comparesDiscardFramewhile ignoringid, mirroring theMultiDrawcarve-out.🔁 Proposed fix
( Some(ResolutionFrame::MultiDraw(left)), Some(ResolutionFrame::MultiDraw(right)), ) if left.draw_sequences.loop_equal(&right.draw_sequences) && left.connive_reentry == right.connive_reentry => {} + ( + Some(ResolutionFrame::Discard(left)), + Some(ResolutionFrame::Discard(right)), + ) if left.source_id == right.source_id && left.results == right.results => {} (Some(left), Some(right)) if left == right => {}🤖 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/resolution.rs` around lines 537 - 571, Update game_state_eq with a Discard-specific comparison arm before the generic frame equality arm. Compare the DiscardFrame contents while excluding the per-operation id, so otherwise-identical paused discard states with freshly allocated DiscardFrameId values are treated as equal; leave all other frame comparisons unchanged.Source: Learnings
crates/engine/src/types/ability.rs (1)
24154-24162: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClear
direct_discard_resultwhen propagating context to descendant nodes.
set_context_recursivecopies the entireSpellContext, so ifdirect_discard_resultreaches it, every descendant receives the stale discard result after it was cleared byset_direct_discard_result_for_immediate_node. Copy onlydirect_discard_result: Nonefor sub/else branches, or otherwise clear it before re-propagating.🤖 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 24154 - 24162, Update Ability::set_context_recursive so descendant sub_ability and else_ability nodes receive a SpellContext with direct_discard_result cleared to None, while preserving the current context for the node itself and propagating all other context fields unchanged.
🧹 Nitpick comments (1)
crates/engine/src/types/game_state.rs (1)
14814-14818: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReconsider boxing
enduring_story.
enduring_storyisBox<HashSet<PlayerId>>, while the analogous sibling fieldcity_blessingis a plainHashSet<PlayerId>. AHashSetheader is small and cheap to move; boxing it adds a heap allocation on construction (Box::default()) and on everyGameState::clone(), even when the set is empty (the common case). This file repeatedly explains that other fields useArc/imspecifically to keepGameState::clone()cheap on the AI-search hot path; boxing a small, usually-emptyHashSetworks against that same goal without an accompanying reason in the diff.Drop the
Boxand useHashSet<PlayerId>directly, matchingcity_blessing, unless there is a documented size-budget reason to box it.♻️ Proposed fix
- pub enduring_story: Box<HashSet<PlayerId>>, + pub enduring_story: HashSet<PlayerId>,And update the two initializers accordingly:
- enduring_story: Box::default(), + enduring_story: HashSet::new(),🤖 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/game_state.rs` around lines 14814 - 14818, Change the GameState field enduring_story from Box<HashSet<PlayerId>> to HashSet<PlayerId>, preserving its serde attributes. Update both enduring_story initializers to construct or assign a plain HashSet directly, matching the city_blessing representation; do not introduce boxing unless an existing documented size-budget requirement requires it.
🤖 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 `@client/src/i18n/locales/en/game.json`:
- Around line 483-484: Update the enduringStoryTooltip translations in
client/src/i18n/locales/en/game.json:483-484,
client/src/i18n/locales/de/game.json:444-445,
client/src/i18n/locales/es/game.json:444-445,
client/src/i18n/locales/fr/game.json:444-445,
client/src/i18n/locales/it/game.json:444-445,
client/src/i18n/locales/pl/game.json:444-445, and
client/src/i18n/locales/pt/game.json:444-445. Use complete, locale-appropriate
wording stating that Enduring Story is gained by a permanent with Storied and
three or more artifacts, legendary permanents, and/or Sagas, and that the
designation lasts for the rest of the game; leave the enduringStory labels and
engine behavior unchanged.
In `@crates/engine/src/game/effects/discard.rs`:
- Around line 89-96: Extract a non-panicking retire_discard_frame(state,
frame_id) helper in crates/engine/src/game/effects/discard.rs, using
take_active_discard().flatten() and debug_assert! for the frame ID instead of
expect/assert_eq!. Replace the duplicated retirement block at lines 89-96 and
366-373 with calls to this helper.
- Around line 642-739: Add a production-path Recruit test that resolves the
effect through resolve, a GameAction, and resolve_ability_chain/GameRunner
rather than calling hand_off_recruit_discard_result directly or manually
populating DiscardedCardResult. Have the test draw a nonland spell, choose it as
Recruit’s discard, and assert DiscardedCardMatchesFilter creates the contingent
token only after the discard is produced through the normal flow; cover the
relevant failure path as well.
In `@crates/engine/src/game/sba.rs`:
- Around line 368-410: Add CR 702.195 citations at all five sites: in
crates/engine/src/game/sba.rs lines 368-410, document check_enduring_story’s
Storied-plus-three-Historic-permanents check under 702.195a; at
crates/engine/src/game/sba.rs line 269, add or extend the citation to cover the
enduring-story call; and in crates/engine/src/game/layers.rs line 1828,
crates/engine/src/game/restrictions.rs line 1638, and
crates/engine/src/game/conditions.rs lines 179-182, annotate the
HasEnduringStory dispatch/evaluation paths under 702.195b.
In `@crates/engine/src/game/trigger_index.rs`:
- Line 669: Add a rules annotation beside the no-op
GameEvent::CityBlessingGained and GameEvent::EnduringStoryGained classification,
using the verified format “CR 702.195b-c: Enduring Story is a designation, not
an inherent trigger event, so EnduringStoryGained produces no trigger-index
key.” Preserve the existing no-key behavior.
In `@crates/engine/src/game/triggers.rs`:
- Line 10386: Add a Comprehensive Rules annotation to the
TriggerCondition::HasEnduringStory match arm, using the required “CR <number>:
<description>” format and the appropriate rule reference and description for
this condition.
In `@crates/engine/src/parser/oracle_condition.rs`:
- Line 301: The HasEnduringStory conversion branches lack the required verified
CR 702.195 annotation. Add the annotation describing that storied grants
endurance story designation for the rest of the game at
crates/engine/src/parser/oracle_condition.rs:301,
crates/engine/src/parser/oracle_effect/conditions.rs:4534, and
crates/engine/src/parser/oracle_trigger.rs:4525, covering each corresponding
HasEnduringStory bridge.
In `@crates/engine/src/parser/oracle_ir/ast.rs`:
- Around line 612-617: Replace the incorrect CR 701.9a and CR 608.2c citations
for Recruit with the verified CR section defining Recruit and its nonland-token
condition. Apply the same citation to the Recruit parser production and the
draw/discard/conditional-token behavior in
crates/engine/src/parser/oracle_ir/ast.rs lines 612-617,
crates/engine/src/parser/oracle_effect/imperative.rs lines 9749-9761, and
crates/engine/src/parser/oracle_effect/imperative.rs lines 11861-11910, using
the Recruit symbols to locate each site.
In `@crates/engine/src/parser/oracle_nom/condition.rs`:
- Around line 1251-1254: Add the verified `CR <number>: <description>` rule
annotation immediately before the `StaticCondition::HasEnduringStory` entry in
the condition parser, documenting the `storied` to `enduring story` mapping
while preserving the existing value and tag behavior.
In `@crates/engine/src/types/ability.rs`:
- Around line 7709-7710: Add verified Comprehensive Rules citations for the
Storied/enduring-story symbols: in crates/engine/src/types/ability.rs lines
7709-7710 (anchor), 8171-8172, 18976-18977, and 19960-19961, cite CR 702.195 on
each HasEnduringStory variant; in crates/engine/src/types/events.rs lines
1347-1350, cite CR 702.195 or 702.195c on GameEvent::EnduringStoryGained; and in
crates/engine/src/types/keywords.rs lines 176 and 632, cite CR 702.195 on
KeywordKind::Storied and CR 702.195a on Keyword::Storied.
In `@crates/engine/src/types/game_state.rs`:
- Around line 14814-14818: Add a verified CR citation in the doc comment
immediately above the enduring_story field, using the format “CR <number>:
<description>” and accurately describing the Enduring Story designation’s
persistent behavior. Keep the existing serde attributes and field declaration
unchanged.
In `@crates/engine/src/types/resolution.rs`:
- Around line 5419-5462: Update
v2_reader_recovers_discard_allocator_and_rejects_duplicate_frame_ids to avoid
serializing the malformed duplicate through
ResolutionStateWire::from_game_state, since validation rejects it first.
Construct a ResolutionStack containing two identical discard frames and inject
it using the existing fixture helper, following the duplicate_draw pattern, then
keep the assertion that deserializing the malformed payload returns an error.
In `@crates/engine/tests/integration/bards_company_recruit.rs`:
- Around line 79-84: Update the Bard's Company fixture check in the integration
test to fail when db.get_face_by_name(BARDS_COMPANY) is absent instead of
printing a skip message and returning. Ensure the test continues into the cast,
discard, and token pipeline only when the required generated fixture is present.
- Around line 59-62: Update the token predicate in the Bard’s Company
integration test to match the Recruit’s base characteristics rather than its
modified current values: use base_power and base_toughness for 1/1, or
separately assert base 1/1 and current power/toughness 2/2 while preserving the
existing token and battlefield checks.
In `@crates/engine/tests/integration/deterministic_game_state_serde.rs`:
- Line 208: Update hash_shape to recurse through Box-wrapped values so
discover_fields records enduring_story with its nested Box<HashSet> shape.
Preserve the existing HASH_SET adapter classification for the inner hash set and
ensure the resulting shape is included in discovered_manifest.
---
Outside diff comments:
In `@crates/engine/src/types/ability.rs`:
- Around line 24154-24162: Update Ability::set_context_recursive so descendant
sub_ability and else_ability nodes receive a SpellContext with
direct_discard_result cleared to None, while preserving the current context for
the node itself and propagating all other context fields unchanged.
In `@crates/engine/src/types/resolution.rs`:
- Around line 537-571: Update game_state_eq with a Discard-specific comparison
arm before the generic frame equality arm. Compare the DiscardFrame contents
while excluding the per-operation id, so otherwise-identical paused discard
states with freshly allocated DiscardFrameId values are treated as equal; leave
all other frame comparisons unchanged.
---
Nitpick comments:
In `@crates/engine/src/types/game_state.rs`:
- Around line 14814-14818: Change the GameState field enduring_story from
Box<HashSet<PlayerId>> to HashSet<PlayerId>, preserving its serde attributes.
Update both enduring_story initializers to construct or assign a plain HashSet
directly, matching the city_blessing representation; do not introduce boxing
unless an existing documented size-budget requirement requires it.
🪄 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: 16937647-70d6-4031-8e0a-8eb09835805a
📒 Files selected for processing (60)
client/src/adapter/types.tsclient/src/components/board/OpponentSeatHeader.tsxclient/src/components/hud/HudBadges.tsxclient/src/components/hud/OpponentHud.tsxclient/src/components/hud/PlayerHud.tsxclient/src/components/hud/__tests__/OpponentHud.designations.test.tsxclient/src/components/hud/__tests__/PlayerHud.designations.test.tsxclient/src/hooks/usePlayerDesignations.tsclient/src/i18n/locales/de/game.jsonclient/src/i18n/locales/en/game.jsonclient/src/i18n/locales/es/game.jsonclient/src/i18n/locales/fr/game.jsonclient/src/i18n/locales/it/game.jsonclient/src/i18n/locales/pl/game.jsonclient/src/i18n/locales/pt/game.jsoncrates/engine/src/ai_support/filter.rscrates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/conditions.rscrates/engine/src/game/costs.rscrates/engine/src/game/coverage.rscrates/engine/src/game/effects/discard.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/engine_debug.rscrates/engine/src/game/engine_payment_choices.rscrates/engine/src/game/engine_replacement.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/engine_tests.rscrates/engine/src/game/gap_analysis.rscrates/engine/src/game/layers.rscrates/engine/src/game/log.rscrates/engine/src/game/public_state.rscrates/engine/src/game/quantity.rscrates/engine/src/game/replacement.rscrates/engine/src/game/restrictions.rscrates/engine/src/game/sba.rscrates/engine/src/game/trigger_index.rscrates/engine/src/game/trigger_matchers.rscrates/engine/src/game/triggers.rscrates/engine/src/game/zone_pipeline.rscrates/engine/src/parser/oracle_condition.rscrates/engine/src/parser/oracle_effect/conditions.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_ir/ast.rscrates/engine/src/parser/oracle_keyword.rscrates/engine/src/parser/oracle_nom/condition.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/src/types/ability.rscrates/engine/src/types/events.rscrates/engine/src/types/game_state.rscrates/engine/src/types/identifiers.rscrates/engine/src/types/keywords.rscrates/engine/src/types/proposed_event.rscrates/engine/src/types/resolution.rscrates/engine/tests/integration/bards_company_recruit.rscrates/engine/tests/integration/deterministic_game_state_serde.rscrates/engine/tests/integration/integration_bending.rscrates/engine/tests/integration/main.rscrates/phase-ai/src/search.rs
b2a75a5 to
1a99376
Compare
61a8eb0 to
b13f9a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/resolution.rs (1)
537-568: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve loop-state equality for discard frames.
game_state_eqfalls through to strictPartialEqforResolutionFrame::Discard.DiscardFrameIdis fresh for everybegin_discardcall. Therefore, two otherwise identical repeated Recruit states compare unequal solely because their operation IDs differ. This can prevent mandatory-loop detection from recognizing a repeated game state.Add a
Discardcomparison branch that compares semantic frame state without requiring equal absolute provenance IDs. Preserve the structural parent relationship when doing this comparison. Based on learnings: “preserveGameState::eqsemantics for deferred trigger loop state” and “do not add fresh per-instance identifiers … unless loop-state equality remains intact.”🤖 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/resolution.rs` around lines 537 - 568, The game_state_eq method currently compares ResolutionFrame::Discard via strict PartialEq, incorrectly treating fresh DiscardFrameId values as state differences. Add a dedicated Discard comparison branch that compares semantic discard state while ignoring absolute provenance IDs, but still requires the structural parent relationship to match; leave all existing frame comparisons unchanged.Source: Learnings
🤖 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/types/resolution.rs`:
- Around line 477-486: Change begin_discard to return an allocation error when
next_discard_frame_id reaches the reserved exhaustion sentinel, rather than
using saturating_add and reusing DiscardFrameId(u64::MAX). Propagate this Result
through the discard-resolution callers and handle the failure before creating or
pushing a DiscardFrame, while preserving normal ID allocation below the
exhaustion boundary.
---
Outside diff comments:
In `@crates/engine/src/types/resolution.rs`:
- Around line 537-568: The game_state_eq method currently compares
ResolutionFrame::Discard via strict PartialEq, incorrectly treating fresh
DiscardFrameId values as state differences. Add a dedicated Discard comparison
branch that compares semantic discard state while ignoring absolute provenance
IDs, but still requires the structural parent relationship to match; leave all
existing frame comparisons unchanged.
🪄 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: 54e83e08-9533-4f6e-b6c1-7a055a8e0c19
📒 Files selected for processing (28)
client/src/i18n/locales/de/game.jsonclient/src/i18n/locales/en/game.jsonclient/src/i18n/locales/es/game.jsonclient/src/i18n/locales/fr/game.jsonclient/src/i18n/locales/it/game.jsonclient/src/i18n/locales/pl/game.jsonclient/src/i18n/locales/pt/game.jsoncrates/engine/src/game/conditions.rscrates/engine/src/game/effects/discard.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/layers.rscrates/engine/src/game/restrictions.rscrates/engine/src/game/sba.rscrates/engine/src/game/trigger_index.rscrates/engine/src/game/triggers.rscrates/engine/src/parser/oracle_condition.rscrates/engine/src/parser/oracle_effect/conditions.rscrates/engine/src/parser/oracle_ir/ast.rscrates/engine/src/parser/oracle_nom/condition.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/src/types/ability.rscrates/engine/src/types/game_state.rscrates/engine/src/types/resolution.rscrates/engine/tests/integration/bards_company_recruit.rscrates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (27)
- crates/engine/src/parser/oracle_condition.rs
- client/src/i18n/locales/pl/game.json
- client/src/i18n/locales/it/game.json
- crates/engine/src/parser/oracle_nom/condition.rs
- crates/engine/tests/integration/main.rs
- crates/engine/src/parser/oracle_trigger_tests.rs
- crates/engine/src/game/triggers.rs
- crates/engine/src/game/engine.rs
- client/src/i18n/locales/es/game.json
- client/src/i18n/locales/en/game.json
- client/src/i18n/locales/pt/game.json
- crates/engine/src/parser/oracle_ir/ast.rs
- client/src/i18n/locales/fr/game.json
- crates/engine/src/game/layers.rs
- crates/engine/tests/integration/bards_company_recruit.rs
- client/src/i18n/locales/de/game.json
- crates/engine/src/game/conditions.rs
- crates/engine/src/parser/oracle_trigger.rs
- crates/engine/src/types/ability.rs
- crates/engine/src/game/restrictions.rs
- crates/engine/src/game/sba.rs
- crates/engine/src/parser/oracle_effect/conditions.rs
- crates/engine/src/types/game_state.rs
- crates/engine/src/game/effects/mod.rs
- crates/engine/src/game/engine_resolution_choices.rs
- crates/engine/src/game/effects/discard.rs
- crates/engine/src/game/trigger_index.rs
| /// Starts one discard operation and returns its unique provenance id. | ||
| pub fn begin_discard(&mut self, source_id: Option<ObjectId>) -> DiscardFrameId { | ||
| let id = DiscardFrameId(self.next_discard_frame_id); | ||
| self.next_discard_frame_id = self.next_discard_frame_id.saturating_add(1); | ||
| self.push_discard(DiscardFrame { | ||
| id, | ||
| source_id, | ||
| results: Vec::new(), | ||
| }); | ||
| id |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make discard ID allocation fail before integer exhaustion.
When next_discard_frame_id is u64::MAX, Line 480 leaves it unchanged. After that frame retires, the next call reuses DiscardFrameId(u64::MAX). A stale replacement event can then bind to a later discard operation. The active u64::MAX frame also fails the allocator validation at Lines 2528-2535.
Reserve an exhaustion sentinel and return an allocation error before ID reuse. Propagate that error through discard resolution.
🤖 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/resolution.rs` around lines 477 - 486, Change
begin_discard to return an allocation error when next_discard_frame_id reaches
the reserved exhaustion sentinel, rather than using saturating_add and reusing
DiscardFrameId(u64::MAX). Propagate this Result through the discard-resolution
callers and handle the failure before creating or pushing a DiscardFrame, while
preserving normal ID allocation below the exhaustion boundary.
Summary by CodeRabbit