fix(engine): record a token battlefield entry even when its events are suppressed (CR 403.3) - #6851
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSuppressed copy-token entries now park battlefield-entry state until copy and as-enters processing settles. Shared helpers record and emit each entry once. Coverage reports now include the head commit SHA. ChangesDeferred Token Battlefield Entry
Coverage Report Provenance
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CopyTokenResolution
participant GameState
participant TokenEntryHelpers
participant PriorityPipeline
CopyTokenResolution->>GameState: Park suppressed token entry
CopyTokenResolution->>TokenEntryHelpers: Schedule object-only emission
TokenEntryHelpers->>GameState: Flush settled battlefield entry
TokenEntryHelpers->>PriorityPipeline: Emit entry events for scanning
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocking: record_committed_token_entry now writes the Suppress-route rows before BecomeCopy, but their content is refreshed only by emit_recorded_token_entry_events in the fully unpaused tail. That misses the very mid-entry pauses this change says it supports.
For example, an Embalm Vizier copying Painter's Servant reaches finish_copy_target_choice_entry after BecomeCopy; current_self_enter_replacement_choice then raises Painter's mandatory color NamedChoice and this function returns before the emit/refresh at the caller's tail. The NamedChoice resume path replays deferred entry events, but it does not invoke emit_recorded_token_entry_events. The two ledger rows therefore remain the pre-copy 0/0 Vizier snapshot instead of the realized Painter's Servant entry.
That is materially incorrect for the CR 400.7/403.3 lookup consumers this PR is updating, and it is the opposite of the unpaused Bear assertion added here. Please carry the record refresh through each post-copy pause/resume (or postpone record construction until the copied characteristics and mandatory as-enters choices are complete), then add an integration regression that drives this copied mandatory-choice pause and asserts both ledgers describe the realized copy exactly once.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/game/effects/token.rs`:
- Around line 1835-1891: Centralize battlefield-entry recording through
record_zone_change for the production paths in counters.rs, token_copy.rs, and
gift_delivery.rs, ensuring each entry creates both synchronized ledger rows
before emitting ZoneChanged events. Update emit_recorded_token_entry_events to
detect and repair or reject mismatched ledgers, specifically preventing the
(None, Some(_)) case from appending a duplicate battlefield-entry record.
🪄 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: 1d51e64b-761f-4273-9f97-c6bed7a3d8b8
📒 Files selected for processing (4)
crates/engine/src/game/effects/counters.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/engine_replacement.rscrates/engine/tests/integration/token_zone_change_index.rs
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
|
🤖 AI text below 🤖 Confirmed, and it is worse than you stated. I reproduced your exact path as an integration Two things I did not know before measuring: 1. The emit is lost on this route too, not just the refresh. 2. Baseline separation. Same probe grafted onto merged
So the missing emit is pre-existing and unchanged by this PR; what this PR changes on this route is Also confirmed: CodeRabbit's related ledger-mismatch Minor. Seven production callers write Taking your second option (postpone construction until the copied characteristics and the Marking this PR draft while that is in flight so it is not sitting on your queue. |
|
Thanks for reproducing this and for separating the baseline behavior from the regression introduced here. I confirmed the follow-up against the unchanged head: it does not resolve the requested change, and the current The Paint-er mandatory-choice path establishes both failures on this head: the newly added rows retain the pre-copy identity, and the deferred entry emit is dropped. Your confirmation of CodeRabbit's Keeping this draft while you rework it is appropriate. Please push the redesigned implementation and the production integration regression before requesting another review. |
…eld entries CR 400.7 + CR 403.3. A `TokenEntryEventEmission::Suppress` token's battlefield entry was recorded by the finalize tail BEFORE `BecomeCopy` resolved and its content refreshed only in `handle_copy_target_choice`'s fully-unpaused tail, so every mid-entry pause kept a pre-copy row — and four of the five pause returns in `finish_copy_target_choice_entry` never reached the deferred emit at all. Measured on the maintainer's named path (Embalm Vizier of Many Faces copying Painter's Servant, whose mandatory as-enters `NamedChoice` returns from `finish_copy_target_choice_entry`): both ledgers held a pre-copy `Vizier of Many Faces` 0/0 row and no entry event was emitted. Replace the split record-here / refresh-there route with one postponed lifecycle: - The `Suppress` finalize tail RECORDS NOTHING. It parks the entry on `GameState::pending_token_battlefield_entry` (serde-persisted, so it survives arbitrary client round trips — CR 614.12a puts the as-enters choice before the permanent enters). - `token::flush_pending_token_battlefield_entry` is the sole consumer. It takes the parked value with `Option::take_if` and, over that one owned value, calls `restrictions::record_zone_change` — the single writer of BOTH CR 400.7 ledgers — and pushes the CR 603.6a entry pair. "Recorded but never emitted", "emitted but never recorded", and the duplicate battlefield-entry row are unrepresentable rather than guarded. - `emit_recorded_token_entry_events` is deleted. Its `(None, Some(_))` arm — which appended a duplicate `battlefield_entries_this_turn` row, reported by CodeRabbit — no longer has a representation: nothing reads a previously-written row back. Three convergence points feed that one authority, and none subsumes another (each is revert-probed): - `finish_copy_target_choice_entry`, for the unpaused route, whose action ends on a stale `CopyTargetChoice` and so would otherwise realize one client round trip late; - the `EmitCommittedCopyTokenEntry` post-action, inside the CR 616.1 counter drain, ahead of that action's trigger scan; - `token::realize_settled_token_battlefield_entry`, one gate (settled `WaitingFor::Priority` + the token still on the battlefield) called from two sites in `engine.rs`: inside `apply_action` immediately before `run_post_action_pipeline`, so the realized pair is trigger-scanned in its own action (CR 603.6a — Soul Warden observes the copy token), and again at the action boundary as the backstop for handlers that return an `ActionResult` straight out of the reducer match. The settled gate is pause-shape-agnostic by construction: it names the settled state, the complement of "any pause", so no `WaitingFor` variant appears in it. That is what covers the as-enters class whose continuation raises a SECOND pause (11 Tribute creatures and the `RevealHand` chains), which a resume-arm hook would have missed. Known partial, measured and pinned: `handle_tribute_choice` builds its `ActionResult` directly out of the reducer match, so the two-pause class realizes at the boundary — after the trigger scan — and its ETB observers do not fire (Soul Warden delta 0). Both ledgers and the emit are correct there; this is strictly better than the previous behaviour, which emitted nothing at all for that class, and it is a property of the reducer's direct-return handlers rather than of this lifecycle. Regressions drive the real cast pipeline and pin all four routes, each with a reach-guard on the exact prompt sequence: the Painter's Servant mandatory-choice pause (post-copy identity on both ledgers exactly once, a non-empty emitted pair carrying the recorder-assigned index, and the ETB observer at +1), the Fanatic of Xenagos two-pause class, the CR 616.1 counter-ordering pause, and the unpaused route; plus a positive control on the untouched `Emit` path. Reported by matthewevans (phase-rs#6851 review) and CodeRabbit; both verified before fixing. Assisted-by: ClaudeCode:claude-opus-5
1bd2705 to
a53bde0
Compare
|
🤖 AI text below 🤖 Redesigned and pushed. I took your second option — postpone construction until the copied The shapeThe
Three convergence points feed that one authority; each is independently revert-probed, and none
(c)'s gate is pause-shape-agnostic by construction: it names the settled state, the complement of Measured, on your named pathEmbalm Vizier of Many Faces → copy Painter's Servant → mandatory colour
One correction to the review, and one accepted partialCorrection. Your review says the Accepted partial, pinned rather than left silent. Two pre-existing defects found while measuring — filed, not folded in
Also noted, not acted on: ProcessFull Disclosure: round 2's plan-review finding was a prose-only correction to the plan document, applied |
github-actions
Bot
added
the
needs-maintainer
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/engine/tests/integration/token_zone_change_index.rs (1)
808-848: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the
token == Nonecase explicit so per-step counters cannot read as vacuous zeros.
matches_tokenreturnsfalsefor every event and every ledger row whentokenisNone. On the declined routedrive.tokenis never set, so all four counters inCopyEntryStepare zero by construction rather than by measurement.declined_copy_replacement_records_the_token_entry_without_parking_itreads the ledgers fromrunner.state()directly and only usesstepsforparked, so it is not blind today. A later per-step assertion on that route would pass for the wrong reason.Consider making the unmeasurable case unrepresentable, for example by storing
Option<TokenCounts>inCopyEntryStepwhen the token id is unknown, or by documenting on the struct that counters are meaningless whiletokenisNone.🤖 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/token_zone_change_index.rs` around lines 808 - 848, Make the unknown-token case explicit in token_entry_step and CopyEntryStep so per-step counters cannot appear as measured zeroes when token is None. Prefer representing the counter group as Option<TokenCounts> (or an equivalent explicit unmeasurable state), returning None when no token id is available and preserving normal counts for Some(token); update consumers and assertions to unwrap or handle that state while keeping parked independently measurable.Source: Path instructions
🤖 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/game/turns.rs`:
- Around line 1161-1166: Update the comment immediately above
pending_token_battlefield_entry’s turn-boundary reset to cite CR 514.2 as the
authority for ending “this turn” state, while retaining CR 400.7 and CR 603.6a
for entry realization and ETB-trigger behavior.
In `@crates/engine/tests/integration/token_zone_change_index.rs`:
- Around line 1094-1095: Update the dedup-key descriptions near the referenced
comments to remove the “CR 603.2c” citation and describe the turn-zone-change
index and batched-replay deduplication as engine implementation details. Apply
the same wording correction to the corresponding description near the later
occurrence, without changing the behavior or surrounding test logic.
---
Nitpick comments:
In `@crates/engine/tests/integration/token_zone_change_index.rs`:
- Around line 808-848: Make the unknown-token case explicit in token_entry_step
and CopyEntryStep so per-step counters cannot appear as measured zeroes when
token is None. Prefer representing the counter group as Option<TokenCounts> (or
an equivalent explicit unmeasurable state), returning None when no token id is
available and preserving normal counts for Some(token); update consumers and
assertions to unwrap or handle that state while keeping parked independently
measurable.
🪄 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: cbd662eb-8fa2-4ac5-b1dd-476d8aa3db9a
⛔ Files ignored due to path filters (1)
crates/engine/tests/fixtures/cr733/authority_matrix.json.gzis excluded by!**/*.gz
📒 Files selected for processing (9)
crates/engine/src/game/effects/counters.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/elimination.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_replacement.rscrates/engine/src/game/scenario_db.rscrates/engine/src/game/turns.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/token_zone_change_index.rs
CodeRabbit on phase-rs#6851. The per-turn clear of `pending_token_battlefield_entry` cited only CR 400.7, which names the ledgers it defends but is not the authority for the reset itself. CR 514.2 is: the cleanup step is where all "until end of turn" and "this turn" effects end. Cite both. Also tightens the one place that read as though CR 603.2c names `turn_zone_change_index`. The index is the engine's own key; the batched zone-change replay guard dedups on it to hold CR 603.2c's once-per-occurrence bound. The citation stays — it is the rule the guard implements, and it matches this file's existing module header — but the sentence now separates rule from mechanism. Comment-only; no behaviour change. Assisted-by: ClaudeCode:claude-opus-5
|
🤖 AI text below 🤖 @coderabbitai — both addressed in 1. 2. The citation itself stays, for two measured reasons. First, this file already establishes that No change at line 1382, and here I think the finding does not apply. That doc comment opens Thanks also for the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline bee984f80
git show --format=fuller --unified=12 bee984f80 -- \
crates/engine/src/game/turns.rs \
crates/engine/tests/integration/token_zone_change_index.rs
printf '\n--- turns.rs context ---\n'
nl -ba crates/engine/src/game/turns.rs | sed -n '1150,1185p'
printf '\n--- token_zone_change_index.rs contexts ---\n'
nl -ba crates/engine/tests/integration/token_zone_change_index.rs | sed -n '1070,1120p'
nl -ba crates/engine/tests/integration/token_zone_change_index.rs | sed -n '1360,1400p'
printf '\n--- annotation guidance ---\n'
fd -a -t f '^CLAUDE\.md$' . -x sh -c 'echo "--- $1 ---"; rg -n -C3 "annotation|rule|citation|CR 603\\.2c|turn_zone_change_index" "$1" || true' sh {}Length of output: 6987 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- current Line 1382 context ---'
sed -n '1360,1405p' crates/engine/tests/integration/token_zone_change_index.rs
printf '%s\n' '--- module header and prior CR 603.2c annotations ---'
sed -n '1,55p' crates/engine/tests/integration/token_zone_change_index.rs
grep -n -C 2 -E 'CR 603\.2c|turn_zone_change_index' \
crates/engine/tests/integration/token_zone_change_index.rs || true
printf '%s\n' '--- CLAUDE.md guidance ---'
find . -name CLAUDE.md -type f -print -exec sh -c '
echo "--- $1 ---"
grep -n -i -C 3 -E "annotation|rule|citation|MagicCompRules|definition_ref" "$1" || true
' sh {} \;Length of output: 46071
The The comment at Line 1095 now separates I withdraw the Line 1382 finding. The existing comment already makes the same distinction. It states the CR 603.2c requirement first, then identifies 🐇 You are interacting with an AI system. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the new parked-entry lifecycle records the row, but still drops ETB trigger emission on a reachable two-pause continuation.
🔴 Blocker
crates/engine/src/game/effects/token.rs:1904-1908 explicitly documents that the handle_tribute_choice direct-return path realizes the parked entry only at the action boundary, after its trigger scan. crates/engine/src/game/engine.rs:376-385 confirms that placement. The production-path test in crates/engine/tests/integration/token_zone_change_index.rs:1150-1161 reaches CopyTargetChoice → NamedChoice → TributeChoice, then deliberately asserts Soul Warden’s life delta is 0 at :1213-1225. That result is rules-incorrect: CR 603.6a says ETB abilities are checked whenever an event puts a permanent onto the battlefield (verified local corpus docs/MagicCompRules.txt:2599; CR 603.2c at :2567). Make the direct-return continuation converge through the normal post-action trigger pipeline, or flush before that handler’s trigger scan; the regression must assert Soul Warden gains 1.
🔴 Required evidence
The required <!-- coverage-parse-diff --> comment for current engine source is not bound to the head: issue comment 5153742352, updated for this run, says only “No card-parse changes detected” with no SHA. Regenerate/update the artifact so it identifies bee984f809e084d2bd0c71c4bbbb3d67ac8d13b4.
✅ Clean
The prior stale/lost Suppress-route ledger finding is addressed by the park/flush design; the old CodeRabbit lifecycle finding is stale. Current CI is green, but it does not remedy the reachable trigger defect.
Recommendation: request changes on this head, then resubmit with the trigger-pipeline fix, a discriminating +1 ETB assertion, and a head-bound parse-diff artifact.
…eld entries CR 400.7 + CR 403.3. A `TokenEntryEventEmission::Suppress` token's battlefield entry was recorded by the finalize tail BEFORE `BecomeCopy` resolved and its content refreshed only in `handle_copy_target_choice`'s fully-unpaused tail, so every mid-entry pause kept a pre-copy row — and four of the five pause returns in `finish_copy_target_choice_entry` never reached the deferred emit at all. Measured on the maintainer's named path (Embalm Vizier of Many Faces copying Painter's Servant, whose mandatory as-enters `NamedChoice` returns from `finish_copy_target_choice_entry`): both ledgers held a pre-copy `Vizier of Many Faces` 0/0 row and no entry event was emitted. Replace the split record-here / refresh-there route with one postponed lifecycle: - The `Suppress` finalize tail RECORDS NOTHING. It parks the entry on `GameState::pending_token_battlefield_entry` (serde-persisted, so it survives arbitrary client round trips — CR 614.12a puts the as-enters choice before the permanent enters). - `token::flush_pending_token_battlefield_entry` is the sole consumer. It takes the parked value with `Option::take_if` and, over that one owned value, calls `restrictions::record_zone_change` — the single writer of BOTH CR 400.7 ledgers — and pushes the CR 603.6a entry pair. "Recorded but never emitted", "emitted but never recorded", and the duplicate battlefield-entry row are unrepresentable rather than guarded. - `emit_recorded_token_entry_events` is deleted. Its `(None, Some(_))` arm — which appended a duplicate `battlefield_entries_this_turn` row, reported by CodeRabbit — no longer has a representation: nothing reads a previously-written row back. Three convergence points feed that one authority, and none subsumes another (each is revert-probed): - `finish_copy_target_choice_entry`, for the unpaused route, whose action ends on a stale `CopyTargetChoice` and so would otherwise realize one client round trip late; - the `EmitCommittedCopyTokenEntry` post-action, inside the CR 616.1 counter drain, ahead of that action's trigger scan; - `token::realize_settled_token_battlefield_entry`, one gate (settled `WaitingFor::Priority` + the token still on the battlefield) called from two sites in `engine.rs`: inside `apply_action` immediately before `run_post_action_pipeline`, so the realized pair is trigger-scanned in its own action (CR 603.6a — Soul Warden observes the copy token), and again at the action boundary as the backstop for handlers that return an `ActionResult` straight out of the reducer match. The settled gate is pause-shape-agnostic by construction: it names the settled state, the complement of "any pause", so no `WaitingFor` variant appears in it. That is what covers the as-enters class whose continuation raises a SECOND pause (11 Tribute creatures and the `RevealHand` chains), which a resume-arm hook would have missed. Known partial, measured and pinned: `handle_tribute_choice` builds its `ActionResult` directly out of the reducer match, so the two-pause class realizes at the boundary — after the trigger scan — and its ETB observers do not fire (Soul Warden delta 0). Both ledgers and the emit are correct there; this is strictly better than the previous behaviour, which emitted nothing at all for that class, and it is a property of the reducer's direct-return handlers rather than of this lifecycle. Regressions drive the real cast pipeline and pin all four routes, each with a reach-guard on the exact prompt sequence: the Painter's Servant mandatory-choice pause (post-copy identity on both ledgers exactly once, a non-empty emitted pair carrying the recorder-assigned index, and the ETB observer at +1), the Fanatic of Xenagos two-pause class, the CR 616.1 counter-ordering pause, and the unpaused route; plus a positive control on the untouched `Emit` path. Reported by matthewevans (phase-rs#6851 review) and CodeRabbit; both verified before fixing. Assisted-by: ClaudeCode:claude-opus-5
CodeRabbit on phase-rs#6851. The per-turn clear of `pending_token_battlefield_entry` cited only CR 400.7, which names the ledgers it defends but is not the authority for the reset itself. CR 514.2 is: the cleanup step is where all "until end of turn" and "this turn" effects end. Cite both. Also tightens the one place that read as though CR 603.2c names `turn_zone_change_index`. The index is the engine's own key; the batched zone-change replay guard dedups on it to hold CR 603.2c's once-per-occurrence bound. The citation stays — it is the rule the guard implements, and it matches this file's existing module header — but the sentence now separates rule from mechanism. Comment-only; no behaviour change. Assisted-by: ClaudeCode:claude-opus-5
The reducer's direct-return arms build their `ActionResult` inside the match and never reach `apply_action`'s tail pipeline, so a CR 403.3 token battlefield entry realized on one of those routes entered with no ETB observer ever seeing it — CR 603.6a requires the check to run whenever an event puts a permanent onto the battlefield. `realize_settled_token_battlefield_entry` now reports whether it realized, and `apply_action_boundary_core` runs the same `run_post_action_pipeline_from` the tail path delegates to, over the slice the realization appended. The convergence is arm-agnostic: the match has 16 non-error direct returns and this is inert on the 15 that never park. Measured on the reviewer's named path (Embalm Vizier of Many Faces -> copy Painter's Servant -> mandatory colour NamedChoice -> Tribute, Soul Warden on the battlefield): Soul Warden's life delta goes 0 -> 1. Reverting the block reproduces the 0, failing at the reach-guard first; with the guard relaxed the life assertion alone flips. The regression pins "OrderTriggers(2)" — Tribute is declined, so Fanatic's CR 702.104b intervening-if is true and its ETB fires alongside Soul Warden's, both P0's, so CR 603.3b requires an ordering prompt. Both `waiting_for` writes are required: `finish_action_boundary` -> `sync_waiting_for` copies the result into the state, so a state-only write is undone, and the life-safety preview never calls `finish_action_boundary`, so a result-only write is insufficient. Assisted-by: ClaudeCode:claude-opus-5
`render_markdown` returned early for the no-change case with the marker and "No card-parse changes detected" and nothing else — no base, no head — while the changed-cards branch printed only the baseline. No branch had ever emitted a head SHA, so the sticky comment could not be tied to the commit it described and regenerating it could not help. Both branches now carry the head, sourced from the `HEAD_SHA` the CI step already exports, with a `--head-sha` flag mirroring the existing `--base-sha`. Deriving it from git would be wrong rather than merely inelegant: that job checks out the synthetic PR merge commit, so git reports the merge SHA, never the PR head. Two consumer constraints shape the wording, each pinned by an assertion with a revert-probe: `scripts/pr_review.py` requires the marker to remain the first line, and it classifies sticky state by the substrings "Baseline pending" and "signature(s)", so the head line must contain neither or a no-change sticky misclassifies as real parse changes. Assisted-by: ClaudeCode:claude-opus-5
bee984f to
35f71f0
Compare
|
🤖 AI text below 🤖 Thanks — both items are addressed at 🔴 Blocker — direct-return continuation now converges through the post-action pipelineYou offered two remedies. I measured the second one first, and it has nothing to attach to:
Measured on the path you named (
The regression at One residual I want on the record rather than discovered later: the pipeline runs from 🔴 Required evidence — the sticky could not identify the head, and regenerating cannot fix itI could not satisfy this by regenerating, and the reason is a defect rather than a stale run. Comment
That generator is ordinary crate source, not a workflow, so I fixed it there: both branches now carry the head, sourced from the Two constraints shaped the wording, both pinned by assertions with revert-probes: One consequence worth stating so it doesn't cost you a round: the sticky names the head it was generated from, so landing these commits necessarily moves it. The regenerated comment will identify This is a class-wide provenance fix rather than a #6851 one, so it is disclosed under Scope Expansion. Two residuals remain that I deliberately did not touch, because they live in
Credit to CodeRabbit for the CR 514.2 citation correction on the per-turn reset in the earlier round. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/bin/coverage_parse_diff.rs`:
- Line 347: Update the argument parsing branch for --head-sha so a present
option with no following value returns a usage error instead of reusing the
existing head_sha fallback; preserve accepting and assigning a supplied value.
Add a regression test covering trailing --head-sha and assert that parsing fails
with the expected usage error.
- Line 445: Update the DiffReport data model to include the head_sha provenance
field, populate it when constructing the report in the coverage diff parser, and
extend the associated tests and expected parse-diff.json output to verify the
field is serialized.
🪄 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: bef5dcf4-b52c-450e-940a-9a54d0e2e72b
⛔ Files ignored due to path filters (1)
crates/engine/tests/fixtures/cr733/authority_matrix.json.gzis excluded by!**/*.gz
📒 Files selected for processing (10)
crates/engine/src/bin/coverage_parse_diff.rscrates/engine/src/game/effects/counters.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/elimination.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_replacement.rscrates/engine/src/game/scenario_db.rscrates/engine/src/game/turns.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/token_zone_change_index.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/engine/src/game/scenario_db.rs
- crates/engine/src/game/turns.rs
- crates/engine/src/game/elimination.rs
- crates/engine/src/game/effects/counters.rs
- crates/engine/src/types/game_state.rs
- crates/engine/src/game/engine_replacement.rs
- crates/engine/src/game/engine.rs
- crates/engine/tests/integration/token_zone_change_index.rs
- crates/engine/src/game/effects/token.rs
Two review findings from CodeRabbit on the head-SHA change. A present-but-valueless `--base-sha` or `--head-sha` silently fell back to the previous value, so a report could name a commit it does not describe. Both now return a usage error. The pre-existing `--base-sha` line is fixed alongside the one this branch added: leaving it lenient would make two flags in the same provenance category behave differently. `--markdown`, `--json` and `--max-clusters` stay lenient by design and a test pins that asymmetry as deliberate — a missing value there omits output the caller can see, so there is nothing to misattribute. `parse-diff.json` carried no provenance at all, while both the Markdown report and the sticky comment direct a reader with truncated output to open it. `DiffReport` now carries `head_sha` and `base_sha`, ordered as the Markdown presents them. Nothing in the repo deserializes that artifact — the four references to it are two workflow write/upload paths and two prose mentions — so the added keys cannot break a consumer. Neither defect is reachable through CI, which never passes `--head-sha` and always pairs its flags with values; this hardens the local and manual surface. Assisted-by: ClaudeCode:claude-opus-5
|
🤖 AI text below 🤖 @coderabbitai — both taken, in Missing One correction on the impact, offered as measurement rather than disagreement about the fix: it is not reachable from CI. Provenance in Verification: three new tests (bin suite 9 → 12), each with a revert-probe that was run and restored byte-identically. The JSON probe swaps head and base rather than deleting a key, so it fails on a symmetric mistake and not merely a missing field. Full suite One related weakness I found while doing this and deliberately did not fix: all five flags still greedily consume the next token, so |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline bc9f94455
printf '\n--- changed coverage tool diff ---\n'
git show --format=fuller --unified=10 bc9f94455 -- crates/engine/src/bin/coverage_parse_diff.rs
printf '\n--- source outline ---\n'
ast-grep outline crates/engine/src/bin/coverage_parse_diff.rs --items all
printf '\n--- option parsing and provenance contexts ---\n'
rg -n -C 5 -- '--base-sha|--head-sha|struct DiffReport|head_sha|base_sha|missing.*value|usage' \
crates/engine/src/bin/coverage_parse_diff.rs
printf '\n--- related file references ---\n'
rg -n -C 3 'parse-diff\.json|DiffReport|head_sha|base_sha' \
crates/engine/src/bin/coverage_parse_diff.rs scripts .github 2>/dev/null || trueLength of output: 81965
The Major finding is addressed. The Minor finding is addressed. Your CI-scope correction is accurate. The change hardens manual and local invocation. The remaining greedy-next-token case is separate from these two findings. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/bin/coverage_parse_diff.rs`:
- Around line 365-366: Update the argument parsing branches for --base-sha and
--head-sha to reject missing, empty, and values beginning with "--" instead of
accepting them as SHA strings. Preserve valid SHA parsing, and add tests
covering empty and option-token values for both flags.
🪄 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: 8643ffe7-4c30-421c-b441-2bf6b0c57ff3
📒 Files selected for processing (1)
crates/engine/src/bin/coverage_parse_diff.rs
Co-authored-by: Lindsey Gray <lindsey.gray@gmail.com>
|
Maintainer hold for current head |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved: current head passes required CI, the parse-diff sticky is bound to this head with no card-parse changes, and the previously requested lifecycle/provenance corrections are verified.
|
🤖 AI text below 🤖 Thanks — and thanks for taking the fixup directly. Your Do not hold the merge for anything below. Two residuals survive your validation. Neither is reachable from Measured on your binary at
If you want them, I have both as a tested patch (2 tests, revert-probes run and restored byte-identically) and will open it as a separate PR against main after this lands — normalizing blank-or-whitespace to |
…nto phase-rs#6851 CI built the MERGE ref (branch + main 96e41b3) while the branch was still based on e12447f, so the census row fired in CI but not locally. Re-anchored per the row's own protocol — re-derive the SET first, prove byte-identity, then move coordinates — never relaxed. MEASURED, three independent ways: 1. Producer SET is still 5 and the partition is still 5/7/25 (total 37). Only ONE coordinate moved: game/engine.rs:10589 => :10640. The other four (game/effects/mod.rs:5918/5995/8949, scoped_library_search.rs:452) are UNMOVED. 2. All five producers re-read at their new coordinates and diffed against the pre-rebase tree at their old ones: BYTE-IDENTICAL. Negative control confirms the diff instrument discriminates — the new tree at the OLD coordinate :10589 is a bare `}`, not the producer. 3. The +51 is fully accounted for by phase-rs#6851's own insertions above this producer: `git diff -U0 e12447f 96e41b3` nets +51 above line 10589 and +51 across the whole file, so predicted 10589+51 = 10640 equals the observed coordinate exactly and phase-rs#6851 adds nothing below it. A sixth producer remains a counted event. Assisted-by: ClaudeCode:claude-opus-5
…nto phase-rs#6851 CI built the MERGE ref (branch + main 96e41b3) while the branch was still based on e12447f, so the census row fired in CI but not locally. Re-anchored per the row's own protocol — re-derive the SET first, prove byte-identity, then move coordinates — never relaxed. MEASURED, three independent ways: 1. Producer SET is still 5 and the partition is still 5/7/25 (total 37). Only ONE coordinate moved: game/engine.rs:10589 => :10640. The other four (game/effects/mod.rs:5918/5995/8949, scoped_library_search.rs:452) are UNMOVED. 2. All five producers re-read at their new coordinates and diffed against the pre-rebase tree at their old ones: BYTE-IDENTICAL. Negative control confirms the diff instrument discriminates — the new tree at the OLD coordinate :10589 is a bare `}`, not the producer. 3. The +51 is fully accounted for by phase-rs#6851's own insertions above this producer: `git diff -U0 e12447f 96e41b3` nets +51 above line 10589 and +51 across the whole file, so predicted 10589+51 = 10640 equals the observed coordinate exactly and phase-rs#6851 adds nothing below it. A sixth producer remains a counted event. Assisted-by: ClaudeCode:claude-opus-5
2 participants
🤖 AI text below 🤖
Summary
Replaces the split record-here / refresh-there route for CR 403.3 token battlefield entries with
one authoritative, resume-safe record/emit lifecycle, per @matthewevans' review. The
Suppressfinalize tail now records nothing and parks the entry on
GameState; a single consumer takes thatone owned value and performs
record_zone_change(the sole writer of both CR 400.7 ledgers) and theCR 603.6a event pair together, so a stale row, a lost emit, and CodeRabbit's duplicate
battlefield-entry row are all unrepresentable rather than guarded.
Round 3 closes the reachable trigger defect from @matthewevans' second review. The direct-return
reducer arms —
handle_tribute_choiceand 15 siblings — build theirActionResultinside the matchand never reach
apply_action's tail pipeline, so a copy token realized on those routes previouslyentered with no ETB observer ever seeing it.
apply_action_boundary_corenow converges them throughthe same
run_post_action_pipeline_fromthe tail uses. A second commit makes the CI parse-diffsticky identify the head it was generated from.
Files changed
Trigger convergence (CR 603.6a)
crates/engine/src/game/engine.rs—apply_action_boundary_coreruns the normal post-actionpipeline when the settled gate realized an entry, over the slice the realization appended
crates/engine/src/game/effects/token.rs—realize_settled_token_battlefield_entryreturnswhether it realized; the park,
flush_pending_token_battlefield_entry(sole consumer)crates/engine/src/types/game_state.rs—PendingTokenBattlefieldEntry+ the parkedGameState::pending_token_battlefield_entry;EmitCommittedCopyTokenEntryslimmed to{ object_id }crates/engine/src/game/engine_replacement.rs— the unpaused convergence pointcrates/engine/src/game/effects/counters.rs— the CR 616.1 counter-drain convergence pointcrates/engine/src/game/elimination.rs,crates/engine/src/game/scenario_db.rs,crates/engine/src/game/turns.rs— abandonment and per-turn clearscrates/engine/tests/integration/token_zone_change_index.rs— production regressionscrates/engine/tests/fixtures/cr733/authority_matrix.json.gz— the new field's write-site entryParse-diff head binding
crates/engine/src/bin/coverage_parse_diff.rs—render_markdownemits the head SHA on both thechanged and unchanged branches;
--base-sha/--head-shareject a missing value;parse-diff.jsoncarries both SHAs
Track
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
Full pipeline, every step in a fresh context. Round 3 (this review cycle): plan → 2 independent
/review-engine-planrounds → implement →/review-impl→ comment-correction pass. Plan/reviewagents ran at
xhigh, implementation agents athigh. Round-3 plan review 1 returned 6 findings(0 blocking); round 2 returned 5 (0 blocking); impl review returned 3 LOW. Every finding was fixed
with code and re-measured. A follow-up commit addresses two CodeRabbit findings on the parse-diff
provenance (below). Earlier rounds are recorded in the previous revision of this body.
Reviewers re-measured rather than trusting reports, and that caught real defects in the work: plan
review corrected three source citations that the previous round's reviewer had signed off as correct
(
engine.rs:7355-7363is theExploreChoicearm, not theCopyTargetChoiceone), and impl reviewrejected the executor's stated proof that the new
boolwas load-bearing as a non-sequitur — theprobes it cited flip pre-existing assertions — then ran a one-bit discriminator that establishes it
properly.
CR references
CR 400.7(the per-turn zone-change ledger) ·CR 403.3(battlefield-entry bookkeeping) ·CR 514.2(the cleanup step ends "this turn" state — the authority for the per-turn reset) ·CR 603.2/CR 603.2c(an ability triggers once per occurrence;turn_zone_change_indexis theengine's own key) ·
CR 603.3b(two same-controller triggers are ordered by their controller — whythe new regression asserts an ordering prompt rather than papering over one) ·
CR 603.4(Fanatic'sintervening-if) ·
CR 603.6a(ETB abilities are checked whenever an event puts a permanent onto thebattlefield — the rule the direct-return arms were violating) ·
CR 614.12a(the as-enters choice ismade before the permanent enters) ·
CR 616.1(competing replacement ordering) ·CR 702.104a/CR 702.104b(Tribute, and "if tribute wasn't paid") ·CR 704.3/CR 704.4(SBA timing) ·CR 704.5f(toughness ≤ 0, the departed-token branch) ·CR 800.4a(a player leaves the game).All grepped in
docs/MagicCompRules.txtbefore use and independently re-verified at the exact linenumbers by the impl reviewer; zero
UNVERIFIED.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— no driftcargo check --workspace --all-targets— exit 0cargo clippy --workspace --all-targets -- -D warnings— exit 0, 0 warningscargo test -p phase-engine --no-fail-fast— 18268 + 12 + 9 + 4374 + 0 = 22663 passed / 0 failed (6+2+7 ignored)cargo test -p phase-ai— 2065 passed / 0 failedParser combinator gate — N/A, no
crates/engine/src/parser/file in the diff (Gate A run anyway, below)The reachable trigger defect, measured on the path @matthewevans named
(
CopyTargetChoice → NamedChoice → TributeChoice, Soul Warden on the battlefield):bee984f80The
+1is produced byengine_priority::run_post_action_pipeline_fromat the action boundary — thesame function
run_post_action_pipelineitself delegates to — over the slice the realizationappended, not by an alternate route. Verified by reverting the new block: the regression fails at its
reach-guard first (
left: […"TributeChoice"] right: […"TributeChoice","OrderTriggers(2)"]), and withthe guard relaxed so execution proceeds, the life assertion alone fails
left: 0, right: 1— exactlythe value the review called out. The fix is arm-agnostic: the reducer match has 16 non-error direct
returns and the convergence is inert on the 15 that never park.
The regression also pins
"OrderTriggers(2)". The(2)is the discriminating part — Tribute isdeclined, so Fanatic's own CR 702.104b intervening-if is true and its ETB fires alongside Soul
Warden's; both are P0's, so CR 603.3b requires an ordering prompt. The sibling routes (Painter,
Watchdog) keep full-vector prompt assertions with no
OrderTriggerselement, so a spurious orderingprompt on those routes fails them.
Revert probes, each actually run and restored byte-identically (md5-verified), including four run
independently by the reviewer:
0 → 1are retained for CR 704.3 SBA ordering and for a counter drain that does not settle in its own action)
false⇒ fails at exactly the arm-iii assertion, nothing elsescan_from → 0⇒ nothing flips (see the disclosure below)left: (0, 0) right: (1, 1)Emitpositive control stays greenwriteln!⇒ bothcontains(head)assertions flip; move it above themarker ⇒ the marker-first assertion flips
Save/load compatibility for the
GameStatefield — measured, not assumed.GameStatecarries nodeny_unknown_fields, and the field is#[serde(default, skip_serializing_if = "Option::is_none")],so an absent key loads as
Noneand aNonevalue is never written back. Census: the repo ships11 real
{"gameState": …}dumps as.json.gzfixtures, all predating this field;pending_token_battlefield_entryoccurs 0 times in every one of them. Every test thatdeserializes one of those dumps passes: 132 passed / 0 failed, plus a serde round-trip unit test.
No compatibility shim, no pristine-dump migration.
Rebased onto
upstream/main36ac6d2a1before pushing. That range touchesengine.rs, whichthis PR modifies, so the full gate set above was re-run after the rebase rather than reused: the
rebase itself was conflict-free, and the post-rebase numbers are the ones reported here (the counts
rose from the pre-rebase run because upstream added tests).
Gate A
Gate A PASS head=bc9f9445524f90c16cbbe27ae720d644fe4da723 base=36ac6d2a1ccb4c891a20403557dfe5ac296a7022
Anchored on
run_post_action_pipeline_fromat anested-slice seam; it has five existing production call sites for exactly this problem
(
engine_payment_choices.rs:1249/1317/1339,stack.rs:2647,engine_resolution_choices.rs:543),and the new boundary call is the same shape. The
state.waiting_for = wf.clone(); result.waiting_for = wf;pair mirrors
apply_action's own post-pipeline sync verbatim — both writes are required, for twodifferent reasons:
finish_action_boundary→sync_waiting_forcopiesresultintostate, so astate-only write is undone, and the life-safety preview never calls
finish_action_boundary, so aresult-only write is insufficient
pending_liminal_entry_resume, the existingOption<…>mid-resolution park onGameStatewith#[serde(default, skip_serializing_if = "Option::is_none")];pending_token_battlefield_entrymirrors it field attribute for field attributeFinal review-impl
Final review-impl PASS head=bc9f9445524f90c16cbbe27ae720d644fe4da723
Claimed parse impact
None.
Scope Expansion
One, requested by the reviewer as required evidence. The
<!-- coverage-parse-diff -->stickycould not be made to identify the head by regenerating it, because no generator branch had ever
emitted a head SHA.
render_markdown(crates/engine/src/bin/coverage_parse_diff.rs) returned earlyfor the no-change case with the marker and
✓ No card-parse changes detected.and nothing else — nobase, no head — while the changed-cards branch printed only the baseline. Comment
5153742352didre-run for
bee984f80(updated_at 2026-08-02T06:57:35Z); its body is byte-identical to that earlyreturn. So the sticky is now head-bound on both branches. This is a class-wide provenance fix, not
a #6851 one.
That generator is ordinary crate source, so no workflow file is touched. The head is sourced from the
HEAD_SHAthe CI step already exports, with a--head-shaflag mirroring the existing--base-sha.A
git rev-parse HEADfallback would have been wrong rather than merely inelegant — that job checksout the synthetic PR merge commit, so git reports the merge SHA, never
pull_request.head.sha. Twoconstraints shaped the wording, both pinned by assertions with revert-probes:
scripts/pr_review.pyrequires the marker to stay the first line, and it classifies sticky state by the substrings
"Baseline pending"and"signature(s)", so the head line must contain neither or a no-change stickymisclassifies as real parse changes.
CodeRabbit then raised two findings on that generator, both taken. A present-but-valueless
--base-sha/--head-shasilently fell back, so a report could name a commit it does not describe;both now error. The pre-existing
--base-shaline was fixed alongside the one this branch added —leaving it lenient would put two flags in the same provenance category on different rules. The
output flags stay lenient and a test pins that asymmetry as deliberate.
parse-diff.jsoncarried noprovenance at all while both the report and the sticky send a truncated reader to open it, so it now
carries both SHAs; nothing in the repo deserializes that artifact (its four references are two
workflow write/upload paths and two prose mentions), so added keys break no consumer. Neither defect
is reachable through CI, which never passes
--head-shaand always pairs its flags with values —this hardens the local and manual surface, and is not a CI bug fix.
Two pre-existing defects found while measuring are filed separately rather than folded in: (1) a stale
CopyTargetChoiceafter a successful copy entry routes the entry event tocollect_triggers_into_deferred, swallowing ETB observers on copy-token entries (reproducesidentically with and without this diff); (2) conceding at the as-enters pause panics on unmodified
mainatelimination.rs:1069.Validation Failures
The round-2 disclosure that the Tribute two-pause class realizes after its trigger scan is retired
— that defect is fixed, not disclosed. Remaining disclosures, none blocking:
Err-restore arm of the new boundary block is unmeasured. It mirrors the existingrestore-on-
Errarm a few lines above it, but no test drives a failing post-action pipeline there.scan_fromvs0has a measured mechanism but no discriminating test. Widening to0flipsnothing today. It is still wrong:
collect_and_drain_observer_triggers_if_settlednever writesconsumed_before_priority_trigger_events, and the exclusion filter coversZoneChangedonly, soevery non-
ZoneChangedobserver event it already collected would be collected twice.event_start = scan_from, the direct-returnhandler's own events — from
effects::tribute::apply_paid, and fromresume_pending_continuation_if_priority, which can resolve a whole ability chain — still get noCR 603.2 scan. That was true before this change too; it is not a regression, but it means "converges
through the normal pipeline" is exact about the realized entry and not about everything the handler
emitted.
TributeChoiceshape.abandon_source_bound_resolution_promptclears the park, but its gate matches onlyWaitingFor::NamedChoiceandWaitingFor::OpponentGuess, so the Painter sibling is safe byconstruction and Tribute is not. Unmeasured; adjacent to the separately-filed concede panic above.
apply_interaction_pre_reconciliation_for_life_safetydocuments itself as "exactly the reducer portion" and returns without
finish_action_boundary; on aparked-entry route it would now also run the pipeline. Not reachable today — that root cannot
produce a parked copy-token entry, and it operates on a clone.
.github/workflows/**, which this PR treats as a hard stop. (a)ci.yml's baseline-not-publishedfallback body names the base only; adding the head there needs one printf argument. (b) When the
engine-source hash is unchanged the step sets
produced=false, so no artifact is uploaded and thesticky silently retains its previous head's body — no generator change can bind a comment that was
never regenerated. Offered as text in the review thread for a maintainer to apply.
A note on the required-evidence item. The sticky names the head it was generated from, so landing
these commits necessarily moves it: the regenerated comment will identify the new head, not
bee984f80. A literal identity check againstbee984f80…is unsatisfiable by construction; thesubstantive requirement — that the artifact be bound to a head — is met.
CI Failures
None.
Pre-existing and untouched:
CR733_CENSUS_STRICT=1fails oncensus.site_count(pinned 3597; acensus run on the pristine pre-rework tree
057d80a65gives 3669), so that pin was already stalebefore this rework. The always-on
cr733_authority_matrix_covers_the_fresh_write_censusgate isgreen. No
GameStatefield is added in this round, so the matrix is unchanged.Summary by CodeRabbit
Bug Fixes
Tools