feat(engine): Captain America, First Avenger via UnattachFrom cost + divided damage

[JacobWoodson]

Implements MAR #88 (Captain America, First Avenger, {R}{W}{U} 4/4). Adds a
reusable "unattach an attachment from the source as a cost" building block, not
a one-off:

• New AbilityCost::UnattachFrom { filter, count } + PayCostKind::UnattachFrom.
  The detached Equipment stays on the battlefield (CR 701.3d) and becomes the
  ability's cost-referent, captured as a CostPaidObjectSnapshot before detach
  (CR 608.2k / 400.7j) so "that Equipment's mana value" resolves via
  QuantityRef::ObjectManaValue { scope: CostPaidObject }.
• Throw: {3}, unattach an Equipment -> deal its mana value divided among 1-3
  targets. The divided total is unknown until the cost is paid, so distribution
  defers (extract_distribution_total None while MV=0) and re-fires after payment
  (CR 601.2c/601.2d/601.2h).
• Change B-1: the DistributeAmong resume was spell-only; it now routes activated
  abilities to push_activated_ability_to_stack vs spells to finalize_cast
  (CR 602.2b), so a divided-damage activated ability resolves as an
  ActivatedAbility, not a Spell. Also propagates distribute onto the targeted
  activation pending and guards the post-cost target re-check against re-opening
  an already-announced division.
• Catch: begin-combat-your-turn trigger attaches up to one target Equipment you
  control to the source, reusing existing Attach + trigger primitives.
• Interactive unattach choice surfaced from pay_activation_costs_after_target_selection
  (targeted-activation post-target path), mirroring the non-self exile detour;
  eligible choices bounded to MV >= announced target count so no impossible
  distribution is reachable and nothing is detached on an unpayable activation.
• AI values the unattach cost ~0 (Equipment is not lost). Single authorized
  mechanical no-op arm added to the dormant mtgish-import crate to keep the
  workspace compiling as AbilityCost grows.

Tests: 10 cast-pipeline tests (activation resolves as ActivatedAbility;
X-divide spell still resolves as Spell; chosen Equipment's MV drives damage;
unpayable-when-MV<targets; {3} charged; Catch attach/no-op/opponent-turn) plus
parser unit tests for the cost combinator and the "that Equipment's mana value"
demonstrative quantity.

Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[github-actions]

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

1 card(s) · ability/DealDamage · added: DealDamage (amount=referenced object's mana value, kind=activated, target=any target, targets=1-3)

Examples: Captain America, First Avenger

1 card(s) · ability/deal · removed: deal (kind=activated)

Examples: Captain America, First Avenger

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

[matthewevans]

🔴 Request changes — two CI-red blockers, both PR-introduced. The feature's own behavior test fails and the test file is a top-level binary.

🔴 Blocker

• The core mechanic doesn't attach. catch_attaches_equipment_to_self_on_your_turn fails at crates/engine/tests/captain_america_throw.rs:657 — "Catch must attach the chosen Equipment to Captain America" (assert_eq! on Option<AttachTarget>, left != right). The Catch ability's UnattachFrom-cost → re-attach path isn't producing the AttachTarget, so the feature is non-functional on its own test. This must be green before the design can be reviewed on the merits.
• Top-level test binary. no_top_level_test_binaries::tests_dir_has_no_top_level_rs_files fails at crates/engine/tests/integration/no_top_level_test_binaries.rs:28. captain_america_throw.rs was added directly under crates/engine/tests/ — each top-level file compiles to its own ~130MB binary that re-links the entire engine on every change. Move the tests into crates/engine/tests/integration/ and add a mod captain_america_throw; line to crates/engine/tests/integration/main.rs.

🟡 Non-blocking

• +1210/−18 files is a large surface for one review; a deeper architecture pass (divided-damage division correctness per CR 510.1c, the UnattachFrom cost seam) is deferred until CI is green, since the attach fix will likely move code.

Recommendation: fix both blockers (make the Catch attach test pass; relocate the test file into integration/ + register the mod), push, and I'll do the full architecture review on green.

[matthewevans]

Hi @JacobWoodson — heads up that I've rebased this branch for you and force-pushed it (c4f2634b27 → ce8f2cdc63). Nothing was wrong with your work; the staleness was ours, so the fix was ours to do.

Why it went stale. main has been running an internal parser refactor (the U5/U6 campaign — ClauseIrBuilder/ClauseDisposition, and the SpecialClause enum being deleted outright). That churn invalidated your branch underneath you, across ~20 files in exactly the areas you touched. Asking you to chase that would have been asking you to win a race whose finish line keeps moving — main landed seven commits during a single rebase of a neighbouring PR. Our policy is that when our own refactor breaks a contributor's branch, we port it.

Something worth knowing: your CI was never actually running. The branch was CONFLICTING, and GitHub only creates the pull_request-triggered workflow runs when it can compute a merge ref. So this PR showed 2 passing checks — which look like a green — when in reality none of the ~13 real jobs had ever executed. It's now at 15 checks and genuinely running. If you ever see a suspiciously small check count, that's the tell.

Two changes I made on top of your commits (your commit is preserved as-is, under your authorship):

1. A parser fix that is our bug, not yours. Your card is the first to trigger it. The generic inline binary-choice splitter try_parse_choose_one_of_inline was matching the bare " or " inside a divided-damage target-count list — "one, two, or three targets" — and trial-parsing the orphan "three targets" tail back through the whole effect→subject→static→imperative cascade. That re-entry overflows small thread stacks, nondeterministically depending on stack headroom. Added a CR 601.2d bail-guard so distribution clauses skip the splitter and the dedicated try_parse_distribute_damage handler claims them, mirroring the four existing bail-guards.

2. Moved captain_america_throw.rs into tests/integration/ and registered it in tests/integration/main.rs. Repo convention (enforced by the no_top_level_test_binaries test): a new top-level file in crates/engine/tests/ becomes its own ~130 MB test binary that re-links the whole engine on every change.

Once CI reports, we'll review the substance on its merits. Since the parser guard has a pool-wide blast radius, I'll also be reading the parse-diff before signing off — that's on us, not you.

Thanks for the patience, and sorry the branch sat conflicted.

[JacobWoodson]

Thanks for the review — both flagged blockers are fixed in c4f2634. Full root-cause + fix context below so the re-review has everything it needs.

Blocker 1 — Catch doesn't attach (catch_attaches_equipment_to_self_on_your_turn)

Two distinct causes, both addressed:

(a) The CI assertion failure itself (0.095s, "not attached", no crash) was a runtime optional-target outcome. Catch targets "up to one target Equipment you control" (min 0, max 1). With exactly one legal Equipment, whether the engine auto-selects it or surfaces the optional target prompt varies by the auto-target path (it auto-attached on the dev box, did not on this CI). Per CR 603.3d the controller chooses, so the test now answers the prompt if one is offered and asserts the attach either way — deterministic on both paths. Since targeting the Equipment is always a legal completion, there is no auto-decline outcome to worry about.

(b) While root-causing, we found and fixed a PRE-EXISTING parser stack overflow on this card's Throw line (not the card's UnattachFrom/attach logic — this card is just the first to hit it). The generic inline binary-choice splitter try_parse_choose_one_of_inline (crates/engine/src/parser/oracle_effect/mod.rs) matched the bare " or " inside the divided-damage count list "…divided as you choose among one, two, or three targets" and trial-parsed the orphan "three targets" tail back through the full effect → subject → static → imperative cascade, re-entering the clause parser several levels deep. Its large per-frame AST locals overflow small (~1–2 MB) thread stacks nondeterministically by stack headroom — causing hard crashes / flakiness in small-stack environments. Fix: a CR 601.2d bail-guard that declines distribution clauses ("divided as you choose among" / "distributed among" / "divided evenly") so the dedicated try_parse_distribute_damage handler claims the whole clause. It mirrors the four existing comparator/target/type/restriction bail-guards in the same function and changes which clauses are accepted for zero cards — it only stops the erroneous split.

Blocker 2 — top-level test binary

Moved crates/engine/tests/captain_america_throw.rs → crates/engine/tests/integration/captain_america_throw.rs and registered it via mod captain_america_throw; in tests/integration/main.rs.

Verification (local)

• Parser fix: engine lib suite 15,690 passed / 0 failed; oracle_effect parser suite 2,522 / 0; parser combinator gate + string-dispatch gate pass.
• Overflow eliminated: the exact failing test run 30× as a standalone binary → 30/30 pass, 0 overflow (was 30/30 overflow at the small test-thread stack).
• Full integration harness on origin/main + this branch → 13 captain-related tests green, including catch_attaches_equipment_to_self_on_your_turn and the other MSH captain cards (no regressions).
• Parse-diff bot on this PR confirms the intended AST: DealDamage (amount=referenced object's mana value, kind=activated, target=any target, targets=1-3).

Heads-up on CI status

This is a fork PR from a non-maintainer account, so the full CI / AI gate workflows on the new push (c4f2634) are gated behind a maintainer's "Approve and run workflows" — they haven't run yet for that reason, not a failure. A maintainer approval will re-run them against the fix.

[matthewevans]

Approving. This supersedes my earlier CHANGES_REQUESTED — both of its reasons (the failing test and the no_top_level_test_binaries violation) were resolved by the maintainer rebase, so that formal state was stale.

Evidence:

• CI is real and green — 13 pass / 0 fail / 3 skipped. Worth stating plainly: before the rebase this PR showed 2 passing checks and looked green while nothing had run. A branch that is CONFLICTING at push time gets no refs/pull/N/merge, so GitHub never creates the pull_request workflow runs; the only things executing were a pull_request_target label job and an external scanner. The port restored actual CI.
• Parse-diff: exactly 1 card / 2 signatures, both Captain America, First Avenger — zero collateral across the pool. The divided-damage clause goes from a mangled deal (kind=activated) to a correct DealDamage (amount=referenced object's mana value, target=any target, targets=1-3).

On the maintainer commit (the CR 601.2d bail-guard): I wrote it, so I had it independently audited rather than trusting my own read. The audit found two real defects, both now fixed: a needle ("distributed among") that matches 0 of 34,632 cards in the Atomic pool, and a phrase list duplicated between the guard and the handler — two lists that must agree forever, with nothing enforcing it. They are now one (lower::DISTRIBUTION_KEYWORDS).

The audit's third finding is real and not fixed here: the guard keys on the distribution verb, but the hazard is the target-cardinality list, so ~32 counter-distribution cards ("Distribute three +1/+1 counters among one, two, or three target creatures") sit on the same stack cliff — and the underlying re-entry has no depth bound at all. That is a pre-existing condition on main which this PR neither introduces nor worsens, and fixing it means redesigning the cardinality vocabulary across three call sites. It belongs in its own PR with its own parse-diff, not smuggled into a contributor's card branch. Filed as #5613.

Thanks for the patience, @JacobWoodson — the branch sat conflicted because of our own U5/U6 refactor, not anything you did.