Fix Land's Edge — confirm existing CostPaidObjectMatchesFilter building block already handles it (test-only) - #5547
Conversation
Land's Edge ("Discard a card: If the discarded card was a land card,
this enchantment deals 2 damage to target player or planeswalker. Any
player may activate this ability.") is listed in the parser-misparse
backlog's root cause #1 (dropped relative-clause/filter restriction).
Investigation found this is a stale backlog entry, not a live bug: the
existing AbilityCondition::CostPaidObjectMatchesFilter building block
(added 2026-05-04, previously only exercised in ConditionInstead-wrapped
form by Agency Coroner/Surtland Flinger/Stormscale Anarch/Grab the Prize)
already correctly handles this card's bare (non-instead) composition --
condition extraction, chunk-boundary protection for the leading "if",
runtime evaluation via the cost-paid discard's LKI snapshot, and a
separate PlayerFilter::All "any player may activate" mechanism. Zero
production code required.
Adds 2 parser unit tests locking the full parse shape (CR 602.1 +
602.1a + 602.2 + 118.1 + 608.2c + 608.2k + 400.7j) and 5 GameRunner
integration tests proving the runtime behavior: discarding a land deals
2 damage, discarding a nonland deals 0 (ability still resolves), the
discard snapshot binds the specifically-chosen object (not any land in
hand), a non-controller can activate and pays from their own hand
(CR 602.1a), and a sibling without the "any player" clause rejects
non-controller activation (CR 117.3d priority-gate correctly enforced).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM
Confirmed fixed (test-only, no production change) by the preceding commit. Root cause #2 (dropped intervening-if): 606 -> 605 cards; totals rebased to 4760 distinct / 4794 total appearances. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: does not clear the test-only value bar — recommend closing (or reducing to a single assertion on the existing sibling test). This is a 573-line standalone test-only PR for a card that already works, exercising a building block that already has coverage.
🔴 Blocker — no regression guarded; the coverage already exists
By your own (accurate) investigation this is a stale backlog entry, not a live bug — Land's Edge already parses and resolves correctly with zero production changes. The bar for a standalone test-only PR is high because the suite is already very large: a new test earns its place only when it covers a real, otherwise-uncovered risk.
The AbilityCondition::CostPaidObjectMatchesFilter building block this PR exercises is already covered by four sibling integration tests — Agency Coroner, Surtland Flinger, Stormscale Anarch, Grab the Prize (crates/engine/tests/integration/agency_coroner_suspected_cost_paid.rs and siblings). Adding a fifth card's 427-line integration file (lands_edge_discard_land_condition.rs) plus 146 parser-test lines for the same already-covered condition is duplicative coverage — real cost to suite runtime and maintenance, near-zero marginal risk reduction.
🟡 The one kernel worth keeping (if it's real)
Your writeup notes the building block was previously exercised only in ConditionInstead-wrapped form, and Land's Edge is the bare (non-instead) composition. If that bare path is a genuinely distinct, currently-untested branch, it is worth one assertion — folded into the existing agency_coroner_suspected_cost_paid.rs sibling (or a single parser unit test locking the bare parse shape), not a new 427-line GameRunner file.
✅ Credit
The investigation is thorough and correct: Oracle text verified, CR annotations (602.1a / 608.2k / 400.7j) accurate, mod line registered, and correctly diagnosed as a non-bug. The diligence isn't in question — the coverage-value is.
Recommendation: close this unless the bare (non-instead) CostPaidObjectMatchesFilter path is genuinely uncovered; if it is, reduce to a single discriminating assertion on the existing sibling test rather than a new integration file.
Parse changes introduced by this PR · 12 card(s), 10 signature(s) (baseline: main
|
… test Addresses matthewevans' review on phase-rs#5547: the CostPaidObjectMatchesFilter building block this PR exercises already has runtime coverage via four sibling integration tests (Agency Coroner, Surtland Flinger, Stormscale Anarch, Grab the Prize). Adding a fifth card's standalone 427-line integration test file for the same already-covered condition was duplicative coverage with real suite-runtime/maintenance cost and near-zero marginal risk reduction. The one genuinely new thing Land's Edge's shape exercises is a parser-level distinction: a BARE (non-instead) composition of CostPaidObjectMatchesFilter, versus the ConditionInstead-wrapped form all four existing sibling cards use. At runtime this is not actually a new code path -- evaluate_condition's CostPaidObjectMatchesFilter arm already fires for any ability.condition, ConditionInstead or not -- so the distinction is provable at the parser level alone. Removed: - crates/engine/tests/integration/lands_edge_discard_land_condition.rs (427 lines, 5 tests) and its main.rs mod line - lands_edge_without_any_player_clause_has_no_activator_filter (a second parser test exercising the separately-established PlayerFilter::All mechanism, not the bare-condition distinction) Kept: lands_edge_discard_land_condition_parses_as_bare_intervening_if, a single parser unit test that locks the full bare parse shape (cost, condition explicitly NOT ConditionInstead, effect, activator_filter) in one assertion block -- the "single discriminating assertion" the review asked for. Verification note: could not get a fresh cargo test run to complete locally after this reduction -- 5 consecutive attempts were killed mid-compile (a background-build contention issue this fork's ~10 concurrent worktrees have hit repeatedly; the shared WORKLIST.md cargo-lock was held by another agent throughout). This change is a pure deletion (no logic modified) of an already-green test suite; the one retained test is byte-identical to its previously-verified-passing form. CI will provide the authoritative signal. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM
|
Thanks for the review — agreed, and pushed a fix. Reduced from the 573-line standalone submission down to a single retained parser test (
One disclosure: I could not get a fresh local |
matthewevans
left a comment
There was a problem hiding this comment.
✅ Approve — the slimmed revision clears the value bar; it now locks a genuinely uncovered parse path.
My prior review declined the +573 version as duplicative (value-bar): CostPaidObjectMatchesFilter was already covered by four sibling tests. The +117/−4 revision answers that directly by targeting the one composition those siblings don't exercise.
✅ Clean
- Bare vs. wrapped is a real, distinct parse path. Every existing
CostPaidObjectMatchesFiltertest inoracle_effect/tests.rsasserts it wrapped inConditionInstead(Stormscale Anarch, Surtland Flinger, Witch's Oven). Land's Edge is the bare intervening-if form (CR 608.2c) — the condition gates the whole effect, not a "…instead" override. Grep-confirmed: no sibling exercises the unwrappedAbilityCondition::CostPaidObjectMatchesFilter. - Discriminating and non-vacuous. The reach-guard (
abilities.len() == 1) blocks a degenerate pass; thelet AbilityCondition::CostPaidObjectMatchesFilter { .. } = cond else { panic! }would fail on the exactcondition: nullregression this card sat under inparser-misparse-backlog.md— which the PR correctly removes from that list. - Locks the full shape, not just the condition: unfiltered
Discardcost,DealDamage(2)to a player-or-planeswalkerOr, andactivator_filter == Some(PlayerFilter::All)for "Any player may activate this ability."
Recommendation: approve and enqueue. Test-only, CLEAN, fully green (both Rust shards + coverage-gate + parser gate). The coverage is unique and load-bearing.
Summary
Closes a stale misparse-backlog entry for Land's Edge (World Enchantment, Chronicles 1995) with zero production code changes — this is a test-only PR.
Real Oracle text (Scryfall, confirmed this session): "Discard a card: If the discarded card was a land card, this enchantment deals 2 damage to target player or planeswalker. Any player may activate this ability."
The card was listed under
docs/parser-misparse-backlog.md's root cause #2 ("Dropped intervening-if / gating condition"). Investigation found this is a stale backlog entry, not a live bug: the existingAbilityCondition::CostPaidObjectMatchesFilterbuilding block (added 2026-05-04, previously only exercised inConditionInstead-wrapped form by Agency Coroner / Surtland Flinger / Stormscale Anarch / Grab the Prize) already correctly handles Land's Edge's bare (non-instead) composition end-to-end:CostPaidObjectMatchesFilter{filter: Typed([Card, Land])}PlayerFilter::Allmechanism for "Any player may activate this ability" (CR 602.2)This PR adds tests proving the claim, closing the coverage gap — it does not change any production behavior.
Files changed
crates/engine/src/parser/oracle_effect/tests.rs— 2 new parser unit tests locking the full parse shapecrates/engine/tests/integration/lands_edge_discard_land_condition.rs(new) — 5 GameRunner integration testscrates/engine/tests/integration/main.rs— mod linedocs/parser-misparse-backlog.md— removed Land's Edge from root cause chore: update coverage stats and badges #2 (606→605 cards; totals rebased to 4760 distinct / 4794 total)Gate A
Anchored on
crates/engine/src/parser/oracle_effect/conditions.rs(parse_cost_paid_object_matches_filter_condition/parse_cost_paid_object_definite_noun_form) — the pre-existing parser this PR's tests exercise, unmodified.crates/engine/tests/integration/agency_coroner_suspected_cost_paid.rs— the existing sibling integration test the newlands_edge_discard_land_condition.rsis modeled on (sameGameScenario/GameRunnerpattern for a cost-paid-object-conditioned activated ability).CR references
Track
Developer
LLM
Model: claude-sonnet-5
Thinking: high
Verification
cargo fmt --all— cleancargo clippy -p engine --all-targets -- -D warnings— clean, zero warnings (confirmed post-rebase)./scripts/check-parser-combinators.sh— clean, exit 0 (confirmed post-rebase)cargo test -p engine --lib -- lands_edge— 2 passed, 0 failed (confirmed post-rebase)cargo test -p engine --test integration -- lands_edge— 5 passed, 0 failed (confirmed post-rebase)activator_filtergate, not a vacuous no-priority artifact.docs/MagicCompRules.txtdirectly.Validation NOT performed (disclosed honestly)
An independent
/review-implpass was launched against this diff but ran for well over an hour without completing or responding to a mid-run status check, far exceeding every other review pass in this session's pipeline (which typically completed in 5–15 minutes). Rather than block indefinitely, I proceeded to ship without that independent review round. In its place: the implementing agent's own thorough self-verification (both new test suites passing,cargo fmt/clippy/check-parser-combinators.shall clean, all 9 CR citations individually verified against the Comprehensive Rules text), plus this PR author's own post-rebase re-confirmation of every one of those checks from a clean rebase onto currentmain. Given the change is test-only (zero production code touched) and both new test suites independently exercise the real parse + realapply()/activation pipeline (not shortcuts), the residual risk from the skipped independent review is judged low, but I am not claiming a review happened that did not finish.Scope Expansion
None.
Validation Failures
None (the skipped
/review-implpass is disclosed above under Validation NOT performed, not as a failure of a check that ran and failed).CI Failures
None.