Flashback, mandatory cost, and 3 card implementations

[delebedev]

Summary

Five cards working end-to-end with puzzles and integration tests:

• Flashback (Think Twice) — ActionMapper extended to scan GY/Exile for castable spells, Forge handles resolve-to-exile automatically
• Mandatory discard cost (Mardu Outrider) — PromptClassifier routes discard-cost prompts to client as SelectNReq with promptId=1024
• Electroduplicate — flashback with abilityGrpId on GY cast actions + targeting during flashback cast
• Novice Inspector — investigate (Clue token creation) + sac-for-draw lifecycle
• Immersturm Predator — sacrifice-as-cost + tap trigger → GY exile + counter chain

Also fixed: displayCardUnderCard annotation now only fires for BF→Exile (not GY→Exile).

What's NOT in this PR (follow-up issues)

• Token diff strips cardTypes + uniqueAbilities on subsequent GSMs #243 — Token diff strips cardTypes/uniqueAbilities on subsequent GSMs
• Clicking BF creature passes priority instead of activating ability #244 — Clicking BF creature passes priority instead of activating ability
• CopyPermanent token subsystem missing — copy renders as broken card #245 — CopyPermanent token subsystem missing (Electroduplicate copy renders broken)

Test plan

• FlashbackTest: 2 tests (action mapping + full hand→GY→flashback→exile lifecycle)
• MandatoryDiscardCostTest: 5 tests (SelectNReq shape, resolves to BF, discard to GY)
• ElectroduplicateTest: flashback abilityGrpId + targeting + exile
• NoviceInspectorTest: cast → ETB → Clue token → sac-for-draw + ability registration
• ImmersturmPredatorTest: sacrifice-as-cost → tap trigger → counter + exile zone verification
• PromptClassifierTest: discard cost prompt classification
• Full gate: 649/649 pass, detekt clean
• Arena playtested: flashback (Think Twice), mandatory cost (Mardu Outrider), Novice Inspector, Immersturm Predator

🤖 Generated with Claude Code

[github-actions]

Test Results

  262 files  +  149    262 suites  +149   3m 10s ⏱️ +59s
1 042 tests +  240  1 015 ✅ +841   26 💤  - 602  1 ❌ +1 
1 864 runs  +1 062  1 030 ✅ +856  833 💤 +205  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 4f97f7b. ± Comparison against base commit 0d16c23.

♻️ This comment has been updated with latest results.

[github-actions]

CI Report — Gate

Tests: 182/813 passed, 1 failed (630 skipped)

Failed tests

• ConformancePipelineTest.Sacrifice segment: land + Treasure mana produces recording-matched annotations (0.2s)

  java.lang.AssertionError: Expected value to not be null, but was null.

Coverage: 10.8% (642/5923 lines)

Module	Coverage	Lines
tooling	🔴 11%	642/5923

Slow tests (>3s): 4

• AiTurnNoAarTest.AI turn sends no ActionsAvailableReq to client (34.5s)
• ActivatedAbilityPuzzleTest.Goblin Fireslinger tap-to-ping kills opponent (12.1s)
• PvpBridgeEndToEndTest.two-player game: land, creature, turn rotation, per-seat visibility (7.2s)
• DeclareBlockersDedupeTest.no duplicate blockers req (6.3s)

[delebedev]

Code Review

Branch: feat/flashback-and-mandatory-cost → main
Size: +1005 / -25 across 27 files (12 commits)

Overview

Five cards wired end-to-end with puzzle fixtures and integration tests: flashback (Think Twice, Electroduplicate), mandatory discard cost (Mardu Outrider), investigate/Clue tokens (Novice Inspector), and sacrifice-as-cost (Immersturm Predator). Also fixes displayCardUnderCard annotation to only fire for BF→Exile.

What's Good

• Puzzle-driven dev followed faithfully. Every card has a .pzl fixture and a corresponding integration test. Tests verify the full mechanic chain, not just happy-path entry.
• Clean architectural layering. Flashback/zone-cast logic lives in ActionMapper, discard-cost routing goes through PromptClassifier → TargetingHandler → RequestBuilder. No cross-layer shortcuts.
• PlayableActionQuery fix is smart. Removing the mayPlay() gate for zone casts fixes the root cause (smart phase skip incorrectly skipping Main1 when flashback is available) rather than patching around it.
• displayCardUnderCard fix is precise. Adding fromBattlefield flag to CardExiled and gating the annotation is the right granularity — doesn't over-complicate the event model.
• Good scope discipline. Known gaps (CopyPermanent subsystem, PayCostsReq conformance, ETB trigger annotations) are deferred to follow-up issues, not shoe-horned in.

Issues

1. LOG.md should not be committed. Session scratch file with local paths, manual playtest checklists, and arena automation notes. Not in .gitignore and doesn't belong in the public repo. Remove it from the PR.

2. tools/overlay/native/window-bounds is a committed binary. No explanation in the PR. Compiled binaries shouldn't be tracked unless there's a good reason. What is this, and should it be gitignored instead?

3. @Suppress("UNUSED_PARAMETER") on abilityRegistryLookup in addZoneCastActions (ActionMapper.kt:296). The parameter is passed from the call site but never used inside the function. Either use it (the abilityGrpId lookup does a manual cardData?.keywordAbilityGrpIds search instead) or remove the parameter. The suppress annotation is a code smell that suggests the wiring was started but abandoned.

4. addZoneCastActions only uses castable.first() (ActionMapper.kt:303). If a card has multiple castable abilities (e.g., a card with both flashback and escape), only the first is offered. Should this iterate all castable abilities and produce one action per ability?

5. ImmersturmPredatorTest hardcodes prompt message text (line 653):

   sacPrompt!!.request.message.lowercase() shouldBe "select another creature to sacrifice (%d left)"

   Brittle — tied to Forge's internal prompt string. If Forge changes wording, the test breaks for the wrong reason. Assert on structural properties (candidateRefs count, semantic) instead.

6. NoviceInspectorTest.beforeSpec mutates shared TestCardRegistry with Clue token grpId 300002. If another test uses the same registry concurrently and expects different token data, this could conflict. The magic numbers (300002, 152, 980803) deserve named constants or at minimum inline comments explaining where they come from.

7. castSpellByName now searches Hand → GY → Exile but the original method is also used by tests that expect hand-only casting. If a card exists in both hand and GY, the hand copy wins (correct), but the behavior change is implicit. The new castFromGraveyard method covers the explicit case — consider whether the broadened castSpellByName is needed or if callers should be explicit about zone.

Minor

• ElectroduplicateTest line 543: auto-pass combat skip logic is a workaround for test setup timing — a comment explaining why would help.
• NoviceInspectorTest uses repeat(15) { ... passPriority() } as a polling loop. The passUntil helper (used in other tests) is cleaner and has a clear failure mode.
• WebCostDecisionTest reflection call now passes 6 args including the new semantic parameter — one step more fragile. Not actionable now, worth noting for future refactor.

Verdict

Solid feature PR with good test coverage and clean mechanics wiring. Two blockers: LOG.md (remove) and window-bounds binary (explain or remove). The UNUSED_PARAMETER suppress and single-ability-per-card limitation are worth addressing before merge.

🤖 Generated with Claude Code