Skip to content

fix(parser): close the sentence after an emblem's granted-ability quote (Nissa)#5465

Merged
matthewevans merged 3 commits into
phase-rs:mainfrom
dhgoal:fix/nissa-emblem-clause-split-5282
Jul 10, 2026
Merged

fix(parser): close the sentence after an emblem's granted-ability quote (Nissa)#5465
matthewevans merged 3 commits into
phase-rs:mainfrom
dhgoal:fix/nissa-emblem-clause-split-5282

Conversation

@dhgoal

@dhgoal dhgoal commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Tier: Frontier
Model: claude-opus-4-8
Thinking: high

Closes #5282.

Summary

Nissa, Who Shakes the World's [−8] ultimate did nothing — activating it resolved straight to a priority window with no library search.

Oracle: "You get an emblem with 'Lands you control have indestructible.' Search your library for any number of Forest cards, put them onto the battlefield tapped, then shuffle."

The emblem's granted static ends in a sentence-final close quote (indestructible."). quote_closes_sentence_before_sequence only treated a closing quote as a sentence boundary when the continuation began with then/if/otherwise/may have. A plain new imperative ("Search your library…") returned false, so it was glued onto the emblem clause and swallowed into the emblem's static description. Result on main: the emblem parsed with empty modifications (no indestructible) and the ability produced no Effect::SearchLibrary at all.

Fix: add clause_is_emblem_creation_head (nom alt/tag) and one gated branch — when the clause-so-far is the emblem head (you get an emblem with "…"), a sentence-ending close quote closes the sentence, so the emblem's sibling effects split into their own clauses.

Per CR 114.1, an emblem is a self-contained command-zone object with no board presence, so a sentence following its granted-ability quote can never be an anaphor referring back to it — unlike a token/permanent granted-ability quote where a trailing "It becomes …" refers to the created object (the case the existing whitelist deliberately protects, and which this change leaves untouched). This handles the whole class of "create an emblem, then do more" ultimates.

Gate A

./scripts/check-parser-combinators.sh — clean. Dispatch is nom alt/tag; no string-matching primitives. Three-dot diff of crates/engine/src/parser/** vs origin/main grepped for forbidden methods (.contains(", .split_once(, .starts_with(", …): no matches.

Anchored on

  • crates/engine/src/parser/oracle_effect/sequence.rs:1314 — new clause_is_emblem_creation_head (combinator-only, mirrors try_parse_emblem_creation's prefix).
  • crates/engine/src/parser/oracle_effect/sequence.rs:1344 — the gated branch in quote_closes_sentence_before_sequence (runs before the existing then/if/otherwise/may have whitelist; only fires for the emblem head).

Verification

  • cargo fmt --all — clean.
  • cargo clippy -p engine --tests -- -D warnings — clean.
  • Parser combinator gate — clean.
  • CR verified against docs/MagicCompRules.txt: 114.1 (emblems in the command zone), 701.23 (Search).
  • Regression test nissa_who_shakes_the_world_ultimate_searches_library_for_forests in crates/engine/tests/integration/issue_5282_nissa_ultimate_emblem_search.rs (registered in tests/integration/main.rs) — builds Nissa from verbatim Oracle text, activates [−8], asserts the driver halts on WaitingFor::SearchChoice (behavioral, not AST-shape). Fails before (got Priority), passes after.
  • Nearby suites green: 34 emblem/loyalty integration tests (Gideon/Elspeth/Wrenn/Jace/Daretti/Momir/Chain Veil), 545 sequence+snapshot lib tests.
  • Blast radius: rebuilt oracle-gen, regenerated card-data.json, diffed the full database vs baseline — exactly 1 card changed (Nissa): emblem static now AddKeyword(Indestructible) on Typed[Land, controller You], plus SearchLibrary Typed[Land, Subtype(Forest)], count: UpTo ("any number"), zero Unimplemented. Zero collateral (the emblem-head gate keeps every other quoted-grant card on the existing path).

Scope Expansion

None.

Closes phase-rs#5282.

Nissa, Who Shakes the World's [-8] ultimate — "You get an emblem with
'Lands you control have indestructible.' Search your library for any
number of Forest cards, put them onto the battlefield tapped, then
shuffle." — did nothing: activating it resolved straight to a priority
window with no library search.

The emblem's granted static ends in a sentence-final close quote
(indestructible."). quote_closes_sentence_before_sequence only treated a
closing quote as a sentence boundary when the continuation began with
then/if/otherwise/may have, so the following "Search your library ..."
sentence was glued onto the emblem clause and swallowed into the emblem's
static description — the emblem parsed with empty modifications and the
ability produced no Effect::SearchLibrary.

Add clause_is_emblem_creation_head (nom alt/tag) and one gated branch: when
the clause-so-far is the emblem head (you get an emblem with "..."), a
sentence-ending close quote closes the sentence. CR 114.1: an emblem is a
self-contained command-zone object with no board presence, so a following
sentence can never be an anaphor referring back to it — unlike a
token/permanent granted-ability quote ("It becomes ..."), which the existing
whitelist deliberately protects and this change leaves untouched.

CR 114.1 + CR 701.23 (Search). Whole-database regen: exactly 1 card changed
(Nissa) — emblem static AddKeyword(Indestructible) on Typed[Land, You] plus
SearchLibrary Typed[Land, Subtype(Forest)] count UpTo. Regression test in
the integration harness drives the [-8] and asserts it halts on a search
choice (fails before, passes after).
@dhgoal dhgoal requested a review from matthewevans as a code owner July 9, 2026 23:12
@superagent-security superagent-security Bot added the contributor:flagged Contributor flagged for review by trust analysis. label Jul 9, 2026
@superagent-security

Copy link
Copy Markdown

🚨 Contributor flagged. Click here for more info: Superagent Dashboard

@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 addresses issue #5282 by updating the clause splitter to treat emblem-creation granted-ability quotes as self-contained sentences, preventing sibling effects (such as Nissa, Who Shakes the World's ultimate search effect) from being swallowed into the emblem's static text. A corresponding integration test has been added to verify this behavior. Feedback on the changes includes correcting the Comprehensive Rules (CR) citations in the comments to refer to CR 113.1 and CR 113.2 instead of CR 114.1, and optimizing the parser helper by using case-insensitive nom combinators (tag_no_case) to avoid unnecessary string allocations.

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.

Comment thread crates/engine/src/parser/oracle_effect/sequence.rs
Comment thread crates/engine/src/parser/oracle_effect/sequence.rs
Comment thread crates/engine/src/parser/oracle_effect/sequence.rs
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main 1cc181725a64)

1 card(s) · ability/ChangeZone · removed: ChangeZone (target=parent target, to=battlefield)

Examples: Nissa, Who Shakes the World

1 card(s) · ability/SearchLibrary · added: SearchLibrary (count=up to 2147483647, find=land Forest)

Examples: Nissa, Who Shakes the World

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

@matthewevans matthewevans self-assigned this Jul 9, 2026
@matthewevans matthewevans removed their assignment Jul 9, 2026
@matthewevans

Copy link
Copy Markdown
Member

Maintainer review update:

  • The parser allocation finding is addressed in edde201270a: the helper now uses the existing tag_no_case combinator directly on the trimmed slice.
  • I verified the CR comments against the current Comprehensive Rules. CR 114.1 is the correct emblem-creation rule (it says effects put emblems into the command zone); CR 113.1/113.2 describe abilities, not emblem creation. The suggested CR replacement is therefore not applicable.
  • The current parse-diff sticky comment matches the intended scope: Nissa gains the SearchLibrary signature and loses the swallowed ChangeZone signature.

Final approval remains held while the post-fix required checks run; the external Contributor trust check is currently pending.

@matthewevans matthewevans self-assigned this Jul 9, 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.

Approved. Right seam, class-general, discriminating test.

  • Seam. quote_closes_sentence_before_sequence is the sole decider of whether a closing quote ends a sentence, so the emblem check belongs exactly here. It covers both subject forms (you get an emblem with " / get an emblem with "), so it serves the emblem class rather than Nissa alone.
  • Blast radius. The parse-diff sticky confirms exactly one card changes corpus-wide: Nissa gains SearchLibrary, loses the swallowed ChangeZone. No collateral re-parses. Nested-quote emblems (Koth's …any target.'") are unaffected — quoted_text_ends_sentence sees ' before the closing quote and returns false early.
  • Test discrimination. nissa_who_shakes_the_world_ultimate_searches_library_for_forests drives the real pipeline via activate(...).resolve() and asserts on WaitingFor::SearchChoice. On revert, the search text is swallowed into the emblem static, no Effect::SearchLibrary is produced, and the assertion flips to fail. Not coverage theater.

Re the Gemini review (both HIGH findings): refuted, do not act on them. CR 114.1 as written is correct. In docs/MagicCompRules.txt, section 113 is "Abilities" and section 114 is "Emblems"; 114.1 reads "Some effects put emblems into the command zone." The suggested CR 113.1/113.2 describe what an ability is, not emblem creation — applying that suggestion would have replaced a correct citation with a wrong one. Gemini's MEDIUM allocation finding was already addressed in edde201270a (the helper now runs tag_no_case directly on the trimmed slice, no to_ascii_lowercase() allocation).

One non-blocking nit, left for a follow-up rather than another round-trip: clause_is_emblem_creation_head re-declares the emblem head prefixes that try_parse_emblem_creation already owns (oracle_effect/mod.rs). The tag_no_case + trailing-quote divergence is justified — sequence.rs matches the raw original-case buffer while try_parse_emblem_creation runs on pre-lowercased text — so a shared combinator would be wrong, but a shared const of the two subject prefixes would remove the duplication. Worth doing when someone next touches that file.

Note: the red "Contributor trust" check is a non-required bot gate and does not block the queue. Both required checks are green on this head.

@matthewevans matthewevans added the bug Bug fix label Jul 10, 2026
@matthewevans matthewevans added this pull request to the merge queue Jul 10, 2026
@matthewevans matthewevans removed their assignment Jul 10, 2026
Merged via the queue into phase-rs:main with commit 42bb113 Jul 10, 2026
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix contributor:flagged Contributor flagged for review by trust analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nissa, who shakes the world — [[nissa, who shakes the world]] taps when ultimating and does not let you search the libr…

2 participants