fix(parser): Cybership combat-damage trigger — top-two to battlefield as 2/2 Cybermen under controller

[ntindle]

Summary

WHO misparse cluster #19. Cybership's combat-damage trigger previously misparsed. This branch parses and resolves it correctly: when this Vehicle deals combat damage to a player, the top two cards of that player's library are put onto the battlefield face down, under the Cybership controller's control (not the library owner's), as 2/2 Cyberman artifact creatures.

The parser now emits an Effect::Manifest with count: 2, a profile of a 2/2 Cyberman artifact creature, target: TriggeringPlayer (the damaged player's library), and enters_under: You (the CR 110.2a controller redirect). The runtime routes the face-down entry through the shared morph/manifest zone-move pipeline with the controller override applied.

This branch was merged with upstream/main, which concurrently refactored the manifest/morph zone-move pipeline to a ZoneMoveRequest builder. Two conflicts were reconciled and two follow-on compile fixes were applied so cluster #19's behavior is preserved against the new API.

Cards

• Cybership (WHO) — Whenever this Vehicle deals combat damage to a player, put the top two cards of that player's library onto the battlefield face down under your control. They're 2/2 Cyberman artifact creatures.

Files changed

Parser / AST:

• parser/oracle_effect/imperative.rs, parser/oracle_effect/mod.rs, parser/oracle_effect/sequence.rs — parse the "put the top N cards of that player's library onto the battlefield face down under your control" manifest clause with controller redirect.
• parser/oracle_trigger.rs, parser/oracle_ir/ast.rs — trigger plumbing for the combat-damage manifest.
• types/ability.rs — Effect::Manifest carries count, profile, and enters_under (controller redirect).

Runtime:

• game/effects/manifest.rs — Effect::Manifest resolver: resolves the library owner, the optional CR 110.2a controller override, and manifests each card through the shared morph helper.
• game/effects/manifest_dread.rs, game/effects/mod.rs — registration/plumbing.
• game/morph.rs — manifest_card applies the under_control_of controller override when present.
• game/engine_resolution_choices.rs — Manifest Dread choice path (reconciled to the new builder API).

Test:

• tests/integration/issue_2890_reality_shift.rs — signature update.

Merge reconciliation

• game/morph.rs: kept upstream's correct source_id attribution on the zone-move call AND preserved cluster chore: update coverage stats and badges #19's .under_control_of(controller) redirect (CR 110.2a).
• game/engine_resolution_choices.rs: took upstream's refactored ZoneMoveRequest::effect(...).face_down(...) builder form for the Manifest Dread path (no controller redirect there — owner == controller).
• game/effects/manifest.rs: passed ability.source_id to the now-source_id-bearing manifest_card, and added the new ControllerRef::ParentTargetOwner → TargetFilter::ParentTargetOwner arm in resolve_enters_under.

CR refs

• CR 110.2a — "If an effect instructs a player to put an object onto the battlefield, that object enters the battlefield under that player's control unless the effect states otherwise." (Cybership's "under your control" controller redirect.)
• CR 108.3 — card ownership (the ParentTargetOwner controller-ref maps to the parent target object's owner).
• CR 701.40a / 701.40e — manifest face-down 2/2 mechanics, one card at a time.
• CR 708.2a / 708.3 — face-down profile application.

Verification

• cargo fmt --all — clean.
• cargo clippy -p engine --all-targets -- -D warnings — clean.
• cargo test -p engine — all suites green.
• cargo run --profile tool --features cli --bin oracle-gen -- data --filter "cybership" — emits the DamageDone (CombatOnly) trigger with Effect::Manifest { target: TriggeringPlayer, count: 2, profile: 2/2 Cyberman artifact, enters_under: You }.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.8 (1M context) 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!

[matthewevans]

Maintainer review — changes requested (one blocking item)

Thanks — this is built on the right seam (reuses morph::manifest_card + the zone-change face_down() / under_control_of() builders rather than re-implementing face-down creation, routes the put-clause through Effect::Manifest, and enters_under is an established parameterization already on ChangeZone/ChangeZoneAll). CR annotations (110.2a / 708.2a / 701.40a / 603.7c) all verify. This is inline-mergeable once the blocking item is fixed — not engine-implementer-tier.

BLOCKING — single-authority violation (controller resolution)

crates/engine/src/game/effects/manifest.rs (resolve_enters_under) reinvents the canonical ControllerRef → PlayerId resolver. The engine already has the single authority, and there is already a wrapper for exactly this purpose:

• crates/engine/src/game/filter.rs:678 — controller_ref_player(state, source_id, source_controller, ability, cref) (exhaustive over every ControllerRef variant).
• crates/engine/src/game/effects/change_zone.rs:61 — resolve_enters_under_player, which already wraps it to resolve an enters_under: Option<&ControllerRef> on a zone-change (matching CR 110.2a annotation).

The hand-rolled version introduces latent semantic bugs the canonical resolver doesn't have:

• It collapses ControllerRef::TargetPlayer | TriggeringPlayer both to TargetFilter::TriggeringPlayer, whereas controller_ref_player keeps them distinct (reads ability.targets for TargetPlayer vs triggering_event_player for TriggeringPlayer).
• It silently falls DefendingPlayer / ChosenPlayer / SourceChosenPlayer back to ability.controller, while the canonical resolver resolves all three.

For Cybership today only ControllerRef::You is produced, so the bug is dormant — but per the codebase's single-authority hard rule, a divergent second implementation of player-reference resolution can't merge. Fix: delete resolve_enters_under and call crate::game::filter::controller_ref_player(state, ability.source_id, Some(ability.controller), Some(ability), &cref), or make change_zone::resolve_enters_under_player pub(crate) and reuse it (mirrors change_zone.rs:76).

Strong recommendation — add a discriminating Cybership test

There's no Cybership-named or combat-damage-driven integration test (grep -rln cybership crates/engine/tests is empty). The new manifest_with_profile_and_controller_override test drives resolve() with a manually-set current_trigger_event, which is shape-ish for the load-bearing trigger binding. Please add an integration test that crews/attacks Cybership and asserts the top two cards enter face-down 2/2 Cybermen under the Cybership controller's control (ownership staying with the library's owner) — that's the discriminating end-to-end guard for the mechanic this PR adds.

Happy to re-review promptly once the resolver is unified. The seam and the CR work are solid.

[matthewevans]

Maintainer follow-up — resolved & enqueuing

Pushed a commit addressing both review items.

BLOCKING (single-authority) — fixed. Deleted the hand-rolled resolve_enters_under in manifest.rs and routed the CR 110.2a enters_under override through the canonical authority: promoted change_zone::resolve_enters_under_player to pub(crate) (it wraps filter::controller_ref_player) and call it from the manifest resolver. This is the same path ChangeZone/ChangeZoneAll use, so the two latent bugs of the divergent copy are gone — TargetPlayer/TriggeringPlayer stay distinct, and DefendingPlayer/ChosenPlayer/SourceChosenPlayer resolve instead of silently falling back to the controller.

Strong recommendation (integration test) — added. New cybership_combat_damage_manifest integration test crews + attacks Cybership through the real combat pipeline and asserts the top two cards of the damaged player's library enter face-down 2/2 Cyberman artifact creatures under the Cybership controller's control (CR 110.2a) while ownership stays with the library owner. The controller == P0 (override) vs owner == P1 assertion is the discriminating guard for the mechanic.

Also fixed two semantic merge collisions from origin/main (the new manifest_card controller arg): zones.rs and issue_3285_face_down_public_zone.rs now pass None (CR 701.40a default).

Verification: cargo fmt clean · clippy -p engine --all-targets -D warnings clean · cargo test -p engine 12287 lib + 1112 integration, 0 failed.

[matthewevans]

Single-authority controller resolution unified via the canonical resolver; discriminating Cybership combat-damage integration test added. Full engine suite green.