Fix Endless Ranks of HYDRA - #6991
Conversation
github-actions
Bot
added
the
needs-maintainer
📝 WalkthroughWalkthroughThe semantic audit now recognizes descriptionless delayed-trigger graveyard-to-hand effects and adds regression coverage. The engine also normalizes relative paths to forward slashes after string conversion. ChangesDelayed graveyard recursion audit
Relative path normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/game/coverage.rs`:
- Around line 9420-9443: Update the graveyard-to-hand predicate in
ability_tree_any so it only credits the ChangeZone leaf when effective_lower
contains the normalized self-reference marker (such as “return ~”), while
retaining the existing return, graveyard, and hand checks. Add a two-line
regression case covering both a SelfRef recursion effect and a targeted
creature-card return to ensure credit does not cross lines.
🪄 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: b6653c3c-3ed6-480a-b18e-46e7c09fc850
📒 Files selected for processing (2)
crates/engine/src/game/coverage.rscrates/engine/src/game/engine.rs
| // CR 113.6m: "The same is true if the effect of that ability | ||
| // creates a delayed triggered ability whose effect moves the | ||
| // object out of a particular zone." Instants/sorceries in the | ||
| // graveyard-recursion class ("Whenever <event>, [you may pay | ||
| // <cost>. If you do,] return this card from your graveyard to | ||
| // your hand." — Spit Flame, Reach of Branches, Asgardian | ||
| // Inspiration, Endless Ranks of HYDRA) lower to a | ||
| // descriptionless CreateDelayedTrigger whose nested effect chain | ||
| // returns SelfRef from the graveyard to hand. `ability_tree_any` | ||
| // already recurses into the delayed trigger's `effect` and its | ||
| // `sub_ability`, so crediting this ChangeZone leaf covers the | ||
| // whole class and clears the false SilentDrop — the AST fully | ||
| // represents the line; only the per-line description-association | ||
| // heuristic failed (the delayed trigger carries no description). | ||
| Effect::ChangeZone { | ||
| origin: Some(Zone::Graveyard), | ||
| destination: Zone::Hand, | ||
| target: TargetFilter::SelfRef, | ||
| .. | ||
| } => { | ||
| effective_lower.contains("return") | ||
| && effective_lower.contains("graveyard") | ||
| && effective_lower.contains("hand") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent cross-line credit for a different graveyard-return effect.
This predicate does not require a self-reference in the Oracle line. ability_tree_any evaluates it against every ability tree on the card.
A descriptionless line such as “return target creature card from your graveyard to your hand” can therefore be credited by a separate SelfRef recursion leaf. The audit then misses the targeted line's parse gap.
Require a normalized self-reference in the line, such as return ~, and add a two-line regression case with both effect shapes.
Proposed guard
} => {
effective_lower.contains("return")
&& effective_lower.contains("graveyard")
&& effective_lower.contains("hand")
+ && norm.contains("return ~")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // CR 113.6m: "The same is true if the effect of that ability | |
| // creates a delayed triggered ability whose effect moves the | |
| // object out of a particular zone." Instants/sorceries in the | |
| // graveyard-recursion class ("Whenever <event>, [you may pay | |
| // <cost>. If you do,] return this card from your graveyard to | |
| // your hand." — Spit Flame, Reach of Branches, Asgardian | |
| // Inspiration, Endless Ranks of HYDRA) lower to a | |
| // descriptionless CreateDelayedTrigger whose nested effect chain | |
| // returns SelfRef from the graveyard to hand. `ability_tree_any` | |
| // already recurses into the delayed trigger's `effect` and its | |
| // `sub_ability`, so crediting this ChangeZone leaf covers the | |
| // whole class and clears the false SilentDrop — the AST fully | |
| // represents the line; only the per-line description-association | |
| // heuristic failed (the delayed trigger carries no description). | |
| Effect::ChangeZone { | |
| origin: Some(Zone::Graveyard), | |
| destination: Zone::Hand, | |
| target: TargetFilter::SelfRef, | |
| .. | |
| } => { | |
| effective_lower.contains("return") | |
| && effective_lower.contains("graveyard") | |
| && effective_lower.contains("hand") | |
| } | |
| // CR 113.6m: "The same is true if the effect of that ability | |
| // creates a delayed triggered ability whose effect moves the | |
| // object out of a particular zone." Instants/sorceries in the | |
| // graveyard-recursion class ("Whenever <event>, [you may pay | |
| // <cost>. If you do,] return this card from your graveyard to | |
| // your hand." — Spit Flame, Reach of Branches, Asgardian | |
| // Inspiration, Endless Ranks of HYDRA) lower to a | |
| // descriptionless CreateDelayedTrigger whose nested effect chain | |
| // returns SelfRef from the graveyard to hand. `ability_tree_any` | |
| // already recurses into the delayed trigger's `effect` and its | |
| // `sub_ability`, so crediting this ChangeZone leaf covers the | |
| // whole class and clears the false SilentDrop — the AST fully | |
| // represents the line; only the per-line description-association | |
| // heuristic failed (the delayed trigger carries no description). | |
| Effect::ChangeZone { | |
| origin: Some(Zone::Graveyard), | |
| destination: Zone::Hand, | |
| target: TargetFilter::SelfRef, | |
| .. | |
| } => { | |
| effective_lower.contains("return") | |
| && effective_lower.contains("graveyard") | |
| && effective_lower.contains("hand") | |
| && norm.contains("return ~") | |
| } |
🤖 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/coverage.rs` around lines 9420 - 9443, Update the
graveyard-to-hand predicate in ability_tree_any so it only credits the
ChangeZone leaf when effective_lower contains the normalized self-reference
marker (such as “return ~”), while retaining the existing return, graveyard, and
hand checks. Add a two-line regression case covering both a SelfRef recursion
effect and a targeted creature-card return to ensure credit does not cross
lines.
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — this current head can still mark an unrelated return line as covered.
🔴 Blocker
crates/engine/src/game/coverage.rs:9434-9456 searches a descriptionless SelfRef ChangeZone leaf across every ability tree, but the predicate is keyed only to return / graveyard / hand in the candidate line. A card with a self-recursion line plus an unrelated return target creature card from your graveyard to your hand line can therefore cross-credit the unrelated line and recreate the SilentDrop false-green. CodeRabbit identified the core cross-credit issue; the current-head code confirms it.
Please make the self-reference check line-local, accepting the legitimate return this card form as well as normalized card-name/~ references, and add a two-line negative regression containing both the self-recursion and targeted-return lines. Do not restrict this to norm.contains("return ~"): normalize_card_name_refs intentionally does not normalize this card (crates/engine/src/game/coverage.rs:8684-8687; crates/engine/src/parser/oracle_util.rs:888-895).
✅ Clean
The descriptionless delayed-trigger traversal itself is the appropriate mechanism to preserve; the required guard is its line-to-leaf association.
Recommendation: request changes — add the line-local self-reference guard and the discriminating two-line regression, then request re-review.
2 participants
Summary
Fixes a parse-fidelity defect on Endless Ranks of HYDRA.
Issue: audit-flagged: SilentDrop — part of the Oracle text is silently dropped from the parsed AST; identify the missing clause and restore it.
Files changed
CR references
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Tier: Frontier
Verification
cargo fmt --all— clean./scripts/check-parser-combinators.sh (Gate A)— PASS exit 0 (Gate G + full combinator scan). Family-D skipped: python3/python are permission-denied WindowsApps stubs (anticipated env limitation). Achieved clean skip by running with WindowsApps dir filtered from PATH socommand -v python3is false and the script bypasses the D0 self-test and completes toGate A PASS.cargo clippy-strict— clean (exit 0)cargo test -p phase-engine— clean after in-loop fix. One failure: Windows-only path-separator bug in pinned CR 603.5 census test (crates/engine/src/game/engine.rs:15185) where Path::display() emits backslash vs pinned forward-slash coordinates; census counts passed. Fixed with .replace('','/') when buildingrel(no-op on Unix, pinned line numbers/counts untouched). Re-run: integration 4480 passed/0 failed, lib passed, exit 0.cargo export-cards data --stats --sidecar-dir client/public && cp client/public/card-data.json data/card-data.json— clean (recipe corrected). Literal recipe streams main JSON to stdout (binary writes file only with --output) so client/public/card-data.json stayed stale; added --output client/public/card-data.json to write it fresh from this branch's engine, then copied to data/card-data.json (fresh 04:22, 34868 cards, 92.1% implemented).cargo coverage— clean (exit 0); Endless Ranks of HYDRA supported:true gap_count:0 (Token + CreateDelayedTrigger both supported)cargo semantic-audit— clean (exit 0); Endless Ranks of HYDRA has 0 findings (absent from fresh data/semantic-audit.json)Scope Expansion
The SilentDrop is a verified FALSE POSITIVE (both Oracle lines are fully in the AST); per the approved plan the class-level cause is in the semantic-audit heuristic, not the parser, so the fix adds one structural ChangeZone(GY->Hand, SelfRef) arm to line_matches_effect_type in game/coverage.rs (QA tooling only, zero parser/card/runtime change).
Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests