Skip to content

Fix Zenith Chronicler draw on first multicolored spell (fixes #4829) - #4851

Merged
matthewevans merged 4 commits into
phase-rs:mainfrom
kiannidev:fix/4829-zenith-chronicler-draw
Jul 2, 2026
Merged

Fix Zenith Chronicler draw on first multicolored spell (fixes #4829)#4851
matthewevans merged 4 commits into
phase-rs:mainfrom
kiannidev:fix/4829-zenith-chronicler-draw

Conversation

@kiannidev

Copy link
Copy Markdown
Contributor

Summary

  • Map Players::Other(Trigger_ThatPlayer) to PlayerFilter::AllExcept { TriggeringPlayer } in mtgish import so "each other player" excludes the caster.
  • Rewrite any-player spell-cast trigger bodies that say "each other player" away from controller-relative Opponent scope.
  • Parse "multicolored" as an NthSpellThisTurn qualifier filter.

Test plan

  • cargo test -p engine --test integration zenith_chronicler
  • cargo test -p mtgish-import --lib each_other_player

Made with Cursor

Map mtgish Other(Trigger_ThatPlayer) to AllExcept TriggeringPlayer, rewrite
any-player spell-cast triggers that say each other player, and parse
multicolored as an Nth-spell qualifier filter.

Co-authored-by: Cursor <cursoragent@cursor.com>
@kiannidev
kiannidev requested a review from matthewevans as a code owner July 2, 2026 02:10
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@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 focused Zenith Chronicler fix. I’m holding this for two changes.

  1. type_only_filter hand-builds the multicolored ColorCount filter with a direct string equality branch. This parser already has oracle_nom::filter::parse_color_property, which is the existing combinator for colorless / monocolored / multicolored. Please route this through that building block instead of adding another hand-rolled color-property construction path.

  2. Please add discriminating coverage for the negative side of the bug: a player casting their first monocolored spell should not trigger Zenith Chronicler. The current runtime test proves the positive multicolored case and the “each other player” scope, but it would not catch a filter that accepts the wrong spell class. The parser test should also assert the exact ColorCount { comparator: GE, count: 2 } filter rather than only filter.is_some().

@matthewevans matthewevans added the bug Bug fix label Jul 2, 2026
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 3 card(s), 4 signature(s) (baseline: main 8e2f7cf3497a)

1 card(s) · trigger/SpellCast · field constraint: on your 1th spell this turnon your 1th multicolored card spell this turn

Examples: Zenith Chronicler

1 card(s) · trigger/SpellCast · field watches: mv 7+colorless card + mv 7+

Examples: Sanctum of Ugin

1 card(s) · trigger/SpellCast · field watches: blue card

Examples: God-Pharaoh's Faithful

1 card(s) · trigger/SpellCast · field watches: multicolored card

Examples: Zenith Chronicler

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

kiannidev and others added 2 commits July 2, 2026 04:55
Route bare multicolored spell qualifiers through parse_color_property with
Card typing, assert the exact ColorCount filter in the parser test, and add
a monocolored negative runtime case.

Co-authored-by: Cursor <cursoragent@cursor.com>
# Conflicts:
#	crates/engine/tests/integration/main.rs
@matthewevans matthewevans self-assigned this Jul 2, 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.

Reviewed the updated head after the requested changes. The multicolored qualifier now goes through the existing parse_color_property building block, the trigger scope rewrite handles the “a player ... each other player” case as AllExcept { TriggeringPlayer }, and the tests cover parser shape plus positive/negative runtime behavior. I also resolved the current main.rs integration-test mod-line conflict and verified formatting/parser-gate during the merge-forward commit; CI can finish under merge-when-ready.

@matthewevans
matthewevans enabled auto-merge July 2, 2026 03:04
@matthewevans matthewevans removed their assignment Jul 2, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jul 2, 2026
@matthewevans matthewevans self-assigned this Jul 2, 2026
@matthewevans
matthewevans removed this pull request from the merge queue due to a manual request Jul 2, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jul 2, 2026
@matthewevans
matthewevans removed this pull request from the merge queue due to a manual request Jul 2, 2026
@matthewevans matthewevans removed their assignment Jul 2, 2026
# Conflicts:
#	crates/engine/tests/integration/main.rs
@matthewevans
matthewevans enabled auto-merge July 2, 2026 03:33

@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.

Re-approving current merge-forward head cff6dcc. The only post-review change is the deterministic origin/main merge resolving the integration main.rs module list after #4852 landed.

@matthewevans
matthewevans added this pull request to the merge queue Jul 2, 2026
Merged via the queue into phase-rs:main with commit 9443e4b Jul 2, 2026
11 checks passed
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