fix(parser): UNSUPPORTED one-off: Last Night Together#4562
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements support for restricted additional combat phases (such as for "Last Night Together" and "Bumi") by introducing an attacker_restriction field to scheduled extra phases and validating declared attackers against this filter. It also adds parsing support for the "each of them" plural-pronoun anaphor in counter placement effects. The reviewer feedback highlights a sibling coverage gap, suggesting that the "each of them" parsing logic should be moved to a central target parser to support other effect types. Additionally, the reviewer notes that using a hardcoded ObjectId(0) as the source ID in the combat attacker restriction check could break source-relative filters, recommending storing and passing the actual source object ID instead.
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.
| // CR 608.2c: "each of them" is the plural-pronoun anaphor to the parent | ||
| // ability's chosen targets (Last Night Together: "Put two +1/+1 counters on | ||
| // each of them"). `parse_target` maps this bare pronoun to an EMPTY `Typed` | ||
| // filter, which matches every battlefield permanent — so the placement would | ||
| // land counters on ALL permanents instead of the two chosen creatures. The | ||
| // singular "it"/"that creature" forms are handled above and the demonstrative | ||
| // "those <type>" form keeps its tracked-set mapping in `parse_target`; the | ||
| // "each of them" pronoun is the remaining gap. Gate strictly on that surface | ||
| // (a trailing word boundary excludes "...themselves"), then resolve through | ||
| // the shared subject-application anaphora authority (`subject.rs`), which maps | ||
| // it to `ParentTarget`. The resolver binds it to the chosen objects via | ||
| // `ability.targets`. | ||
| if let Some(((), after)) = nom_on_lower(on_rest, on_rest, |i| { | ||
| let (i, ()) = value((), tag("each of them")).parse(i)?; | ||
| let (i, ()) = | ||
| value((), peek(alt((eof, tag(" "), tag("."), tag(","), tag(";"))))).parse(i)?; | ||
| Ok((i, ())) | ||
| }) { | ||
| if let Some(application) = super::subject::parse_subject_application(on_text, ctx) { | ||
| if matches!(application.affected, TargetFilter::ParentTarget) { | ||
| let offset = lower.len() - after.len(); | ||
| return ( | ||
| TargetFilter::ParentTarget, | ||
| &text[offset..], | ||
| application.multi_target, | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[HIGH] Sibling coverage gap for "each of them" plural-pronoun anaphor. Evidence: crates/engine/src/parser/oracle_effect/counter.rs:213-241.
Why it matters: While this special-case interceptor correctly fixes counter placement, "each of them" is a general plural-pronoun anaphor (CR 608.2c) that can appear in other effect types (e.g., "destroy each of them", "exile each of them", "tap each of them"). Handling this only in resolve_counter_placement_target leaves sibling effects vulnerable to the same bug where "each of them" maps to an empty Typed filter and wrongly affects the entire battlefield.
Suggested fix: Move the "each of them" parsing logic and its mapping to ParentTarget into the central target parser (e.g., oracle_target.rs or subject.rs) so that all sibling effect parsers benefit from this correction automatically.
References
- Sibling coverage: If a fix landed in one site of a class, did the siblings need the same fix? Name them in the finding. (link)
| pub fn passes_combat_attacker_restriction(state: &GameState, obj_id: ObjectId) -> bool { | ||
| match &state.current_combat_attacker_restriction { | ||
| None => true, | ||
| // CR 500.10a + CR 508.1c: the restricted extra combat phase is only ever | ||
| // added to the active player's turn (the scheduling spell's controller), | ||
| // so a controller-relative restriction subject ("creatures you control") | ||
| // resolves `ControllerRef::You` against the active player. A bare | ||
| // `FilterContext::neutral()` carries no `source_controller` and would make | ||
| // such a restriction match nothing, wrongly excluding the active player's | ||
| // creatures. The current concrete-set / controller-None `Typed` subjects | ||
| // (Last Night Together's chosen set, Bumi's "land creatures") are | ||
| // unaffected; this only future-proofs controller-scoped subjects. | ||
| Some(filter) => matches_target_filter( | ||
| state, | ||
| obj_id, | ||
| filter, | ||
| &FilterContext::from_source_with_controller(ObjectId(0), state.active_player), | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Hardcoded source ObjectId(0) in combat attacker restriction check. Evidence: crates/engine/src/game/combat.rs:617.
Why it matters: Using a dummy ObjectId(0) as the source ID in FilterContext means any source-relative filters (e.g., checking characteristics of the source card, or SameController / SameColor checks) nested within the restriction will fail to resolve correctly or match nothing. Under CR 611.2c, rules-modifying continuous effects are evaluated continuously, so preserving the actual source object ID is necessary for rules-correctness if future cards in this category use source-relative restrictions.
Suggested fix: Store the source ObjectId alongside the restriction in ExtraPhase and GameState, and pass it to FilterContext::from_source_with_controller instead of ObjectId(0).
Parse changes introduced by this PR · 28 card(s), 32 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Requesting changes for current-head backend blockers.
[HIGH] each of them is fixed only in counter placement instead of the target/anaphor authority. crates/engine/src/parser/oracle_effect/counter.rs:213-241 intercepts each of them inside resolve_counter_placement_target and maps it to ParentTarget. That fixes the counter half of Last Night Together, but each of them is a general CR 608.2c plural-pronoun anaphor, not a counter-specific grammar rule. Other effects with the same phrase shape (destroy/exile/tap/bounce each of them, etc.) can still route through the central target parser and get the old broad/empty target behavior. Please move this into the shared target/anaphor parsing path so all sibling effect parsers get the same binding, then keep the counter test as one consumer regression.
[MED] Scheduled combat restrictions lose source provenance and use ObjectId(0). passes_combat_attacker_restriction evaluates the active restriction with FilterContext::from_source_with_controller(ObjectId(0), state.active_player) (crates/engine/src/game/combat.rs:613-618). The scheduled state stores only Option<TargetFilter>, so any source-relative restriction nested inside this general new field will evaluate against a dummy object rather than the effect source. This is especially risky because the PR presents attacker_restriction as a reusable AdditionalPhase/ExtraPhase building block. Please carry the actual source object/controller with the scheduled restriction and use that provenance when evaluating the filter.
[HIGH] Move "each of them" plural-pronoun anaphor to central target parser.
The "each of them" → ParentTarget binding was only handled inside
`resolve_counter_placement_target` in counter.rs. Other effect parsers
(destroy, exile, bounce, tap, etc.) would still route through the "each"
+ type-phrase arm in oracle_target.rs and produce an all-matching
TargetFilter::Any, causing those effects to apply to every battlefield
permanent instead of the parent ability's chosen targets (CR 608.2c).
Fixed by adding a "each of" + parse_word_bounded("them") arm in
`parse_target_with_syntax` (oracle_target.rs), between the existing
"each of those" and "each of <N> target" arms. Resolution delegates to
`resolve_pronoun_target(ctx, "them")` which applies the same trigger-
subject vs. compound-anaphor dispatch as the bare "them" pronoun. The
special-case block is removed from counter.rs — the general path at
line 277 of resolve_counter_placement_target now handles it via the
central parser. Added two unit tests in oracle_target.rs and kept the
counter regression test (last_night_together_restricted_combat.rs) as
a consumer-level proof.
[MED] Store source ObjectId alongside attacker restriction to avoid dummy.
`passes_combat_attacker_restriction` was using ObjectId(0) as the
FilterContext source, breaking any source-relative restriction predicate
(e.g. "creatures that share a color with this card") on future cards.
Fixed by adding `attacker_restriction_source: Option<ObjectId>` to
ExtraPhase and `current_combat_attacker_restriction_source` to GameState.
additional_phase.rs sets the source to ability.source_id when scheduling
a restricted combat; turns.rs propagates it to the current field and
clears it alongside the restriction at end-of-combat. combat.rs now
reads the stored source via unwrap_or(ObjectId(0)) (the existing concrete-
set/Typed subjects are unaffected; this future-proofs source-relative
subjects per CR 611.2c).
Updated all ExtraPhase literal construction sites (15 in engine src +
6 in test files) to include attacker_restriction_source: None.
Verified: cargo clippy -p engine -p phase-ai -- -D warnings: 0 errors/warnings.
cargo test -p engine: all tests pass (including last_night_together_
restricted_combat, each_of_them_is_parent_target,
each_of_themselves_does_not_match_each_of_them_arm).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Restricted-combat state survives an end-the-turn escape. Evidence: end_turn_to_cleanup clears only extra_phases before jumping to Cleanup at crates/engine/src/game/turns.rs:116-120, while start_next_turn later clears old extra_phases at crates/engine/src/game/turns.rs:698 but never resets current_combat_attacker_restriction or current_combat_attacker_restriction_source. Why it matters: if a restricted extra combat is active and an effect like Time Stop / Obeka ends the turn during that combat, the new active restriction can remain resident into cleanup and the next turn until a natural BeginCombat happens to overwrite it. That stale state can make unrelated attacker queries / AI fallback checks apply the previous combat's restriction outside its CR 511.3 duration. Suggested fix: clear both current restriction fields in the same early-turn-end cleanup path (and preferably at turn start as a defensive reset), then add a regression that starts a restricted combat, invokes the end-the-turn path, and asserts both fields are cleared.
# Conflicts: # crates/engine/src/parser/oracle_trigger.rs
# Conflicts: # crates/engine/src/parser/oracle_trigger_tests.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Restricted-combat state still survives EndTheTurn. Evidence: crates/engine/src/game/turns.rs:117 clears only extra_phases; crates/engine/src/game/turns.rs:701 clears only old extra_phases at turn start; restriction cleanup is only on normal EndCombat at crates/engine/src/game/turns.rs:2221. Why it matters: ending the turn during combat removes creatures from combat and skips to cleanup, so the combat restriction must expire rather than leaking into later turns/AI attacker queries. Suggested fix: clear the restricted-combat state on the end-the-turn path as well, with a regression that ends the turn during the extra combat duration.
Reviewed current head c61eab3a95de3cf8d06b7de5ec2ec2310f0b3329.
2 participants
Summary
Fixes a parser misparse affecting 1 card(s) in the Doctor Who Commander precons.
Root cause: UNSUPPORTED one-off: Last Night Together
Cards corrected
Fix
Implemented cluster 61 (Last Night Together) fully, rules-correct, and generalized the fix to the whole "only X can attack during that combat phase" class (also fixing Bumi, Unleashed). Both cards now parse with 0 Unimplemented.
ROOT CAUSE: the sixth sentence "Only the chosen creatures can attack during that combat phase" parsed to Effect::Unimplemented{name:"only"}. The deliverable was to fold it into the scheduled additional combat and enforce it.
DESIGN (no new Effect/TargetFilter variant — leaf-level field additions only, per the parameterize-don't-proliferate gate):
VERIFICATION (worktree not under Tilt — ran cargo directly): cargo fmt clean; cargo clippy -p engine -p phase-ai --all-targets -D warnings CLEAN; cargo test -p engine = 16044 passed / 0 failed across 120 suites; cargo test -p phase-ai = 1107+ passed / 0 failed; oracle_trigger module 826 passed (incl. new Bumi test); gen-card-data confirms LNT 0 Unimplemented with attacker_restriction=ParentTarget and Bumi 0 Unimplemented with attacker_restriction=Typed([Land,Creature]). RUNTIME PROOF: new cast-pipeline test (last_night_together_restricted_combat.rs) casts LNT choosing 2 of 3 controlled creatures, advances into the scheduled combat, and asserts get_valid_attacker_ids == the 2 chosen (3rd excluded), validate_attackers([unchosen]) errs, validate_attackers([chosen]) ok, and the restriction clears after combat — also proving parent->sub-ability target propagation reaches the AdditionalPhase resolver.
PARSER DIFF GATE: my added parser lines contain zero string-dispatch (only the typed Vec::contains enum-membership assertion inside a #[test]). The repo-wide check-parser-combinators.sh exits 1, but every flagged line is a pre-existing committed line in files I did not touch (it diffs the whole tree vs an old base ff799f8); my added lines do not appear.
CR ANNOTATIONS (each grepped/verified against docs/MagicCompRules.txt in this worktree before writing): CR 115.1, CR 601.2c, CR 608.2c (anaphor "the chosen creatures" ARE the spell's chosen targets, resolved by applying the rules of English); CR 608.2h (affected set determined once, at resolution -> snapshot to fixed tracked set); CR 611.2c (rules-modifying continuous effect, re-evaluated per declaration — covers Bumi's Typed subject); CR 508.1c (active player checks attacker restrictions); CR 511.3 (combat phase over -> restriction ends); CR 500.8 (extra phases); CR 506.1 (combat phase); CR 500.10a (additional phase only on controller's turn). The plan's prior CR 603.7 misattribution was NOT propagated.
NOTE ON DATA ARTIFACTS: gen-card-data also regenerated crates/engine/data/known-tokens.toml and crates/engine/data/oracle-subtypes.json (plus gitignored client/public/card-data.json). These are pipeline byproducts, not hand edits — the orchestrator should decide whether to stage them. No commit was made.
Files changed
CR references
Verification
cargo fmt --all— pass./scripts/check-parser-combinators.sh <upstream-merge-base>— passcargo clippy -p engine --all-targets -- -D warnings— passcargo test -p engine— passoracle-gen confirm: last night together— passCards confirmed re-parsed correctly: Last Night Together
🤖 Generated with Claude Code