Skip to content

Implement Enlist keyword (CR 702.154) - #2535

Merged
matthewevans merged 2 commits into
phase-rs:mainfrom
philluiz2323:feat/enlist-keyword
Jun 6, 2026
Merged

Implement Enlist keyword (CR 702.154)#2535
matthewevans merged 2 commits into
phase-rs:mainfrom
philluiz2323:feat/enlist-keyword

Conversation

@philluiz2323

Copy link
Copy Markdown
Contributor

Summary

Implements the Enlist keyword (CR 702.154), closing #2534. Keyword::Enlist was parsed but inert. It is synthesized as an optional Attacks trigger (the Provoke shape): the optional body taps an untapped creature you control; a reflexive sub-ability pumps the attacker (SelfRef) by that creature's power until end of turn (+X/+0).

CR

CR 702.154a (verified against docs/MagicCompRules.txt:5114): "As this creature attacks, you may tap up to one untapped creature you control … When you do, this creature gets +X/+0 until end of turn, where X is the tapped creature's power."

How X is read (the one cross-cutting piece)

The pump must affect the attacker while reading a different creature's power. X is read anaphorically: the just-tapped creature is the resolution's "that creature" referent (CR 608.2c), reached via Effect::Pump's PtValue::Quantity(QuantityRef::Power { scope: Anaphoric }).

To make that resolve, parent_referent_context_from_events is extended to capture a single tapped creature (from a PermanentTapped event, snapshot live) as the anaphoric referent — tried last, after the existing sacrifice/move/reveal referents, so it only fills the "that creature" slot when nothing else did. Per CR 608.2c a later instruction may refer to a creature an earlier instruction tapped, so this is a correct generalization, not Enlist-specific plumbing.

Reuses TriggerMode::Attacks + Effect::Tap + Effect::Pump with PtValue::Quantity + QuantityRef::Power. No new Effect or subsystem.

Changes

  • database/synthesis.rs: build_enlist_trigger, is_enlist_trigger, the triggers_for/matcher arms, synthesize_enlist, and the synthesize_all wiring (beside Provoke/Melee).
  • game/effects/mod.rs: tapped_object_context_from_events + its hook in parent_referent_context_from_events.

Eligibility note (honest scope)

The tap filter is "another untapped creature you control." CR 702.154a's "didn't choose to attack with" is largely covered (attackers tap unless they have vigilance), but the "has haste or has been under your control since the turn began" (summoning-sickness) refinement has no FilterProp yet, so it is left as a follow-up tightening rather than blocking the core mechanic. Flagging for reviewer visibility.

Tests

  • Synthesis shape: optional Attacks trigger; body taps an untapped you-control creature; reflexive Pump of SelfRef by Power{Anaphoric} with +0 toughness; idempotency; no-op without keyword; triggers_for/matcher roundtrip.
  • Referent: a single tapped creature is captured as the anaphoric referent (carrying its power); multiple tapped creatures yield no singular referent.

Verification

  • cargo fmt --all — clean.
  • cargo clippy -p engine --all-targets --features proptest -- -D warnings — clean.
  • scripts/check-parser-combinators.sh — passes.
  • Unit tests run in CI (local toolchain has a binutils/dlltool gap). The end-to-end resolution (attack → tap → anaphoric pump) is exercised by CI; the referent and synthesis-shape tests pin the pieces.

Closes #2534

Keyword::Enlist was parsed but inert. Synthesize it as an optional Attacks
trigger (the Provoke shape): the optional body taps an untapped creature you
control; a reflexive sub-ability pumps the attacker (SelfRef) by that
creature's power until end of turn (+X/+0).

X is read anaphorically: the just-tapped creature is the resolution's 'that
creature' referent (CR 608.2c), reached via Effect::Pump's
PtValue::Quantity(QuantityRef::Power { scope: Anaphoric }). To make that work,
extend parent_referent_context_from_events to capture a single tapped creature
(a PermanentTapped event, snapshot live) as the anaphoric referent — tried
last, after the existing sacrifice/move/reveal referents, so it only fills the
'that creature' slot when nothing else did. Per CR 608.2c a later instruction
may refer to a creature an earlier instruction tapped, so this is a correct
generalization, not Enlist-specific plumbing.

Reuses TriggerMode::Attacks + Effect::Tap + Effect::Pump with
PtValue::Quantity + QuantityRef::Power. No new Effect or subsystem.

Eligibility note: the tap filter is 'another untapped creature you control'.
CR 702.154a's 'didn't choose to attack with' is largely covered (attackers
tap unless vigilant); the 'haste or controlled since the turn began'
(summoning-sickness) refinement has no FilterProp yet and is left as a
follow-up tightening rather than blocking the core mechanic.

Tests: synthesis-shape (optional Attacks trigger; Tap over untapped
you-control creature; reflexive Pump of SelfRef by Power{Anaphoric}; +0
toughness), idempotency, no-op, triggers_for/matcher roundtrip; plus referent
unit tests (a single tapped creature is captured; multiple are not).

Closes phase-rs#2534

@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 implements the Enlist keyword mechanic (CR 702.154a) by synthesizing an optional attacks trigger that taps an untapped creature and pumps the attacker, and captures the tapped creature as an anaphoric referent (CR 608.2c). Feedback highlights two issues: first, the target filter incorrectly allows tapping the enlisting creature itself or other attacking creatures with vigilance; second, duplicate events for the same permanent can cause the referent capture to fail, which can be resolved by deduplicating the tapped object IDs.

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.

Comment thread crates/engine/src/database/synthesis.rs Outdated
Comment on lines +4343 to +4347
let tap_target = TargetFilter::Typed(
TypedFilter::creature()
.controller(ControllerRef::You)
.properties(vec![FilterProp::Untapped]),
);

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.

high

[HIGH] Target filter allows tapping the enlisting creature itself or other attacking creatures with vigilance.

Evidence: crates/engine/src/database/synthesis.rs:4343-4347.

Why it matters: CR 702.154a specifies that you may tap up to one untapped creature you control that you didn't choose to attack with. The current filter only checks for FilterProp::Untapped, which incorrectly allows tapping the enlisting creature itself (if it has vigilance) or other attacking creatures with vigilance.

Suggested fix: Exclude the enlisting creature and other attacking creatures from the target filter (e.g., by using FilterProp::Another and/or checking for non-attacking status if those properties exist).

References
  1. Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. Convenience shortcuts that get rules wrong are not simpler; they are wrong. (link)

Comment on lines +801 to 819
fn tapped_object_context_from_events(
state: &GameState,
events: &[GameEvent],
) -> Option<CostPaidObjectSnapshot> {
let mut tapped = events.iter().filter_map(|event| match event {
GameEvent::PermanentTapped { object_id, .. } => {
state
.objects
.get(object_id)
.map(|obj| CostPaidObjectSnapshot {
object_id: *object_id,
lki: obj.snapshot_for_mana_spent(),
})
}
_ => None,
});
let first = tapped.next()?;
tapped.next().is_none().then_some(first)
}

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.

medium

[MED] Multiple events for the same permanent can cause the referent capture to fail.

Evidence: crates/engine/src/game/effects/mod.rs:801-819.

Why it matters: If a single permanent is tapped but generates multiple PermanentTapped events (e.g., due to replacement effects or duplicate event dispatch), tapped.next().is_none() will return false and fail to capture the referent.

Suggested fix: Deduplicate the tapped object IDs or verify that all tapped events refer to the same object_id before returning the snapshot.

Suggested change
fn tapped_object_context_from_events(
state: &GameState,
events: &[GameEvent],
) -> Option<CostPaidObjectSnapshot> {
let mut tapped = events.iter().filter_map(|event| match event {
GameEvent::PermanentTapped { object_id, .. } => {
state
.objects
.get(object_id)
.map(|obj| CostPaidObjectSnapshot {
object_id: *object_id,
lki: obj.snapshot_for_mana_spent(),
})
}
_ => None,
});
let first = tapped.next()?;
tapped.next().is_none().then_some(first)
}
fn tapped_object_context_from_events(
state: &GameState,
events: &[GameEvent],
) -> Option<CostPaidObjectSnapshot> {
let mut tapped_ids = events.iter().filter_map(|event| match event {
GameEvent::PermanentTapped { object_id, .. } => Some(*object_id),
_ => None,
});
let first_id = tapped_ids.next()?;
for id in tapped_ids {
if id != first_id {
return None;
}
}
state.objects.get(&first_id).map(|obj| CostPaidObjectSnapshot {
object_id: first_id,
lki: obj.snapshot_for_mana_spent(),
})
}
References
  1. Edge cases: Simultaneous events, multi-target/modal interactions, and duplicate event handling should be robustly covered. (link)

@matthewevans matthewevans added the needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) label Jun 6, 2026
@mike-theDude

Copy link
Copy Markdown
Collaborator

Architecture Review

Implementation review (engine, keyword). Findings only.

Nice reuse — Enlist is synthesized as the optional Attacks (Provoke) shape with Effect::Tap + a reflexive Effect::Pump { SelfRef, +Power{Anaphoric}/+0 }, no new Effect. The anaphoric-X mechanism is the clever part and is correctly modeled. Two findings: one on eligibility (rules fidelity), one on the cross-cutting referent change.


[MED] The enlist tap filter is over-permissive vs CR 702.154a — it allows two illegal targets.

CR 702.154a: "you may tap up to one untapped creature you control that you didn't choose to attack with and that either has haste or has been under your control continuously since this turn began." The PR's filter is just "another untapped creature you control," missing both qualifiers:

  1. Summoning sickness (author-disclosed): without the "has haste or under your control since turn began" check, a freshly-cast (summoning-sick) creature can be enlisted, which is illegal. There's no FilterProp for this today, so it needs one (e.g. HasHasteOrControlledSinceTurnBegan, reading the existing summoning_sick/control-timestamp state).
  2. Vigilant attackers (consequence of using "untapped" as a proxy): the PR notes "attackers tap unless they have vigilance" — but that's exactly the hole. A vigilance creature that is attacking stays untapped, so the "untapped" filter would let you enlist a creature you chose to attack with, violating "that you didn't choose to attack with." The correct predicate is "not among this combat's attackers," independent of tap state.

Both are real CR deviations (the #1 hard rule is rules-correctness). The first is honestly flagged; the second is implied by the same shortcut. Suggested fix: add the not-summoning-sick FilterProp and exclude declared attackers from the tap filter rather than relying on tapped-ness.


[LOW] The anaphoric tapped-referent is a shared-resolution change — verify no existing single-tap effect regresses.

Extending parent_referent_context_from_events to treat a single PermanentTapped creature as the "that creature" referent is the right seam for Enlist, but it changes anaphora resolution for every effect, not just Enlist. It's well-guarded (tried last, after sacrifice/move/reveal; single-tap only), and "that creature" after a tap is usually the tapped creature — but any existing single-tap effect whose later "that creature"/Power{Anaphoric} was meant to reference a different creature (and previously resolved to None) would now silently bind to the tapped one. The referent unit tests cover the new path but not a regression sweep. Suggested check: grep for resolution chains that emit a single tap and then read an anaphoric object/power ref, and confirm none flip behavior.

🤖 Generated with Claude Code

@matthewevans matthewevans added enhancement New feature or request area:engine Core rules engine mechanic:keyword ai-contribution PR opened via docs/AI-CONTRIBUTOR.md flow and removed needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) labels Jun 6, 2026
@matthewevans

Copy link
Copy Markdown
Member

Pushed a maintainer follow-up commit (bc1d97c) addressing the review findings:

  • tightened Enlist tap eligibility to require another untapped creature you control that is not currently attacking and either has haste or is not summoning-sick, via a reusable FilterProp::HasHasteOrControlledSinceTurnBegan engine predicate
  • kept the Enlist synthesis at the existing optional attack-trigger/tap/reflexive-pump seam, using existing TargetFilter/TypedFilter composition rather than adding a new effect
  • deduplicated duplicate PermanentTapped events before binding the anaphoric tapped-creature referent, so duplicate events for the same permanent still resolve as one singular referent
  • added focused tests for Enlist target-filter synthesis, the new eligibility predicate, duplicate tap referent handling, and enum serialization coverage

Verification: cargo fmt --all, git diff --check, parser combinator gate, Tilt test-engine logs (11,452 passed), and the repository pre-push hook suite passed (cargo fmt --check, clippy, card-data validate, parser tests, phase-ai tests, oracle-gen, coverage regression, frontend lint/type-check). GitHub CI is running on the pushed commit.

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maintainer follow-up addressed the Enlist eligibility and tapped-referent review findings. Architecture check: correct engine synthesis/filter seam, reusable eligibility predicate, no frontend logic, CR annotations verified.

@matthewevans
matthewevans enabled auto-merge June 6, 2026 17:40
@matthewevans
matthewevans added this pull request to the merge queue Jun 6, 2026
Merged via the queue into phase-rs:main with commit 20cacbe Jun 6, 2026
10 checks 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 area:engine Core rules engine enhancement New feature or request mechanic:keyword

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Keyword::Enlist is parsed but inert — no attack-time tap-and-pump (CR 702.154)

3 participants