test(engine): pin occurrence-exactness of the queued-context witness

[matthewevans]

Follow-up to #7058, addressing the CodeRabbit review comment left on it.

The finding, and why its remedy is rejected

The comment asked to carry an action-scoped occurrence identifier through PendingTriggerContext and ConsumedTriggerEventOccurrence, so that two equal-valued GameEvent::ZoneChanged entries in one collector slice are consumed as separate occurrences (CR 603.2c). It was filed as Major / Heavy lift.

The finding names a real property worth guaranteeing. Its remedy is rejected on three independent grounds:

1. The identity already exists. ZoneChangeRecord::turn_zone_change_index is a per-turn index assigned by restrictions::record_zone_change — its sole production grower — and it is a compared field inside GameEvent's derived PartialEq. Two ZoneChanged values from distinct occurrences therefore cannot be byte-identical. Two that are byte-identical denote one occurrence emitted twice, which is precisely what filter_already_collected_trigger_events_from should collapse.

2. The evidence was a test fixture. The comment reasoned from ZoneChangeRecord::test_minimal, which leaves the index at 0 for both events. That is a property of the fixture, not a production contract. All four production sites that construct a ZoneChanged into an event buffer ship the allocator's value.

3. The remedy would regress CR 104.4b. deferred_triggers is compared inside GameState::eq. Injecting a fresh per-instance id there makes two otherwise-identical loop iterations compare unequal — the mandatory-loop draw never fires, the engine burns auto_pass_loop_max_iterations, and a rules-correct draw degrades into a CR 732.2 halt. This is the same shape pip_id on ManaUnit caused. The sibling ledger consumed_before_priority_trigger_events is safe only because it is excluded from equality — an exclusion deferred_triggers does not have.

What this change does instead

Converts the shipped doc comment's asserted premise into an enforced one. No production code changes. GameState::eq, normalize_for_loop and loop_states_equal are untouched, and ZoneChangeRecord keeps its derive — keeping it is the mechanism, so CR 104.4b is discharged vacuously.

Three regression rows, each verified to fail for its stated mechanism under a targeted falsifier, not merely to fail:

Row	Pins	Red when
U5	the equality link at the filter authority itself	ZoneChangeRecord's PartialEq is hand-written to skip the index
U6	the within-Library reposition family at the real production mover (CR 400.7 zero-bump; per CR 701.20b a reveal is not a zone change)	the zones.rs library-insert stamp is zeroed
N5	allocator→event fidelity across the search-delivery park path	the zones.rs ordinary-arm stamp is zeroed

N5 asserts pre-drain by design and must not pass priority — a pass runs the drain, empties the queue and makes the assertion vacuous. It is guarded by assert_observers_were_parked, which requires a non-empty queue, so a wrongly-shaped slice fails loudly rather than passing silently.

Also corrects the harness note in search_delivery_observer_dedup.rs, whose universal was already inaccurate before this change (N4 asserts on deferred_triggers without passing priority). It is now scoped to park-path rows that assert what the parked observers did.

Verification

• Tests: 18,559 lib + 4,598 integration, zero failures. Clippy -D warnings exit 0. Run in an isolated target dir at the candidate commit — Tilt watches a different checkout and was not cited.
• Parse impact: none. Base and candidate card-data.json are byte-identical (a25d3944…, 99,502,331 bytes both sides), produced by two genuinely distinct engine builds from the same pinned AtomicCards.json. Non-degenerate: 35,657 rows, 35,286 with non-empty parse_details on both sides.
• All CR citations (400.7, 603.2c, 701.20b, 701.30a, 724, 730.3c) grepped against docs/MagicCompRules.txt.

Escalated, not fixed here

Four engine defects surfaced while establishing the above. Each is filed rather than folded in:

• engine: library-destination ChangesZoneAll triggers over-fire on within-library repositions, and the zone-change ledger is polluted (CR 400.7 / CR 701.20b) #7063 — library-destination ChangesZoneAll observers over-fire on within-library repositions, and the zone-change ledger is polluted (so gating the matcher alone would be the wrong fix)
• engine: EffectZoneChoice SelectCards arm accepts duplicate card ids — five sibling validators reject them #7064 — the EffectZoneChoice SelectCards arm accepts duplicate card ids, while five sibling validators reject them; duplicates traverse the multiplayer transport intact
• engine: deferred_triggers survives the turn boundary while turn_zone_change_index resets — cross-turn occurrence-witness aliasing #7065 — deferred_triggers survives the turn boundary while turn_zone_change_index resets (cross-turn occurrence-witness aliasing)
• engine: deserialized states bypass the zone-change occurrence allocator — turn_zone_change_index defaults to 0 on restore #7066 — deserialized states bypass the occurrence allocator; the index defaults to 0 on restore

Plus #7068, a measurement-tooling defect found by this work: coverage-parse-diff can report a spurious oracle_changed and silently skip cards due to nondeterministic array ordering colliding on lowercased names.

Known bounds, stated rather than implied

• The occurrence-exactness proof is scoped within one turn, which is what the basis supports; engine: deferred_triggers survives the turn boundary while turn_zone_change_index resets — cross-turn occurrence-witness aliasing #7065 is the cross-turn residual and engine: deserialized states bypass the zone-change occurrence allocator — turn_zone_change_index defaults to 0 on restore #7066 the deserialization one, both named in the doc block itself.
• The parse-diff result bounds parse change over the surface coverage-report projects into parse_details. Replacement-effect details are known not to be projected there, which is why the byte-identity of card-data.json — not the comparator's clean verdict — is the load-bearing evidence.

Summary by CodeRabbit

• Bug Fixes

  • Improved tracking and deduplication of repeated zone-change events.
  • Ensured identical events can be distinguished by their occurrence order.
  • Corrected handling of repeated within-library repositioning and searched cards delivered together.

• Tests

  • Added regression coverage for queued-trigger consumption, library repositioning, and search delivery.
  • Expanded documentation of known edge cases and validation scenarios.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: cb4fea0f-3d7d-41dc-9b93-5d0c8e04f926

📥 Commits

Reviewing files that changed from the base of the PR and between 8c6e1d1 and 25499ef.

📒 Files selected for processing (4)

• crates/engine/src/game/triggers.rs
• crates/engine/src/game/triggers_dedup_regression_tests.rs
• crates/engine/src/game/zones.rs
• crates/engine/tests/integration/search_delivery_observer_dedup.rs

---

📝 Walkthrough

Walkthrough

The change documents ZoneChanged occurrence identity and adds regression coverage for queued-witness consumption, repeated library repositioning, and multi-card search delivery. Tests verify turn_zone_change_index uniqueness and correspondence with zone-change ledger records.

Changes

Zone-change occurrence exactness

Layer / File(s)	Summary
Occurrence-specific deduplication semantics crates/engine/src/game/triggers.rs, crates/engine/src/game/triggers_dedup_regression_tests.rs	The documentation defines occurrence identity, production emission paths, known residual gaps, and queued-witness consumption. The regression test confirms that a witness consumes only its named ZoneChanged occurrence.
Library reposition occurrence validation crates/engine/src/game/zones.rs	The regression test verifies that repeated same-card library repositions emit distinct records identified by consecutive turn_zone_change_index values without changing incarnation or battlefield-entry identity.
Search delivery occurrence validation crates/engine/tests/integration/search_delivery_observer_dedup.rs	The integration test verifies distinct indices for two lands delivered by one Harrow search, shared indices for repeated observers of one land, and matching ledger records. The harness documentation identifies pre-drain and priority-pass test paths.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

• engine: deferred_triggers survives the turn boundary while turn_zone_change_index resets — cross-turn occurrence-witness aliasing #7065 — Relates to cross-turn turn_zone_change_index reuse in deferred triggers.
• engine: deserialized states bypass the zone-change occurrence allocator — turn_zone_change_index defaults to 0 on restore #7066 — Relates to deserialization gaps affecting occurrence exactness and trigger deduplication.

Possibly related PRs

• phase-rs/phase#6881 — Addresses duplicate search-generated ZoneChanged observer triggers and occurrence-specific deduplication.
• phase-rs/phase#7012 — Covers turn_zone_change_index assignment and ZoneChanged ledger semantics.
• phase-rs/phase#7058 — Extends ZoneChanged deduplication logic and occurrence-specific regression tests.

Suggested labels: enhancement

Suggested reviewers: lgray, andriypolanski, jacobwoodson

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the regression coverage added for occurrence-exactness of queued-context witnesses.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch ship/pin-occurrence-exactness-queued-context-witness

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ast-grep (0.45.0)

crates/engine/src/game/triggers.rs

ast-grep timed out on this file

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Generated for head 00ecbc560eb0bbfb073f8212beebef34c6fb58ef.

Parse changes introduced by this PR

✓ No card-parse changes detected.