Skip to content

fix(parser): bind perpetual cost grants to prior referents#5098

Open
ntindle wants to merge 1 commit into
phase-rs:mainfrom
ntindle:codex/misparse-root19-perpetual-cost
Open

fix(parser): bind perpetual cost grants to prior referents#5098
ntindle wants to merge 1 commit into
phase-rs:mainfrom
ntindle:codex/misparse-root19-perpetual-cost

Conversation

@ntindle

@ntindle ntindle commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Parse bound It perpetually gains "This spell costs ..." clauses as ApplyPerpetual cost modifiers when the effect chain already has a prior object referent.
  • Keep standalone It perpetually gains ... honest-red instead of silently binding it to the source.
  • Remove Bloodsprout Talisman, Geistchanneler, and Nightclub Bouncer from root cause chore: update coverage stats and badges #19 in docs/parser-misparse-backlog.md.

Root Cause

These Alchemy clauses were falling through to generic continuous-modification parsing. That produced GenericEffect entries with UntilEndOfTurn duration instead of persistent ApplyPerpetual cost modifications. The missing piece was the bound-pronoun subject form after a prior ChooseFromZone or targeted clause.

Card-Data Audit

Full raw card-data.json before/after exports both contained 35,397 cards. The only raw card entries that changed were:

  • bloodsprout talisman: GenericEffect / UntilEndOfTurn / Any -> ApplyPerpetual / ParentTarget / ModifyCost Reduce {1}
  • geistchanneler: GenericEffect / UntilEndOfTurn / Any -> ApplyPerpetual / ParentTarget / ModifyCost Reduce {2}
  • nightclub bouncer: GenericEffect / UntilEndOfTurn / ParentTarget -> ApplyPerpetual / ParentTarget / ModifyCost Raise {2}

Related perpetual-cost cards that still require separate root-cause fixes, such as Chronicler of Worship, Garruk, Wrath of the Wilds, Lumbering Lightshield, and Thought Rattle, were intentionally unchanged.

coverage-parse-diff reports no parse_details changes because this PR changes the serialized effect shape rather than the coverage detail text.

Verification

  • cargo fmt --all
  • ./scripts/check-parser-combinators.sh
  • RUSTC_WRAPPER= ZIG_GLOBAL_CACHE_DIR=/tmp/phase-zig-global-cache ZIG_LOCAL_CACHE_DIR=/tmp/phase-zig-local-cache cargo test -p engine --features cli --lib perpetual_modify_cost
  • RUSTC_WRAPPER= ZIG_GLOBAL_CACHE_DIR=/tmp/phase-zig-global-cache ZIG_LOCAL_CACHE_DIR=/tmp/phase-zig-local-cache cargo clippy -p engine --all-targets -- -D warnings
  • Full raw card-data audit against origin/main: changed exactly bloodsprout talisman, geistchanneler, and nightclub bouncer
  • RUSTC_WRAPPER= ZIG_GLOBAL_CACHE_DIR=/tmp/phase-zig-global-cache ZIG_LOCAL_CACHE_DIR=/tmp/phase-zig-local-cache cargo run -p engine --features cli --bin card-data-validate -- /tmp/phase-root19-full/head/card-data.json
  • RUSTC_WRAPPER= ZIG_GLOBAL_CACHE_DIR=/tmp/phase-zig-global-cache ZIG_LOCAL_CACHE_DIR=/tmp/phase-zig-local-cache cargo run -p engine --features cli --bin coverage-parse-diff -- /tmp/phase-root19-full/base/card-data.json /tmp/phase-root19-full/head/card-data.json --base-sha origin/main --markdown /tmp/phase-root19-full/coverage-parse-diff.md --json /tmp/phase-root19-full/coverage-parse-diff.json

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the parser misparse backlog documentation (docs/parser-misparse-backlog.md) to reflect the resolution of several cards. Specifically, it decrements the total count of implicated cards and updates the 'Perpetual (Alchemy) duration mis-mapped to UntilEndOfTurn' backlog item by removing Bloodsprout Talisman, Geistchanneler, and Nightclub Bouncer from the list, reducing its count from 71 to 68. There are no review comments, and I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@ntindle ntindle marked this pull request as ready for review July 4, 2026 19:35
@ntindle ntindle requested a review from matthewevans as a code owner July 4, 2026 19:35
@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 3 card(s), 3 signature(s) (baseline: main 201c535f9d33)

3 card(s) · ability/ApplyPerpetual · added: ApplyPerpetual

Examples: Bloodsprout Talisman, Geistchanneler, Nightclub Bouncer

2 card(s) · ability/ReduceCost · removed: ReduceCost (affects=any target, duration=until end of turn, grants=ReduceCost, target=any target)

Examples: Bloodsprout Talisman, Geistchanneler

1 card(s) · ability/RaiseCost · removed: RaiseCost (affects=any target, duration=until end of turn, grants=RaiseCost, target=parent target)

Examples: Nightclub Bouncer

1 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

@matthewevans

Copy link
Copy Markdown
Member

Holding review on this head because GitHub currently reports the branch as conflicting with main (mergeStateStatus: DIRTY, mergeable: CONFLICTING). The parse-diff evidence is present and checks are green, but I need the branch brought current/resolved before I can review the actual mergeable diff. Please rebase or merge main and push a new head; I will review it after the conflict is cleared.

@matthewevans matthewevans self-assigned this Jul 5, 2026
@matthewevans matthewevans added the bug Bug fix label Jul 5, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. I found one remaining behavior blocker, and the branch is also currently conflicting with main:

[MED] Empty ChooseFromZone paths can apply the perpetual cost grant to the source. Evidence: crates/engine/src/parser/oracle_effect/mod.rs:7216, crates/engine/src/game/effects/choose_from_zone.rs:79, and crates/engine/src/game/effects/perpetual.rs:104. Why it matters: Bloodsprout Talisman / Geistchanneler with no eligible card should do nothing, but the new ParentTarget sub-ability can resolve with no inherited targets and fall back to modifying the source. Suggested fix: make ApplyPerpetual { target: ParentTarget } no-op when no parent/choice/event object exists, and add a runtime empty-choice regression.

Also, current head is mergeable: CONFLICTING / mergeStateStatus: DIRTY; the conflict needs to be resolved before this can be considered for queueing.

@matthewevans matthewevans removed their assignment Jul 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants