fix(parser): parse Roar saga chapter II nested tap-mana grant (#5978)#6210
Conversation
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
There was a problem hiding this comment.
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.
Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
4892e0b to
bb241d4
Compare
|
Reviewed current head 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 |
matthewevans
left a comment
There was a problem hiding this comment.
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.
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 anUnimplementedcost on a malformed activated ability scoped toSelfRef(the saga), instead of aGrantStaticAbilityon creatures you control. A second bug:parse_quoted_rule_static_modificationsrejected the line becausefind_cost_separatorsaw{T}:inside the nested ability even when that colon belongs inside quotes.Fix: hoist
promote_nested_ability_quotesto the sharedgrammarmodule (already used by emblem creation), call it fromclassify_quoted_inner, and mask quoted spans before the cost-separator guard so static-line grants likeCreatures you control have "{T}: …"route toGrantStaticAbilitycorrectly.Changes
crates/engine/src/parser/oracle_static/grammar.rs— sharedpromote_nested_ability_quotes; quote-aware cost-separator guard inparse_quoted_rule_static_modifications.crates/engine/src/parser/oracle_static/keyword_grant.rs— promote nested single quotes inclassify_quoted_innerbefore dispatch.crates/engine/src/parser/oracle_static/mod.rs— re-exportpromote_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 -- --checkcargo test -p engine --lib roar_chapter_two_grants_creature_tap_mana_abilitycargo test -p engine --lib effect_koth_emblem_grants_activated_ability_to_mountainscargo test -p engine --lib parser::oracle_saga::tests./scripts/gen-card-data.sh) — Roar chapter II execute body should showGrantStaticAbilitywith innerGrantAbility { cost: Tap, is_mana_ability: true }, notUnimplementedcost fragments