Fix Culvert Ambusher - #6831
Conversation
…in disjunctive splits
Fix Culvert Ambusher, and its class.
"When this creature enters or is turned face up, target creature blocks this turn
if able." parsed to a single `TriggerMode::ChangesZone`. The "or is turned face up"
branch was silently discarded with ZERO `Effect::Unimplemented`, so the card
reported as fully supported while never triggering on its disguise flip — a
silent-wrong-behavior misparse (docs/parser-misparse-backlog.md root cause 6,
"Disjunctive (or-list) collapsed to first branch"), not a coverage gap.
The seam is the event-head lexicon that `split_or_event_compound` consults to decide
whether " or " joins two events. It knew active-voice/transitive verbs plus a few
passive and copular heads, but had no intransitive state-change family — so every
`<event> or becomes/is <state>` pair lost its second branch.
Adds one scope-limited sibling combinator with a CLOSED allow-list of four
complements, each with a probe-verified standalone mode, so both halves of a split
land on fully supported ground:
becomes/become monstrous -> TriggerMode::BecomeMonstrous (CR 701.37a-b)
becomes/become tapped -> TriggerMode::Taps (CR 701.26a + CR 603.2e)
is/are turned face up -> TriggerMode::TurnFaceUp (CR 116.2b + CR 708.7)
is/are dealt damage -> TriggerMode::DamageReceived (CR 120.1)
CR 603.2e is the authorizing rule: it names "becomes" as a trigger-event word and
draws the event-vs-state line the allow-list encodes. Two productions, one per
voice — not a head x complement product, because the voices have disjoint
complements ("is monstrous" and "is tapped" are states; "is tapped for mana" is a
different event, CR 106.12a).
ZERO new engine surface. All four modes already exist, are in the matcher registry,
are indexed, and have live event producers. `types/` is untouched.
The lexicon has NINE consumers across five functions. The new heads are composed at
exactly TWO — the or-split gate and `extract_subject_text` — and the other seven stay
narrow, each with a named counterexample in place. `extract_subject_text` MUST be
widened: Cryoshatter ("becomes tapped or is dealt damage") has a new-family head in
its FIRST half, and a narrow terminator reconstructs "When enchanted creature becomes
tapped is dealt damage". Widening the cross-subject gate instead would split subject
disjunctions as event disjunctions (Donna Noble, The Bus Runner) — pinned by test.
`parse_event_verb_start` is left byte-identical.
Also fixed, as a consequence: a serial list whose LEADING leg is state-change used to
reconstruct duplicate-mode garbage — "Whenever ~ becomes tapped, attacks, or dies"
gave [Taps, Taps, Taps] with descriptions like "becomes tapped attacks", a CR 603.2c
triple-fire. It now gives [Taps, Attacks, ChangesZone]. Claimed and pinned rather
than shipped unclaimed. Trailing/middle-leg legs remain honestly `Unimplemented`.
Reconstruction runs on `TextPair` rather than slicing the original-case string with
offsets taken from the lowercase one, so the second half keeps its subject's casing
("Whenever a Detective you control is turned face up") instead of lowercasing display
text that flows to card-data `parse_details`.
Impact, measured by whole-corpus differential in both directions: exactly 14 cards
change and nothing else — Culvert Ambusher, Case of the Pilfered Proof, Concert
Kaboomist, Efreet Weaponmaster, Gadget Technician, Offender at Large, Ponyback
Brigade, Rakish Scoundrel, Alpha Deathclaw, Protector of the Wastes, Champions of the
Shoal, Cryoshatter, A-Radha, A-Zar Ojanen. Concert Kaboomist keeps its pre-existing
`where_x_binding` gap on both halves — honestly still red.
Coverage honesty: no line is newly ACCEPTED. All 14 were already accepted with half
of each silently dropped; this recovers the dropped half. Recorded as an owed
refactor at the allow-list: it is a DETECTION list, so an unadmitted complement still
makes a second branch vanish with no marker; the stronger long-term seam is to detect
the open `becomes|is|are <participle>` shape and route unadmitted complements to
`Effect::unimplemented`.
Tests: 15 parser rows + 4 runtime rows. The runtime rows drive the real pipeline
(`PlayFaceDown` -> `TurnFaceUp` -> trigger target choice -> `MustBlock`) and red at
their reach-guards when the production change is reverted. Revert-to-red measured per
seam over the full suite: or-split gate alone -> 7 lib + 2 integration; subject
terminator alone -> 3; subject recase alone -> 1; admitting `blocked` -> 3.
`entering_tapped_does_not_fire_the_becomes_tapped_arm` pins CR 603.2e and was proven
non-vacuous by injecting `PermanentTapped` at the ETB tap site.
Backlog hygiene: 7 cards removed from root cause 6 (239), totals 4732->4725 and
4766->4759, and the ranked table's stale 247 corrected to match.
Verification: cargo fmt clean, clippy-strict 0 warnings, 18015 lib + 4214 integration
passing, Gate A/G PASS, Gate P PASS.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe parser now recognizes selected state-change and passive event heads in disjunctive trigger compounds. Tests cover supported and unsupported forms, exclusions, subject extraction, serial reconstruction, replacement effects, and runtime behavior. ChangesTrigger parsing and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant OracleText
participant OracleTriggerParser
participant TriggerCondition
participant GameState
OracleText->>OracleTriggerParser: parse disjunctive trigger text
OracleTriggerParser->>TriggerCondition: match supported or unsupported event heads
TriggerCondition-->>OracleTriggerParser: return trigger arms and coverage gaps
OracleTriggerParser->>GameState: register parsed trigger conditions
GameState-->>OracleTriggerParser: execute applicable trigger arm
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
needs-maintainer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/parser/oracle_trigger_tests.rs`:
- Around line 21904-21918: Add positive reach-guard assertions to the Donna
Noble case in cross_subject_state_change_or_not_split: after confirming one
trigger, assert TriggerMode::DamageReceived and validate that the trigger’s
subject filter has the expected Or shape. Preserve the existing Bus Runner
assertions and ensure the Donna Noble checks prove the input reached
split_cross_subject_event_compound rather than merely producing one trigger.
In `@crates/engine/src/parser/oracle_trigger.rs`:
- Around line 7960-7968: Make the silent-vanish refactor discoverable outside
the doc comment near parse_event_head_start by adding either a concrete issue
reference or an ignored test documenting that unadmitted complements, such as
“enters or is turned face down,” must produce Effect::Unimplemented. If adding a
test, mark it #[ignore] and capture the expected open-head-shape behavior for
becomes|is|are <participle> without changing current parsing behavior.
- Around line 7800-7805: Update the call to append_shared_object_if_bare_event
in the surrounding parser logic to pass the trimmed second_half.lower value,
ensuring its offset aligns with the trimmed second_event and object extraction
remains correct.
In
`@crates/engine/tests/integration/culvert_ambusher_turn_face_up_force_block.rs`:
- Around line 24-44: Update must_block_targets so a MustBlock effect with any
affected TargetFilter other than SpecificObject causes an immediate panic
instead of being discarded. Preserve returning the object ID for SpecificObject
variants and keep the existing filtering of effects without MustBlock
modifications.
In `@docs/parser-misparse-backlog.md`:
- Around line 6-7: Reconcile the counts in docs/parser-misparse-backlog.md
before merging: recalculate the table totals, list-entry totals, and heading
counts from the underlying entries, then update the distinct-card and
total-appearance figures and all inconsistent root-cause counts, including root
causes 1, 2, 3, 5, 19, 22, 27, 28, 30, and 31. Preserve root cause 6 at 239 if
the recalculation confirms it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 54045478-e22b-4c51-bce8-083927ab1e62
📒 Files selected for processing (5)
crates/engine/src/parser/oracle_trigger.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/tests/integration/culvert_ambusher_turn_face_up_force_block.rscrates/engine/tests/integration/main.rsdocs/parser-misparse-backlog.md
|
Generated for head Parse changes introduced by this PR · 17 card(s), 20 signature(s) (baseline: main
|
…ge head family Four of the five findings on PR phase-rs#6831, plus the reasoning for declining the fifth. 1. Cross-subject reach-guard. `cross_subject_state_change_or_not_split`'s Donna Noble leg asserted only `len() == 1`, which a line that never reached the cross-subject gate would also satisfy. CodeRabbit proposed asserting `TriggerMode::DamageReceived` — that would FAIL: Donna Noble is honestly `Unknown` today (probe-verified). The real discriminator is the `Unknown` PAYLOAD, which must still carry the whole cross-subject condition including "is dealt damage"; widening that gate splits the line and truncates the payload to the bare subject. Asserted that instead. 2. Shared-object offset alignment. `extract_shared_object` derives the object offset as `original.len() - rest_lower.len()`, which is only meaningful when both views are trimmed identically — and it was handed a TRIMMED original alongside an UNTRIMMED lower. Pre-existing: the base commit passed the untrimmed lower too. Probed as NOT reachable in production (the condition arrives already trimmed from `find_effect_boundary`, and both trailing-whitespace variants of the Mirkwood Bats-class shape parse correctly today), so this is defensive — but it makes the invariant hold BY CONSTRUCTION here instead of depending on an upstream caller. 3. The owed refactor is now an executable specification, not just a doc comment: `unadmitted_state_change_head_should_be_a_strict_failure`, `#[ignore]`d. The allow-list is a DETECTION list, so an unadmitted head makes the second branch vanish with no `Effect::Unimplemented` — the same silent-wrong signature as the bug this family fixes. Confirmed the test genuinely reds when run (`got ["ChangesZone"]`), so it captures a real gap rather than documenting a vacuous one. Left ignored because it asserts behavior the engine does not have yet and zero printed cards need it. 4. `must_block_targets` now PANICS on an unexpected `affected` filter instead of silently discarding it. Discarding made the negative rows vacuous in the one case that matters: a MustBlock that really applied, to a filter the helper does not understand, read as "no MustBlock at all". Declined, with reasons, in the PR reply: the global backlog recount across root causes 1/2/3/5/19/22/27/28/30/31. That drift is pre-existing and a whole-file recount would collide with other agents' concurrent list edits (CLAUDE.md multi-agent safety), which is why this change decrements only what it touches. Root cause 6 is confirmed at 239 — header, list length, and ranked-table row now all agree, repairing a pre-existing 247-vs-246 skew. Verification: cargo fmt clean, clippy-strict 0 warnings, 18015 lib + 4214 integration passing (ignored 6 -> 7 for the new spec test). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CodeRabbit round addressed in
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocked — the new closed event-head splitter still lets an unadmitted state-change branch vanish while the card remains falsely supported.
🔴 Blocker
[MED] Unadmitted becomes|is|are <participle> complements are silently dropped instead of preserving honest unsupported coverage. Evidence: crates/engine/src/parser/oracle_trigger.rs:7952-7968 explicitly documents that the closed allow-list drops every unadmitted complement; split_or_event_compound only splits admitted heads at :7782-7819, after which parse_trigger_lines_at_index_ir falls through to one trigger at :838-865. The current ignored regression at crates/engine/src/parser/oracle_trigger_tests.rs:21904-21943 demonstrates the failure: "enters or is turned face down"yields one trigger with zeroEffect::Unimplemented, so the card appears supported. Why it matters: parser coverage goes green while a rules-bearing branch was lost. Suggested fix: detect the open state-change head shape and route unadmitted complements through the existing strict-failure / Effect::unimplemented` authority, rather than letting the closed support list control whether the complement exists.
🟡 Non-blocking
The current CodeRabbit findings that were resolved or are stale are acknowledged and are not blockers here; the remaining coverage-honesty gap is independent.
CI status
The parse-diff artifact is from the prior head (04:27) while this head was pushed at 04:42, and Rust/card-data checks are still running. No approval or enqueue will occur until a current artifact and required checks complete; this formal blocker is independently about coverage honesty.
Recommendation: request changes — implement open-head detection with strict failure for unadmitted complements, add an active (non-ignored) coverage-honesty regression, then request a new current-head review.
|
Accepted — you're right, and I want to be straight about it: this is the gap I knew about and chose to defer. CodeRabbit raised it and my own review rounds raised it; I recorded it in-code as an "OWED REFACTOR" with an Two corrections to my earlier comment on this PR, before anything else:
On the fix — my investigation suggests it is smaller and better than a bolted-on strict-failure pathThree things I measured before starting, because they change the shape of the answer:
So rather than adding a parallel strict-failure route, the open head can simply split and let the real single-event parser be the detector per branch: an admitted complement reaches its real mode, an unadmitted one lands on Two things that must survive the change, and I'm treating them as the risk surface:
The Re-planning through |
Addresses the review blocker on PR phase-rs#6831: the closed complement allow-list was a DETECTION list, so an unadmitted `becomes|is|are <participle>` made the second branch VANISH while the card still reported as supported — parser coverage green with a rules-bearing branch lost, the same silent-wrong signature the head family was written to remove. Measured: "enters or is turned face down" gave ONE trigger. The fix is subtraction. The support list is DELETED — both complement functions and all four hand-maintained `parse_event_word` entries go. Detection becomes an open `becomes|become|is|are <alpha1>` shape, and the real single-event mode parser becomes the arbiter: an admitted complement reaches its real mode, an unadmitted one lands on `TriggerMode::Unknown`, which every coverage authority already treats as unsupported (`is_card_supported`, `check_trigger`, `build_trigger_item`). Nothing in the parser decides whether a complement EXISTS based on whether we SUPPORT it. The honesty carrier is the MODE, not `Effect::unimplemented`. Measured: `unimpl` is false on BOTH arms of the blocker fixture — the effect ("draw a card") parses fine; only the trigger EVENT is unmodelled. Synthesizing an `Effect::Unimplemented` onto a well-parsed effect would mislabel the gap and corrupt the `parse_details` tree. Complements previously excluded for zero corpus demand now work for free, each landing on a registered mode: `becomes untapped` -> Untaps, `becomes attached` -> Attached, `becomes crewed` -> BecomesCrewed, `is tapped for mana` -> TapsForMana (CR 106.12a — a real event, so admitting it is correct). Three guards survive, and unlike the closed-list era ALL THREE now ablate to real measured breakage over the full suite: * `blocks or becomes blocked` in `is_existing_compound_mode` (CR 509.1h) — dropping it regresses 53 cards and reds 1 test. Under the closed list this entry was provably dead (0 reds), which is why an earlier round deleted it; an open head matches `becomes blocked`, so it is load-bearing again. * `fight or become blocked`, split out into its own `suppresses_duplicate_event_arm` predicate rather than smeared into the fused-mode guard, because it is NOT a fused mode — it prevents a duplicate `Fight` arm (double draw on one fight event). Dropping it regresses Neyith and reds 1 test. * `tied for ` after `is`/`are` (CR 603.2 + CR 603.8) — a comparison predicate is a game STATE inside a trigger condition, never an event. Dropping it regresses Preacher of the Schism (loses half its while-gate, gains a bogus arm, flips Supported -> Unsupported) and Call to Arms, and reds 2 tests. CR 603.2e is NOT cited as authorizing the open head. Its actual wording is "SOME trigger events use the word 'becomes'", which constrains WHEN becomes-triggers fire and does not license "every `becomes X` is an event" — and the counterexample is measured ("becomes greater than 4 or becomes less than 2" splits and drops half a gate). 603.2e is scoped to transition-vs-persistent-state on the `becomes` arm only; `parse_open_state_complement` cites CR 603.1 + CR 603.2 because it is voice-agnostic. One documented exposure, stated rather than hidden: a subject containing a `that is <participle>` relative clause truncates ("a creature that is enchanted attacks or dies" flips Supported -> Unsupported). Zero printings — WotC contracts to `that's`, which `tag("is ")` cannot match — and the failure direction is toward unsupported WITH an emitted gap, strictly safer than a vanishing branch. Pinned by a parser row and a coverage row rather than guarded, because a guard would have zero ablation signal. Corpus impact, differential in both directions over ~34,470 Oracle-bearing cards: exactly ONE card changes, and it is a repair. Imprison This Insolent Wretch's arm 2 went from the ungrammatical "When the chosen player is attacked becomes the target of a spell or ability" to the correct reconstruction. Both arms stay `Unknown` with `Unimplemented` present, so no coverage transition anywhere in the corpus. Also pins three previously UNPINNED cards: Artifact Possession / Haunting Wind / Powerleech ("becomes tapped or a player activates …" must not split). Tests: 6 new active rows plus the previously `#[ignore]`d spec now active and satisfied. The coverage half is asserted at the coverage authority in a new integration test, not at parser shape. Verification: cargo fmt clean, clippy-strict 0 warnings, 18022 lib + 4217 integration passing, Gate A/G PASS, Gate P PASS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Blocker addressed in Detection is now an open One place where I did not do what you asked, and whyYou asked to "route unadmitted complements through the existing strict-failure /
Related: your named test ( Guards: all three now ablate to real breakageYou were right to be sceptical of guards in this seam — I deleted one last round precisely because it ablated to zero. Under an open head that changes. Each was removed individually and the full lib suite re-run:
A fourth ( A CR correction I owe youMy previous head cited CR 603.2e as the authorizing rule for the family. That was an over-read and it is now gone. 603.2e's actual wording is "Some trigger events use the word 'becomes'" — it constrains when becomes-triggers fire and does not license "every One exposure, disclosed rather than hiddenA subject containing a ImpactDifferential in both directions over ~34,470 Oracle-bearing cards: exactly one card changes, and it is a repair. Also pins three cards that had no test coverage at all before this change: Verification at
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/engine/src/parser/oracle_trigger.rs (1)
8066-8084: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResolve the contradictory CR-scope sentence in the doc comment.
Line 8070 states the combinator is voice-agnostic and reached from both arms. Line 8073 then states "It stays scoped to the
becomesarm inparse_state_change_event_start." The pronoun "It" is ambiguous, and the two claims read as a direct contradiction. The intended referent is the CR 603.2e citation, not the combinator. Name the referent explicitly, and start theparse_event_boundarysentence on its own line.📝 Proposed doc rewrite
/// The governing rules are the general trigger-condition / trigger-event rules, NOT /// CR 603.2e: this combinator is VOICE-AGNOSTIC — reached from both the /// `becomes`/`become` arm and the `is`/`are` arm — and 603.2e speaks specifically /// about the word "becomes", so it cannot govern roughly half of these inputs. It -/// stays scoped to the `becomes` arm in `parse_state_change_event_start`. `parse_event_boundary` peeks eof/space/`,`/`.`, and its `space1` arm +/// stays scoped to the `becomes` arm in `parse_state_change_event_start`. +/// +/// `parse_event_boundary` peeks eof/space/`,`/`.`, and its `space1` arm /// is load-bearing: a qualifier may sit between the head and the `or`, which is whatReplace the ambiguous "It" with "The CR 603.2e citation".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_trigger.rs` around lines 8066 - 8084, Update the doc comment for parse_open_state_complement: replace the ambiguous “It” with “The CR 603.2e citation” to distinguish the citation from the voice-agnostic combinator, and begin the parse_event_boundary sentence on its own line.crates/engine/tests/integration/disjunctive_state_change_head_coverage_honesty.rs (1)
1-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReorder the module doc: the NOTE block splits a sentence.
The sentence that starts at line 5 ends at line 6 with "which is the coverage authority's unsupported". Its continuation is at line 12, "marker (
game/coverage.rs: ...)". The NOTE ON THE VEHICLE paragraph sits between the two halves. Both halves read as truncated. Move the NOTE paragraph below the completed sentence.📝 Proposed doc reorder
//! The `becomes|is|are <complement>` trigger-event head is detected as an OPEN //! shape, so an unmodelled complement still produces its own trigger arm. That arm -//! lands on `TriggerMode::Unknown`, which is the coverage authority's unsupported -//! NOTE ON THE VEHICLE: `is_card_supported` is private, so it cannot be asserted -//! from an integration test. `card_face_gaps` is the equivalent PUBLIC authority — -//! it applies the same `Unknown(_) || !registry.contains_key` predicate through -//! `check_trigger`, so `gaps.is_empty()` is the assertable form of "supported". -//! -//! marker (`game/coverage.rs`: `is_card_supported`, `check_trigger`, +//! lands on `TriggerMode::Unknown`, which is the coverage authority's unsupported +//! marker (`game/coverage.rs`: `is_card_supported`, `check_trigger`, //! `build_trigger_item`). Before the open head the second branch vanished and the //! card reported as fully supported — parser coverage went green while a //! rules-bearing branch was lost. //! +//! NOTE ON THE VEHICLE: `is_card_supported` is private, so it cannot be asserted +//! from an integration test. `card_face_gaps` is the equivalent PUBLIC authority — +//! it applies the same `Unknown(_) || !registry.contains_key` predicate through +//! `check_trigger`, so `gaps.is_empty()` is the assertable form of "supported". +//! //! The open shape's false-positive surface (a head match INSIDE a subject noun🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/tests/integration/disjunctive_state_change_head_coverage_honesty.rs` around lines 1 - 19, Reorder the module documentation so the sentence describing `TriggerMode::Unknown` as the coverage authority’s unsupported marker is contiguous and complete. Move the “NOTE ON THE VEHICLE” paragraph, including its `is_card_supported` and `card_face_gaps` explanation, after that sentence without changing the documentation content.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/engine/src/parser/oracle_trigger.rs`:
- Around line 8066-8084: Update the doc comment for parse_open_state_complement:
replace the ambiguous “It” with “The CR 603.2e citation” to distinguish the
citation from the voice-agnostic combinator, and begin the parse_event_boundary
sentence on its own line.
In
`@crates/engine/tests/integration/disjunctive_state_change_head_coverage_honesty.rs`:
- Around line 1-19: Reorder the module documentation so the sentence describing
`TriggerMode::Unknown` as the coverage authority’s unsupported marker is
contiguous and complete. Move the “NOTE ON THE VEHICLE” paragraph, including its
`is_card_supported` and `card_face_gaps` explanation, after that sentence
without changing the documentation content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b529ffa2-b49e-4162-b01f-d822ee453056
📒 Files selected for processing (4)
crates/engine/src/parser/oracle_trigger.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/tests/integration/disjunctive_state_change_head_coverage_honesty.rscrates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/engine/tests/integration/main.rs
- crates/engine/src/parser/oracle_trigger_tests.rs
# Conflicts: # crates/engine/tests/integration/main.rs # docs/parser-misparse-backlog.md
|
Merged
Post-merge verification at
|
|
Maintainer hold for current head The prior coverage-honesty blocker is addressed on this head. I am awaiting the still-running current-head Next step: re-check the terminal results for this exact SHA, then resume the maintainer disposition. |
|
All four checks you named are terminal and green at
Nothing is in progress. The only non-SUCCESS in the whole rollup is One open question for you, because it costs you a CI cycleCodeRabbit left a trivial-but-valid doc nitpick on It is a two-line doc-comment fix with no code change. I have deliberately not pushed it, because doing so would move the head and restart exactly the checks you said you're waiting on, and you control the queue. Your call:
I'd mildly prefer the second, purely so you aren't re-waiting on a comment typo, but I have no stake in it. |
…n-head round Both CodeRabbit nitpicks on PR phase-rs#6831, both my own wording, no code change. 1. `parse_open_state_complement` ended with "It stays scoped to the `becomes` arm", where the referent is the CR 603.2e CITATION, not the combinator — so it read as directly contradicting the preceding "this combinator is VOICE-AGNOSTIC" sentence. Names the referent explicitly. 2. The coverage-honesty integration test's module doc had my NOTE ON THE VEHICLE paragraph inserted into the MIDDLE of a sentence, leaving both halves reading as truncated ("...the coverage authority's unsupported" / "marker (game/coverage.rs: ...)"). Moved below the completed sentence. Doc comments only. Verification: cargo fmt clean, clippy-strict 0 warnings, 18299 lib + 4383 integration passing, Gate A/G PASS, Gate P PASS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both CodeRabbit nitpicks fixed in
Doc comments only — no code, no test-logic change. @matthewevans — this supersedes the choice I offered you, and I'm sorry for the extra cycle. You'd said you were holding on the four named checks at Verification at For the record on review state: the |
|
Maintainer hold — current head The coverage-honesty review is now clean. I applied the small maintainer fixup that removed the unrelated CR 603.1b citation from the event-head lexicon annotation; CR 603.2e remains the governing rule for the state-change semantics. The push started a new current-head CI run. Required Rust lint/tests and card-data coverage checks are queued or running, so approval and merge-queue handling resume only after their terminal results and the current-head parse-diff are available. The branch also currently reports |
|
All checks are terminal and green at On Thank you for the fixup — and one observation about it, for your judgement rather than as a request. Removing What's left, though, is That's the same scope mismatch CodeRabbit flagged one function below, on Entirely your call; I'm raising it only because I'd just spent a round on this precise class of over-scoped citation and it seemed worse to notice and say nothing. Happy to push it together with the |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved after current-head review: the open state-change head preserves modeled arms and converts unmodeled arms to coverage-visible TriggerMode::Unknown; runtime and coverage-honesty regressions discriminate.
matthewevans
removed
the
needs-maintainer
2 participants
Summary
Fixes Culvert Ambusher and its class:
"When this creature enters or is turned face up, …"parsed to a singleTriggerMode::ChangesZone, silently discarding the"or is turned face up"branch with zeroEffect::Unimplemented— so the card reported as fully supported while never triggering on its disguise flip. The disjunctive trigger-event splitter's head lexicon knew active-voice verbs plus a few passive/copular heads but had no intransitive state-change family, so every<event> or becomes/is <state>pair lost its second branch. This isdocs/parser-misparse-backlog.mdroot cause 6 ("Disjunctive (or-list) collapsed to first branch") — a silent-wrong-behavior misparse, not a coverage gap. 14 cards, zero new engine surface.Revised after review. The first version detected the head with a CLOSED allow-list of four complements. A maintainer blocked that correctly: a closed detection list means an unadmitted complement makes the second branch vanish while the card still reports supported — the same silent-wrong signature this PR exists to remove. Detection is now an open
becomes|become|is|are <alpha1>shape with the real single-event mode parser as the arbiter; unadmitted complements land onTriggerMode::Unknown, which every coverage authority already reports as unsupported. The hand-maintained support list is deleted, not relocated. Corpus impact of that redesign, differentialled both ways over ~34,470 cards: exactly one card changes and it is a repair (Imprison This Insolent Wretch, adescriptionreconstruction, no coverage transition).Three guards survive and — unlike the closed-list era — all three now ablate to real measured breakage:
blocks or becomes blocked(53 cards),fight or become blocked(1),tied forafteris/are(2). A fourth was written and removed for ablating to zero. One exposure is disclosed rather than hidden: athat is <participle>relative clause inside a subject truncates (zero printings; fails toward unsupported with an emitted gap), pinned by a parser row and a coverage row.CR correction: the earlier head cited CR 603.2e as authorizing the family. That was an over-read — 603.2e says "Some trigger events use the word 'becomes'" — and it is now scoped to transition-vs-persistent-state on the
becomesarm only, withparse_open_state_complementciting CR 603.1 + CR 603.2 because it is voice-agnostic.Files changed
crates/engine/src/parser/oracle_trigger.rs— the fix: one scope-limited sibling combinator (parse_state_change_event_start+ two voice complements +parse_event_head_start), composed at 2 of the lexicon's 9 consumers; the other 7 left narrow with a named counterexample in place; reconstruction moved ontoTextPair; a provably-dead fused-mode guard removed;parse_event_verb_startleft byte-identical.crates/engine/src/parser/oracle_trigger_tests.rs— 15 parser rows (T1–T14 plus the CR 508.1f overlap pin).crates/engine/tests/integration/culvert_ambusher_turn_face_up_force_block.rs— new, 4 runtime rows driving the real pipeline.crates/engine/tests/integration/main.rs— themodline (alphabetical; no new top-leveltests/*.rs).docs/parser-misparse-backlog.md— list hygiene for root cause 6.Track
Non-developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
CR 603.2eis the authorizing rule for the new family: it names "becomes" as a trigger-event word (its own examples are "becomes attached" / "becomes blocked") and draws the event-vs-state line the allow-list encodes.CR 603.1bauthorizes one written ability carrying several trigger conditions, with the representational gap stated explicitly rather than glossed: the parser emits N independentTriggerDefinitions, which 603.1b does not itself license — what makes it equivalent is that the conditions key on disjointGameEvents (CR 603.2,CR 603.2c), and 603.1b's own subject matter (an "all of those conditions" instruction) appears on zero printed cards.Complement authorities:
CR 701.37a/CR 701.37b(monstrous),CR 701.26a+CR 603.2e(becomes tapped),CR 116.2b+CR 708.7(turned face up),CR 120.1(dealt damage). Also cited:CR 106.12a(theis tapped for manaexclusion — a distinct event with its ownTapsForManamode),CR 508.1f(declaring an attacker taps it — the newly-created attacks/becomes-tapped overlap),CR 508.3a,CR 509.1c,CR 509.1h(the fusedBlocksOrBecomesBlockedmode),CR 603.1,CR 603.2,CR 603.3d,CR 614.1,CR 702.168a/CR 702.168d(disguise),CR 708.2,CR 708.3,CR 708.8.Every number was grepped in
docs/MagicCompRules.txtbefore it was written, and each rule's text was read to confirm it describes the annotated code. Two citations were corrected during review:CR 603.2chad been cited for an at-most-one-arm property it does not establish (it says only "An ability triggers only once each time its trigger event occurs"), andCR 603.2g— the prevention/replacement rule — had been leading whereCR 120.1is the authority.Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo fmt --all --check— clean.cargo clippy-strict(clippy --all-targets -- -D warnings, whole workspace) — clean, 0 warnings.cargo test -p phase-engine—ok. 18299 passed; 0 failed; 6 ignored(lib) ·ok. 4383 passed; 0 failed; 2 ignored(integration) — figures are post-merge with 72 upstream commits; the change itself measured 18022 + 4217 before the merge ·ok. 8 passedandok. 9 passed(bin unit tests)../scripts/check-parser-combinators.sh— see Gate A. No// allow-noncombinatoranywhere in this change../scripts/check-prelowered-ratchet.sh— Gate P PASS (no producer count increased). Its 5 "below ceiling" advisories are pre-existing and in files this change does not touch.cargo coverage/cargo semantic-audit/./scripts/gen-card-data.sh— CI-owned:./scripts/setup.sh --agentneedsjqandpnpm, neither installable in this environment, soclient/public/card-data.jsoncannot be generated here. Substituted by whole-corpus parser differentials (below), taken in both directions by three independent agents over 35 458 Oracle-bearing cards.Note
A caveat worth landing in the repo's own tooling knowledge.
./scripts/check-parser-combinators.shwith no argument setsBASE=$(git merge-base origin/main HEAD). On an uncommitted branch that equalsHEAD, which flips the script intoDIFF_MODE=--cachedand — with nothing staged — prints a vacuousGate A PASS. The Gate A output below was taken after the final commit, wherehead != base, and I confirmed it actually scansoracle_trigger.rs. Anyone running Gate A pre-commit should pass an explicit base (./scripts/check-parser-combinators.sh HEAD).Revert-discrimination, measured per seam over the full suite (not asserted — each revert was applied, the suite re-run, and the file restored byte-identically):
parse_event_head_start→ narrow)extract_subject_text→ narrow)TextPairoriginal → lower)Ddescriptionassertionblockedto the allow-list"a turn-face-up trigger must have gone on the stack and asked for a target"— the original bug reproducing at runtimeentering_tapped_does_not_fire_the_becomes_tapped_armpinsCR 603.2eand was proven non-vacuous by injectingGameEvent::PermanentTappedat the ETB tap site ingame/zone_pipeline.rs: exactly that test went red, the other three stayed green, and the file was restored byte-identically.Gate A
Gate A PASS head=5bef81ee7c66d402d06e094267868b7a72eb9c95 base=6719cc549f2850e396e4801189f2479b7064673a
Anchored on
crates/engine/src/parser/oracle_trigger.rs:7748—parse_event_verb_start, the existing event-head lexicon this change siblings: same nestedalt()ofparse_event_word/parse_event_phrasegroups, same module, sameOracleResult<'_, ()>shape, same CR-annotated per-group style. The new family is modelled on it directly, and it is left byte-identical.crates/engine/src/parser/oracle_trigger.rs:7595—split_serial_event_compound, the sibling splitter that already establishes the lockstep original/lowercase reconstruction idiom, its comment stating the house rule verbatim: "Split the original and lowercase forms in lockstep on the same ASCII delimiters rather than slicingconditionwith byte offsets taken fromcond_lower." That is the precedent theTextPairreconstruction follows.crates/engine/src/parser/oracle_util.rs:25—TextPair, named inCLAUDE.mdas the mandated replacement for manually computing&text[prefix.len()..];split_triggeralready builds one over this same pair of strings.Final review-impl
Final review-impl PASS head=5bef81ee7c66d402d06e094267868b7a72eb9c95
Provenance of that line, so it is auditable rather than taken on trust. The full
independent read-only pass ran against
b7e56d5and returned no BLOCKER and no MAJOR,verdict "ready to open as a pull request", with exactly one MINOR: a comment in
split_or_event_compoundoverclaimed that every span is "taken at ONE position validin both" views of the
TextPair, when the offset is in fact still derived from thelowercase view — which the sibling
split_serial_event_compounddocuments as the thingto avoid.
b7f52a8isb7e56d5with only that comment rewritten to the reviewer'sown suggested wording (
git diff b7e56d5 b7f52a8= 10 insertions / 5 deletions, zeronon-comment lines), and a further SHA-matching confirmation pass against
b7f52a8returned zero findings, having re-derived Gate A itself and checked each of the
reword's four claims against the code:
TextPair::new'sdebug_assert_eq!byte-lengthparity (
oracle_util.rs:32, plus a second lowercase-equality assert and ais_char_boundaryassert insplit_at— so the comment now claims less than the codeguarantees),
posgenuinely lowercase-derived,split_serial_event_compound's stricterrule present verbatim with its
İ/ẞrationale, and the replaced rawcondition[..pos]slicing confirmed present at the base commit. Every gate above wasre-run at
b7f52a8.Four rounds of independent, context-isolated review ran (three plan rounds, then implementation rounds). The plan loop was substantive, not ceremonial — it changed the design three times:
find_effect_boundarytruncates the condition before the serial splitter runs, so a new-family leg could never reach those gates. The widening and the test asserting it were dropped."Whenever ~ becomes tapped, attacks, or dies"→[Taps, Taps, Taps]with descriptions like"becomes tapped attacks", a CR 603.2c hazard) into the correct[Taps, Attacks, ChangesZone]. That capability is now claimed and pinned rather than shipped unclaimed under a comment denying it.recase_spanhelper was replaced with theTextPairbuilding block; ~30 stale line-number anchors were removed in favour of function names; a false unreachability invariant on thecapitalize_firstfallback was corrected (parse_trigger_linesispubwith five non-stripping production callers); two CR citations were fixed; and a provably-deadis_existing_compound_modeguard was deleted once measurement showed dropping it reds 0 of 18 015 tests while admittingblockedto the allow-list reds 3 — the constraint is pinned at the seam a future widener actually edits.My own starting design was also refuted on evidence and is not what shipped: I proposed a
becomes/become/is/are × complementcross-product, which is unsound because the voices have disjoint complements (is monstrousandis tappedare states;is tapped for manais a different event). It became two voice-scoped productions. I had also identified 2 consumers of the lexicon; there are 9, across 5 functions.Claimed parse impact
14 cards. Each gains a correct second
TriggerDefinitionsharing the effect body; nothing else in the corpus changes. Verified in both directions by three independent agents (whole-corpus differential over 35 458 Oracle-bearing cards; plus an independent Scryfall class enumeration that partitions 21 matching cards into exactly these 14 plus 3As … entersreplacements, 3 "becomes tapped or a player activates", and Illusionary Mask).is turned face up→TurnFaceUp: Culvert Ambusher, Case of the Pilfered Proof, Concert Kaboomist, Efreet Weaponmaster, Gadget Technician, Offender at Large, Ponyback Brigade, Rakish Scoundrelbecomes monstrous→BecomeMonstrous: Alpha Deathclaw, Protector of the Wastesbecomes tapped→Taps: Champions of the Shoal, A-Radha Coalition Warlord, A-Zar Ojanen Scion of Efravais dealt damage→DamageReceived: CryoshatterTwo cards additionally change display text only (
descriptionrecasing from theTextPairfix, no mode/filter delta): Millicent Restless Revenant (spirit→Spirit), Acornelia Fashionable Filcher (squirrel→Squirrel).Coverage honesty. No line is newly accepted — all 14 were already accepted with half of each silently dropped; this recovers the dropped half. All four destination modes pre-exist, are in the matcher registry, are indexed, and have live event producers, so both halves of every split lower to a fully modelled trigger. No new
Effect::Unimplementedwas needed and none was introduced. Concert Kaboomist keeps its pre-existingwhere_x_bindinggap on both halves and stays honestly red under root cause 5.Deliberately not claimed, still red: trailing/middle-leg serial generalization (
"Whenever ~ enters, attacks, or becomes tapped"still yields one trigger carryingEffect::Unimplemented); Neyith of the Dire Hunt'sbecome blockedbranch; Donna Noble; Giant Oyster / Merieke Ri Berit / Tawnos's Coffin / The Pandorica / Coffin Queen (theirbecomes untappeddisjunction sits inside an activated-ability line that routes elsewhere); theis tapped for manafamily (excluded per CR 106.12a).Disclosed follow-up (owed refactor, recorded in-code, not attempted here). The allow-list is a detection list, so an unadmitted complement still makes a second branch vanish with no marker — measured:
"enters or is turned face down"yields one trigger and reports as supported, the same silent-wrong signature as the bug being fixed. This change strictly shrinks that pre-existing hole (0 printed cards need the unadmitted shapes), but the stronger long-term seam is to detect the open head shape (becomes|is|are <participle>) and route unadmitted complements toEffect::unimplemented, demoting the list from "what we recognize" to "what we support". That is also a better consolidation trigger than a lexicon count. It needs a design at the splitter'sOption<Vec<String>>re-parse boundary, not a widenedalt().Scope Expansion
None.
Validation Failures
None.
CI Failures
One, and it is pre-existing on
main— not from this change.Frontend (lint, type-check, test)fails atclient/src/game/controllers/__tests__/aiController.test.ts:138—"re-queries after the dispatch layer returns the engine's tagged stale outcome without
fabricating an action",
expected "vi.fn()" to be called 3 times, but got 4 times.Evidence it is not mine:
git diff 6719cc5 9433ca3 --name-onlytouches zeroclient/files (5 files: 2parser, 2 engine tests, 1 doc).
mainfails the identical test with the identical assertion at the same line— run
30602816316(main@ 03:59) and the run before it.mainwas green through01:59, so this broke on
mainbetween 01:59 and 03:18.Every other check is green, including all Rust jobs,
Card data (generate, validate, coverage), WASM, Tauri, and the lobby worker.