Add static forced-attack requirement parsing ("attacks each combat if able unless") — Galactus - #6885
Add static forced-attack requirement parsing ("attacks each combat if able unless") — Galactus#6885JacobWoodson wants to merge 5 commits into
Conversation
…tacks [player] each combat if able unless [cond]") mechanic
github-actions
Bot
added
the
needs-maintainer
📝 WalkthroughWalkthroughThe PR adds fixed and dynamically matched defenders for ChangesForced-attack defender flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant OracleText
participant StaticParser
participant Combat
participant AttackDeclaration
OracleText->>StaticParser: parse forced-attack text
StaticParser->>Combat: create MustAttackPlayer with Matching defender
Combat->>Combat: evaluate the current player filter
Combat->>AttackDeclaration: enforce attack against the selected opponent
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Generated for head Parse changes introduced by this PR · 1 card(s), 2 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: 2
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle_static/evasion.rs (1)
2522-2543: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared "trailing unless-or-fail" pattern.
parse_forced_attack_defender_static_body(Lines 2536-2542) repeats the same shape as the existingparse_subject_combat_rule_static(Lines 2453-2464, unchanged): strip an optional trailing period, return the definition unchanged if nothing remains, otherwise require a recognizedunlessclause viaparse_unless_static_conditionor fail closed. A shared helper removes this duplication and keeps the two rule-static parsers in lockstep as the "unless" handling evolves.♻️ Proposed helper extraction
+fn attach_mandatory_unless<'a>( + mut def: StaticDefinition, + rest: &'a str, + text: &str, + lower: &str, +) -> Option<StaticDefinition> { + if rest.trim().is_empty() { + return Some(def); + } + let tp = TextPair::new(text, lower); + def.condition = Some(super::shared::parse_unless_static_condition(&tp)?); + Some(def) +} + fn parse_forced_attack_defender_static_body(text: &str) -> Option<StaticDefinition> { let lower = text.to_lowercase(); let (subject_lower, filter, rest) = nom_primitives::scan_preceded(&lower, parse_attacks_required_defender_nom)?; let subject = text[..subject_lower.len()].trim(); let affected = parse_rule_static_subject_filter(subject)?; let mut def = StaticDefinition::new(StaticMode::MustAttackPlayer { player: RequiredDefender::Matching { filter }, }) .affected(affected) .description(text.to_string()); let (rest, _) = opt(tag::<_, _, OracleError<'_>>(".")).parse(rest).ok()?; - if rest.trim().is_empty() { - return Some(def); - } - let tp = TextPair::new(text, &lower); - def.condition = Some(super::shared::parse_unless_static_condition(&tp)?); - Some(def) + attach_mandatory_unless(def, rest, text, &lower) }Apply the same call at the end of
parse_subject_combat_rule_staticas a follow-up.🤖 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/parser/oracle_static/evasion.rs` around lines 2522 - 2543, Extract the repeated trailing-unless handling into a shared helper near parse_subject_combat_rule_static, covering optional period removal, empty-tail success, and parse_unless_static_condition failure. Update both parse_subject_combat_rule_static and parse_forced_attack_defender_static_body to use this helper while preserving their existing StaticDefinition construction and conditional behavior.
🤖 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_static/evasion.rs`:
- Around line 2467-2482: Update both documentation comments associated with
parse_required_defender_selector and the shared most-life restriction to cite CR
102.2 for opponents in free-for-all games and CR 102.3 for team multiplayer, and
cite CR 508.1b for choosing among tied legal defending players. Remove the
incorrect description of CR 608.2d as the tie-resolution rule while preserving
the parser behavior.
In `@crates/engine/tests/integration/galactus_forced_attack_most_life.rs`:
- Around line 148-158: Update galactus_required_defender_reevaluated_live to use
one parked_galactus fixture initialized with P1 at 30 life and P2 at 20 life,
then change the life totals before invoking declare. Through the production
GameAction::DeclareAttackers path, verify P1 is rejected after the swap and P2
is accepted in the same game state, preserving the combat-state assertion.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_static/evasion.rs`:
- Around line 2522-2543: Extract the repeated trailing-unless handling into a
shared helper near parse_subject_combat_rule_static, covering optional period
removal, empty-tail success, and parse_unless_static_condition failure. Update
both parse_subject_combat_rule_static and
parse_forced_attack_defender_static_body to use this helper while preserving
their existing StaticDefinition construction and conditional behavior.
🪄 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: 16da09b4-c0da-45eb-b40e-3ea2e765224a
📒 Files selected for processing (17)
crates/engine/src/ai_support/candidates.rscrates/engine/src/database/encore_tests.rscrates/engine/src/game/combat.rscrates/engine/src/game/effects/encore.rscrates/engine/src/game/effects/force_attack.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_static/dispatch.rscrates/engine/src/parser/oracle_static/evasion.rscrates/engine/src/parser/oracle_static/mod.rscrates/engine/src/parser/oracle_static/tests.rscrates/engine/src/types/game_state.rscrates/engine/src/types/statics.rscrates/engine/tests/integration/galactus_forced_attack_most_life.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/must_attack_player_attribution.rscrates/engine/tests/integration/rules/combat.rscrates/phase-ai/tests/scenarios.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the attack-requirement representation and parser handling need correction before this can merge.
🟡 Medium — tied defender requirements lose their directive grouping
crates/engine/src/game/combat.rs:3118-3194 and :3936-3942 flatten Matching tied defenders into player pairs, deduplicate them, and create one requirement per player. That loses the directive's alternative-set grouping and multiplicity. For example, Galactus tying P1/P2 together with a fixed P1 requirement can consequently permit P2 even though the combined requirements are not satisfied. This is not rules-correct under CR 508.1d.
Represent a matching directive as one alternative-set requirement rather than independent per-player requirements, and add a production-pipeline regression covering a tied-most-life P1/P2 directive together with a fixed-P1 requirement.
🟡 Medium — parser silently drops unmodelled riders
crates/engine/src/game/evasion.rs:2533-2542 accepts any nonempty remainder, then parses an unless clause from the original text. An unmodelled rider before unless is therefore silently discarded. Require an all-consuming recognized suffix, or return an honest unsupported result, and add a reach-guarded negative test proving the unrecognised rider cannot be swallowed.
🔵 Low — CR citation is inapplicable
crates/engine/src/game/evasion.rs:2467-2472 cites CR 608.2d for static attack-requirement tie resolution. CR 608.2d governs choices made during spell or ability resolution, not this static requirement behavior. Remove or replace the citation with the applicable static/attack-requirement rationale.
Recommendation: revise the requirement model, parser suffix handling, and rule annotation, then resubmit for review.
…ernative-sets Addresses the PR phase-rs#6885 review (matthewevans + CodeRabbit). 1. CR 508.1d - tied defender grouping (combat.rs). A RequiredDefender::Matching directive that resolves to multiple players (e.g. every opponent tied for the most life) was flattened into per-player requirements and deduped into the shared fixed-player set, losing the alternative-set grouping: a Galactus most-life tie {P1,P2} plus a fixed-P1 requirement could wrongly permit P2. Introduce AttackRequirement::MustAttackAnyOf (obeyed by attacking ANY current member) and ResolvedRequiredDefender so each directive stays one requirement - a Matching directive is a single alternative-set, kept distinct from any coexisting Fixed requirement so the fixed requirement retains its CR 701.15c multiplicity and the max-requirement solver forces the fixed member. 2. Parser rider-swallow (oracle_static/evasion.rs). The forced-attack body parsed the unless clause from the whole line, so an unmodelled rider between the recurring-combat suffix and unless was silently dropped. Require the tail to begin exactly with an unless gate, else decline (honest unsupported result). 3. CR annotations. The static attack-requirement selector cited CR 608.2d (resolution-time choices) for tie resolution; replace with CR 508.1b (active player choosing among tied legal defenders) + CR 508.1d, and cite CR 102.2 (two-player) / CR 102.3 (team multiplayer) for opponent scoping. Tests: solver regression (matching_tie_plus_fixed_forces_the_fixed_defender) plus two brute-force-oracle cases for MustAttackAnyOf; a reach-guarded negative parser test proving an unmodelled rider is not swallowed; and the live re-evaluation integration test now mutates life totals in one fixture and drives reject-then-accept through GameAction::DeclareAttackers. Verified: cargo fmt, parser combinator Gate A, clippy (-D warnings), full phase-engine suite (4374 passed), Galactus integration suite (5 passed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@matthewevans thanks for the review — pushed
Also rewrote Verified: |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the revised alternative-set model fixes the prior grouping issue, but the forced-attack parser still admits an unimplemented unless condition as supported coverage.
🔴 Blocker
crates/engine/src/parser/oracle_static/evasion.rs:2556-2558 checks only that the tail starts with unless , then delegates the whole line to parse_unless_static_condition. That helper deliberately returns StaticCondition::Not { Unrecognized { .. } } for an unknown condition (crates/engine/src/parser/oracle_static/shared.rs:3286-3292). The runtime consequently fails closed because Unrecognized evaluates true (crates/engine/src/game/layers.rs:1427), but the coverage gates only recognize a top-level Unrecognized condition (crates/engine/src/game/coverage.rs:5999-6001, :7021-7023).
So a line such as ~ attacks an opponent each combat if able unless you satisfy an unmodelled condition. is accepted as a MustAttackPlayer static, behaves as permanently inactive, and is reported supported rather than honestly unsupported. The new test at crates/engine/src/parser/oracle_static/tests.rs:8956-8981 only covers an extra rider before unless, not an unknown condition after it.
Please make this parser path decline (or otherwise preserve an unimplemented marker the coverage predicates detect) whenever the unless condition is not fully modeled, and add a reach-guarded regression for that suffix shape. This must apply to the grammar class, not just Galactus.
✅ Clean
The current head does preserve tied defender grouping as one alternative-set requirement, and the live re-evaluation regression now exercises the production declaration path in a single fixture. The CR 508.1d rule text was checked locally: it requires the declaration to obey the maximum possible number of requirements.
Recommendation: request changes for the coverage-honesty gap above, then rerun the current-head parse-diff and CI before approval.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the current head still reports an unmodelled forced-attack unless condition as supported.
🔴 Blocker
crates/engine/src/parser/oracle_static/evasion.rs:2556-2559 accepts every tail beginning unless and stores parse_unless_static_condition. For an unknown inner condition, that helper returns StaticCondition::Not { Unrecognized { .. } } at crates/engine/src/parser/oracle_static/shared.rs:3286-3292. static_has_unimplemented_parts detects only a top-level Unrecognized condition at crates/engine/src/game/coverage.rs:5999-6001; it therefore marks this unsupported grammar as supported. At runtime Unrecognized evaluates true (crates/engine/src/game/layers.rs:1427), so the enclosing Not makes the attack requirement permanently inactive.
The existing negative test at crates/engine/src/parser/oracle_static/tests.rs:8967-8989 only rejects an extra rider before unless. Add a reach-guarded grammar-class test for an unknown condition after unless (for example, ~ attacks an opponent each combat if able unless you satisfy an unmodelled condition.), and make that path decline or retain a coverage-visible unimplemented marker.
✅ Clean
The current parse-diff is bound to 7d30f917221321649473e6603d9f5eaebd326c93 and shows the intended one-card Galactus change. The alternative-set representation preserves a tied Matching directive separately from a fixed defender, and the integration tests exercise live defender re-evaluation through GameAction::DeclareAttackers.
Recommendation: fix the coverage-honesty gap and provide a fresh current-head parser/CI artifact before another approval review.
… fully modeled Addresses the PR phase-rs#6885 re-review blocker (matthewevans). The forced-attack body accepted any tail beginning `unless ` and stored the result of parse_unless_static_condition. For an unrecognized inner condition that helper returns Not(Unrecognized), which (a) the coverage detector (static_has_unimplemented_parts) misses because it only flags a TOP-LEVEL Unrecognized, so the card is falsely reported supported, and (b) evaluates permanently false at runtime (Unrecognized is true; the wrapping Not negates it), silently disabling the whole attack requirement. Decline the parse when the resolved condition contains an Unrecognized clause anywhere in its tree (recursing through Not/And/Or), so a not-fully-modeled `unless` gate leaves the line honestly unsupported (coverage red) instead of shipping a broken, falsely-supported static. Galactus's own gate is fully modeled and still parses. Adds forced_attack_defender_static_rejects_unmodelled_unless_condition: a reach-guarded grammar-class test proving an unknown condition AFTER `unless` declines while a modeled gate still parses (one level deeper than the existing rider test, which guards text BEFORE `unless`). Verified on the current head (7d30f91): cargo fmt, parser combinator Gate A, clippy (-D warnings), full phase-engine suite (18359 lib + 4400 integration tests, 0 failed). Card-data coverage/semantic-audit run in the CI "Card data" job (the card-data/AtomicCards inputs are gitignored and R2-served, absent in a fresh checkout). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — current head 111b2d990f2e20cfef0d134dbf473b5ea33e92ad still has a coverage-honesty blocker.
parse_forced_attack_defender_static_body accepts every tail beginning unless and installs parse_unless_static_condition (crates/engine/src/parser/oracle_static/evasion.rs:2556-2559). For an unmodelled inner condition, that helper emits Not(Unrecognized) (crates/engine/src/parser/oracle_static/shared.rs:3286-3292). Coverage detects only a top-level Unrecognized (crates/engine/src/game/coverage.rs:5999-6001), while runtime evaluates Unrecognized true (crates/engine/src/game/layers.rs:1573), so the enclosing Not makes the attack requirement inactive yet reports the card supported.
Make an unrecognised unless condition decline this static (or retain a coverage-visible unsupported marker) and add a reach-guarded grammar-class regression for ~ attacks an opponent each combat if able unless you satisfy an unmodelled condition. The existing test only rejects a rider before unless.
The only parse-diff artifact is bound to prior head 7d30f917..., not this head: #6885 (comment). Please provide fresh current-head parse-diff and completed checks after the semantic fix.
|
@matthewevans thanks for the careful re-review — good catch on the coverage-honesty gap. Pushed Fix for the 🔴 blocker. Added the grammar-class test you asked for — Verification on the current head ( Ready for another look when you have a chance. |
|
Maintainer hold for current head |
JacobWoodson
changed the title
2 participants
Summary
Adds engine support for the Static forced-attack requirement parsing ("[creature] attacks [player] each combat if able unless [cond]") mechanic, unlocking 1 card(s) in this deck.
Cards unlocked
Files changed
CR references
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Tier: Frontier
Verification
cargo fmt --all— clean (exit 0)./scripts/check-parser-combinators.sh (Gate A)— clean (Gate G PASS + Gate A PASS incl. cross-product detector). Initial run failed only becausecommand -v python3resolved the WindowsApps stub (Permission denied); fixed in-loop by prepending the real msys2 python3 (/c/msys64/mingw64/bin) to PATH — a session env fix, no repo change — after which the (D0) self-test and full gate passcargo clippy-strict (Tilt down -> fallback)— clean (exit 0, no warnings/errors)cargo test -p phase-engine (Tilt down -> fallback)— clean (4374 passed, 0 failed, 2 ignored; all 9 Galactus forced-attack tests pass, incl. galactus_forced_attack_most_life integration suite + parser static tests)./scripts/gen-card-data.sh (Tilt down -> fallback)— FAILED (environment-only, deterministic across 2 attempts): scripts/fetch-token-sets.sh:23mapfile -t CODES < <(...)needs /dev/fd, which is absent in this Git Bash; /dev is read-only so it cannot be symlinked, and msys2 bash hangs when invoked. Fails at the token-fetch substep (a cached no-op; 333 token sets already present) BEFORE any card generation. Not a mechanic/repo defect. Worked around by regenerating card-data.json via the identical oracle-gen tool the script uses (34868 cards, 92.1%), placed at data/card-data.json + client/public/card-data.json so steps 4-5 ran against current-code datacargo coverage— clean (exit 0); Galactus, Devourer of Worlds -> supported=true, gap_count=0cargo semantic-audit— clean (exit 0); 284 flagged_cards, Galactus NOT among them (0 findings)Scope Expansion
None.
Validation Failures
See review/cross-check notes.
CI Failures
mapfile -t CODES < <(jq ... | tr -d '\r' | sort -u)): bash process substitution requires /dev/fd, which does not exist in this Bash environment (only /proc/self/fd exists; /dev is a read-only filesystem so /dev/fd cannot be created; the msys2 bash that has /dev/fd hangs when launched from the tool shell). This is an ENVIRONMENT limitation in a cached-no-op token-fetch substep (333 token sets already present locally) that runs BEFORE any card generation — it is NOT a defect in the forced-attack mechanic or repo code, and I did not edit the repo script (multi-agent safety). Deterministic across 2 attempts. Mitigation: regenerated card-data.json with the exact oracle-gen tool gen-card-data.sh invokes (reading data/mtgjson/AtomicCards.json, which downloaded successfully), so the downstream coverage/semantic-audit steps ran against current-code data and both PASS. passed=false is set solely because this literal command did not exit clean per the strict rule; every substantive mechanic check is green (fmt, Gate A, clippy, 4374 engine tests incl. 9 Galactus tests, coverage supported:true gap:0, semantic-audit clean).Summary by CodeRabbit
New Features
Bug Fixes