Skip to content

Add Necropotence - #671

Merged
matthewevans merged 12 commits into
phase-rs:mainfrom
bruno-branco-brito:card/necropotence
May 21, 2026
Merged

Add Necropotence#671
matthewevans merged 12 commits into
phase-rs:mainfrom
bruno-branco-brito:card/necropotence

Conversation

@bruno-branco-brito

@bruno-branco-brito bruno-branco-brito commented May 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds engine support for Necropotence and the broader face-down-exile class (Bomat Courier, Asmodeus the Archfiend, Knowledge Vault).

Files changed

  • crates/engine/src/game/effects/exile_top.rs
  • crates/engine/src/game/effects/mod.rs
  • crates/engine/src/game/effects/deal_damage.rs
  • crates/engine/src/game/coverage.rs
  • crates/engine/src/game/dungeon.rs
  • crates/engine/src/game/visibility.rs
  • crates/engine/src/parser/oracle_effect/mod.rs
  • crates/engine/src/parser/oracle_effect/imperative.rs
  • crates/engine/src/parser/oracle_ir/ast.rs
  • crates/engine/src/parser/oracle_trigger.rs
  • crates/engine/src/parser/oracle_replacement.rs
  • crates/engine/src/parser/oracle.rs
  • crates/engine/src/parser/oracle_ir/snapshots/...__bomat_courier_ir.snap
  • crates/engine/src/parser/oracle_ir/snapshots/...__bomat_courier_lowered.snap
  • crates/engine/src/types/ability.rs
  • crates/mtgish-import/src/convert/action.rs

CR references

  • CR 406.1 — exile zone is a holding area for objects (authorizing reference for treating ExileTop as destination=Exile for tracked-set chain assembly).
  • CR 406.3 — a card exiled face down can't be examined by any player except when instructions allow it (authorizing rule for the new Effect::ExileTop { face_down: true } redaction path).
  • CR 603.7 — effects may create delayed triggered abilities (authorizing rule for the recall delayed-trigger pathway).
  • CR 603.7c — delayed triggered abilities that refer to a particular object (authorizing rule for tracked-set object binding).
  • CR 701.9 — Discard (authorizing rule for the on-discard trigger event).

(CR 614.10 / CR 614.1b already implement the "Skip your draw step." static-ability piece via StaticMode::SkipStep { step: Phase::Draw } — not modified by this PR.)

Review round 2 — gemini-code-assist findings addressed

  • HIGH (face-down fidelity, CR 406.3 / Rule R17): Effect::ExileTop is now parameterized with a face_down field. The imperative parser strips a trailing "face down" qualifier via a new strip_exile_top_face_down helper (nom tag for dispatch, post-tag char-boundary check), threading the bool through ZoneCounterImperativeAst::ExileTop into the lowered effect. The resolver flips obj.face_down after move_to_zone (foretell pattern). visibility.rs generalizes the prior foretold-only filter so every face-down card in Exile is redacted for non-owner viewers via hide_card. New resolver tests (exile_top_face_down_sets_object_face_down_flag, face-up counter-test) and a parser test (exile_top_card_of_your_library_face_down_parses_with_face_down_true) cover the new behavior; Bomat Courier IR + lowered snapshots refreshed to show face_down: true. The field uses #[serde(default, skip_serializing_if = "std::ops::Not::not")] so existing JSON card-data for face-up exiles is byte-identical.
  • MED (im::Vector for tracked-set buildup, Rule R7): Resolved by eliminating the local collection altogether — the generic chain processor in effects::resolve_ability_chain already publishes ExileTop's affected ids via affected_objects_from_events (mapped to the Exile destination zone), so the resolver no longer maintains its own buffer. This also fixes a pre-existing double-publish that surfaced as a regression in compound_zone_change_chain_unifies_tracked_set (lib_card was receiving two PlayFromExile grants because both the leaf resolver and the chain processor were publishing). The publishing-side test exile_top_publishes_tracked_set_when_followed_by_recall_delayed_trigger now drives through effects::resolve_ability_chain so the chain-layer publish runs in test as it does in live game resolution.

Track

Non-developer

LLM

Model: claude-opus-4-7
Thinking: medium

Verification

Local verification skipped — see CI status checks.

Validation Failures

None.

CI Failures

None.

Bruno Branco e Brito and others added 3 commits May 21, 2026 11:06
Extends the existing exile-then-return-to-battlefield tracked-set pathway
(Aetherling family) to cover the exile-top-then-recall-to-hand class
(Necropotence, Bomat Courier, Asmodeus, Knowledge Vault — ~16 cards).

- `is_exile_effect` now matches `Effect::ExileTop` alongside `ChangeZone {
  destination: Exile, .. }` so cross-clause anaphors against a prior
  `ExileTop` participate in the tracked-set publish/consume protocol
  (CR 406.1 + CR 603.7).
- `contains_implicit_tracked_set_pronoun` adds a hand-recall branch
  composed via nested `alt()` over the pronoun axis ("put {that card,
  them, it} into your hand"). The battlefield-recall branch is unchanged.
- The chain-assembly `prev_zone` site recognizes `ExileTop` as having
  destination `Zone::Exile`, so `stamp_delayed_returns` correctly stamps
  `origin: Exile` on the delayed-trigger return effect for the
  resolver's CR 400.7 referent-zone guard.

Also adds two locking tests:
- Necropotence snapshot test (`oracle_ir/snapshot_tests.rs`) — full
  three-layer card: static (skip draw) + on-discard trigger (exile from
  graveyard) + activated (pay 1 life, exile top, delayed recall).
- Focused trigger test asserting `Whenever you discard a card, exile
  that card from your graveyard.` lowers to `TriggerMode::Discarded`
  with the body's anaphor lifted from `ParentTarget` to
  `TriggeringSource`.

Face-down on `Effect::ExileTop` remains unmodeled — adding a `face_down:
bool` parameter requires resolver + multiplayer hidden-info filter work
and is deferred. Tracked-set binding works by ObjectId regardless of
face state, so recall resolves correctly today.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The parser-side changes mark Necropotence-class delayed triggers with
`uses_tracked_set: true`, but the resolver still needs to publish the
set of exiled ObjectIds for the recall to bind to. Mirror the existing
gate used by `change_zone::resolve`:

- Promote `next_sub_needs_tracked_set` to `pub(crate)` so sibling
  resolvers can re-use the existing detector.
- After moving each top card to exile, push its ObjectId onto the
  publish list when `next_sub_needs_tracked_set(ability)` is true, then
  call `publish_tracked_set` once. Free of cost when no downstream
  reference exists.

Locks the behavior with a focused test that constructs the parsed
Necropotence-shape ability directly (ExileTop -> CreateDelayedTrigger
{uses_tracked_set: true}) and asserts the published set contains the
exiled card after resolution.

CR 603.7 + CR 406.1.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The snapshot test referenced `.snap` files that were never committed, so
in CI (no `cargo insta accept`) insta writes them as `.snap.new` and
fails the test. Functional coverage of all three layers is already
locked by dedicated unit tests:

- `Skip your draw step.` -> `StaticMode::SkipStep { step: Phase::Draw }`
  is exercised in `game/turns.rs` SkipStep tests (which name
  Necropotence directly).
- The "whenever you discard a card, exile that card from your
  graveyard" trigger shape (mode, controller filter, ParentTarget ->
  TriggeringSource lift, ChangeZone graveyard -> exile) is locked in
  `parser/oracle_trigger.rs::trigger_necropotence_you_discard_exile_from_graveyard`.
- The ExileTop -> tracked-set -> delayed recall pathway is locked in
  `game/effects/exile_top.rs::exile_top_publishes_tracked_set_when_followed_by_recall_delayed_trigger`.

