perf(engine): let a priority pass skip the legality clone - #6977
Conversation
`SimulationFilter` validated `PassPriority` by cloning the whole `GameState` and running `apply_interaction_for_simulation` on it — a throwaway simulation of the very dispatch the caller was about to perform for real. At a priority window `legal_actions_full` also builds a `PriorityCastProbe`, which clones again and flushes layers even when clean. Two full clones to answer a question CR 117.3d makes near-constant: may this player pass? phase-ai's forward projection pays that at every step. Measured in the AI gate (dev profile, Medium, `--games 3`): 371/371 projections exceeded the 15ms wall-clock cap, i.e. the evasion policy's velocity term produced no signal at all; with the cap lifted, 48,019 loop iterations and one projection at 22.97s. Extract the reducer's own two guards into `priority::pass_priority_legality` (CR 723.5 seat-vs-submitter, CR 732.2c divergence latch) and add `pass_priority_structurally_legal`, which pairs that authority with a refusal on the two parked-continuation fields. Three call sites delegate to it and none restates it: the reducer arm, a fifth `SimulationFilter` structural hatch alongside the four #6973 shipped, and a `resolve_choice` fast path that emits the pass without enumerating. The hatch is sound in the direction that matters — it accepts only what the authoritative simulation accepts — for every pre-boundary rejection path. Mid-boundary is bounded but not closed: the pass boundary uniquely runs the CR 117.4/608 resolution seam, so resolution can park a continuation the fallible drains then read. Every such park coincides with installing a live non-Priority prompt, and both drains re-test `waiting_for == Priority`, so the park is skipped and caught by the field gate at the next window. One link in that chain is documented rather than proved, and no test in this change can falsify it; the residual and its scope note ship in the test's doc comment so a green T4b is never misread as evidence about it. Note `PassPriority` is the only hatched action that reaches the resolution seam — the #6973 precedent supplies the shape here, not the safety argument.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesPassPriority structural legality
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AIProjection
participant Priority
participant Engine
participant Simulation
AIProjection->>Priority: check structural PassPriority legality
alt structurally legal
Priority-->>AIProjection: legal
AIProjection->>Engine: return PassPriority
Engine->>Engine: apply accepted pass
else unsupported or illegal
Priority-->>AIProjection: fallback required
AIProjection->>Simulation: enumerate and simulate actions
Simulation-->>AIProjection: projected choice
end
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/ai_support/filter.rs`:
- Around line 1529-1546: Update pass_oracle_accepts to create and hold the same
SimulationProbeGuard used by SimulationFilter::fallback_simulation before
calling apply_interaction_for_simulation, preserving the existing actor and
semantic-owner resolution and result handling.
- Around line 219-238: Correct the CR annotations in the invariant comments: in
crates/engine/src/ai_support/filter.rs lines 219-238, remove or update the CR
117.4 statement so it does not attribute continuation drains or trigger-target
processing to that rule; in crates/phase-ai/src/projection.rs lines 316-338,
replace the CR 601.2g–h citation with CR 118.3b, CR 119.4, and CR 616.1,
matching the continuation rules used by pass_priority_structurally_legal.
In `@crates/phase-ai/src/projection.rs`:
- Around line 339-340: Update the match in the priority-handling logic to borrow
state.waiting_for by reference instead of moving it from the shared GameState
borrow, and dereference the matched player value before passing it to
priority::pass_priority_structurally_legal.
🪄 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: 49cf3317-0daf-4ef3-9ae7-d0456cdd4562
📒 Files selected for processing (6)
crates/engine/src/ai_support/filter.rscrates/engine/src/game/engine.rscrates/engine/src/game/priority.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/pass_priority_structural_legality.rscrates/phase-ai/src/projection.rs
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
… cites `pass_oracle_accepts` is the reference implementation every pass-hatch soundness assertion compares against, but it omitted the `SimulationProbeGuard` that `fallback_simulation` enters before applying the candidate. Its doc comment claimed parity "minus the perf counter", which was false. The guard suppresses top-level loop reconciliation and ring accumulation, so an oracle without it can return a verdict production legality filtering never produces — leaving the comparisons green while constraining nothing. Enter the guard in the same position and say in the doc comment why the parity is load-bearing. Also correct two CR annotations. CR 117.4 covers an all-pass succession resolving the top of the stack or ending the phase or step; it was carrying attribution for the continuation drains and trigger-target selection as well, which are consequences of the resolution it starts. Attribute those to CR 608.2 and CR 603.3d respectively. In the projection fast path, cite both parked fields' own `GameState` annotations rather than one: CR 601.2h for `pending_cost_move_resume`, CR 118.3b + CR 119.4 for `pending_deferred_life_cost_resume`. No behavior change outside `#[cfg(test)]`; the rest is comments.
Gate status: both AI-gate failures are pre-existing and tree-wideEverything else on this PR is green. The two red checks are the pair #6967 tracks, and neither is currently capable of passing on any branch. Evidence below, and I have not refreshed any baseline — that diff is the maintainer's call. Paired-seed AI gate — a job timeout, not a verdictRan 1h0m17s and ended on The nightly variant of the same suite is given Same job, same outcome on unrelated branch Decision-cost perf gate — comparing against a baseline from a different card pool8 FAIL / 21 PASS. The gate diagnoses itself in its own output:
Identical failure on the unrelated branch above: same 8 FAIL / 21 PASS, same self-diagnosis note. What the counters say about this changeComparing this PR's run against that unrelated branch's run, 19 of 29 counters are byte-identical — including
The targeted counter is down ~16%, and the two counters that fall with it are the per-clone downstream work — an avoided legality clone is also an avoided SBA snapshot build and legend-rule gate scan. The −3031 appearing twice is the same avoided clones counted at two sites. Stated honestly: this is a cross-branch comparison, not a paired base-vs-candidate measurement. The two runs have different card data and different code beyond this change, so the direction is consistent with the change but is not proof of its magnitude. The proof-grade evidence for this PR is the unit level — |
1 participant
Refs #6967 — the projection cost measured here is one input to that gate's wall-clock failures; this does not on its own close it.
What
SimulationFiltervalidatedPassPriorityby cloning the wholeGameStateand runningapply_interaction_for_simulationon it — a throwaway simulation of the very dispatch the caller was about to perform for real. At a priority windowlegal_actions_fullalso builds aPriorityCastProbe, which clones again and flushes layers even when they are clean. Two full state clones, to answer a question CR 117.3d makes near-constant: may this player pass?This adds a fifth structural hatch alongside the four #6973 shipped, so a pass is decided without the clone.
Why now
phase-ai's forward projection pays that cost at every step. Measured in the AI gate (dev profile, Medium,--games 3):371/371 means the evasion policy's velocity term produced no signal at all —
project_tois not an anytime search, so a truncated projection returnsErrand scores 0.0. The wall clock was not protecting a working feature; it had reduced the feature to pure overhead, and lifting it made the underlying cost visible rather than creating it.Design
One authority, three delegating call sites, none of which restates the judgement:
game::priority::pass_priority_legality— the reducer's own two guards, extracted: CR 723.5 seat-vs-submitter, and the CR 732.2c divergence latch.game::priority::pass_priority_structurally_legal— that authority plus a refusal on the two parked-continuation fields.(Priority, PassPriority)reducer arm, the newSimulationFilterhatch, and aresolve_choicefast path that emits the pass without enumerating.An earlier revision expressed the field gate separately in the hatch and the projection. That is exactly the drift this shape exists to prevent, and review caught it: the projection gated on
classify_payment_continuation's verdict while the hatch gated on the fields, and those are not equivalent —classifyreturnsNotAffiliatedat six sites where a field is stillSome.Correctness
The hatch must never accept what the authoritative simulation would reject (
ai_support/filter.rs). Every pre-boundary rejection path is closed: the reducer's two guards, actor authorization, and the two fallible continuation drains, each gated on a field the predicate refuses on.One residual is bounded but not closed, and ships documented. The pass boundary uniquely runs the CR 117.4/608 resolution seam — no cast boundary does — so resolution can park a continuation that the fallible drains then read after the predicate ran. Every such park coincides with installing a live non-
Priorityprompt, and both drains re-testwaiting_for == Priority, so the park is skipped and caught by the field gate at the next window. One link in that chain is read from documentation rather than proved, and no test in this change can falsify it. The scope note saying so is in the test's own doc comment, not only in the planning docs, so a green T4b is never misread as evidence about it. Follow-up unit is named in the plan.Worth stating plainly: #6973 supplies the shape here, not the safety argument.
PassPriorityis the only hatched action that reaches the resolution seam.Tests
15 tests. Every discriminating one was watched go red against a deliberately broken copy rather than assumed to fail.
assert_eq!onperf_countersintegers. Nothing timing-based anywhere —grepthe diff forDuration|Instant|elapsed|sleepand the only hits areCoreType::Instantand a card name. A wall-clock assertion is the defect this change exists to fix.PlayLand, which has no structural hatch and would otherwise incrementstate_clone_for_legalityand fail those tests with a correct implementation.T4b's resolving fixtures assert two independent observables — stack depth decreasing and CR 608.2n graveyard arrival — so a fixture degrading into a bare priority handoff fails on both.Verification
cargo clippy -p phase-engine -p phase-ai --all-targets -- -D warnings: 0 warningscrates/engine/src, so three changed files flip it even though no parser file is touched): 0 clusters, and base/candidatecard-data.jsonare byte-identical from separately built binaries.Unplanned edit, flagged
the_cr_603_5_prompt_census_is_pinned_so_a_sixth_producer_is_a_counted_eventpins five prompt-producer sites by absolutefile:line. Extracting the reducer arm shortened the file by 7 lines above one of them, movingengine.rs:11427→:11420. Repaired via the procedure that test's own doc comment mandates, with a drift-log entry: the line is byte-identical by sha256 across the shift, the only hunk above it accounts for exactly −7, and the census's own producer-count partition asserts would have caught a real set change independently of the coordinate.Adjacent defect found, NOT fixed here
coverage-parse-diffhas a latent masking channel.coverage-report'scardsarray order is nondeterministic between runs, and 30 lowercased card-name keys are duplicated in the corpus; the comparator keys aBTreeMapby lowercased name, so last-wins resolves a colliding key to a different card per run. It surfaced here as a spuriousoracle_changed: 1, disproved by the byte-identical normalized payloads. Pre-existing, present identically on both sides, not introduced by this change — but on a future change a real parse regression on a name-colliding card could be silently carved out the same way. Deserves its own issue.Summary by CodeRabbit
Performance Improvements
Bug Fixes
Tests