refactor(engine): unify tap effects into Effect::SetTapState (#2777)

[dripsmvcp]

Phase 1 of #2777 (parameterize Effect sibling-clusters).

What

Collapse the four tap-state siblings — Tap, Untap, TapAll, UntapAll — into one parameterized variant:

Effect::SetTapState { target: TargetFilter, scope: EffectScope, state: TapStateChange }

with new typed axes EffectScope { Single, All } (a reusable scope primitive for the remaining #2777 clusters) and TapStateChange { Tap, Untap }. Mapping: Tap→{Single,Tap}, Untap→{Single,Untap}, TapAll→{All,Tap}, UntapAll→{All,Untap}.

Why scope is a real field (not a TargetFilter fold)

Unlike the *All collapse in #1693/#1736 (where Destroy behaves identically once you have the object set), the tap Single and All resolvers genuinely diverge: Single resolves chosen / SelfRef / TrackedSet subjects and runs a resolution-timed multi-target prompt; All iterates the battlefield by filter. So scope dispatches between two real code paths.

Behavior preserved at every site

• target_filter(): Single→Some(target), All→None (was Tap/Untap-has-target, *All-no-target).
• EffectKind kept whole (4 variants) and effect_variant_name() still maps (scope,state) → the 4 legacy strings, so coverage classification and EffectResolved tags are byte-stable.
• Every match arm that distinguished the four variants branches on (scope, state) — no behavior-erasing catch-all (AI polarity Single+Tap=Harmful / Single+Untap=Beneficial; coverage swallowed-clause guards; the resolver; the "did-anything" / event-context arms).
• Forge importer constructors and the build-unbreaking mtgish-import destructures updated (mechanical rename only; ReplacementEvent::Tap/Untap — a different enum — left untouched).
• Committed snapshots/fixtures whose serde "type" tag flips were regenerated.

Scope

The other ~13 #2777 clusters (Destroy/Bounce/Counter/Pump/Goad/DoublePT/…) are deferred to follow-up PRs that reuse EffectScope. Shipping one cluster with full verification (the prior all-at-once attempt #2778 was closed for failing CI).

Verification

cargo build (full workspace incl. mtgish-import), clippy -D warnings, cargo test -p engine (11233 lib + 807 integration), cargo test -p phase-ai — all green; snapshots blessed; a building-block set_tap_state_routes_all_four_quadrants test added. The card-data swallowed-clause gate is coverage-neutral by construction (pure variant rename; the coverage.rs swallow detectors are byte-identical) — CI’s card-data job confirms.

CR 701.26a / CR 701.26b.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request refactors the game engine's tap and untap effects by collapsing the legacy Tap, Untap, TapAll, and UntapAll variants of Effect into a single parameterized SetTapState variant. This variant uses EffectScope (Single or All) and TapStateChange (Tap or Untap) to determine its behavior, simplifying the codebase and reducing variant proliferation in accordance with style guide rule R3. The changes are propagated across the engine, parser, AI policies, and tests. Feedback on the pull request identifies a minor violation of style guide rule R6 in overload.rs, where a comment incorrectly references CR 702.96e instead of CR 702.96b for Overload rules.

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.

[gemini-code-assist]

[MEDIUM] Hallucinated CR rule number in comment.

The comment cites CR 702.96e, but the rules for Overload text-changing effects are defined under CR 702.96b. Please update the reference to CR 702.96b to ensure strict fidelity and compliance with the repository's style guide (R6).

Suggested change

	// CR 702.96e + CR 701.26a/b: overload promotes single-target tap/untap to
	// its mass scope, carrying the tap/untap polarity through. (Only the Tap
	// polarity appears in the current overload corpus; Untap promotion is
	// type-supported for parity.)
	// CR 702.96b + CR 701.26a/b: overload promotes single-target tap/untap to
	// its mass scope, carrying the tap/untap polarity through. (Only the Tap
	// polarity appears in the current overload corpus; Untap promotion is
	// type-supported for parity.)

References

1. Every rules-touching line of engine code must carry a comment of the form CR : . The number must be verified against docs/MagicCompRules.txt before writing. ^(link)

[matthewevans]

Review: refactor(engine): unify tap effects into Effect::SetTapState (#2777 Phase 1)

Verdict: Approve with one required fix — a single hallucinated CR subpart (CR 702.96e) must be corrected before merge. Everything else is a clean, genuinely behavior-preserving collapse and a well-judged precedent for the #2777 cluster program. I verified the refactor against the working tree at head 6bb267b (diff fetched via the GitHub API, not gh pr diff).

Below, findings first, then explicit assessments of the five falsifiable claims.

---

Findings

[MED] Hallucinated CR subpart CR 702.96e in overload.rs. Evidence: crates/engine/src/game/effects/overload.rs overload arm comment (// CR 702.96e + CR 701.26a/b: overload promotes single-target tap/untap…). grep -nE "^702\.96[a-z]" docs/MagicCompRules.txt resolves only 702.96a/b/c — there is no 702.96e. Why it matters: the repo's CR-annotation mandate requires every CR number be grep-verified; a wrong number creates false confidence the rule was checked. Suggested fix: cite CR 702.96a (the "replace every instance of 'target' with 'each'" text-changing rule that actually justifies the Single→All promotion). This is the same issue Gemini raised — confirmed, and the precise replacement is 702.96a (the text-replacement clause), not 702.96b (Gemini's suggestion, which is the "no targets required" clause). The other 23 CR citations added by the PR all verify.

[LOW] runtime_card_export_fixture.json lost its trailing newline. Evidence: crates/engine/tests/fixtures/runtime_card_export_fixture.json (}\n → } with \ No newline at end of file); the file contains no tap-effect content, so this is an unrelated whitespace change (likely an editor stripping the final newline). Why it matters: violates the repo "always include a newline at end of file" convention and pollutes an otherwise mechanical diff. Suggested fix: restore the trailing newline.

[LOW, non-blocking] process_one_tap/process_one_untap now propagate EffectError::ObjectNotFound where the legacy mass path silently skipped. Evidence: crates/engine/src/game/effects/tap_untap.rs — old resolve_tap_all/resolve_untap_all used if let Some(obj) = state.objects.get_mut(...) (skip-on-miss); the new shared helpers use .ok_or(EffectError::ObjectNotFound)?. Why it matters: if a replacement somehow removes the object between replace_event→Execute and the mutation, the whole mass resolution now errors instead of skipping. This is essentially unreachable (the id came from the current battlefield snapshot and replace_event returned Execute), and the single-target path always used this stricter form, so behavior is unified, not regressed. Noted for completeness only.

---

Claim verification

(A) Byte-stable variant naming / serde tags — CONFIRMED (with the correct nuance).

• effect_variant_name() (types/ability.rs) exhaustively maps all four (scope,state) quadrants back to "Tap"/"Untap"/"TapAll"/"UntapAll" — no catch-all.
• EffectKind is kept whole (4 variants) and From<&Effect> maps all four quadrants, so GameEvent::EffectResolved { kind } serde tags are unchanged.
• coverage.rs::effect_type_name short-circuits SetTapState to effect_variant_name() rather than reading the serde tag, so per-effect coverage labels stay byte-identical.
• Correctly, the Effect serde "type" tag does flip "Tap"→"SetTapState" (regenerated in the replacement snapshot + etb_tapped golden). I confirmed no frontend or runtime consumer reads the Effect "type" tag for tap effects (TapForConvoke/TapCreatures/UntapChoice in the client are unrelated enums), and integration_cards.json migrated cleanly — all 32 effect-level taps are now SetTapState; the 115 remaining "type":"Tap"/"Untap" strings are AbilityCost::Tap and TapStateChange payloads (correctly untouched).

(B) No behavior-erasing catch-all across AI polarity + coverage + resolver — CONFIRMED. Every site that distinguished the four legacy variants branches on (scope,state) and lets the unmatched quadrants fall to the same arm they hit before the collapse:

• effect_classify.rs: Single+Untap→Beneficial, Single+Tap→Harmful; All falls to the Contextual catch-all (== legacy TapAll/UntapAll, which had no explicit arm). extract_target_filter: Single→Some(target), All→None. Correct.
• etb_value.rs: only Single+Tap scores 0.12; all else →0.0 (legacy had only Effect::Tap). Correct.
• anti_self_harm.rs: matches Single+Untap. cast_facts.rs: Single→target-required, All→false. redundancy_avoidance.rs: Single+Tap/Single+Untap dispatched; All explicitly in the no-op group. All correct.
• Engine: target_filter() (Single→Some/All→None), extract_event_context_filter (Single only), affected_objects_from_events (Single publishes the tapped set; All→default), mandatory_parent_effect_performed (branches on state only — exactly equivalent to the old Tap|TapAll / Untap|UntapAll grouping). No quadrant silently mis-routed.

(C) EffectScope primitive design — SOUND, with a precedent caveat. scope is genuinely load-bearing: resolve_single runs the target/SelfRef/TrackedSet + resolution-prompt machinery (tap_untap_target_ids + prompt_resolution_tap_untap_choice), while resolve_all iterates the battlefield via matches_target_filter over resolved_object_filter. These are two real, divergent code paths (CR 115 targeting vs CR 608.2 resolution-time population selection), so the field is not foldable into TargetFilter — the PR even proves this empirically by keeping mtgish "tap each" as Single with a multi-match filter. Putting the axis at the Effect leaf is the right seam for the cluster program: it's exactly the X/XAll sibling-cluster smell CLAUDE.md says to parameterize. Caveat for the 13× reuse: tap is the easy cluster because Single and All carry identical fields. Several other *All siblings carry scope-specific fields (BounceAll.count, DestroyAll/Destroy share cant_regenerate, DamageAll.player_filter, ChangeZoneAll drops single-only modifiers — see overload.rs). For those, a naive { scope, ...union } would leave Single carrying meaningless count/player_filter. EffectScope is the correct axis name, but follow-up PRs must apply per-cluster judgment (or a wrapper) where fields diverge rather than blindly unioning. Recommend stating this in the #2777 tracking issue so later clusters don't over-fold.

(D) mtgish-import correctness — CONFIRMED. action.rs and replacement.rs are mechanical and faithful. Notably, TapEachPermanent/UntapEachPermanent are preserved as scope: Single (with a multi-match filter), exactly mirroring the legacy Effect::Tap { target: convert_permanents(..) } — the conservative, behavior-preserving choice (promoting these to All would be an out-of-scope behavior change; whether the legacy single-scope mapping is itself ideal is a pre-existing question). ReplacementEvent::Tap/Untap (a different enum) is correctly left untouched — verified zero references in the diff. Golden fixtures (etb_tapped, etb_replacement_plus_trigger) regenerated consistently.

(E) CR 701.26 verification — CONFIRMED. docs/MagicCompRules.txt:3514 = 701.26a To tap a permanent…, :3516 = 701.26b To untap a permanent…. Tap/untap is exactly 701.26a/b as cited throughout.

---

Tests

The set_tap_state_routes_all_four_quadrants building-block test is discriminating — it drives resolve_set_tap_state (the production entry) through all four quadrants and asserts the actual tapped-state outcome, with Single going through the target path and All through the population-filter path (distinct fixtures: object-target vs typed creature filter). The integration assertions are also discriminating (issue_1308 asserts All/Untap, urge_to_feed asserts Single/Tap), not loosened to SetTapState { .. }. Good coverage of the routing.

I did not run the suite directly (Tilt is up and rebuilding a dirty concurrent tree; test-engine's historical error is attributable to unrelated in-progress work on main, not this branch, which I reviewed via FETCH_HEAD). The change is behavior-preserving by construction and the regenerated snapshots/goldens confirm the serde shape.

Net: strong, idiomatic execution of the repo's #1 architecture principle. Fix the CR 702.96e → CR 702.96a citation (and ideally restore the fixture newline) and this is good to merge.

[dripsmvcp]

Both addressed:

• [MED] CR citation — corrected CR 702.96e → CR 702.96a in overload.rs, and reworded the comment to reference the text-change rule (replace every "target" with "each") that actually justifies the single→all promotion. Used 702.96a per the maintainer note rather than 702.96b — 702.96a is the text-replacement clause; 702.96b is the "no targets required" clause. Grep-verified against docs/MagicCompRules.txt (702.96a/b/c are the only subparts; there is no 702.96e).
• [LOW] fixture newline — restored the trailing newline on runtime_card_export_fixture.json.

The non-blocking ObjectNotFound propagation note is left as-is (unified with the single-target path, essentially unreachable, as you noted).

[matthewevans]

Re-review (since 10:42Z): ✅ Required fix landed — clean approve.

Both follow-up commits address my prior findings exactly:

• CR fix: the hallucinated CR 702.96e is gone; overload.rs now cites CR 702.96a (the text-change "replace 'target' with 'each'" rule justifying the Single→All promotion) and CR 702.96b — both grep-verified against docs/MagicCompRules.txt.
• Fixture: runtime_card_export_fixture.json's trailing newline is restored (the file is no longer in the diff).

No other changes; the behavior-preserving tap-cluster collapse (Effect::SetTapState) reviewed earlier stands. LGTM.

[dripsmvcp]

Brought current with main — the branch had gone DIRTY after recent landings. Merged upstream/main in (commit 0ca37b5e); the conflicts were all in the shared files the SetTapState collapse touches (types/ability.rs, effects/mod.rs, the two oracle_effect parsers, printed_cards.rs, and integration_cards.json). Resolved by keeping the SetTapState {scope,state} handling and folding in main's concurrent additions (e.g. AddCounter moving out of the target_filter() chain). Re-verified: build clean, set_tap_state_routes_all_four_quadrants + full lib (11253) + integration (817) green, clippy clean, parser gate clean.

[natefinch]

Review (review-impl lenses): refactor(engine): unify tap effects into Effect::SetTapState

Reviewed the ground-truth API diff (head 0ca37b5e) end-to-end. This is a clean, genuinely behavior-preserving collapse of the four legacy Tap/Untap/TapAll/UntapAll variants into one parameterized Effect::SetTapState { target, scope, state }. It is the textbook "parameterize, don't proliferate" refactor: the two new axes are typed enums (EffectScope::{Single,All}, TapStateChange::{Tap,Untap}), not bools, and the parameterization stays within one CR section (701.26).

Seam/idiom gates: pass. The collapse lives at the right layer (types/ability.rs + the effect resolver), and every consumer was migrated in lockstep.

Verification performed:

• Resolver equivalence — resolve_all now routes through process_one_tap/process_one_untap (replacement-aware) instead of the old inline ProposedEvent loops; behavior is equivalent and slightly more robust (explicit ObjectNotFound). resolve_single preserves the SelfRef/TrackedSet/resolution-prompt path and the EffectResolved push.
• target_filter() correctly exposes the target only for scope: Single and returns None for All, matching legacy TapAll/UntapAll (non-targeting).
• Tooling compatibility — effect_variant_name and EffectKind::from map the (scope, state) quadrants back to the four legacy labels, so coverage/diagnostic tooling keys on the same names.
• AI policies (effect_polarity, redundancy_avoidance, etb_value, anti_self_harm, cast_facts) all gate on scope: Single, faithfully reproducing the legacy behavior where only Tap/Untap (not the mass scopes) were matched.
• Frontend — unaffected; the client does not enumerate Effect type strings (only log/cost/phase tags, which are unrelated AbilityCost::Tap/Untap-phase concerns).
• CR citations — 701.26a/b and 702.96a verified against docs/MagicCompRules.txt. The prior CR 702.96e hallucination (raised by Gemini + matthewevans) is resolved; overload.rs now cites 702.96a (the "replace 'target' with 'each'" text-change clause), which is more precise than Gemini's suggested 702.96b — confirmed and folded, not echoed.
• Data — card-data.json is gitignored/regenerated; the only committed fixture (integration_cards.json) and the two golden JSONs were regenerated to the new shape. All CI green (Rust fmt/clippy/test/coverage, parser gate, frontend, card-data, WASM, Tauri).
• New building-block test exercises all four (scope, state) quadrants through resolve_set_tap_state.

One non-blocking observation:

• [LOW] serde default divergence for SetTapState::target. Evidence: types/ability.rs — target defaults to Any for all scopes, whereas legacy TapAll/UntapAll defaulted to TargetFilter::None. Why it matters: a hypothetical scope: All value deserialized with target omitted would mass-affect every permanent (Any) instead of the legacy no-op (None). Not reachable today — every mass emitter supplies an explicit filter and card-data is regenerated (no persisted old data), so this is purely a defensive-default note. Suggested fix (optional): split the serde default by scope, or drop the target default so mass emitters must always be explicit.

VERDICT: approve

[dripsmvcp]

Thanks for the thorough second pass. On the [LOW] serde-default note (SetTapState::target defaulting to Any vs legacy TapAll/UntapAll's None): agreed it's unreachable today — every mass emitter supplies an explicit filter and card-data is regenerated, so no persisted data omits target. I've left the default as-is for this PR rather than fold a scope-split serde default in here, since that's a deserialization-contract change with broader implications worth its own focused change; happy to follow up separately if you'd prefer the defensive split. The behavioral collapse is unchanged.

[matthewevans]

Maintainer sign-off: 4 sibling tap variants collapsed into parameterized Effect::SetTapState (#2777 Phase 1) — parameterize-don't-proliferate at the prescribed seam; behavior preservation traced by two reviewers; hallucinated CR 702.96e corrected to 702.96a; brought current with main by author.

[matthewevans]

Maintainer note: the merge queue bisected this PR out (E0599 in the speculative group) — three enters-tapped test fixtures using Effect::Tap landed on main (attractions.rs / keywords.rs / morph.rs) after this branch's last update. Pushed 74b949f merging current main and porting them to SetTapState { Single, Tap } per this PR's own convention. Same concurrent-variant class as #2799's kick; expected cost of enum collapses on a fast main. Re-enqueued.