The IR snapshot only added human-reviewable artifact value, not
functional coverage. Removing it unbreaks CI without losing assertions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added ai-contribution PR opened via docs/AI-CONTRIBUTOR.md flow needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) labels May 21, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the ExileTop effect to support publishing tracked sets, enabling downstream delayed triggers (like Necropotence's end-step recall) to correctly reference exiled cards. It also includes parser tests for discard-to-exile triggers and adjusts the visibility of the next_sub_needs_tracked_set helper. Feedback indicates that the implementation currently lacks support for face-down exile, violating CR 406.3 and Rule R17 regarding hidden information integrity. Additionally, the use of a standard Vec for collecting exiled IDs violates Rule R7, which requires persistent data structures (im::Vector) for GameState collections to maintain efficient structural sharing.

Comment thread crates/engine/src/game/effects/exile_top.rs
Comment thread crates/engine/src/game/effects/exile_top.rs Outdated
matthewevans and others added 7 commits May 21, 2026 06:39
Adds face-down fidelity to `Effect::ExileTop` so cards exiled face down
are redacted for non-owner viewers per CR 406.3, closing the deferred
gap flagged by gemini-code-assist on PR phase-rs#671. Also switches the local
exiled-id builder to `im::Vector` to satisfy Rule R7.

Engine:
- `Effect::ExileTop` gains `#[serde(default, skip_serializing_if=...)]`
  `face_down: bool` (default false; absent in JSON for face-up exiles
  so existing card-data stays byte-identical).
- `exile_top::resolve` extracts `face_down`, flips `obj.face_down` on
  each moved object (foretell-pattern, post `move_to_zone`), and builds
  the tracked-set via `im::Vector::push_back`.
- `visibility.rs` generalizes the prior foretold-only filter to any
  face-down exile, so Necropotence / Bomat Courier / Asmodeus / Knowledge
  Vault all hide their exile pile from opponents.

Parser:
- Imperative `parse_exile_ast` strips a trailing "face down" qualifier
  via a new `strip_exile_top_face_down` helper, threading the bool
  through `ZoneCounterImperativeAst::ExileTop` to the lowered effect.
- Bomat Courier IR + lowered snapshots refreshed (face_down=true).

Tests:
- `exile_top_face_down_sets_object_face_down_flag` and the face-up
  counter-test verify the resolver side.
- `exile_top_card_of_your_library_face_down_parses_with_face_down_true`
  verifies the parser side.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The parser-combinator gate (scripts/check-parser-combinators.sh) flagged
`.strip_prefix("face down")` as string-method dispatch. Replace with
`tag::<_, _, OracleError<'_>>("face down").parse(...)`; the post-tag
char-boundary check is structural validation (not dispatch) and
remains in pure Rust.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The generic chain processor in `effects::resolve_ability_chain` already
publishes ExileTop's affected objects via `affected_objects_from_events`
(which maps `Effect::ExileTop` to the Exile destination zone). The
internal `publish_tracked_set` call added in b8a7dcf was double-
publishing the same ids, breaking
`compound_zone_change_chain_unifies_tracked_set` (lib_card received
two PlayFromExile grants instead of one).

- Remove the internal publish + local id buffer in `exile_top::resolve`;
  mirrors `change_zone::resolve`, which already delegates publishing to
  the chain layer.
- Adapt
  `exile_top_publishes_tracked_set_when_followed_by_recall_delayed_trigger`
  to drive through `effects::resolve_ability_chain` so the chain
  processor's publish runs (the leaf resolver no longer publishes).
- Resolves the MED `im::Vector` review concern by eliminating the
  local collection entirely — there's nothing left to back with a
  persistent container.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@matthewevans
matthewevans enabled auto-merge (squash) May 21, 2026 16:19
@matthewevans
matthewevans disabled auto-merge May 21, 2026 16:44
@matthewevans
matthewevans merged commit f17dab1 into phase-rs:main May 21, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-contribution PR opened via docs/AI-CONTRIBUTOR.md flow needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants