Skip to content

fix(parser): parse Roar saga chapter II nested tap-mana grant (#5978)#6210

Merged
matthewevans merged 7 commits into
phase-rs:mainfrom
andriypolanski:fix/5978-roar-chapter-ii-nested-quote-grant
Jul 20, 2026
Merged

fix(parser): parse Roar saga chapter II nested tap-mana grant (#5978)#6210
matthewevans merged 7 commits into
phase-rs:mainfrom
andriypolanski:fix/5978-roar-chapter-ii-nested-quote-grant

Conversation

@andriypolanski

Copy link
Copy Markdown
Contributor

Closes #5978

Summary

Discord report: Roar of the Fifth People chapter II does not give creatures the tap-for-mana ability.

Oracle: This Saga gains "Creatures you control have '{T}: Add {R}, {G}, or {W}.'"

Root cause: the saga chapter grant path extracted the double-quoted inner text but left the nested single-quoted tap ability unpromoted. Quote splitting then broke {T} into an Unimplemented cost on a malformed activated ability scoped to SelfRef (the saga), instead of a GrantStaticAbility on creatures you control. A second bug: parse_quoted_rule_static_modifications rejected the line because find_cost_separator saw {T}: inside the nested ability even when that colon belongs inside quotes.

Fix: hoist promote_nested_ability_quotes to the shared grammar module (already used by emblem creation), call it from classify_quoted_inner, and mask quoted spans before the cost-separator guard so static-line grants like Creatures you control have "{T}: …" route to GrantStaticAbility correctly.

Changes

  • crates/engine/src/parser/oracle_static/grammar.rs — shared promote_nested_ability_quotes; quote-aware cost-separator guard in parse_quoted_rule_static_modifications.
  • crates/engine/src/parser/oracle_static/keyword_grant.rs — promote nested single quotes in classify_quoted_inner before dispatch.
  • crates/engine/src/parser/oracle_static/mod.rs — re-export promote_nested_ability_quotes.
  • crates/engine/src/parser/oracle_effect/mod.rs — emblem path uses shared helper (no duplicate logic).
  • crates/engine/src/parser/oracle_saga.rs — Roar chapter II regression: GrantStaticAbility → creatures you control → {T} mana (R/G/W), UntilHostLeavesPlay.

Test Plan

  • cargo fmt --all -- --check
  • cargo test -p engine --lib roar_chapter_two_grants_creature_tap_mana_ability
  • cargo test -p engine --lib effect_koth_emblem_grants_activated_ability_to_mountains
  • cargo test -p engine --lib parser::oracle_saga::tests
  • Regenerate card-data (./scripts/gen-card-data.sh) — Roar chapter II execute body should show GrantStaticAbility with inner GrantAbility { cost: Tap, is_mana_ability: true }, not Unimplemented cost fragments

@superagent-security superagent-security Bot added the contributor:flagged Contributor flagged for review by trust analysis. label Jul 19, 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 introduces support for parsing nested single-quoted granted abilities (such as those found in 'Roar of the Fifth People' Chapter II) by promoting them to double quotes before dispatching to downstream parsers. It adds the promote_nested_ability_quotes helper, updates parse_quoted_rule_static_modifications to strip quoted segments before checking for cost separators, and includes a comprehensive integration test for the Saga chapter ability. As there were no review comments provided, I have no feedback to evaluate.

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.

@github-actions

github-actions Bot commented Jul 19, 2026

Copy link
Copy Markdown

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

🟢 Added (1 signature)

  • 1 card · ➕ ability/grant static ability · added: grant static ability (affects=self, duration=while on battlefield, grants=grant static ability)
    • Affected (first 3): Roar of the Fifth People

🔴 Removed (1 signature)

  • 1 card · ➖ ability/grant ability · removed: grant ability (affects=self, duration=while on battlefield, grants=grant ability)
    • Affected (first 3): Roar of the Fifth People

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

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

Blocked — the shared nested-quote promotion regresses two existing Aura grants.

🔴 Blocker

[HIGH] The helper promotes from the first grant-verb quote to the final apostrophe, collapsing multiple nested abilities into one body. Evidence: crates/engine/src/parser/oracle_static/grammar.rs:1076-1095 chooses body.rfind('\''), and keyword_grant.rs:1854-1858 applies it to every quoted grant. The current CI run fails snapshot_old_growth_troll and snapshot_harold_and_bob_first_numens; both verified Oracle strings contain more than one nested ability segment. Why it matters: the resulting parse contains malformed/unimplemented activated abilities, so the general fix breaks existing card behavior. Suggested fix: parse/promote each anchored nested quote pair independently (preserving adjacent grants), then add explicit regression assertions for Old-Growth Troll and Harold and Bob rather than accepting the snapshot changes.

[MED] The new Roar test proves parser shape only. Evidence: crates/engine/src/parser/oracle_saga.rs:587-655 inspects AbilityDefinition fields and never drives a creature activation through the game pipeline. Why it matters: the issue reports that creatures do not receive a usable tap-for-mana ability, which AST shape alone cannot establish. Suggested fix: add a registered runtime scenario using Roar’s verified chapter-II Oracle text, resolve the chapter, activate a controlled creature, and assert mana enters the correct player pool.

✅ Clean

Scryfall’s current Roar face text matches the test input, and the current-head parse-diff reports only Roar’s intended signature change.

Recommendation: request changes — repair the general quote-pair handling, preserve the two existing Aura-grant parses, and add the runtime regression before re-review.

@matthewevans matthewevans added the bug Bug fix label Jul 19, 2026
@matthewevans matthewevans removed their assignment Jul 19, 2026
@matthewevans matthewevans self-assigned this Jul 19, 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.

Blocked — the maintainer port is complete, but the current head still has the two earlier parser/test defects.

🔴 Blockers

[HIGH] promote_nested_ability_quotes still spans multiple independent quoted grants. At grammar.rs:1076-1096, it selects the first grant opener and the final apostrophe (rfind), so a body with more than one nested granted ability is collapsed into one quoted body. Please parse and promote each pair independently, with regressions for Old-Growth Troll and Harold and Bob, First Numens.

[HIGH] The added test remains parser-shape coverage only. oracle_saga.rs:594-660 parses a hand-written chapter line and inspects the AST; it does not resolve Roar's actual chapter trigger and then activate a granted creature ability. Add a GameScenario/GameRunner regression that exercises that production path and verifies the creature can tap for one of the promised colors.

The Summer Bloom mainline port is in 4892e0b; it was mechanical and preserved both changesets. CI/parse-diff need to finish for this new head after the substantive fixes.

Recommendation: request changes.

@matthewevans matthewevans removed their assignment Jul 19, 2026
@andriypolanski
andriypolanski force-pushed the fix/5978-roar-chapter-ii-nested-quote-grant branch from 4892e0b to bb241d4 Compare July 20, 2026 04:38
@matthewevans

Copy link
Copy Markdown
Member

Reviewed current head 7f52dcae.

The prior quote-pair defect is resolved: promotion now closes each anchored nested quote pair, with Old-Growth Troll/Harold regressions, and the Roar test now drives the Saga chapter through runtime activation. The latest commit is a test-module ordering fix; I found no new source-level blocker.

Held pending the new head's required CI and a fresh parse-diff artifact. Contributor trust remains ACTION_REQUIRED, so maintainers must also clear that gate before any approval or queue action. No approval has been issued. Evidence: current checks are in progress and the only parse-diff comment predates this head. Confidence: high.

@matthewevans matthewevans self-assigned this Jul 20, 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: nested quote-pair promotion now preserves adjacent grants, and the Saga pipeline regression proves Roar chapter II grants and resolves the creature tap-for-mana ability.

@matthewevans
matthewevans added this pull request to the merge queue Jul 20, 2026
@matthewevans matthewevans removed their assignment Jul 20, 2026
Merged via the queue into phase-rs:main with commit 00c1056 Jul 20, 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.

Roar of the fifth people — [[Roar of the Fifth People]] 's second ability does not give creatures the ability to tap fo…

3 participants