feat(phase-ai): pick the least-costly land for a self-bounce, never the loop - #6679
Conversation
…he loop Closes phase-rs#4730. A "return a land you control" ETB (Azorius Chancery and the rest of the Ravnica/MOM bounce-land cycle) forces the controller to return one of their own lands at resolution (CR 608.2c). No policy scored that choice, so the AI kept returning the bounce-land it had just played — replaying and re-bouncing it every turn, the tempo loop reported in phase-rs#4730. `land_sequencing` fixed the play-sequencing half and explicitly deferred this half to "its own target-selection policy"; this is that policy. New `policies/self_bounce_target.rs` (SelfBounceTargetPolicy): - Fires only on the resolution-time land bounce — a WaitingFor::EffectZoneChoice returning battlefield lands you control to hand — leaving value self-bounce of creatures (blink, save-from- removal) to the value policies. Structural, no card names. - Scores each candidate land by how little returning it costs: the ability's own source (the just-played bounce-land) takes a strong penalty so it loses to any other land (CR 608.2c loop guard); an already-tapped land is preferred (its mana is spent this turn); an untapped land is mildly penalized but still beats looping the source. Covers the whole bounce-land class, not one card. Tests: 4 new self-bounce tests (pure return_desirability ordering + composed verdict over a real EffectZoneChoice/SelectCards context) + full 1537-test phase-ai lib suite pass; clippy -p phase-ai --all-targets -D warnings clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
superagent-security
Bot
added
the
contributor:flagged
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
|
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 (1)
📝 WalkthroughWalkthroughAdds ChangesSelf-bounce target selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AiDecisionContext
participant PolicyRegistry
participant SelfBounceTargetPolicy
AiDecisionContext->>PolicyRegistry: route EffectZoneChoice decision
PolicyRegistry->>SelfBounceTargetPolicy: evaluate SelectCards candidates
SelfBounceTargetPolicy->>AiDecisionContext: return desirability score
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 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/phase-ai/src/policies/self_bounce_target.rs`:
- Around line 44-54: Replace the raw score values in RETURN_SOURCE_PENALTY,
RETURN_TAPPED_BONUS, and RETURN_UNTAPPED_PENALTY with the corresponding
PolicyVerdict penalty and bonus band helpers. Preserve the existing outcome
mapping: source return uses the penalty band, tapped-land return uses the bonus
band, and untapped non-source return uses the penalty band.
- Around line 108-116: Update the WaitingFor::EffectZoneChoice pattern in the
self-bounce resolution policy to bind and require is_cost_payment: false,
excluding cost-payment land returns while preserving resolution-time bounce
handling. Add a neutral regression test covering a controlled land returned to
hand as an activated-ability cost.
🪄 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: 80173a2e-b937-4ac8-b515-39a38a7577ea
📒 Files selected for processing (5)
crates/phase-ai/src/policies/mod.rscrates/phase-ai/src/policies/registry.rscrates/phase-ai/src/policies/self_bounce_target.rscrates/phase-ai/src/policies/tests/mod.rscrates/phase-ai/src/policies/tests/self_bounce_target.rs
| /// Penalty for returning the ability's own source — the just-played bounce-land. | ||
| /// Returning it re-buys the land drop for nothing and loops, so it must lose to | ||
| /// any other land the pool offers. | ||
| pub(crate) const RETURN_SOURCE_PENALTY: f64 = -3.0; | ||
| /// Bonus for returning an already-tapped land: its mana is spent this turn, so | ||
| /// replaying it next turn costs the least tempo. | ||
| pub(crate) const RETURN_TAPPED_BONUS: f64 = 1.0; | ||
| /// Penalty for returning an untapped land: doing so forfeits mana still | ||
| /// available this turn. Milder than the source penalty — an untapped non-source | ||
| /// land is still a better return than looping the bounce-land. | ||
| pub(crate) const RETURN_UNTAPPED_PENALTY: f64 = -0.5; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the registry’s PolicyVerdict band helpers rather than raw score constants.
The literal -3.0, 1.0, and -0.5 bypass the required verdict-band contract, making this policy’s influence unstable when composed with other tactical policies. Map the three outcomes to the established penalty/bonus helpers.
As per path instructions, “New tactical policies must use the PolicyVerdict band helpers, not raw sentinel scores.”
Also applies to: 140-144
🤖 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/phase-ai/src/policies/self_bounce_target.rs` around lines 44 - 54,
Replace the raw score values in RETURN_SOURCE_PENALTY, RETURN_TAPPED_BONUS, and
RETURN_UNTAPPED_PENALTY with the corresponding PolicyVerdict penalty and bonus
band helpers. Preserve the existing outcome mapping: source return uses the
penalty band, tapped-land return uses the bonus band, and untapped non-source
return uses the penalty band.
Source: Path instructions
| let WaitingFor::EffectZoneChoice { | ||
| player, | ||
| source_id, | ||
| effect_kind: EffectKind::ChangeZone, | ||
| zone: Zone::Battlefield, | ||
| destination: Some(Zone::Hand), | ||
| cards: pool, | ||
| .. | ||
| } = &ctx.decision.waiting_for |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exclude land-return cost payments from this resolution policy.
is_cost_payment is ignored, so any activated-ability cost that returns a controlled land to hand receives this source/tapped heuristic. Gate on is_cost_payment: false and add a neutral regression test; this policy is intended for resolution-time bounce effects.
Proposed guard
destination: Some(Zone::Hand),
cards: pool,
+ is_cost_payment: false,
..As per path instructions, policies must be composable building blocks for the intended class rather than unrelated selections.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let WaitingFor::EffectZoneChoice { | |
| player, | |
| source_id, | |
| effect_kind: EffectKind::ChangeZone, | |
| zone: Zone::Battlefield, | |
| destination: Some(Zone::Hand), | |
| cards: pool, | |
| .. | |
| } = &ctx.decision.waiting_for | |
| let WaitingFor::EffectZoneChoice { | |
| player, | |
| source_id, | |
| effect_kind: EffectKind::ChangeZone, | |
| zone: Zone::Battlefield, | |
| destination: Some(Zone::Hand), | |
| cards: pool, | |
| is_cost_payment: false, | |
| .. | |
| } = &ctx.decision.waiting_for |
🤖 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/phase-ai/src/policies/self_bounce_target.rs` around lines 108 - 116,
Update the WaitingFor::EffectZoneChoice pattern in the self-bounce resolution
policy to bind and require is_cost_payment: false, excluding cost-payment land
returns while preserving resolution-time bounce handling. Add a neutral
regression test covering a controlled land returned to hand as an
activated-ability cost.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the policy's ranking is tested only through a direct call, leaving the production registry/routing seam unproven.
🔴 Blocker
[MED] Missing registry-routed regression for SelfBounceTargetPolicy. Evidence: crates/phase-ai/src/policies/tests/self_bounce_target.rs:165-212 constructs EffectZoneChoice but calls SelfBounceTargetPolicy.verdict directly; production depends on registry registration at crates/phase-ai/src/policies/registry.rs:401 and EffectZoneChoice routing through crates/phase-ai/src/decision_kind.rs:100-201. Why it matters: the direct probe stays green if the policy is omitted from PolicyRegistry or no longer receives the routed decision kind, so it does not protect the shipped behavior. Suggested fix: add a registry-routed EffectZoneChoice / SelectCards regression that asserts PolicyId::SelfBounceTarget is present and preserves the ordering: tapped other land > untapped other land > source. Follow the production-seam pattern in crates/phase-ai/src/policies/tests/poison.rs:788-831.
Recommendation: request changes — add the registry-routed regression, then request another review.
…licy Round-1 review (phase-rs#6679): the verdict tests called SelfBounceTargetPolicy directly, so they would stay green if the policy were dropped from PolicyRegistry or the EffectZoneChoice decision-kind routing changed — they did not protect the shipped seam. Adds, following the poison policy's production-seam pattern: - registry_registers_the_policy: PolicyRegistry::default() contains PolicyId::SelfBounceTarget. - registry_routes_the_land_bounce_ordering: routes each SelectCards selection through PolicyRegistry::verdicts (classify → filter by DecisionKind → activation → verdict) and asserts the production ordering tapped-other > untapped-other > source. The shared `eval` helper gained a `route` flag so the direct and routed paths build the identical EffectZoneChoice context. cargo test -p phase-ai --lib self_bounce — 6 pass; clippy -p phase-ai --all-targets -D warnings clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Fixed at head [MED] Registry-routed regression addedYou're right — the verdict tests probed
The shared
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved on current head 56424aeacd6ce373d435edaa52242182d3e28bf1: the registry-routed EffectZoneChoice regression proves SelfBounceTargetPolicy is registered and preserves source < untapped other < tapped other ranking.
2 participants
Tier: Frontier
Model: claude-opus-4-8
Closes #4730.
Summary
When a "return a land you control" ETB forces the AI to return one of its own lands, pick the least-costly one — and never the land that just entered.
Azorius Chancery and the rest of the Ravnica/MOM bounce-land ("Karoo") cycle enter and their ETB returns "a land you control" to hand (CR 608.2c, chosen at resolution). No policy scored that choice, so the AI kept returning the bounce-land it had just played — putting it back in hand to be replayed and bounced again, the tempo loop the reporter observed.
land_sequencingfixed the play-sequencing half of this report and its own doc-comment deferred the other half verbatim:This is that policy.
How it works
The choice surfaces as a
WaitingFor::EffectZoneChoicereturning battlefield lands you control to hand; each candidate is aSelectCardsselection. Detection is structural (no card names), so it covers every "return a land you control" bouncer:A value self-bounce of a creature (blink, save-from-removal) is deliberately left to the value policies — this policy only governs the land-bounce class.
Files changed
crates/phase-ai/src/policies/self_bounce_target.rs(new) ·policies/tests/self_bounce_target.rs(new)crates/phase-ai/src/policies/mod.rs,policies/registry.rs(PolicyId + registration),policies/tests/mod.rsCR references
CR 608.2c— a non-targeted "return a land you control" is chosen by its controller as the spell/ability resolvesCR 305.1— lands (the class this policy scopes to)Every number grep-verified against
docs/MagicCompRules.txt.Implementation method (required)
Method: manual — traced the deferred
#4bgap fromland_sequencing.rs, confirmed the resolution-time bounce surfaces asEffectZoneChoice/SelectCards(mirroringSacrificeValuePolicy), and added a dedicatedSelectTarget-population policy.Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Performance
verdictreturnsneutralimmediately unless the candidate is aSelectCardsunder anEffectZoneChoicethat returns AI-controlled battlefield lands to hand — a handful of field checks. No board scan beyond confirming the eligible pool is lands, no affordability sweep, nofind_legal_targets.Verification
cargo test -p phase-ai --lib— 1537 passed; 0 failed (4 new self-bounce tests: the purereturn_desirabilityorderingsource < untapped < tapped, and the composedverdictover a realEffectZoneChoice/SelectCardscontext — prefers a spent land over the bounce source, and stays neutral for a non-land pool and a non-hand destination)cargo clippy -p phase-ai --all-targets -- -D warnings— cleancargo fmt --all— cleancargo ai-gate— this policy activates only on a resolution-time land self-bounce, which none of the three gate decks (mono-red burn, affinity, Selesnya enchantress) can produce, so it is inert on the gate suite; CI's paired-seed AI gate confirms on this head.Notes
PolicyVerdict::score), not a rawScoreliteral.land_sequencing'sBOUNCE_DEPRIORITIZE); a color/mana-redundancy refinement (prefer returning a land whose color you have duplicates of) is a natural follow-up left out to keep this change focused.Summary by CodeRabbit