Fix Raubahn, Bull of Ala Mhigo - #7399
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds Raubahn integration tests for dynamic Ward parsing and attack-trigger target construction. It registers the test module and removes Raubahn from the parser misparse backlog. ChangesRaubahn integration coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR adds localized Raubahn regression coverage and updates parser backlog documentation without changing production behavior; no actionable merge-blocking risk remains beyond normal checks and review. 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
There was a problem hiding this comment.
Request changes — the current head has two blocking gaps.
🔴 Blocker
crates/engine/tests/integration/raubahn_bull_of_ala_mhigo.rs:130 writes GameObject.combat_status, and :134 reads GameObject.triggers; neither field exists. The current-head Rust lint and card-data jobs both fail with E0609 at those exact lines, so the integration test binary and card-data pipeline cannot build. Model combat through GameState.combat / CombatState and obtain the live trigger via trigger_definitions (and its definition reference) rather than nonexistent object fields.
🔴 Blocker
The claimed Raubahn parser behavior is already present in the base: crates/engine/src/parser/oracle_keyword.rs:732-744 recognizes the source-power Ward payload, and crates/engine/src/parser/oracle.rs:4135-4144 normalizes Raubahn's name to ~. This diff instead changes the generic Ward fallback at oracle_keyword.rs:746-830; for example it changes handling of Ward—discard a card at random (Alpharael). There is no current-head <!-- coverage-parse-diff --> artifact and no production test or parse accounting for that actual generic delta.
Recommendation: repair the test against the real combat/trigger authorities, then provide current-head parse-diff accounting and discriminating production evidence for every changed Ward payload class.
Preserve the current shared-engine P2P host authority introduced by phase-rs#7398 while retaining Raubahn Ward regression coverage. Co-authored-by: @Lcola98 <75585494+keloide@users.noreply.github.com>
|
Maintainer port complete — awaiting fresh current-head evidence. This PR is now at Please let CI and the |
Co-authored-by: Lcola98 <75585494+Lcola98@users.noreply.github.com>
|
Maintainer fixup |
Co-authored-by: Lcola98 <75585494+Lcola98@users.noreply.github.com>
matthewevans
left a comment
There was a problem hiding this comment.
Approved for merge queue at c213804: the test-only Raubahn regression coverage is clean, and the Category 16 summary now matches its 83-card list.
Summary
Address maintainer review feedback by repairing Raubahn's regression test against the engine's real combat and trigger authorities, and removing unrelated generic Ward parser changes. The base parser already normalizes Raubahn's name to
~and supports its source-power Ward cost; this PR now provides compiling regression evidence and removes the stale backlog entry.Files changed
crates/engine/tests/integration/main.rs— registers the Raubahn integration test.crates/engine/tests/integration/raubahn_bull_of_ala_mhigo.rs— full-card parser, combat target-slot, and runtime Ward coverage usingCombatStateandtrigger_definitions.docs/parser-misparse-backlog.md— removes the already-supported card from root cause 16.Track
Developer
LLM
Model: gpt-5.6
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
CR 702.21a(Ward) andCR 608.2h(current information/LKI at resolution).Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same implementation seams.
cargo fmt --all— PASS.git diff --check— PASS../scripts/check-parser-combinators.sh— PASS; Gate A output below.cargo test -p phase-engine --test integration raubahn_bull_of_ala_mhigo --no-fail-fast— local compiler was terminated by SIGTERM while compilingphase-engine; CI-owned replacement run is pending.Current-head parse-diff accounting — no production parser file is changed in this follow-up, so no card parse behavior is intentionally changed; the CI card-data/coverage job will provide the current-head artifact.
Gate A
Gate A PASS head=cfe998add675f208737916ef8b9a36f800360cad base=e2355987c9a89fee6a8b9c6b39d9bbb7641ac45d
Anchored on
crates/engine/tests/integration/incredible_hulk_enrage_attacking.rs:109— existing live-combat regression setup usesstate.combat = Some(CombatState { ... })withAttackerInfo.crates/engine/tests/integration/infantry_shield_mobilize_grant.rs:74— existing live-trigger regression readstrigger_definitions.iter_unchecked()and the stored trigger definition.crates/engine/tests/integration/issue_3864_swords_two_targets.rs:36— existing integration coverage builds production target slots withbuild_target_slotsand asserts their legal targets.Final review-impl
Final review-impl PASS head=cfe998add675f208737916ef8b9a36f800360cad
Claimed parse impact
None. The base parser already supports Raubahn's normalized source-power Ward payload; this follow-up changes only regression coverage and backlog bookkeeping.
Scope Expansion
None.
Validation Failures
cargo test -p phase-engine --test integration raubahn_bull_of_ala_mhigo --no-fail-fast— the localrustcprocess compilingphase-enginewas terminated by signal 15 before producing Rust diagnostics. The corrected test source now uses existingCombatStateandtrigger_definitionsfields.CI Failures
The prior CI run failed with E0609 for the nonexistent test fields, which this head fixes. Replacement Rust lint/card-data/test checks are pending on the updated head.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation