Skip to content

feat(parser): bind an Aura's "It" continuation sentence to the enchanted creature - #5688

Merged
matthewevans merged 4 commits into
phase-rs:mainfrom
minion1227:minion_aura_it_continuation
Jul 12, 2026
Merged

feat(parser): bind an Aura's "It" continuation sentence to the enchanted creature#5688
matthewevans merged 4 commits into
phase-rs:mainfrom
minion1227:minion_aura_it_continuation

Conversation

@minion1227

Copy link
Copy Markdown
Contributor

CR 608.2c

Bug

In an Aura, a continuation sentence whose subject is the pronoun "It" refers to the enchanted creature, not the Aura object. But at the line level the pronoun resolves to self, so the second sentence's static parsed with the correct modifications and the wrong affected scope — SelfRef — applying to the Aura (which is not a creature on the battlefield) instead of the enchanted creature:

  • Spider-Man No More — "Enchanted creature is a Citizen with base power and toughness 1/1. It has defender and loses all other abilities." → the defender + ability-loss static bound to the Aura.
  • Retro-Mutation — "Enchanted creature gets +0/+2. It can't attack and loses all abilities."

Before: the "It" static's affected was SelfRef. After: [Creature] + EnchantedBy — same as the first sentence.

Fix

In parse_multi_sentence_statics: capture the attached scope established by the first sentence (a filter carrying FilterProp::EnchantedBy/EquippedBy) and rebind any later sentence whose subject is the bare pronoun "It" from SelfRef to that scope.

Why it's the right seam / minimal blast radius

  • Scoped to the multi-sentence static assembler — the one place that already sees all sibling sentences — rather than threading aura context down into the pronoun resolver.
  • Only fires when (a) the card established an attached (EnchantedBy/EquippedBy) scope and (b) a later sentence's subject is literally "It". A self-name (~) subject keeps its SelfRef; a non-aura multi-sentence anthem is untouched (both regression-tested).
  • No modification is changed — only the affected scope of the mis-bound continuation static.

Tests

  • aura_it_continuation_binds_to_enchanted_creature — Spider-Man No More: both statics bind EnchantedBy (none left on SelfRef), and the "It" static keeps RemoveAllAbilities + AddKeyword(Defender).
  • non_aura_multi_sentence_anthem_scope_unchanged — regression: "Creatures you control get +1/+1. Creatures you control have flying." stays controller-scoped, no EnchantedBy.
  • Full parser::oracle_static::tests (1052 passed, 0 failed).

CR verification

  • CR 608.2c — resolution reads the whole text and applies the rules of English; "It" is anaphoric to the enchanted creature (verified in docs/MagicCompRules.txt).

AI-contributor notes

  • Rules-correct: the Aura's continuation now affects the enchanted creature per CR 608.2c, not the Aura.
  • Builds the class: any Aura/Equipment with an "It" continuation static (Spider-Man No More, Retro-Mutation, …) inherits the fix from one seam.
  • Verified: live-parsed Spider-Man No More / Retro-Mutation and a non-aura control before/after; cargo fmt, CI-exact clippy -p engine --all-targets --features proptest -D warnings, and the full oracle_static module are green.

🤖 Generated with Claude Code

…ted creature

CR 608.2c: In an Aura, a continuation sentence whose subject is the pronoun "It"
refers to the enchanted creature, not the Aura object itself. But `It` resolves to
`SelfRef` at the line level (it has no attachment context there), so the second
sentence's static bound to the Aura — which is not a creature on the battlefield —
and its effect applied to nothing:

- Spider-Man No More — "Enchanted creature is a Citizen ... It has defender and
  loses all other abilities." bound the defender/ability-loss to the Aura.
- Retro-Mutation — "Enchanted creature gets +0/+2. It can't attack and loses all
  abilities." likewise.

Fix: a targeted post-pass in `parse_multi_sentence_statics`. When an earlier
sentence establishes an attached scope (`FilterProp::EnchantedBy`/`EquippedBy`) and
a later sentence's subject is the bare pronoun "It", rebind that sentence's
`SelfRef` static to the attached scope. The mods were already parsed correctly;
only the affected scope changes. A self-name (`~`) subject is left untouched, and a
non-aura multi-sentence anthem (no attached scope, no "It" subject) is unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@minion1227
minion1227 requested a review from matthewevans as a code owner July 12, 2026 17:12

@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 implements rules support for resolving the pronoun "It" in continuation sentences of Aura and Equipment cards, rebinding self-references to the enchanted or equipped creature. Feedback highlights a violation of the repository's architectural rules, specifically the use of manual string prefix checks (starts_with) instead of idiomatic nom parser combinators for parsing dispatch.

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 on lines +979 to +981
fn segment_subject_is_pronoun_it(segment: &str) -> bool {
segment.trim_start().to_lowercase().starts_with("it ")
}

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.

medium

Avoid verbatim string equality or manual prefix checks for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested alt and tag sequences) to prevent combinatorial explosion and improve maintainability.

Suggested change
fn segment_subject_is_pronoun_it(segment: &str) -> bool {
segment.trim_start().to_lowercase().starts_with("it ")
}
fn pronoun_it(input: &str) -> IResult<&str, &str, OracleError<'_>> {
tag_no_case("it")(input)
}
References
  1. Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested alt and tag sequences) to prevent combinatorial explosion and improve maintainability.

@github-actions

github-actions Bot commented Jul 12, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR

Baseline pending — no synthetic merge parent is available for PR head d62d9574c694f4a9d607b02d96e401c26afc8f35.

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

Request changes: one parser-policy defect and missing required parse-diff evidence block merge.

🔴 Blocker

  • crates/engine/src/parser/oracle_static/shared.rs:979-980 introduces segment.trim_start().to_lowercase().starts_with("it "). The current head's failed Rust lint (fmt, clippy, parser gate) check identifies this dispatch: new Oracle parsing must use nom combinators. Replace it with a word-boundary-aware tag("it") parser over the normalized segment rather than a string-prefix check.

  • crates/engine/src/parser/oracle_static/shared.rs:907-990 changes parser behavior, but the required <!-- coverage-parse-diff --> artifact remains baseline-pending for the branch rather than providing a measured card diff. That leaves the claimed two-card blast radius and coverage honesty unverified. Refresh the branch after the baseline publishes, then include the resulting gained/lost/changed-card evidence before approval.

🟡 Non-blocking

  • crates/engine/src/parser/oracle_static/tests.rs:25277-25303 exercises only Enchanted creature gets +1/+1. Spider-Man No More's Oracle text is: “Enchanted creature is a Citizen with base power and toughness 1/1. It has defender and loses all other abilities.” The first sentence takes the type-changing fallback at shared.rs:1602-1617, while shared.rs:985-990 also includes EquippedBy without a corresponding test. Add full-parser coverage for Spider-Man No More, Retro-Mutation, and an Equipment continuation such as Bride’s Gown.

✅ Clean

  • crates/engine/src/parser/oracle_static/shared.rs:907-971 is the right ownership seam for sibling-sentence context because it has every static segment available.
  • crates/engine/src/parser/oracle_static/tests.rs:25291-25303 does verify that the intended defender and ability-removal modifications survive the rebind.

Recommendation: request changes—use the nom subject parser, add discriminating card-class tests, then regenerate and inspect the parse-diff before re-review.

@matthewevans matthewevans self-assigned this Jul 12, 2026
@matthewevans matthewevans added the bug Bug fix label Jul 12, 2026
@matthewevans matthewevans removed their assignment Jul 12, 2026
…) bridge

The parser combinator gate rejected the string-method dispatch
`segment.trim_start().to_lowercase().starts_with("it ")` in
`segment_subject_is_pronoun_it`. Route the "it " match through `nom_tag_tp`
(nom `tag()` over the TextPair lower half) so the pronoun test stays on the
combinator path; `trim_start`/`to_lowercase` remain as structural whitespace
normalization and lower-half construction, not parsing dispatch.

Semantics-preserving: `nom_tag_tp(.., "it ").is_some()` matches iff the
lowercased, left-trimmed segment begins with "it ", identical to the prior
`starts_with`. No behavior change; clears the nom-combinator mandate gate.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@matthewevans matthewevans self-assigned this Jul 12, 2026
@matthewevans matthewevans removed their assignment Jul 12, 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.

Request changes: the parser-policy defect is fixed, but required parse-diff evidence and discriminating card-class coverage still block merge.

🔴 Blocker

  • crates/engine/src/parser/oracle_static/shared.rs:907-1000 changes parser behavior, but the only <!-- coverage-parse-diff --> artifact remains baseline-pending and is not current-head evidence. Wait for the generated gained/lost/changed-card result and inspect it before approval; without it, the parser blast radius and coverage honesty are unverified.

  • crates/engine/src/parser/oracle_static/tests.rs:25407-25433 tests only a simplified Aura grant. Spider-Man No More’s Oracle text is: “Enchanted creature is a Citizen with base power and toughness 1/1. It has defender and loses all other abilities.” Its first sentence takes the type-change fallback at shared.rs:1692-1707; shared.rs:989-996 also handles EquippedBy, but no Equipment continuation is exercised. Add exact-card/full-parser coverage for Spider-Man No More, Retro-Mutation, and an Equipment example before this is approved.

🟡 Non-blocking

  • The Rust and card-data checks are still running on this maintainer-updated head. Their completion is not the review gate, but their current output cannot substitute for the required current-head parse diff.

✅ Clean

  • crates/engine/src/parser/oracle_static/shared.rs:984-986 correctly replaces the prior ad-hoc prefix check with the existing TextPair + nom_tag_tp(..., "it ") parser path.

Recommendation: request changes—add the discriminating Aura/Equipment cases and re-review once the current-head parse-diff artifact is available.

@matthewevans matthewevans self-assigned this Jul 12, 2026
…ation rebind

Address review: the prior test used a simplified "Enchanted creature gets
+1/+1" first sentence, which does not exercise the same scope-establishing path
as the real cards. Replace/extend with discriminating, exact-Oracle-text cases:

- aura_it_continuation_binds_to_enchanted_creature — exact Spider-Man No More
  text. Its first sentence is a TYPE-CHANGE ("is a Citizen with base power and
  toughness 1/1"), which establishes the EnchantedBy scope via the type-change
  path rather than the simple pump path; asserts the "It" continuation inherits
  that scope (RemoveAllAbilities + Defender), not SelfRef. Reminder text stripped.
- aura_it_continuation_binds_retro_mutation — exact Retro-Mutation text
  ("is a Turtle ... 0/1. It can't attack and loses all abilities.").
- equipment_it_continuation_binds_to_equipped_creature — class-level coverage of
  the EquippedBy arm of the rebind. No printed Equipment currently pairs a
  scope-establishing first sentence with a bare-"It" static continuation, so
  this uses representative grammatical text to exercise the arm, per the
  test-the-building-block convention.

All three pass against the current head; full parser::oracle_static::tests
module green (1054 passed, 0 failed).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@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: current-head parser re-review is clean; the complete parse-diff and green required checks confirm the Aura/Equipment continuation scope.

@matthewevans
matthewevans added this pull request to the merge queue Jul 12, 2026
@matthewevans matthewevans removed their assignment Jul 12, 2026
@minion1227

Copy link
Copy Markdown
Contributor Author

Thanks for the review — addressed both blockers.

🔴 Discriminating card-class coverage (tests.rs): replaced the simplified Enchanted creature gets +1/+1 case with exact-Oracle-text, full-parser coverage. All three pass on the current head (parser::oracle_static::tests: 1054 passed, 0 failed):

  • aura_it_continuation_binds_to_enchanted_creatureexact Spider-Man No More text. Its first sentence is the type-change (is a Citizen with base power and toughness 1/1), so this now exercises the scope being established via the type-change path — not the simple pump path the old test used — and asserts the It continuation inherits EnchantedBy (carrying RemoveAllAbilities + Defender), never SelfRef. Trailing reminder text is stripped.
  • aura_it_continuation_binds_retro_mutationexact Retro-Mutation text (is a Turtle ... 0/1. It can't attack and loses all abilities.).
  • equipment_it_continuation_binds_to_equipped_creature — class-level coverage of the EquippedBy arm. One honest note: I checked Scryfall and no printed Equipment currently pairs a scope-establishing first sentence with a bare-It static continuation (Bride's Gown / Groom's Finery / Mind Carver all use conditional …as long as… forms that route elsewhere). So per the test-the-building-block convention this exercises the EquippedBy arm with representative grammatical text rather than a single named card.

🔴 Parse-diff evidence: the head is now current — rebased on top of your origin/main merge (b363ef8ff) — so the regenerated gained/lost/changed-card diff will reflect current-head. CI is green on the prior push (parser gate + coverage-gate both pass).

✅ nom path: the It subject test runs through nom_tag_tp(..., "it ") per your Clean note; trim_start/to_lowercase remain structural only.

Merged via the queue into phase-rs:main with commit 6f7cee8 Jul 12, 2026
3 checks passed
@minion1227
minion1227 deleted the minion_aura_it_continuation branch July 18, 2026 06:12
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