feat(phase-ai): add vehicles deck-feature axis + VehicleDeploymentPolicy - #6790
feat(phase-ai): add vehicles deck-feature axis + VehicleDeploymentPolicy#6790minion1227 wants to merge 4 commits into
Conversation
superagent-security
Bot
added
the
contributor:flagged
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
github-actions
Bot
added
the
needs-maintainer
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds structural Vehicle deck analysis, exposes Vehicle commitment through ChangesVehicle detection and feature integration
Policy signal and configuration
Vehicle deployment policy
Policy validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Deck as DeckFeatures
participant Feature as VehiclesFeature
participant Policy as VehicleDeploymentPolicy
participant State as GameState
Deck->>Feature: detect deck
Feature-->>Deck: commitment
Policy->>State: inspect untapped AI-controlled creatures
State-->>Policy: available crew power
Policy-->>Deck: crewable or uncrewable verdict
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
This current head ( Next step: an authorized maintainer will manually resolve and review the conflicts after classifying their attribution; no contributor action is requested until then. |
5a793aa to
c763008
Compare
|
Rebased onto The conflict was mine and predictable: this branch was cut before #6786 (discard-matters) merged as Four hunks, all in the two shared registration files:
Every one was a plain both-sides-added collision with no shared trailing token, so each keeps both entries. (Noting the contrast with the earlier Because a bad resolution here would silently revert the axis that just merged rather than merely fail to build, I verified both survive at every registration point:
Verification on the rebased head:
The |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/phase-ai/src/policies/crew_timing.rs (1)
17-49: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDoc comment claims
commitment == 0.0"goes silent," but the code can never returnNone.
.max(MIN_CREW_ACTIVATION)guaranteesSome(0.25)even atcommitment == 0.0— it never returnsNone. This contradicts both the inline comment ("only a deck with no Vehicles at all (commitment 0.0) goes silent") and, more subtly, sits oddly next to theMIN_CREW_ACTIVATIONdoc's own "damped rather than zeroed" framing (which is what the code actually does). Either the code should special-case a true opt-out at zero commitment, or the misleading "goes silent" sentence should be removed so the comment matches the actual (always-damped) behavior.🐛 Proposed fix (if a real opt-out at zero is intended)
fn activation( &self, features: &DeckFeatures, _state: &GameState, _player: PlayerId, ) -> Option<f32> { - // CR 702.122: previously `activation-constant Some(1.0)` — the exact - // initial-Crew action self-gates in `verdict`, so a constant was safe, - // but it applied identical weight in a dedicated Vehicles shell and in a - // deck running one incidental Copter. `features::vehicles` now supplies - // that deck signal. The floor is deliberately NOT applied: crewing is - // still a real decision in a low-commitment deck, so the policy keeps - // firing at a reduced weight rather than switching off, and only a deck - // with no Vehicles at all (commitment 0.0) goes silent. - Some(features.vehicles.commitment.max(MIN_CREW_ACTIVATION)) + // CR 702.122: previously `activation-constant Some(1.0)` — applied + // identical weight in a dedicated Vehicles shell and in a deck running + // one incidental Copter. `features::vehicles` now supplies that deck + // signal, damped rather than zeroed for any nonzero commitment. + if features.vehicles.commitment <= 0.0 { + None + } else { + Some(features.vehicles.commitment.max(MIN_CREW_ACTIVATION)) + } }🤖 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/crew_timing.rs` around lines 17 - 49, Update the CrewTimingPolicy::activation documentation to match the implemented behavior: remove the claim that commitment 0.0 returns silence, while preserving the explanation that commitment is damped to MIN_CREW_ACTIVATION rather than disabling the policy.
🧹 Nitpick comments (1)
crates/phase-ai/src/policies/tests/vehicle_deployment.rs (1)
186-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for
CantCrewand modified-crew-power paths.The PR claims (and the
verdict()implementation includes) honoringCantCrewstatics (CR 702.122d) and modified crew-power effects viaobject_crew_power_contribution, but no test in this file exercises either branch — every test only varies plain power/tapped/controller. Given these are the two CR-carve-outs this policy specifically claims to honor, they're worth covering directly (e.g. a creature under aCantCrewstatic that would otherwise supply enough power, and a creature crewed "as though its power were N greater").🤖 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/tests/vehicle_deployment.rs` around lines 186 - 289, Add tests in the vehicle verdict test suite covering both claimed carve-outs: verify a qualifying creature that would otherwise satisfy the requirement is excluded when affected by a CantCrew static, and verify modified crew power through object_crew_power_contribution can make an otherwise insufficient creature crewable. Use the existing state, creature, vehicle_in_hand, and verdict_for helpers and assert the corresponding verdict kinds.
🤖 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.
Outside diff comments:
In `@crates/phase-ai/src/policies/crew_timing.rs`:
- Around line 17-49: Update the CrewTimingPolicy::activation documentation to
match the implemented behavior: remove the claim that commitment 0.0 returns
silence, while preserving the explanation that commitment is damped to
MIN_CREW_ACTIVATION rather than disabling the policy.
---
Nitpick comments:
In `@crates/phase-ai/src/policies/tests/vehicle_deployment.rs`:
- Around line 186-289: Add tests in the vehicle verdict test suite covering both
claimed carve-outs: verify a qualifying creature that would otherwise satisfy
the requirement is excluded when affected by a CantCrew static, and verify
modified crew power through object_crew_power_contribution can make an otherwise
insufficient creature crewable. Use the existing state, creature,
vehicle_in_hand, and verdict_for helpers and assert the corresponding verdict
kinds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5236792a-1722-4454-aad5-02169f534cf1
📒 Files selected for processing (12)
crates/phase-ai/src/config.rscrates/phase-ai/src/features/mod.rscrates/phase-ai/src/features/reanimator.rscrates/phase-ai/src/features/tests/mod.rscrates/phase-ai/src/features/tests/vehicles.rscrates/phase-ai/src/features/vehicles.rscrates/phase-ai/src/policies/crew_timing.rscrates/phase-ai/src/policies/mod.rscrates/phase-ai/src/policies/registry.rscrates/phase-ai/src/policies/tests/mod.rscrates/phase-ai/src/policies/tests/vehicle_deployment.rscrates/phase-ai/src/policies/vehicle_deployment.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocked — Vehicle deployment currently grants a live crew bonus to a Vehicle that has no Crew ability.
🔴 Blocker
crates/phase-ai/src/features/vehicles.rs:155-169returnsSome(0)for everyVehiclesubtype that has noKeyword::Crew;crates/phase-ai/src/policies/vehicle_deployment.rs:82-140uses that shared result as a live crew cost. Consequently a subtype-only Vehicle reaches theavailable >= requiredbranch (including with no creatures, because0 >= 0) and receivesvehicle_deployment_crewableplus a positive score. This is not a parsing fallback at this seam: CR 702.122a says, “Crew is an activated ability of Vehicle cards. ‘Crew N’ means ‘Tap any number of other untapped creatures you control with total power N or greater: This permanent becomes an artifact creature until end of turn.’” A type-line subtype alone does not grant that activated ability. Split the deck-archetype classifier (where an honest parser-missing-keyword fallback is useful) from a livecrew_requirementauthority that returnsSome(N)only forKeyword::Crew; routeVehicleDeploymentPolicythrough the latter. Add a registry-level test whose cast candidate has the Vehicle subtype but no Crew keyword and asserts the policy is neutral/vehicle_deployment_na, even when a board would otherwise make a Crew 0 candidate score.
🟡 Non-blocking
crates/phase-ai/src/policies/crew_timing.rs:44-48says commitment0.0“goes silent,” butmax(MIN_CREW_ACTIVATION)always returnsSome(0.25). Correct the comment or implement the stated opt-out; the policy behavior and its explanation must agree.crates/phase-ai/src/policies/tests/vehicle_deployment.rs:188-287does not exercise the policy’s claimedobject_has_cant_crewor modifiedobject_crew_power_contributionbehavior. Add direct policy-level fixtures for those two live branches while extending this suite, so the registry path cannot silently stop using the engine authorities.
Recommendation: request changes — separate deck archetype detection from a live Crew-ability requirement, add the subtype-only negative at the registry seam, and refresh the adjacent policy evidence.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/crew_timing.rs`:
- Around line 44-48: Update CrewTimingPolicy::activation so a
vehicles.commitment value of exactly 0.0 returns None or otherwise remains
inactive before applying MIN_CREW_ACTIVATION; retain the existing clamped
activation behavior for nonzero commitment values.
🪄 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: d0368594-dd9b-413f-bfbe-de0a01432a41
📒 Files selected for processing (12)
crates/phase-ai/src/config.rscrates/phase-ai/src/features/mod.rscrates/phase-ai/src/features/reanimator.rscrates/phase-ai/src/features/tests/mod.rscrates/phase-ai/src/features/tests/vehicles.rscrates/phase-ai/src/features/vehicles.rscrates/phase-ai/src/policies/crew_timing.rscrates/phase-ai/src/policies/mod.rscrates/phase-ai/src/policies/registry.rscrates/phase-ai/src/policies/tests/mod.rscrates/phase-ai/src/policies/tests/vehicle_deployment.rscrates/phase-ai/src/policies/vehicle_deployment.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/phase-ai/src/features/tests/mod.rs
- crates/phase-ai/src/policies/tests/mod.rs
- crates/phase-ai/src/policies/mod.rs
- crates/phase-ai/src/policies/tests/vehicle_deployment.rs
- crates/phase-ai/src/policies/registry.rs
- crates/phase-ai/src/features/mod.rs
- crates/phase-ai/src/features/tests/vehicles.rs
- crates/phase-ai/src/config.rs
- crates/phase-ai/src/policies/vehicle_deployment.rs
- crates/phase-ai/src/features/vehicles.rs
Addresses the blocker and both non-blocking findings on phase-rs#6790. [BLOCKER] `crew_requirement` returned `Some(0)` for any Vehicle subtype without `Keyword::Crew`, and `VehicleDeploymentPolicy` consumed that as a live crew cost. `available >= required` then read `0 >= 0` — true on an EMPTY board — so a permanent with no crew ability scored `vehicle_deployment_crewable` and a positive bonus. CR 702.122a opens "Crew is an activated ability of Vehicle cards"; a type line does not grant it. The defect was conflating two different questions behind one predicate: - **Archetype membership** (deck classification): is this card part of the Vehicles plan? A Vehicle whose crew keyword the parser missed still is, so the subtype fallback is honest here — it only scales how strongly the axis activates. Now `vehicle_archetype_member`. - **Crew requirement** (live scoring): what does crewing this cost right now? Only `Keyword::Crew` answers that. Now `crew_requirement` / `crew_requirement_parts`, strict, and what the policy routes through. `detect` uses membership for `vehicle_count` and the strict authority for `total_crew_cost`, so a subtype-only Vehicle joins the archetype while adding nothing to pay. [NB] `crew_timing`'s comment claimed commitment 0.0 "goes silent" while `max(MIN_CREW_ACTIVATION)` always returned `Some(0.25)`. Implemented the stated opt-out rather than softening the comment: a deck with no Vehicles has no crew decision to weigh, so the policy returns `None`; above zero the weight is damped, not floored out. [NB] The suite never exercised the engine crew authorities the module docs claim. Added direct fixtures for both live branches: a creature under `StaticMode:: CantCrew` (CR 702.122d) contributing nothing, and `CrewContribution` in both kinds — `PowerDelta` ("as though its power were N greater") and `ToughnessInsteadOfPower`. Tests 34 -> 41. Each fix is proven by mutation: - restoring the subtype fallback in the policy reds all three subtype-only tests, including the registry-seam one the review asked for; - deleting the `object_has_cant_crew` call reds `cant_crew_creature_does_not_contribute`; - replacing `object_crew_power_contribution` with a raw `.power` sum reds both contribution tests — proving the policy consults the authority rather than merely importing it. Verified: `cargo test -p phase-ai` 1894 passed, `cargo clippy -p phase-ai -p phase-engine --all-targets --features proptest -- -D warnings` exit 0, `check-engine-authorities.sh` exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All three fixed at 🔴 Blocker — correct, and a real scoring bug
The root defect was mine and structural: I hung two different questions off one predicate.
Split accordingly: Added the registry-seam negative you asked for, plus its empty-board and full-board twins — the full-board case matters because it proves the rejection comes from the missing ability, not from an empty battlefield. 🟡
|
| Mutation | Tests that went RED |
|---|---|
| restore the subtype fallback in the policy | all 3 subtype-only tests, incl. the registry-seam one |
delete the object_has_cant_crew call |
cant_crew_creature_does_not_contribute |
replace object_crew_power_contribution with a raw .power sum |
power_delta_contribution_is_honored, toughness_instead_of_power_contribution_is_honored |
That last mutation is the one that matters for your second note: it demonstrates the policy genuinely consults the authority rather than merely importing it, so the registry path cannot silently stop using it.
Verification
cargo test -p phase-ai— 1894 passed, 0 failedcargo clippy -p phase-ai -p phase-engine --all-targets --features proptest -- -D warnings— exit 0./scripts/check-engine-authorities.sh— exit 0Gate A PASS head=722795f67a201173fe13837e2d7cc0e4264e3938 base=1604a6f3023413a34740d6d268d77872b3150b0f
Body's ## Gate A updated to this head. Requesting re-review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/vehicle_deployment.rs`:
- Around line 373-393: Strengthen
registry_stays_silent_for_a_subtype_only_vehicle by requiring the
VehicleDeployment verdict to exist before inspecting it. Replace the optional
if-let handling with an expectation that clearly states VehicleDeploymentPolicy
must be registered and routed for this CastSpell candidate, then retain the
existing reason-kind and zero-delta assertions.
🪄 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: 90770952-1227-49dd-99d5-c58d57cee148
📒 Files selected for processing (5)
crates/phase-ai/src/features/tests/vehicles.rscrates/phase-ai/src/features/vehicles.rscrates/phase-ai/src/policies/crew_timing.rscrates/phase-ai/src/policies/tests/vehicle_deployment.rscrates/phase-ai/src/policies/vehicle_deployment.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/phase-ai/src/features/tests/vehicles.rs
- crates/phase-ai/src/policies/crew_timing.rs
- crates/phase-ai/src/features/vehicles.rs
`CrewTimingPolicy` ran with `activation-constant Some(1.0)` — it applied
identical weight in a dedicated Vehicles shell and in a deck holding one
incidental Smuggler's Copter, because no vehicles deck-feature existed for it to
scale by. This adds that axis and wires it in, so the constant is gone.
CR 702.122a: "Crew N" taps any number of OTHER untapped creatures you control
with total power N or greater. A Vehicle is therefore not a body the deck buys
with mana — it is one it buys with board presence, and an uncrewed Vehicle is not
a creature at all.
Feature (`features/vehicles.rs`), structural over `CardFace` AST:
- `vehicle_count` / `total_crew_cost` from `Keyword::Crew { power }`, with the
Vehicle subtype as a fallback so a Vehicle whose crew keyword the parser has
not attached still joins the archetype (contributing no crew cost).
- `crew_body_count` / `total_crew_power` — creatures that can actually pay a crew
cost. A Vehicle is excluded even when also printed as a creature, because crew
taps OTHER creatures; `PtValue::Variable` ("*") is excluded rather than
guessed; power 0 taps for nothing.
- `commitment` — geometric mean over (Vehicle density, crew support). Both
pillars mandatory: Vehicles with no bench never become creatures, and a bench
with no Vehicles is just a creature deck.
Policy (`policies/vehicle_deployment.rs`, `CastSpell`) asks the question that
comes BEFORE `CrewTimingPolicy`'s: will deploying this Vehicle now produce a body
or a brick? It adds value only when the board already meets the crew requirement
and withholds it otherwise — never a veto, since holding a Vehicle for a board
that may never arrive is its own mistake.
Shared authorities rather than reimplementation:
- `VEHICLE_SUBTYPE` is promoted from `features::reanimator` instead of being
redefined, so two features cannot disagree about what a Vehicle is.
- Per-creature crew power goes through the engine's
`object_crew_power_contribution` + `object_has_cant_crew`, so a creature that
crews by toughness or "as though its power were N greater" (CR 702.122a) and
one under a `CantCrew` static (CR 702.122d) are counted exactly as the real
crew payment would count them. Summing `.power` here would have been silently
wrong for both.
Calibration is computed, not asserted from intuition, and one anchor exposed a
real defect: the first pillar scaling let a single incidental Copter in a
20-creature deck reach 0.456 — above the floor — because a saturated bench
compensated for near-zero Vehicle density. Retuned so it lands at 0.373 and is
correctly rejected. Anchors pinned by test: 8+16 -> 1.000, 5+10 -> 0.833,
3+6 -> 0.645, 2+4 -> 0.527, 1+20 -> 0.373 (below the 0.40 floor).
`vehicle_deployment_bonus` lands in `UNTUNED_POLICY_PENALTY_FIELDS` pending a
paired-seed calibration.
34 tests. Four invariants were mutated independently and each killed exactly its
own guard, with no cross-masking: the self-crew exclusion, the untapped
requirement, the controller requirement, and the positive-power requirement.
Verified: `cargo test -p phase-ai` 1844 passed, `cargo clippy -p phase-ai
-p phase-engine --all-targets --features proptest -- -D warnings` exit 0,
`check-engine-authorities.sh` exit 0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses the blocker and both non-blocking findings on phase-rs#6790. [BLOCKER] `crew_requirement` returned `Some(0)` for any Vehicle subtype without `Keyword::Crew`, and `VehicleDeploymentPolicy` consumed that as a live crew cost. `available >= required` then read `0 >= 0` — true on an EMPTY board — so a permanent with no crew ability scored `vehicle_deployment_crewable` and a positive bonus. CR 702.122a opens "Crew is an activated ability of Vehicle cards"; a type line does not grant it. The defect was conflating two different questions behind one predicate: - **Archetype membership** (deck classification): is this card part of the Vehicles plan? A Vehicle whose crew keyword the parser missed still is, so the subtype fallback is honest here — it only scales how strongly the axis activates. Now `vehicle_archetype_member`. - **Crew requirement** (live scoring): what does crewing this cost right now? Only `Keyword::Crew` answers that. Now `crew_requirement` / `crew_requirement_parts`, strict, and what the policy routes through. `detect` uses membership for `vehicle_count` and the strict authority for `total_crew_cost`, so a subtype-only Vehicle joins the archetype while adding nothing to pay. [NB] `crew_timing`'s comment claimed commitment 0.0 "goes silent" while `max(MIN_CREW_ACTIVATION)` always returned `Some(0.25)`. Implemented the stated opt-out rather than softening the comment: a deck with no Vehicles has no crew decision to weigh, so the policy returns `None`; above zero the weight is damped, not floored out. [NB] The suite never exercised the engine crew authorities the module docs claim. Added direct fixtures for both live branches: a creature under `StaticMode:: CantCrew` (CR 702.122d) contributing nothing, and `CrewContribution` in both kinds — `PowerDelta` ("as though its power were N greater") and `ToughnessInsteadOfPower`. Tests 34 -> 41. Each fix is proven by mutation: - restoring the subtype fallback in the policy reds all three subtype-only tests, including the registry-seam one the review asked for; - deleting the `object_has_cant_crew` call reds `cant_crew_creature_does_not_contribute`; - replacing `object_crew_power_contribution` with a raw `.power` sum reds both contribution tests — proving the policy consults the authority rather than merely importing it. Verified: `cargo test -p phase-ai` 1894 passed, `cargo clippy -p phase-ai -p phase-engine --all-targets --features proptest -- -D warnings` exit 0, `check-engine-authorities.sh` exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…kipping it
Addresses the CodeRabbit finding on `722795f67`.
`registry_stays_silent_for_a_subtype_only_vehicle` used
`if let Some(verdict) = found { .. }`, so if `VehicleDeploymentPolicy` were ever
dropped from routing for this candidate — activation regressing to `None`, say —
`found` would be `None`, the body would be skipped, and the test would pass
having verified nothing. That is the vacuous-negative shape, and it was
undermining the exact "registered AND routed AND neutral" invariant the test
exists to pin.
Replaced with `.expect(..)`, matching the sibling test at the same production
seam. Proven non-vacuous: forcing `activation` to return `None` now reds this
test, where previously it would have passed.
Separately, the CodeRabbit note about `CrewTimingPolicy` staying active at
commitment 0.0 targets `c763008b8` — the commit BEFORE the fix. The opt-out
landed in `722795f67` and is present at `crew_timing.rs:51`; no further change
is needed there.
Verified: `cargo test -p phase-ai --lib vehicle` 41 passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The VehicleDeployment registry-seam test can pass when the policy is not routed at all. Evidence: crates/phase-ai/src/policies/tests/vehicle_deployment.rs:384-392 finds the verdict and asserts only inside if let Some. Why it matters: a regression in the policy's decision-kind registration or activation can omit the verdict entirely while this test still passes, so it does not establish the claimed registry coverage. Suggested fix: require the VehicleDeployment verdict with .expect(...) (or an equivalent assertion) before checking its neutral result.
[MED] CrewTiming must not opt out merely because the cached deck feature is zero when a live CrewVehicle action is already being evaluated. Evidence: crates/phase-ai/src/policies/crew_timing.rs:46-54 returns None for zero commitment, and policies/registry.rs:476-479 then skips the verdict. features/vehicles.rs:201-217 deliberately excludes non-fixed printed power from the deck-time bench, while features/tests/vehicles.rs:169-175 pins a zero-commitment vehicle deck with no printed bench. Why it matters: a later token, variable-power, or otherwise live creature can make a legal crew action available, but the timing safeguard is silently absent exactly because its deck-time classification is conservative. Suggested fix: keep CrewTiming activated at zero via Some(features.vehicles.commitment.max(MIN_CREW_ACTIVATION)), correct the zero-commitment comment, and add a registry-seam test proving a live CrewVehicle candidate still emits a CrewTiming verdict when cached commitment is 0.0.
722795f to
ec7601a
Compare
|
Both CodeRabbit findings triaged at
if features.vehicles.commitment <= 0.0 {
return None;
}
Some(features.vehicles.commitment.max(MIN_CREW_ACTIVATION))I implemented the stated opt-out rather than softening the comment, exactly as the earlier maintainer note asked. Weak assertion — genuine, and mine. Fixed. That is the vacuous-negative shape, and I introduced it in a test written for a review finding. Replaced with Proven non-vacuous: forcing Also rebased onto Verification at this head:
Body's |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/vehicle_deployment.rs`:
- Around line 112-118: Update the crew contribution aggregation around
object_crew_power_contribution to filter out non-positive values before summing,
so only contributions greater than zero affect vehicle availability while
preserving the existing Crew selection behavior.
🪄 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: 60a61376-2118-4a84-ada4-b8f1f9864cbf
📒 Files selected for processing (12)
crates/phase-ai/src/config.rscrates/phase-ai/src/features/mod.rscrates/phase-ai/src/features/reanimator.rscrates/phase-ai/src/features/tests/mod.rscrates/phase-ai/src/features/tests/vehicles.rscrates/phase-ai/src/features/vehicles.rscrates/phase-ai/src/policies/crew_timing.rscrates/phase-ai/src/policies/mod.rscrates/phase-ai/src/policies/registry.rscrates/phase-ai/src/policies/tests/mod.rscrates/phase-ai/src/policies/tests/vehicle_deployment.rscrates/phase-ai/src/policies/vehicle_deployment.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/phase-ai/src/policies/mod.rs
- crates/phase-ai/src/features/tests/mod.rs
- crates/phase-ai/src/features/tests/vehicles.rs
- crates/phase-ai/src/policies/tests/mod.rs
- crates/phase-ai/src/features/mod.rs
- crates/phase-ai/src/features/reanimator.rs
- crates/phase-ai/src/config.rs
- crates/phase-ai/src/policies/registry.rs
- crates/phase-ai/src/features/vehicles.rs
- crates/phase-ai/src/policies/tests/vehicle_deployment.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] CrewTiming is suppressed for a legal live crew decision whenever the deck-time Vehicles feature happens to score zero. Evidence: crates/phase-ai/src/policies/crew_timing.rs:46-54 returns None at zero commitment, and crates/phase-ai/src/policies/registry.rs:476-479 therefore omits the policy entirely. But features/vehicles.rs:201-217 intentionally excludes variable/non-fixed printed power from its deck-time bench, and features/tests/vehicles.rs:169-175 establishes that a Vehicle deck with no printed bench has commitment == 0.0. Why it matters: a token or another live creature can later make CrewVehicle legal, yet the policy's timing safeguard then produces no verdict because a conservative static classification was treated as a live-action gate. Suggested fix: remove the zero-commitment early return, retain MIN_CREW_ACTIVATION through Some(features.vehicles.commitment.max(MIN_CREW_ACTIVATION)), correct the comment, and add a registry/action test proving that cached zero commitment with a legal CrewVehicle candidate still emits CrewTiming.
Reverts the zero-commitment opt-out added in the previous round, per review. The earlier note offered a choice — correct the comment, or implement the stated opt-out — and I picked the opt-out. That was the wrong branch, for a reason that lives in this PR's own code: `features::vehicles::crew_capable_power` deliberately excludes non-fixed printed power (`PtValue::Variable`), and a decklist cannot see tokens at all. So `commitment == 0.0` means "no bench provable at deck-build time", NOT "cannot crew" — `features/tests/vehicles.rs` pins exactly that case. A Vehicles deck whose bench is tokens or `*`-power creatures scores zero and can still reach a legal `CrewVehicle` action, so gating a LIVE action on a conservative STATIC classification removed the timing safeguard precisely where it is most needed. `activation` therefore never opts out. `MIN_CREW_ACTIVATION` damps the weight instead of zeroing it, so a crew action the AI is already evaluating still gets judged while a dedicated shell continues to outweigh an incidental Copter. The comment now explains that rather than describing behavior the code does not have — the defect the first note was really pointing at. Two regressions, both proven RED by reintroducing the early return: - `activation_never_opts_out_even_at_zero_commitment` — activation level. - `registry_emits_a_crew_verdict_at_zero_commitment` — the production seam, with a legal `CrewVehicle` candidate and cached commitment 0.0. - `activation_scales_above_the_floor` pins the other direction, so damping cannot silently become a flat constant again. Verified: `cargo test -p phase-ai` 1897 passed, `cargo clippy -p phase-ai -p phase-engine --all-targets --features proptest -- -D warnings` exit 0, `check-engine-authorities.sh` exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed at
You were right and I want to name why I got it wrong, because the reasoning is the useful part. The earlier note offered a choice — correct the comment, or implement the stated opt-out. I picked the opt-out, treating "commitment 0.0" as "this deck has no Vehicles". But that reading is contradicted by this PR's own detector: That is the same class of error as this PR's original blocker — using a deck-classification answer at a live seam — which makes it worth recording rather than quietly patching. Regressions, both proven RED by reintroducing the early return:
For clarity on the two earlier MEDs, since they landed against older commits:
Verification at this head:
Body's |
|
Implementation review is clean at Approval and enqueue are held solely on external CI: |
Re-review requested — head
|
| Round | Finding | Resolution |
|---|---|---|
| 1 🔴 | subtype-only Vehicle got a synthesised Crew 0, satisfied by an empty board |
split vehicle_archetype_member (deck) from strict crew_requirement (live); 3 subtype-only tests, all RED under the old fallback |
| 1 🟡 | crew_timing comment contradicted its code |
corrected |
| 1 🟡 | engine crew authorities never exercised | CantCrew + both CrewContribution kinds; RED under a raw .power sum |
| 2 | weak if let Some registry assertion |
.expect(..); RED when activation is forced to None |
| 3 | zero-commitment opt-out suppressed a live crew decision | reverted; 2 regressions RED under the early return |
I am running the paired-seed control locally against this exact head as well, since the CI job has not completed on any of my recent heads. I will post the diff when it finishes — happy to hold for it if you would rather decide on that evidence, or to rebase / re-run anything else you want.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
🔴 Blocker — vehicle-deployment overestimates legal crew power
VehicleDeploymentPolicy sums object_crew_power_contribution after filtering only for AI control, untapped state, creature type, and CantCrew at crates/phase-ai/src/policies/vehicle_deployment.rs:101-118. It omits the engine's object_cant_tap eligibility rule.
The authoritative crew path requires a controlled, untapped creature and !object_cant_tap(state, id) in is_tappable_creature_for_cost (crates/engine/src/game/engine.rs:9959-9967), then revalidates the same restriction and rejects the payment at crates/engine/src/game/engine.rs:10173-10182. Therefore a CantTap 3/3 is counted as available here for a Crew 3 Vehicle even though it cannot legally pay Crew; the policy can award the crewable deployment bonus for an uncrewable Vehicle.
Please make the policy use/reuse the authoritative crew-eligibility predicate/query rather than maintaining a partial duplicate filter, and add a discriminating policy test with a CantTap-only 3/3 and a Crew 3 Vehicle. It must assert the neutral/uncrewable verdict and fail if the object_cant_tap exclusion is removed.
CodeRabbit Major thread disposition
Refuting and resolving CodeRabbit's non-positive-contribution suggestion: no filter is needed. object_crew_power_contribution returns (base + delta).max(0) at crates/engine/src/game/static_abilities.rs:1860-1888, so the summed contribution cannot be negative. The alleged negative-sum failure mode is impossible on this head.
2 participants
Summary
Adds a
vehiclesdeck-feature axis and a companionVehicleDeploymentPolicy, and uses the axis to remove anactivation-constantthat had no deck signal behind it.CrewTimingPolicyranSome(1.0) // activation-constant, applying identical weight in a dedicated Vehicles shell and in a deck holding one incidental Smuggler's Copter — because no vehicles feature existed for it to scale by. 23 axes exist; none covered Vehicles.CR 702.122a: "Crew N" taps any number of other untapped creatures you control with total power N or greater. A Vehicle is not a body the deck buys with mana — it is one it buys with board presence, and an uncrewed Vehicle is not a creature at all.
Files changed
crates/phase-ai/src/features/vehicles.rs(new)crates/phase-ai/src/features/tests/vehicles.rs(new)crates/phase-ai/src/policies/vehicle_deployment.rs(new)crates/phase-ai/src/policies/tests/vehicle_deployment.rs(new)crates/phase-ai/src/features/mod.rscrates/phase-ai/src/features/tests/mod.rscrates/phase-ai/src/features/reanimator.rscrates/phase-ai/src/policies/mod.rscrates/phase-ai/src/policies/tests/mod.rscrates/phase-ai/src/policies/registry.rscrates/phase-ai/src/policies/crew_timing.rscrates/phase-ai/src/config.rsTrack
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: not-applicable — AI heuristics under
crates/phase-ai/only. Nocrates/engine/file is touched: no parser, effect-resolution, targeting, stack or rules-behavior change. The axis reads already-parsed AST and calls existing engine authorities.CR references
CR 702.122a— Crew N: tap any number of OTHER untapped creatures with total power N or greater. Verified atdocs/MagicCompRules.txt:4882. Drives both pillars and every policy guard.CR 702.122d— "can't crew Vehicles" statics, honored viaobject_has_cant_crew.CR 205.3— printed subtype line, for the Vehicle subtype fallback.Grep-verified before being written. No CR annotation is attached to ordinary implementation logic.
Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo test -p phase-ai— 1844 passed, 0 failed, plus integration binaries green (includingevery_policy_penalty_is_tuning_registered_or_explicitly_untunedandactivation_marker_lint, which now passes with the constant removed).cargo test -p phase-ai --lib vehicle— 34 passed.cargo clippy -p phase-ai -p phase-engine --all-targets --features proptest -- -D warnings— exit 0../scripts/check-engine-authorities.sh— exit 0.Mutation evidence. Four CR 702.122a invariants were broken independently, and each killed exactly its own guard with no cross-masking:
a_vehicle_is_not_its_own_crew_bodytapped_creatures_do_not_crewopponent_creatures_do_not_crewzero_power_creature_is_not_a_crew_bodycargo ai-gateis not attached: no quick-filter deck clearsVEHICLES_FLOOR, so the policy is inert there. Happy to run a paired-seed control on this head if you want it.Gate A
Gate A PASS head=9a6409fb881687b1c6dbdb58e24e0c9a9a9a909e base=1604a6f3023413a34740d6d268d77872b3150b0f
Anchored on
crates/phase-ai/src/features/draw_matters.rs:75— the established axis shape:detect()folding per-DeckEntryCardFaceAST into counts,pub(crate)parts-based predicates shared with the policy, and acommitmentscalar; the samecommitment::geometric_meantwo-pillar formula with calibration anchors in the docstring.crates/phase-ai/src/policies/draw_payoff.rs:45— the sameTacticalPolicyshape:activation()as a single floor-plus-scale knob, cheap-to-expensiveverdict()ordering where a card-local AST check rejects non-members before any board scan, and apub(crate)cap constant the bounded-score test asserts against.Final review-impl
Final review-impl PASS head=9a6409fb881687b1c6dbdb58e24e0c9a9a9a909e
Two things caught in self-review before submission:
VEHICLE_SUBTYPEalready existed infeatures::reanimator, so it is promoted rather than redefined. And per-creature crew power goes through the engine'sobject_crew_power_contribution/object_has_cant_crewrather than summing.power— which would have been silently wrong for a creature that crews by toughness, one that crews "as though its power were N greater", and one under aCantCrewstatic.Claimed parse impact
None. No parser, card-data or Oracle-text code is touched.
Scope Expansion
One, disclosed.
crates/phase-ai/src/policies/crew_timing.rsis modified beyond adding the new axis: itsactivation()previously returned a constant and now scales byfeatures.vehicles.commitment, damped by aMIN_CREW_ACTIVATIONfloor so a crew decision the AI is already looking at still gets judged in a low-commitment deck rather than being switched off. That behavioral change to an existing policy is the entire point of the axis, but it is a change to code this PR did not otherwise need to touch, so it is declared rather than folded into the summary. Itsactivation-constantmarker is removed andactivation_marker_lintpasses.Validation Failures
None.
CI Failures
None known at submission.
🤖 Generated with Claude Code
Summary by CodeRabbit