test(ai): make the retarget slot-legality rows discriminate in both directions - #7480
Conversation
…irections Follow-up to #7477, answering its review. Row 2f proved the per-slot authority REJECTS a pool member legal only for another slot, but could not prove it ADMITS a legal change: its surviving candidate equalled the current slot-0 target, so it passed through `retarget_slot_violation`'s unchanged-position exemption. A generator that dropped every changed proposal would still have passed it. Closing that needs three players, not a third pool member. Slot 0's filter is "an opponent of P0", which at two players admits exactly P1 — and P1 is the current target, so no legal change exists to offer. Row 2g uses a 3-player scenario and, more importantly, keeps the current slot-0 target OUT of the pool, so no exempt non-change is available and the only candidate that can survive is a genuine slot-0-legal change. A drop-guard asserts that absence, so putting the current target back fails the row loudly instead of quietly reverting it to proving half of what it claims. Row 2h pins the `None` contract that #7477 deliberately introduced. Under `Single` scope an empty enumeration means every pool member fails the per-slot check, and `apply_retarget` would reject any submission built from that pool, so the fallback refuses rather than laundering an engine gap into an AI retry loop. Nothing pinned that, so a future change could have silently restored a rejected submission. Its negative assertion carries a positive control that the pool is non-empty, so `None` means "every candidate was filtered out" and never "there was nothing to filter". Verified by perturbation rather than by passing: removing the `slot_legal` filter from the generator's `Single` arm reds all three slot-legality rows and leaves row 2b — which does not test slot legality — green. Also drops a false card attribution the earlier PR left behind because it sat outside that change's frozen scope. Redirect reads "You may choose new targets for target spell", which is the CR 115.7d `RetargetScope::All` template, not "change the target of". Bolt Bend and Misdirection are correct there.
|
Warning Review limit reached
Next review available in: 5 minutes Limit details: You’ve used all 2 included reviews currently available under your plan. 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)
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 |
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
1 participant
Summary
Follow-up to #7477, answering its review. Makes the retarget slot-legality rows discriminate in both directions, pins the
Nonecontract #7477 deliberately introduced, and drops a false card attribution that PR left behind because it sat outside its frozen scope.Files changed
crates/phase-ai/tests/retarget_fallback_action.rs— rows 2g (admit path) and 2h (Nonecontract)crates/engine/src/types/ability.rs— one-word comment fix: Redirect is not a "change the target of" cardTrack
Developer
LLM
Model: claude-opus-5[1m]
Tier: Frontier
Thinking: high
Implementation method (required)
Method: not-applicable — test-only additions plus a one-word comment correction; no engine behaviour changes. The engine change these rows cover shipped via
/engine-implementerin #7477.CR references
No new CR annotations; both were grepped in
docs/MagicCompRules.txt.Verification
At head
1c828d88879ee52e21e0f16e6f40e4a9c0bcdf35:cargo test -p phase-ai --test retarget_fallback_action— 4 passed, 0 failed (2 existing + 2 new)cargo clippy -p phase-ai --all-targets -- -D warnings— exit 0cargo fmt --all— clean./scripts/check-parser-combinators.sh— Gate A PASSVerified by perturbation, not by passing. Removing
.filter(|new_targets| slot_legal(new_targets))from the generator'sSinglearm (perturbation confirmed applied: match count 1 → 0 before running) produces:fallback_multi_role_retarget_action_is_slot_legal(2f)fallback_multi_role_retarget_admits_a_legal_slot_change(2g, new)fallback_multi_role_retarget_yields_none_when_no_pool_member_is_slot_legal(2h, new)fallback_retarget_action_is_legal(2b)The last row is the point: the perturbation reds exactly the rows that claim the property and leaves the one that does not. A perturbation that reds everything would prove far less. The file was restored and verified clean afterwards.
Gate A
Gate G PASS (router/grant architecture: strict router vs permissive grant boundary intact)
Gate A PASS head=1c828d88879ee52e21e0f16e6f40e4a9c0bcdf35 base=a3565f7fd2eefe57ab6ee48bd6adac00136bad7b
Anchored on
crates/phase-ai/tests/retarget_fallback_action.rs:156— row 2f, whose fixture shape and two structural reach-guards row 2g reuses via the extractedpark_multi_role_retargethelpercrates/engine/tests/integration/love_on_the_battlefield_combat_counters.rs:145— existingGameScenario::new_n_player(3, ..)usage; the 3-player scenario is an established pattern, not a new oneFinal review-impl
Final review-impl PASS head=1c828d88879ee52e21e0f16e6f40e4a9c0bcdf35
These rows answer CodeRabbit's review of #7477 directly. Row 2f's one-sidedness and the unpinned
Nonecontract were its two findings; both are closed here, and the fix was validated by the perturbation table above rather than by the rows passing.Claimed parse impact
None. No parser file is touched; the only non-test change is a comment.
Scope Expansion
The
types/ability.rscomment fix is intentionally included. It is the third instance of the same false Redirect attribution; #7477 fixed two and could not fix this one without adding a tenth path to a frozen scope. Recording it there rather than sweeping it silently is what made it findable now.Validation Failures
None.
CI Failures
None.