refactor(parser): convert the remaining continuation scans to arena bindings (U6-C2b)#5582
Conversation
…indings (U6-C2b) Completes the C2b+C3 merge. The plumbing and the DigFromAmong anchor/range landed in 09a8481db3 (C3); this converts the rest. ONE positional scan now remains in the whole assembly path — `CopyRetarget` — and it is structurally deferred (see below). Converted (all top-level shape lookups), with full-pool dual-run and per-site non-vacuity fire counts: | site | role | binds (35k) | divergences | fires when forced | |------|------|-------------|-------------|-------------------| | RestDestination | LastWithRole(DigOrRevealUntil) | 283 | 0 | 283/283 | | CantRegenerate | LastWithRole(DestroyLike) | 135 | 0 | 133/135 | | FaceDownProfile | LastWithRole(FaceDownProfileHolder)| 8 | 0 | 6/8 | `DigOrRevealUntil` is deliberately a DIFFERENT set from C3's `DigOrMill`. The old `last_dig` registry was a `Dig|Mill|RevealUntil` union, but the two consumers want different sets — binding `RestDestination` to the union would have mis-bound on any chain containing a `Mill`. Roles name a set, not a vibe. `FaceDownProfileHolder` registers only nodes that ALREADY carry a profile: "already has one" is the binding condition itself (a node without one is not this antecedent at all), so it belongs in the registry, not in a guard. A FIFTH scan, not in the original audit: `FaceDownProfileSpec` (CR 708.2a). My audit said "four continuation scans"; there were five. Converted rather than shipping a completeness claim that wasn't true. DEFERRED to C5 — `CopyRetarget` (**186 binds**, well covered, not rare): `set_copy_retarget_in_ability` RECURSES into `sub_ability` and its "predicate" IS the mutation (it patches as it walks). That is the same recursive-first-vacant-slot class as attach_alt_cost / attach_mana_retention / find_prev_play_from_exile_permission, which a TOP-LEVEL arena structurally cannot serve. It needs the node-granularity work, and converting it on a top-level registry would silently mis-bind. Verification: full 35,396-card pool (policy, not the fixture). 16111 lib tests pass, 79 snapshots zero drift, zero .snap.new, clippy -D warnings clean, combinator gate clean. Legacy scans deleted only after each site was individually proven clean.
![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 refactors antecedent resolution in the parser by introducing dedicated roles (DigOrRevealUntil, DestroyLike, and FaceDownProfileHolder) and tracking their corresponding nodes in AssemblyEnv. This replaces manual backward-walk searches in sequence.rs with structured resolution via env.resolve. The review feedback highlights multiple violations of Repository Style Guide R6, noting that several of these new rules-touching match arms and resolution blocks lack mandatory CR annotations.
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.
| if matches!( | ||
| &*def.effect, | ||
| Effect::Destroy { .. } | Effect::DestroyAll { .. } | ||
| ) { | ||
| self.destroy_like_nodes.push(index); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotation for DestroyLike matching. Evidence:
crates/engine/src/parser/oracle_effect/assembly.rs:564-569. Why it matters: Every rules-touching line of engine code must carry a comment of the formCR <number>: <description>per Repository Style Guide R6. Suggested fix: Add a CR 701.8 comment describing the destruction matching.
// CR 701.8: To destroy a permanent is to move it from the battlefield to its owner's graveyard.
if matches!(
&*def.effect,
Effect::Destroy { .. } | Effect::DestroyAll { .. }
) {
self.destroy_like_nodes.push(index);
}References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
| if matches!( | ||
| &*def.effect, | ||
| Effect::ChangeZoneAll { | ||
| face_down_profile: Some(_), | ||
| library_position: None, | ||
| random_order: false, | ||
| .. | ||
| } | Effect::ChangeZone { | ||
| face_down_profile: Some(_), | ||
| .. | ||
| } | Effect::Manifest { | ||
| profile: Some(_), | ||
| .. | ||
| } | Effect::TurnFaceDown { | ||
| profile: Some(_), | ||
| .. | ||
| } | ||
| ) { | ||
| self.face_down_profile_nodes.push(index); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotation for FaceDownProfileHolder matching. Evidence:
crates/engine/src/parser/oracle_effect/assembly.rs:570-589. Why it matters: Every rules-touching line of engine code must carry a comment of the formCR <number>: <description>per Repository Style Guide R6. Suggested fix: Add a CR 708.2a comment describing the face-down profile matching.
// CR 708.2a: At the time a card becomes face down, its controller secretly chooses a creature type.
if matches!(
&*def.effect,
Effect::ChangeZoneAll {
face_down_profile: Some(_),
library_position: None,
random_order: false,
..
} | Effect::ChangeZone {
face_down_profile: Some(_),
..
} | Effect::Manifest {
profile: Some(_),
..
} | Effect::TurnFaceDown {
profile: Some(_),
..
}
) {
self.face_down_profile_nodes.push(index);
}References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
| let bound = env.resolve( | ||
| defs, | ||
| super::assembly::AntecedentSelector::LastWithRole( | ||
| super::assembly::AntecedentRole::DestroyLike, | ||
| ), | ||
| None, | ||
| super::assembly::OnMiss::Ignore, | ||
| ); |
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotation for DestroyLike resolution. Evidence:
crates/engine/src/parser/oracle_effect/sequence.rs:3553-3560. Why it matters: Every rules-touching line of engine code must carry a comment of the formCR <number>: <description>per Repository Style Guide R6. Suggested fix: Add a CR 701.19c comment describing the regeneration restriction.
// CR 701.19c: A regeneration shield is a replacement effect that can be applied when a permanent would be destroyed.
let bound = env.resolve(
defs,
super::assembly::AntecedentSelector::LastWithRole(
super::assembly::AntecedentRole::DestroyLike,
),
None,
super::assembly::OnMiss::Ignore,
);References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
| let bound = env.resolve( | ||
| defs, | ||
| super::assembly::AntecedentSelector::LastWithRole( | ||
| super::assembly::AntecedentRole::DigOrRevealUntil, | ||
| ), | ||
| None, | ||
| super::assembly::OnMiss::Ignore, | ||
| ); |
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotation for DigOrRevealUntil resolution. Evidence:
crates/engine/src/parser/oracle_effect/sequence.rs:3606-3613. Why it matters: Every rules-touching line of engine code must carry a comment of the formCR <number>: <description>per Repository Style Guide R6. Suggested fix: Add a CR 701.20 comment describing the card-reveal/dig destination resolution.
// CR 701.20: To reveal a card, its controller shows it to all players.
let bound = env.resolve(
defs,
super::assembly::AntecedentSelector::LastWithRole(
super::assembly::AntecedentRole::DigOrRevealUntil,
),
None,
super::assembly::OnMiss::Ignore,
);References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
| let bound = env.resolve( | ||
| defs, | ||
| super::assembly::AntecedentSelector::LastWithRole( | ||
| super::assembly::AntecedentRole::FaceDownProfileHolder, | ||
| ), | ||
| None, | ||
| super::assembly::OnMiss::Ignore, | ||
| ); |
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotation for FaceDownProfileHolder resolution. Evidence:
crates/engine/src/parser/oracle_effect/sequence.rs:3929-3936. Why it matters: Every rules-touching line of engine code must carry a comment of the formCR <number>: <description>per Repository Style Guide R6. Suggested fix: Add a CR 708.2a comment describing the face-down profile resolution.
// CR 708.2a: At the time a card becomes face down, its controller secretly chooses a creature type.
let bound = env.resolve(
defs,
super::assembly::AntecedentSelector::LastWithRole(
super::assembly::AntecedentRole::FaceDownProfileHolder,
),
None,
super::assembly::OnMiss::Ignore,
);References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
Parse changes introduced by this PR✓ No card-parse changes detected. |
1 participant
Continues the U6 assembly-arena work (A → B1 → C1 → C2 → C3). Converts the remaining top-level positional backward scans in
apply_clause_continuationinto explicit arena antecedent bindings.Converted
RestDestinationLastWithRole(DigOrRevealUntil)CantRegenerateLastWithRole(DestroyLike)FaceDownProfileSpecLastWithRole(FaceDownProfileHolder)DigOrRevealUntilis deliberately a different set from C3'sDigOrMill. The oldlast_digregistry was aDig|Mill|RevealUntilunion, but its two consumers want different subsets — bindingRestDestinationto the union would have silently mis-bound on any chain containing aMill. A role names a set, not a vibe.FaceDownProfileHolderregisters only nodes that already carry a profile. "Already has one" is the binding condition itself — a node without a profile is not this antecedent at all — so it belongs in the registry rather than in a guard at the use site.A fifth scan turned up that the original 69-site audit missed (
FaceDownProfileSpec, CR 708.2a). Converted, rather than shipping a four-of-four completeness claim that wasn't true.Deliberately deferred to C5 —
CopyRetarget(186 binds; well-covered, not rare)set_copy_retarget_in_abilityrecurses intosub_abilityand mutates as it walks — its "predicate" is the patch. That is the same recursive first-vacant-slot class asattach_alt_cost_to_prior_cast_from_zone,attach_mana_retention_to_prior_mana, andfind_prev_play_from_exile_permission_mut, which a top-level arena structurally cannot serve. It needs the node-granularity change; binding it to a top-level registry now would silently mis-bind. It is the one positional scan left in the assembly path, and it is left on purpose.Verification
Dual-run old-vs-new over the full
data/card-data.json(35,396 cards), not the 2,410-face fixture — every binding here is rare enough that the fixture is systematically blind to it. Per-site fire counts taken with each selector perturbed independently (a chained perturbation makes a downstream site report a phantom 0 because it is never reached). Legacy scans deleted only after each site was individually proven clean.16,111 lib tests pass · 79 snapshots, zero drift, zero
.snap.new·clippy -D warningsclean · combinator gate clean.