Add Birgi, God of Storytelling // Harnfel, Horn of Bounty

[mcbradd]

Adds Birgi, God of Storytelling // Harnfel, Horn of Bounty (KHM #123, modal DFC).

Model: claude-fable-5
Tier: Frontier
Thinking: enabled

What this PR actually changes

The card was already ~98% supported by existing building blocks (SpellCast trigger → Mana { expiry: EndOfTurn }, ModifyActivationLimit for boast-twice, ExileTop + GrantCastingPermission::PlayFromExile for Harnfel, ChooseModalFace for MDFC casting). The only code change is a swallow-detector fix: the activation-limit carve-out in detect_dynamic_qty recognized only "twice each turn", so Birgi's templating "can boast twice during each of your turns rather than once" raised a false-positive SwallowedClause{DynamicQty} even though the line parses fully to ModifyActivationLimit { keyword: "boast", new_limit: 2 }.

Fix (class-general, not card-specific): extract a shared twice_is_activation_limit(cleaned) predicate keyed on the "rather than once" limit-raise idiom — grep-confirmed unique in the corpus to the ModifyActivationLimit parser family (CR 702.142b makes boast a referable ability; CR 602.5b governs the usage-limit raise) — and route both previously-duplicated call sites (detect_dynamic_qty, cleaned_twice_is_only_dynamic_marker) through it so they cannot drift. The existing "twice that"/"twice x" dynamic-multiplier exclusions are preserved and tested. Also removes a pre-existing citation of nonexistent CR 605.1g (grep of the CR text returns nothing), replaced with the two verified rules above.

Zero new enum variants; zero serde/protocol shape changes.

Test coverage (8 integration + 2 unit)

• birgi_mana_trigger_uses_stack_and_stamps_end_of_turn_expiry — strict shape: red pool empty at commit and trigger on the stack (CR 605.1b — a SpellCast trigger is not a mana ability), resolved mana is exactly {R} with ManaExpiry::EndOfTurn asserted specifically (a misparse dropping the expiry fails), persists across steps/phases (CR 614.17 overriding CR 106.4), drains at turn end.
• birgi_two_casts_add_two_red — accumulation.
• boast_twice_with_birgi_rejects_third / boast_once_without_birgi_rejects_second — the limit raise (CR 702.142a base) with a funded-pool guard proving rejection is the limit, not mana; the without-Birgi leg proves the raise comes from Birgi's static.
• opponents_boast_creature_not_raised_by_your_birgi / opponents_cast_does_not_trigger_birgi_mana — controller scoping negatives, each with a positive reach-guard.
• harnfel_back_face_cast_grants_play_from_exile_this_turn — real-card MDFC test: CastSpell → ChooseModalFace { back_face: true }, discard-activation exiles top 2, both carry PlayFromExile until end of turn, a control card exiled by another source carries no grant. DB-guarded per house pattern.
• harnfel_exiles_top_two_and_grants_play_this_turn — DB-independent synthetic sibling.
• Unit: birgi_boast_limit_static_does_not_flag_dynamic_qty (with a reach-guard asserting the line parses to ModifyActivationLimit — not vacuous) and twice_is_activation_limit_scopes_to_fixed_limits_only ("twice that many"/"twice x" still flag).

Fixture note: integration_cards.json gains exactly the two new card entries (structural name-set diff: +2, −0, no shared card touched); a full deterministic regen was deliberately avoided because the committed fixture is stale relative to current generator output and a regen would sweep in ~130 unrelated card diffs. The Birgi entry matches post-fix generator output (no parse_warnings — the field is serialize-if-nonempty and this PR removes the false positive that produced it).

Anchored on

Citations at PR base (88f0a7ecb):

parser/swallow_check.rs (the changed detector):

• swallow_check.rs:2064-2066 — the pre-existing inline twice_is_activation_limit carve-out (with the "twice that"/"twice x" exclusions) that this PR extracts into the shared predicate.
• swallow_check.rs:486 — the analogous "activate only twice each turn" marker in detect_activate_limit (a different detector; intentionally untouched, same annotated allow-noncombinator marker-scan pattern).
• swallow_check.rs:2037 — OTHER_DYNAMIC_MARKERS, the established marker-list pattern the predicate composes with.

Parser family the predicate is keyed to:

• parser/oracle_static/dispatch.rs:2735-2748 — the ModifyActivationLimit combinator (take_until("rather than once") + tag("rather than once"), CR 702.142b annotation) — the sole in-tree consumer of the idiom.
• game/restrictions.rs:778-852 — effective_activation_limit / activation_limit_from_statics, the runtime consumer of the parsed limit.

Integration tests (tests/integration/birgi.rs):

• tests/integration/peter_parker_modal_back_face_cast.rs:23-30,81-82 — the MDFC back-face-cast pattern (load_db guard + ChooseModalFace { back_face: true }) the Harnfel test follows.
• tests/integration/escape_to_wilds_2879.rs:44-52 — the CastingPermission::PlayFromExile grant-assertion precedent.
• game/mana_abilities.rs:99 — is_triggered_mana_ability, the classifier the stack-not-inline assertion exercises (CR 605.1b).

Gate A

Gate G PASS (router/grant architecture: strict router vs permissive grant boundary intact)
Gate A PASS head=ae212c260970962790f7f0f88a34684a9446fa7a base=88f0a7ecbc393ea95e447059da85096bbf2866d8

Review record (final review-impl)

Independent reviewer (separate agent, adversarial lenses; re-ran all tests itself rather than trusting the executor's paste): APPROVE-WITH-CHANGES, both changes applied before this PR:

• [MED — applied] The Birgi fixture entry, copied from pre-fix card data, still carried the SwallowedClause{DynamicQty} parse_warnings this PR eliminates — committed test data contradicting the change. Fixed by surgically removing the field (matching serialize-if-nonempty generator output); structural diff re-verified +2/−0.
• [LOW — applied] Doc comment mischaracterized CR 702.142b as the limit rule; reworded (702.142b = referability, 602.5b = usage-limit raise).
• [LOW — informational, intentionally not changed] The fixture's serialized is_mana_ability: true on Birgi's cast trigger is pre-existing generator output; the runtime classifier (mana_abilities::is_triggered_mana_ability) correctly overrides it and birgi_mana_trigger_uses_stack_and_stamps_end_of_turn_expiry empirically pins stack behavior. Hand-editing the field would diverge the fixture from generator output; flagged here for maintainer awareness as a parser-output currency issue outside this PR's scope.

Reviewer independently verified: fixture additions-only via structural JSON diff; "rather than once" appears on exactly 1 card in the full corpus and is the WotC limit-raise idiom (class-general, not verbatim-card matching); all negative assertions carry positive reach-guards; every CR cited in the diff grep-verified against the CR text (106.4, 601.2a, 602.5b, 605.1b, 614.17, 702.142a, 702.142b).

Verification: fmt clean; swallow_check lib tests 149 passed; integration birgi 8 passed; clippy -p engine --all-targets exit 0 with zero warnings; parser combinator gate PASS (G + A); cargo coverage exit 0; cargo semantic-audit exit 0 (baseline unchanged).

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request extracts the logic for identifying whether "twice" denotes a fixed-count activation limit into a shared helper function twice_is_activation_limit in swallow_check.rs. This helper prevents false-positive DynamicQty swallow warnings for cards like Birgi, God of Storytelling, whose boast-limit static is a fixed activation limit rather than a dynamic quantity. The PR also adds comprehensive unit tests and a new integration test suite (birgi.rs) to verify Birgi's mana trigger, boast-twice limit, and Harnfel's back-face modal cast and play-from-exile behaviors. I have no feedback to provide as there are no review comments to assess.

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]

Parse changes introduced by this PR

✓ No card-parse changes detected.

[matthewevans]

Changes requested — the detector seam is promising, but the current test evidence is not merge-ready.

🔴 Blockers

• crates/engine/tests/integration/birgi.rs:238,269 attributes the raised boast limit to CR 702.142b, but that rule only defines what it means for an effect to refer to a boast ability. Likewise :310,366,481,574 attributes the duration of the play permission to CR 601.2a, which describes beginning to cast a spell. Please replace or remove these citations using verified rules that actually support each assertion (for example, CR 702.142a plus the applicable activation restriction, and CR 611.2a for a resolution-created until-end-of-turn duration). The new 614.17 duration commentary should be similarly narrowed or paired with the duration rule.
• The Harnfel paths only inspect a CastingPermission field (:353-386 and :561-594). They never exercise its consumer. Add a runtime assertion that an exiled spell is castable/plays this turn and that an unplayed sibling is no longer eligible after the turn changes; producer-state assertions alone cannot catch a broken permission-consumption or expiry path.

🟡 Evidence gap

• The current parse-diff says “No card-parse changes,” while the local generated Birgi card data still contains the DynamicQty swallowed-clause warning that this PR says it removes. Please reconcile that artifact before re-review so the card-level parser effect is independently visible.

The shared twice_is_activation_limit predicate and its reach-guarded detector test are otherwise directionally sound. Request re-review after the citations, consumer/expiry regression, and parse-diff discrepancy are addressed.

[mcbradd]

Thanks for the detailed review. All three blockers are addressed in a40a7afaf. Point-by-point:

Blocker 1 — CR citations (birgi.rs)

Ran the validate-cr-annotations protocol and re-grepped every number against docs/MagicCompRules.txt, including your suggested ones.

Boast limit (was CR 702.142b @ ~238, ~269). You're right — 702.142b only defines referability:

702.142b Effects may refer to boast abilities. If an effect refers to a creature boasting, it means its boast ability being activated.

The base limit is 702.142a, enforced as an activation restriction under 602.5:

702.142a … "[Cost]: [Effect]. Activate only if this creature attacked this turn and only once each turn."
602.5 A player can't begin to activate an ability that's prohibited from being activated.
602.5b If an activated ability has a restriction on its use (for example, "Activate only once each turn"), the restriction continues to apply…

Retagged the boast doc-comment + assertion to CR 702.142a + CR 602.5 (Birgi's static raises that once-each-turn restriction to twice; the third activation exceeds even the raised limit). Applied the same fix to the swallow_check.rs comments and the birgi_boast_limit_static_does_not_flag_dynamic_qty unit test so the whole PR is consistent (702.142b there is now used only where it belongs — for boast being a referable ability).

Play-from-exile duration (was CR 601.2a @ ~310, ~366, ~481, ~574). Agreed — 601.2a is about proposing/putting a spell on the stack, not duration. Replaced all four with CR 611.2a:

611.2a A continuous effect generated by the resolution of a spell or ability lasts as long as stated by the spell or ability creating it (such as "until end of turn"). If no duration is stated, it lasts until the end of the game.

This also matches the engine's own annotation at game/turns.rs (prune_end_of_turn_casting_permissions → CR 514.2 + CR 611.2a).

614.17 duration commentary. Narrowed: 614.17 now cites only the "don't lose this mana" can't-effect (persistence within the turn), and the duration/drain side is paired with CR 611.2a (+ CR 106.4 for the default mana-emptying that resumes once the until-end-of-turn duration ends at cleanup).

Blocker 2 — consumer/expiry runtime coverage (birgi.rs)

Both Harnfel tests previously only inspected the produced CastingPermission. Added a shared drill, assert_play_from_exile_consumed_then_expires, invoked at the end of both harnfel_exiles_top_two_and_grants_play_this_turn (synthetic) and harnfel_back_face_cast_grants_play_from_exile_this_turn (real modal-back-face). It exercises the CONSUMER, not just the grant:

• (a) consume this turn — spell_objects_available_to_cast(state, P0) surfaces the exiled card; then runner.cast(to_cast).resolve() drives the real cast pipeline and asserts resolution via CastOutcome deltas: assert_zone(&[to_cast], Zone::Graveyard) + assert_life_delta(P0, 1) (the exiled card is a real 0-cost "You gain 1 life" sorcery via the new castable_top helper, so the grant has something to actually play). Then asserts the card leaves P0's cast path.
• (b) expire next turn — asserts the still-unplayed sibling is castable before the turn ends, advances one turn, then asserts it is no longer in spell_objects_available_to_cast and that its PlayFromExile grant was pruned (CR 611.2a duration end / CR 514.2 cleanup).

Followed the existing issue_6102_ragavan_exile_cast consumer pattern (spell_objects_available_to_cast + cast(...).resolve() + zone/path assertions) and the card-test foot-gun conventions (CastOutcome deltas, no verbatim-string coupling).

Local run:

running 8 tests
test birgi::harnfel_exiles_top_two_and_grants_play_this_turn ... ok
test birgi::harnfel_back_face_cast_grants_play_from_exile_this_turn ... ok
... (all 8 pass)
test result: ok. 8 passed; 0 failed

Blocker 3 — parse-diff "No card-parse changes detected" (reconciliation)

This is correct by design, not a stale artifact — the swallow-detector carve-out does not change any card's parse tree.

Traced the pipeline: coverage_parse_diff.rs diffs only the parse_details tree (diff_level(&b.parse_details, &h.parse_details, …)), comparing each item's source_text / supported / details. The swallowed-clause warning is not part of parse_details — it lives at the card-coverage level (gap_details → Swallow:DynamicQty, the card-level supported flag, and the aggregate diagnostics map). See coverage.rs: a fired SwallowedClause demotes the card and adds a Swallow:<detector> gap; it never mutates a parse_details item.

Before/after evidence (Birgi entry from the locally-generated data/coverage-data.json, which predates the fix):

Before (pre-fix):

"supported": false, "gap_count": 1,
"gap_details": [{"handler": "Swallow:DynamicQty",
  "source_text": "[warning:swallowed-clause] DynamicQty — Creatures you control can boast twice during each of your turns rather than once."}],
"parse_details": [
  {"category":"trigger","label":"SpellCast","supported":true, "children":[{"label":"Mana","supported":true,"details":[["mana","{R}"],["expiry","EndOfTurn"]]}]},
  {"category":"static","label":"ModifyActivationLimit(boast,2)","supported":true,"details":[["affects","you control creature"]]}
]

The ModifyActivationLimit(boast,2) static is already supported: true in parse_details before the fix — Birgi always parsed correctly; the only defect was a false-positive DynamicQty swallow warning. After the fix that warning is suppressed (verified by birgi_boast_limit_static_does_not_flag_dynamic_qty: !has_swallowed_detector(&parsed, "DynamicQty")), so:

• parse_details is byte-identical before/after (both items still supported: true) → coverage-parse-diff legitimately reports "No card-parse changes detected."
• The real delta — supported: false → true, gap_details dropping Swallow:DynamicQty, diagnostics["Swallow:DynamicQty"] decremented — is a coverage/diagnostics change, surfaced by the coverage-regression gate (a supported flip), not by the parse-detail sticky. data/coverage-data.json is gitignored / CI-generated, so there's no checked-in artifact to regenerate; anyone can reproduce by generating coverage on both refs and diffing Birgi's parse_details (identical).

Evidence package

swallow_check.rs is parser-side, so per §0.1.2 — Gate A (./scripts/check-parser-combinators.sh):

Gate G PASS (router/grant architecture: strict router vs permissive grant boundary intact)
Gate A PASS head=a40a7afaf base=88f0a7ecb

(This PR's swallow-detector change is comment-only in this commit; the carve-out itself keeps its allow-noncombinator annotations on the classified-text marker scan.)

Verification: cargo fmt --all clean; cargo clippy -p engine --tests -- -D warnings clean; birgi::* (8) + swallow_check::tests::{birgi_boast_limit_static, twice_is_activation_limit} (2) all pass. Tilt was unreachable from my shell, so these were run directly.

[mcbradd]

@matthewevans — all three blockers are addressed in a40a7af (details in the comment above). Re-requesting review; I don't have permission to formally re-request reviewers on a fork PR, so flagging here. Thanks!

[matthewevans]

Changes requested — the prior CR and runtime-permission blockers are resolved, but the new DynamicQty exemption is still broader than the typed parser seam.

🔴 Blocker

• crates/engine/src/parser/swallow_check.rs:2060-2067 treats any unit containing "rather than once" as an activation-limit form. That is not tied to the only parser production that earns this exemption, StaticMode::ModifyActivationLimit in crates/engine/src/parser/oracle_static/dispatch.rs:2735-2757. An unsupported future clause such as a copy/repeat instruction using “twice … rather than once” (without twice that or twice x) would now skip the DynamicQty detector even though no typed activation-limit carrier was produced. The same predicate also feeds the repeat_for suppression at swallow_check.rs:2271.

  Please make the exemption evidence-backed: retain the existing narrow textual shape only as a locator, but require the scoped UnitEvidence to contain StaticMode::ModifyActivationLimit { .. } before suppressing the marker. Thread that evidence through the repeat-for helper as needed, and add a negative regression proving a non-ModifyActivationLimit “twice … rather than once” unit still reports DynamicQty.

✅ Clean

• The current head’s Harnfel tests now drive the real cast consumer and assert expiry across a turn boundary (birgi.rs:158-205, 466-468, 671-673), addressing the previous producer-only test gap.
• The revised citations are supported by the checked local CR source: 702.142a, 602.5b, 611.2a, 514.2, 614.17, 106.4, 605.1b, and 712.11b.
• The current parse-diff artifact reports no unrelated card-parse changes.

Recommendation: narrow the detector exemption to the typed ModifyActivationLimit evidence, add the negative coverage-honesty regression, and request re-review on that head.

[mcbradd]

Re-review requested on head 1b809c4 — both asks from the last round are addressed:

• Evidence-backed exemption: the rather than once arm of twice_is_activation_limit now requires the scoped UnitEvidence to carry StaticMode::ModifyActivationLimit { .. } (via the existing key-anchored any_static_mode helper) before suppressing the DynamicQty marker — the textual shape is retained as a locator only. The evidence parameter is threaded through both consumers: the has_marker gate in detect_dynamic_qty and the repeat_for suppression path via cleaned_twice_is_only_dynamic_marker. The multiplier veto (twice that/twice x) remains decisive even when the carrier is present.
• Negative regression: dynamic_qty_flags_unbacked_rather_than_once_wording drives detect_dynamic_qty with a twice … rather than once unit whose evidence carries no ModifyActivationLimit and asserts the DynamicQty diagnostic IS reported; reverting only the evidence gate makes it fail. The unit-level predicate test also pins that identical wording flips on evidence alone.

Also merged upstream/main (merge commit ee4966c) to clear the conflict — the integration_cards.json fixture conflict was resolved as a union: upstream's full regeneration (2931 entries, including its refresh of ~1350 existing cards and removal of the stale intermediate chirography fixture) plus this branch's two new entries (Birgi, Harnfel) → 2933 entries.

Verified locally: clippy clean, 186 swallow-check lib tests and all 8 Birgi/Harnfel integration tests pass; parser combinator gates (G + A) pass pre-commit.

🤖 Generated with Claude Code

[matthewevans]

Maintainer follow-up: the adversarial pass found that an unrelated repeat_for carrier could still suppress an unbacked rather than once dynamic-quantity warning. I fixed that narrow bypass in ec07f2c and added a regression test that supplies repeat_for without ModifyActivationLimit evidence. The PR is now held for the fresh CI run; I will not approve or enqueue it until that head is green.

[mcbradd]

Thanks for the adversarial pass and for ec07f2cc84 — the repeat_for bypass you found is real, and your fix for it is correct. I've kept your production change, guard, helper, assertion and comments exactly as written.

The red CI on that head is a fixture escaping slip, not a logic problem. In the new test the fixture string at crates/engine/src/parser/swallow_check.rs:8957 uses \\ where the line continuation \ was intended, so the newline and indentation are no longer elided — the string becomes ...rather \⏎ than once. and the substring rather than once is never present. Your guard at swallow_check.rs:2287 therefore never fires, cleaned_twice_is_only_dynamic_marker returns true, the repeat_for suppression returns early, and no diagnostic is emitted — which is the assertion failure at swallow_check.rs:8962 that CI reports (11373 passed, 1 failed on shard 2/2).

Your adjacent test dynamic_qty_flags_unbacked_rather_than_once_wording (swallow_check.rs:8936-8938) uses the same sentence with the single backslash, so I matched the new fixture to it. That is the entire change: one character, in the fixture literal only.

One-line proof: restore the \\ and dynamic_qty_flags_unbacked_rather_than_once_with_repeat_for fails with repeat_for must not hide unbacked 'rather than once' wording; with the single \ the swallow_check module is 151 passed / 0 failed / 2 ignored. The guard is genuinely load-bearing either way — it just needed a fixture that reaches it. The eight Birgi integration tests also pass, and cargo clippy -p engine --all-targets -- -D warnings is clean.

I also merged upstream/main (we were 10 behind). The only conflict was integration_cards.json, which is a single-line generated file so any edit on both sides collides. I took your side of that file wholesale and re-added only the two Birgi faces, then asserted that all 2945 of your keys are byte-identical in the merged fixture (2947 total = your 2945 + the two Birgi faces, nothing dropped) — 160 cards had values updated by your recent parser fixes and none were touched. Deliberately not regenerated: the local card-data.json predates those fixes, so regeneration would have reverted them.

[matthewevans]

Approved after the repaired fixture, evidence-backed DynamicQty exemption, current no-change parse diff, and focused re-review. Merge-when-ready will wait for the current CI run.