ship/ai perf 6826 targeted exchange - #7049
Conversation
…dition The AI decision-cost regression bisected to #6826: `root_action_is_allowed` re-ran a full `validated_candidate_actions_for_semantic_owner` pass per action over a list that was itself that enumeration's output, `replay_exact_candidate` enumerated a second time, and the cheap shape guard ran after the clone, so it saved nothing. Add `root_may_yield_adverse_exchange`: a clone-free, allocation-free predicate read from live `GameState` before any enumeration or clone. It is deliberately over-approximating — `true` means "cannot prove non-rejection", and Step 5 wires it as an early-out, so it can only skip preview work and can never lose a `Reject`. Its entry set is the union of four ability authorities (`base_abilities`, `abilities`, `back_face`, `cleave_variant`) so a cast-time face or text swap cannot install a shape the guard did not already walk (CR 601.2a, CR 712.11b / 715.3a / 720.3a, CR 702.148b + CR 612). A `layers_dirty` rail falls open on a pending layer-6 grant (CR 613.1f, CR 704.3). Reorder `root_action_is_allowed` to consult the guard before enumerating. Extract the wildcard-free `AbilityDefinition`/`Effect` traversal out of `game/printed_cards.rs` into `types/ability_visit.rs`, parameterized by a visitor, so the conjure-name walk and the shape predicate share one authority and a newly added carrier fails both fixtures instead of silently escaping one.
…ion abilities `root_may_yield_adverse_exchange` answers from the source object's STORED ability lists, but the binder composes STORED ⊕ SYNTHESIZED. `casting::activation_ability_definition` resolves an `ability_index` at or past `obj.abilities.len()` from four families that are synthesized on read (`runtime_granted_cycling_abilities` and its three siblings) and written into no `GameObject` field, so the guard was answering an activation from lists that provably cannot contain the definition being bound. The contract is that `false` PROVES the verdict cannot be `Reject`; it held only because none of those four families carries `Effect::Fight` or target-sourced damage today — a payload accident, not a proof. Fall open when `ability_index >= source.abilities.len()` (CR 602.2b). The rail holds regardless of what those families come to synthesize, which is what the Activation branch needs: the payload is card-data-driven through `database/synthesis.rs`, so it cannot be discharged by reading one function. Tighten `RootBinding::matches_pending`'s `Cast` arm to require `activation_ability_index.is_none()`, mirroring the `Activation` arm. Without it a cast root authenticated against an activation-sourced `PendingCast` and was judged against an activated ability's spine. Document the basis mismatch as falsifier clause (b2), enumerating all three synthesis seams with the disposition of each, and state the payload-dismissal criterion that separates them: dismissing a seam on its payload is admissible only when that payload is closed by reading one bounded, non-data-driven function in full (awaken rider), never over a data-driven surface where the only available negative is a grep (the runtime-granted families). Replace the audit instrument — "what do these functions RETURN" cannot see an append that happens ~950 lines after the return — with "what does the ability look like where `PendingCast` is constructed". Add a back-reference on `build_awaken_rider` so the coupling is discoverable from the site whose payload would have to change. Restate the leaf/judge superset invariant at all three sites: a new `Effect` variant cannot create a `Reject` (both judges hard-match named variants), but widening either judge without widening the leaf silently narrows the guard below the reject set. Tests pin the index boundary rather than a card, because no current runtime-granted payload is adverse and a card fixture would stay green with the rail deleted.
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds complete ability traversal, clone-free adverse-exchange detection, bounded targeted-exchange previews, and AI search prefiltering. It also replaces printed-card traversal code and adds coverage for nested abilities, exchange shapes, and preview budgets. ChangesAdverse exchange evaluation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant phase_ai as phase-ai search
participant guard as root_may_yield_adverse_exchange
participant state as GameState
participant exchange as targeted_exchange_verdict
phase_ai->>guard: inspect cast or activation root
guard->>state: traverse ability definitions and effect carriers
guard-->>phase_ai: adverse-exchange shape result
phase_ai->>exchange: evaluate targeted exchange when shape exists
exchange->>state: enumerate candidates and replay bounded previews
exchange-->>phase_ai: targeted exchange verdict
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/engine/src/game/printed_cards.rs (1)
842-863: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep a compile-time check on the name-extraction leaf.
The traversal moved to
types::ability_visit, which keeps itsEffectmatch wildcard-free. This leaf now uses_ => {}. A futureEffectvariant that carries a static card name will therefore compile silently and never be seeded intocard_face_registry. The paired fixturewalker_covers_every_nested_carrierpins carriers, not name-bearing leaves, so nothing fails.Group the current no-name variants explicitly, or add a small exhaustive helper that classifies "does this effect name a card", so the compiler forces the decision here as it does in
ability_visit.As per coding guidelines: "wildcard
_match arms where the enum is known and an exhaustive match would let the compiler catch missing variants".🤖 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/game/printed_cards.rs` around lines 842 - 863, Update collect_conjure_names so its Effect match explicitly lists every current no-name variant instead of using the wildcard arm, preserving the existing Conjure and Meld handling. Keep the match exhaustive so adding a future name-bearing Effect variant produces a compile-time error requiring it to be classified and seeded.Source: Coding guidelines
crates/engine/src/ai_support/targeted_exchange.rs (1)
104-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the guard coupling by symbol, not by file position. Both documents encode a load-bearing cross-file contract as
file.rs:<line>citations. An unrelated edit incasting.rs,zones.rs,splice.rs,stickers.rs,overload.rs, orlayers.rssilently retargets those pointers, and the audit procedure the guard doc prescribes then reads the wrong code. Cite the enclosing function or item name in both places, and keep a line range only where the exact span matters.
crates/engine/src/ai_support/targeted_exchange.rs#L104-L298: replace the roughly thirty positional citations (casting.rs:497,casting.rs:6938-6942,casting.rs:11028-11039,zones.rs:482,zones.rs:508-513,stickers.rs:490,game/layers.rs:2028,splice.rs:145,overload.rs:99-108, and the rest) with the function or item names they refer to.crates/engine/src/game/effects/awaken.rs#L49-L50: replacecasting.rs:6938-6942with the name of the casting function that appends this rider.🤖 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/ai_support/targeted_exchange.rs` around lines 104 - 298, Replace positional file-and-line citations in the targeted_exchange_verdict guard documentation with the enclosing function, method, or item names they identify, retaining line ranges only when an exact span is essential; update every cited reference throughout targeted_exchange.rs, including casting, zones, splice, stickers, overload, layers, and related symbols. In awaken.rs, replace the casting.rs line citation with the name of the casting function that appends the rider. Apply the documentation-only change at both specified sites and preserve the existing cross-file contract details.
🤖 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/ai_support/targeted_exchange.rs`:
- Around line 104-298: Replace positional file-and-line citations in the
targeted_exchange_verdict guard documentation with the enclosing function,
method, or item names they identify, retaining line ranges only when an exact
span is essential; update every cited reference throughout targeted_exchange.rs,
including casting, zones, splice, stickers, overload, layers, and related
symbols. In awaken.rs, replace the casting.rs line citation with the name of the
casting function that appends the rider. Apply the documentation-only change at
both specified sites and preserve the existing cross-file contract details.
In `@crates/engine/src/game/printed_cards.rs`:
- Around line 842-863: Update collect_conjure_names so its Effect match
explicitly lists every current no-name variant instead of using the wildcard
arm, preserving the existing Conjure and Meld handling. Keep the match
exhaustive so adding a future name-bearing Effect variant produces a
compile-time error requiring it to be classified and seeded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 46679e6c-d798-45b1-8c29-c6dd4c3a03ac
📒 Files selected for processing (9)
crates/engine/src/ai_support/mod.rscrates/engine/src/ai_support/targeted_exchange.rscrates/engine/src/game/effects/awaken.rscrates/engine/src/game/printed_cards.rscrates/engine/src/types/ability_visit.rscrates/engine/src/types/mod.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/targeted_exchange_preview_budget.rscrates/phase-ai/src/search.rs
The falsifier and audit-instrument prose encodes a load-bearing cross-file contract. Bare `file.rs:<line>` citations silently retarget when an unrelated edit shifts lines in `casting.rs`, `overload.rs`, `specialize.rs`, or `game_object.rs`, and the audit procedure then reads the wrong code — the same failure mode the comment itself dissects two paragraphs earlier. Replace the bare positional citations with the enclosing symbol (`prepare_spell_cast_with_variant_override_inner`, `overload::transform_effect_in_place`, `specialize::specialize_permanent`, `printed_cards::apply_back_face_to_object`, `CleaveFormState`, `GameObject::specialize_faces`, `awaken::append_awaken_rider`). Citations that already named their symbol keep their line as a hint. Raised by CodeRabbit on PR #7049. Documentation only; no behavior change.
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
…#6826 cost (phase-rs#7053) phase-rs#7044 refreshed the baseline onto the phase-rs#6826 regression itself (layers_full_eval 3495 -> 15877). Once phase-rs#7049 fixed that regression the gate carried ~3x slack -- real cost 5464 against a 15877 baseline under a ~6% band -- so it could no longer catch a new decision-cost regression. Re-measured on a clean worktree against an immutable card-data snapshot (scoped card_data_hash 670a4a14, covering 46/46 scenario deck cards): counter | old | new | threshold layers_full_eval | 15877 | 5464 | 5801 state_clone_for_legality | 19342 | 11099 | 11717 restriction_static_mode_gate_scans | 155747 | 110891 | 116499 sba_battlefield_snapshot_builds | 27462 | 19626 | 20671 scripts/validate-ai-perf-reproducibility.sh: PASSED (margin+band). 25/25 band runs clean, "0 OVER-MARGIN of 29 counters", and worst_current == baseline on every counter across 125 cold processes. CI budget: T_run_max 90s * 2.5 + T_build ~630s = ~14.3 min < 25 min. (The script reported T_build=1s from a warm cache; the 630s figure is the cold isolated server-release build measured separately, which is the conservative ceiling the check intends.) git_sha stamps 7f5f0b4, where the 25-run validation ran. Re-confirmed against f26f4e3 (current main, including phase-rs#7051 and phase-rs#7017): 0 FAIL, 29 PASS, +0 on every counter. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
1 participant
Summary by CodeRabbit