feat(phase-ai): add cost-reduction deck-feature axis + CostReductionPolicy - #6743
Conversation
superagent-security
Bot
added
the
contributor:flagged
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
|
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 context-free card-face filter evaluation, deck-time cost-reduction analysis, and a registered tactical policy that rewards deploying reducers before discounted spells. ChangesCost Reduction Decision Support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DeckFeatures
participant CostReductionFeature
participant EngineFilter
participant PolicyRegistry
participant CostReductionPolicy
DeckFeatures->>CostReductionFeature: analyze deck reducers and spell coverage
CostReductionFeature->>EngineFilter: match filters against CardFace
EngineFilter-->>CostReductionFeature: context-free admission
CostReductionFeature-->>DeckFeatures: commitment score
PolicyRegistry->>CostReductionPolicy: evaluate CastSpell candidate
CostReductionPolicy-->>PolicyRegistry: deploy bonus, defer penalty, or neutral verdict
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…olicy
`features::mana_ramp` explicitly deferred this shape — "`StaticMode::ModifyCost`
is deliberately out of scope — cost reducers are a follow-up feature". This is
that follow-up.
A Goblin Electromancer / Baral / Foundry Inspector / Medallion effect is
acceleration that never taps for mana: every later spell costs less for as long
as the permanent survives (CR 601.2f). The engine already applies the discount
at cast time, so the AI is never overcharged — but nothing valued *deploying*
the reducer. `mana_ramp` only sees effects that add mana, so a deck whose whole
acceleration plan is cost reduction read as having no ramp at all.
Feature (`features/cost_reduction.rs`), structural over `CardFace` AST:
- `reducer_count` — cards carrying a board-wide `ModifyCost { Reduce }` that
applies to spells YOU cast. Mirrors the eligibility tests
`casting::collect_cost_modifiers` applies at cast time, so deck-time
classification and the resolver agree by construction: `Reduce` only (`Raise`
is Thalia, `Minimum` is Trinisphere), never a `SelfRef` self-cost reduction
(CR 113.6), never opponent-scoped.
- `total_discount` — summed generic discount. CR 118.7a: a generic reduction
affects only the generic component, so the magnitude is `generic`, not the
amount's full mana value.
- `discounted_count` / coverage — deck cards a reducer's `spell_filter`
actually admits, delegating every type leaf to the engine's CR 205 authority
(`matches_type_filter_against_face`, widened to `pub`) rather than re-deriving
type semantics. An unverifiable filter reports no coverage, so the axis fails
OFF rather than claiming a discount the deck may not get.
- `commitment` — geometric mean over (reducer density, coverage): both pillars
mandatory. Reducers that discount nothing are blanks; spells with no reducer
are just spells.
Policy (`policies/cost_reduction.rs`, `CastSpell`):
- `cost_reduction_deploy_engine` — credit for deploying a reducer, scaled by
capped saved mana across the remaining grip.
- `cost_reduction_defer_to_engine` — nudge against casting past an unplayed,
cheaper reducer (the `RampTimingPolicy::defer_to_ramp` shape).
- The card-local static check runs first, so every non-reducer candidate is
rejected after reading one card's AST; only a confirmed reducer pays for the
hand walk. No battlefield sweep, no `find_legal_targets`, no affordability
query in the search inner loop.
Both scoring scalars land in `UNTUNED_POLICY_PENALTY_FIELDS` pending a
paired-seed calibration.
33 tests: per-axis detection, both calibration anchors, every exclusion
(`Raise`/`Minimum`/`SelfRef`/opponent-scope/zero-generic/unverifiable-filter),
format-size neutrality, the bounded-score ceiling, and a registry-routed
regression covering registration + `CastSpell` routing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…two elided assumptions Self-review of the cost-reduction axis before final submission: - The doc comments named `casting::collect_cost_modifiers`; the actual engine authority is `casting::collect_battlefield_cost_modifiers` (crates/engine/src/game/casting.rs:7179). A citation that does not resolve is worse than none — it claims a verification that never happened. - `your_spell_discount` silently ignored `dynamic_count`. Record that this is deliberate: a scaling reducer's multiplier is unknowable at deck-analysis time, so the per-unit `generic` is reported, understating rather than inventing a value. - `castable_cards_in_hand` counts remaining casts without narrowing to the reducer's `spell_filter`. Record why that is correct rather than sloppy: the narrowing is applied once per game through `commitment`, which `activation` multiplies in, instead of per candidate in the search inner loop. Documentation only — no behavior change; the 33 tests are untouched and green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/features/cost_reduction.rs`:
- Around line 157-208: Expose the Reduce-mode, non-SelfRef, non-Opponent-scoped
eligibility logic used by casting::collect_cost_modifiers as a shared engine
helper for StaticDefinition/CardFace-compatible inputs, following the existing
matches_type_filter_against_face pattern. Update both
casting::collect_cost_modifiers and phase-ai's your_spell_discount to call this
authoritative helper, leaving your_spell_discount responsible only for
extracting and validating the generic amount.
- Around line 173-208: Update your_spell_discount to account for
ModifyCost::Reduce dynamic_count by resolving the multiplier and scaling the
generic discount by it, matching the engine’s amount *
resolve_quantity(dynamic_count) behavior. Preserve the existing filtering for
mode, SelfRef, opponent scope, and zero generic reductions, and return the
scaled value for saved_mana/total_discount.
In `@crates/phase-ai/src/policies/cost_reduction.rs`:
- Around line 116-154: Update castable_cards_in_hand and
hand_holds_cheaper_reducer to evaluate each reducer’s spell_filter against the
candidate spell before counting it as relevant. Thread the candidate spell or
matching context through their callers, preserve existing behavior for
spell_filter: None, and add a regression test where a reducer’s filter does not
match the candidate spell.
In `@crates/phase-ai/src/policies/registry.rs`:
- Around line 145-149: Move the CR 121.1 documentation comment so it directly
annotates DrawPayoff rather than CostReduction, and add an accurate verified CR
citation and description for CostReduction if one is required by the surrounding
policy annotations. Keep SelfBounceTarget unchanged and ensure each variant’s
documentation describes its own domain.
🪄 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: cf1e186a-d035-49d1-bfd1-a81cc165ae95
📒 Files selected for processing (12)
crates/engine/src/game/filter.rscrates/phase-ai/src/config.rscrates/phase-ai/src/features/cost_reduction.rscrates/phase-ai/src/features/mana_ramp.rscrates/phase-ai/src/features/mod.rscrates/phase-ai/src/features/tests/cost_reduction.rscrates/phase-ai/src/features/tests/mod.rscrates/phase-ai/src/policies/cost_reduction.rscrates/phase-ai/src/policies/mod.rscrates/phase-ai/src/policies/registry.rscrates/phase-ai/src/policies/tests/cost_reduction.rscrates/phase-ai/src/policies/tests/mod.rs
| pub(crate) fn your_spell_discount_parts<'a>( | ||
| statics: impl IntoIterator<Item = &'a StaticDefinition>, | ||
| ) -> u32 { | ||
| statics | ||
| .into_iter() | ||
| .filter_map(your_spell_discount) | ||
| .fold(0u32, u32::saturating_add) | ||
| } | ||
|
|
||
| /// CR 601.2f: the generic discount this one static gives to spells you cast, or | ||
| /// `None` when it is not a board-wide reduction of your own spells. | ||
| /// | ||
| /// Mirrors the eligibility tests `casting::collect_cost_modifiers` applies at | ||
| /// cast time, so deck classification and the resolver agree by construction: | ||
| /// `Reduce` mode only, never a `SelfRef` self-cost reduction, and never an | ||
| /// opponent-scoped modifier. | ||
| fn your_spell_discount(def: &StaticDefinition) -> Option<u32> { | ||
| let StaticMode::ModifyCost { | ||
| mode: CostModifyMode::Reduce, | ||
| amount, | ||
| .. | ||
| } = &def.mode | ||
| else { | ||
| // `Raise` (Thalia) and `Minimum` (Trinisphere) are taxes, not discounts. | ||
| return None; | ||
| }; | ||
|
|
||
| // CR 113.6: a `SelfRef` reduction is "this spell costs {N} less" — resolved | ||
| // by `apply_self_spell_cost_modifiers` for the spell being cast, and never | ||
| // applied from a battlefield permanent to other spells. It is a property of | ||
| // one card, not a deck-wide engine. | ||
| if matches!(def.affected, Some(TargetFilter::SelfRef)) { | ||
| return None; | ||
| } | ||
|
|
||
| // CR 601.2f: caster scope. `Opponent` is a discount handed to the other | ||
| // side; only `You` and an unscoped modifier reduce spells you cast. | ||
| if let Some(TargetFilter::Typed(typed)) = &def.affected { | ||
| if matches!(typed.controller, Some(ControllerRef::Opponent)) { | ||
| return None; | ||
| } | ||
| } | ||
|
|
||
| // CR 118.7a: only the generic component of a cost can be reduced by a | ||
| // generic reduction, so the discount magnitude is `generic`. A reduction of | ||
| // zero generic mana (a purely colored `amount`) moves no cost here and is | ||
| // not counted as an engine. | ||
| let ManaCost::Cost { generic, .. } = amount else { | ||
| return None; | ||
| }; | ||
| (*generic > 0).then_some(*generic) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Caster/mode eligibility is hand-duplicated instead of exposed from the engine, unlike the type axis.
your_spell_discount reimplements CR 601.2f eligibility (Reduce-only, not SelfRef, not Opponent-scoped) as a parallel copy of casting::collect_cost_modifiers's rules, per this file's own doc comment ("Mirrors the eligibility tests casting::collect_cost_modifiers applies at cast time, so deck classification and the resolver agree by construction"). The type axis avoided this exact problem by having matches_type_filter_against_face elevated to pub in engine::game::filter so phase-ai calls the authority directly instead of re-deriving CR 205 semantics. The caster/mode axis didn't get the same treatment — it's a hand-copy that can silently drift from the actual resolver if collect_cost_modifiers's rules ever change (new caster-scope variant, adjusted SelfRef handling, etc.), causing deck-feature detection and policy scoring to disagree with real game behavior.
Suggested direction
Expose an authoritative, CardFace/StaticDefinition-compatible eligibility check from engine (mirroring how matches_type_filter_against_face was elevated), and have casting::collect_cost_modifiers and phase-ai's your_spell_discount both call it.
Based on learnings, the repo's CLAUDE.md explicitly states phase-ai "should orchestrate/scoring using engine-provided semantics, not duplicate game rules."
🤖 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/features/cost_reduction.rs` around lines 157 - 208,
Expose the Reduce-mode, non-SelfRef, non-Opponent-scoped eligibility logic used
by casting::collect_cost_modifiers as a shared engine helper for
StaticDefinition/CardFace-compatible inputs, following the existing
matches_type_filter_against_face pattern. Update both
casting::collect_cost_modifiers and phase-ai's your_spell_discount to call this
authoritative helper, leaving your_spell_discount responsible only for
extracting and validating the generic amount.
Source: Path instructions
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed against current head d360a8a5f55425e4b630d40f907f1bd655d1c653.
This cannot merge yet: the new feature and policy score reductions that the casting authority would not apply, while also missing supported reducer classes. The existing CodeRabbit findings are confirmed against this head; the condition gap below is an additional current-head blocker.
[MED] Deck-time filter matching discards every nonempty TypedFilter.properties list. Evidence: crates/phase-ai/src/features/cost_reduction.rs:243-249 requires typed.properties.is_empty(), even though StaticMode::ModifyCost carries the whole TargetFilter; the engine's matches_target_filter_against_face is the context-free CardFace authority at crates/engine/src/game/filter.rs:1263-1294. Why it matters: reducers for color/colorless/historic/mana-value spell classes are silently treated as discounting no deck cards, so discounted_count and commitment are wrong. Suggested fix: expose and extend/reuse the engine's full static face matcher for every context-free TargetFilter property, rather than reimplementing the type axis here; add property-filter regressions, including an unsupported/live-only property failing closed.
[MED] The tactical policy counts and rewards every nonland card rather than the spells each reducer actually discounts. Evidence: crates/phase-ai/src/policies/cost_reduction.rs:79-96 derives only a generic amount, then castable_cards_in_hand at :121-138 counts all nonlands; hand_holds_cheaper_reducer at :143-153 likewise never inspects spell_filter. Why it matters: a narrow reducer is rewarded for irrelevant future cards and causes an incorrect sequencing penalty for a spell it cannot reduce. Suggested fix: retain/evaluate each qualifying reducer's spell_filter against the candidate and hand spell through an engine-owned matcher; cover both deploy-credit and defer-penalty mismatches, while preserving the unfiltered case.
[MED] dynamic_count reductions are valued as a fixed base amount. Evidence: crates/phase-ai/src/features/cost_reduction.rs:173-207 returns only amount.generic, while the cast authority resolves dynamic_count and applies it as a multiplier in crates/engine/src/game/casting.rs:7307-7324. Why it matters: the policy credits a reduction when the live multiplier is zero and under-credits it when the multiplier exceeds one. Suggested fix: expose an engine-owned live modifier evaluation for policy use (or conservatively fail this feature off when a live multiplier cannot be evaluated); add zero and greater-than-one multiplier regressions.
[MED] Conditional reducers are treated as unconditional. Evidence: the feature's qualifying path at crates/phase-ai/src/features/cost_reduction.rs:173-221 never examines StaticDefinition.condition, but collect_cost_modifiers gates every modifier with evaluate_cost_mod_static_condition at crates/engine/src/game/casting.rs:7278-7290. Why it matters: the AI can reward deploying a reducer during a turn/state in which it provides no discount. Suggested fix: consume the same engine eligibility/evaluation authority, or conservatively fail off when a deck-time condition cannot be evaluated; add a regression for an own-turn candidate with a negated own-turn condition.
[MED] Board-wide ModifyCost eligibility is duplicated in phase-ai instead of having one authority. Evidence: your_spell_discount recreates Reduce/SelfRef/opponent checks at crates/phase-ai/src/features/cost_reduction.rs:169-207, whereas the casting authority additionally owns functioning-zone, player-scope, condition, selected-target, filter, and dynamic-count checks at crates/engine/src/game/casting.rs:7222-7324. Why it matters: the two paths have already diverged in conditions and dynamic counts and will continue to drift for future reducer forms. Suggested fix: move/expose structured eligibility facts and live evaluation from the engine, then make both casting and phase-ai consume that authority rather than parallel predicates.
[LOW] The CR 121.1 doc comment is attached to CostReduction rather than DrawPayoff. Evidence: crates/phase-ai/src/policies/registry.rs:145-149; CR 121.1 in docs/MagicCompRules.txt:1142 defines drawing a card. Why it matters: the enum documentation states the wrong rule/domain and leaves DrawPayoff undocumented. Suggested fix: move the verified CR 121.1 comment directly above DrawPayoff; add only a separately verified, accurate comment for CostReduction if the surrounding policy IDs require one.
The parse-diff evidence for this head reports no card-parse changes. CI is green; AI gates remain in progress and are not used as a blocker for this review. Please address the correctness and ownership gaps above, then request re-review.
d360a8a to
ef0fa8c
Compare
github-actions
Bot
added
the
needs-maintainer
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed against current head ef0fa8c9b70fbd1419ce115fd224cf0b8b0c8b03.
The 21-line documentation follow-up does not resolve the implementation blockers from the prior review, so this remains changes requested.
[HIGH] The phase-AI feature duplicates only part of the engine's cost-modifier authority. Evidence: crates/phase-ai/src/features/cost_reduction.rs:173-215 recognizes a Reduce static based on its definition, while crates/engine/src/game/casting.rs:7217-7324 additionally selects functioning statics and applies source zone, caster scope, condition, spell-filter, and dynamic-multiplier semantics before a modifier exists. Why it matters: the AI can award a cost-reduction payoff to a static that cannot reduce the candidate player's future spells in the live game. Suggested fix: expose/reuse an engine-owned classification or evaluation seam that agrees with cost collection, rather than maintaining a partial phase-AI copy.
[MED] Dynamic reductions and static conditions are deliberately ignored by the feature/policy but are live casting semantics. Evidence: crates/phase-ai/src/features/cost_reduction.rs:205-215 credits one fixed generic unit and does not resolve dynamic_count; crates/phase-ai/src/policies/cost_reduction.rs:79-96 scores that fixed value; crates/engine/src/game/casting.rs:7278-7324 gates modifiers on condition and resolves dynamic_count. Why it matters: the policy can favor deploying a reducer that is currently disabled or value a zero-multiplier reducer as if it saved mana. Suggested fix: make the scoring path consume the same condition and quantity semantics as the casting authority, with tests for an inactive condition and a zero/nonzero dynamic multiplier.
[MED] The policy counts every nonland card in hand even when the reducer's spell_filter admits only a subset. Evidence: crates/phase-ai/src/policies/cost_reduction.rs:83,122-162 calls castable_cards_in_hand, which filters only lands, whereas the engine applies the modifier's spell_filter at crates/engine/src/game/casting.rs:7293-7305. Why it matters: an instant-and-sorcery or artifact-only reducer is rewarded for unrelated future spells, producing incorrect sequencing incentives. Suggested fix: evaluate the specific reducer's filter for each candidate future spell through a shared engine authority; deck-level commitment is not a substitute for the live hand's composition.
[MED] Deck-time filter matching rejects static property filters that the engine can evaluate against a card face. Evidence: crates/phase-ai/src/features/cost_reduction.rs:249-265 requires typed.properties.is_empty(), while crates/engine/src/game/filter.rs:1263-1294 supports context-free property evaluation, including HasSupertype. Why it matters: valid reducers such as filters restricted by a printed supertype are silently treated as covering no spells. Suggested fix: delegate the full context-free filter match to matches_target_filter_against_face instead of reproducing only the type axis.
[LOW] The CR annotation is attached to the wrong policy reason. Evidence: crates/phase-ai/src/policies/registry.rs:147-149 documents drawing-card payoff semantics immediately above CostReduction, not DrawPayoff. Why it matters: rule annotations are part of the engine's audit trail and this one misstates the concern represented by the enum member. Suggested fix: move the CR 121.1 comment to DrawPayoff and annotate CostReduction with the relevant cost-modification rule if an annotation is intended.
The parser parse-diff reports no changes, and pending AI gates are not blockers for this review outcome; the blockers above are current-head behavioral and ownership defects.
Addresses the review on phase-rs#6743. Every MED shared one root cause — phase-ai MIRRORED `collect_battlefield_cost_modifiers`'s eligibility instead of sharing it — so this fixes the ownership rather than patching each symptom. Engine (new single authority): - `StaticDefinition::board_wide_cost_modifier()` + `CostModifierCasterScope` (types/ability.rs) is now the one structural definition of "is this a board-wide cost modifier and what are its terms": rejects `Minimum` and `SelfRef` (CR 113.6), and names the caster scope so a caller WITH a PlayerId (the casting pipeline) and one WITHOUT (deck analysis) ask the same question. `collect_battlefield_cost_modifiers` now consumes it, so the two paths cannot drift again. - `matches_target_filter_against_face_scoped` + `context_free_prop_matches_face` (game/filter.rs) extend the CR 205 face authority to every context-free `FilterProp` — mana value (CR 202.3), color (CR 105.2), keyword, supertype, token-ness — and return `Option<bool>` so a live-only property is *unanswerable* and fails closed by construction rather than by a caller remembering to. `FaceControllerScope` names the previously-implicit controller assumption. phase-ai (consumes, no longer mirrors): - `your_spell_discount` reads the engine authority; the hand-rolled type-axis matcher is deleted. Color-, mana-value- and keyword-scoped reducers now produce coverage instead of silently reading as discounting nothing. - New `live_your_spell_discounts` resolves what a structural read cannot: `condition` fails off (a card in hand has no truthful "as long as" answer), and `dynamic_count` is resolved through the engine's `resolve_quantity`, so a multiplier of zero earns no credit and a multiplier above one scales it. - The policy counts and penalizes only spells a reducer actually discounts, matched through the engine's live `matches_target_filter` with the reducer as filter source. - `PolicyId::DrawPayoff` gets its CR 121.1 doc comment back; `CostReduction` gets its own verified CR 601.2f one. Tests 33 -> 44. Six new regressions, each proven RED by breaking the specific invariant it guards: color/mana-value/negated-color coverage, live-only property failing closed, controller-scoped filter admitted, filter-narrowed deploy credit and defer penalty, conditional reducer earning nothing, and zero vs positive dynamic multipliers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed at [MED] Board-wide [MED] [MED] [MED] Conditional reducers treated as unconditional. [MED] Policy rewarded/penalized every nonland card. [LOW] Misplaced CR 121.1 comment. Verification (head
Re-running |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/engine/src/types/ability.rs (1)
20885-20989: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo unit tests for the new shared cost-modifier authority.
cost_modifier_caster_scope,board_wide_cost_modifier,admits, andadmits_own_controllerare new public building blocks that both the casting pipeline and phase-ai's deck feature detection now depend on (per PR objectives), but this file'stestsmodule has no coverage for any of them — not even the basic You/Opponent/Any scope resolution or theMinimum/SelfRefexclusion inboard_wide_cost_modifier.As per path instructions: "Reuse existing shared building blocks before adding utility or inline extraction logic; test the building block and its parameter range rather than a single card case." A single-authority function consumed by two different subsystems is exactly the case where a small parameter-range unit test (You/Opponent/Any × with/without spell_filter × Minimum/other modes × SelfRef/non-SelfRef
affected) pays for itself.🤖 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/types/ability.rs` around lines 20885 - 20989, The new shared cost-modifier helpers lack unit coverage. Add focused parameterized tests in the existing tests module for cost_modifier_caster_scope and CostModifierCasterScope::admits/admits_own_controller across You, Opponent, and Any, and for StaticDefinition::board_wide_cost_modifier covering accepted modes, spell_filter presence, and rejection of Minimum and SelfRef affected values.Source: Path instructions
🤖 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/game/filter.rs`:
- Around line 1346-1393: Update the FilterProp::AnyOf branch in
context_free_prop_matches_face to short-circuit and return Some(true) as soon as
any operand evaluates to Some(true), without evaluating or propagating later
None results. Preserve Some(false) when all operands are answerable and false,
and return None only when no true operand exists and at least one operand is
unanswerable.
---
Nitpick comments:
In `@crates/engine/src/types/ability.rs`:
- Around line 20885-20989: The new shared cost-modifier helpers lack unit
coverage. Add focused parameterized tests in the existing tests module for
cost_modifier_caster_scope and
CostModifierCasterScope::admits/admits_own_controller across You, Opponent, and
Any, and for StaticDefinition::board_wide_cost_modifier covering accepted modes,
spell_filter presence, and rejection of Minimum and SelfRef affected 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: 916b0b3c-ac6f-448e-985e-6370ad70653f
📒 Files selected for processing (8)
crates/engine/src/game/casting.rscrates/engine/src/game/filter.rscrates/engine/src/types/ability.rscrates/phase-ai/src/features/cost_reduction.rscrates/phase-ai/src/features/tests/cost_reduction.rscrates/phase-ai/src/policies/cost_reduction.rscrates/phase-ai/src/policies/registry.rscrates/phase-ai/src/policies/tests/cost_reduction.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/phase-ai/src/policies/registry.rs
- crates/phase-ai/src/policies/cost_reduction.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — two blockers remain on head f8d3e4d.
🔴 Blocker
[HIGH] Raw keyword membership bypasses the engine keyword authority. Evidence: crates/engine/src/game/filter.rs:1379-1380 adds face.keywords.contains calls; CI run 30410592730 fails the Engine authority gate on those exact lines. Why it matters: this creates a second keyword-evaluation path that can diverge from the established authority. Suggested fix: use the existing keyword authority, or add the gate-prescribed structural annotation with a justification.
🟡 Blocker
[MED] FilterProp::AnyOf does not implement three-valued OR. Evidence: crates/engine/src/game/filter.rs:1387-1389 uses try_fold, so [Some(true), None] produces None; the caller at filter.rs:1314-1320 then rejects it. Why it matters: a matching alternative can be discarded whenever a later alternative is unknown. Suggested fix: implement three-valued OR and add cases covering [Some(true), None], [None, Some(true)], and all-unknown inputs.
The parse-diff is current and reports no card changes. Relevant CR citations were verified and are not blockers.
Recommendation: address both findings, then request re-review.
|
Run on this head and as a paired control on the PR's parent ( That is the entire difference — the OS thread id inside a pre-existing panic message, which varies per process. Every game result, deck verdict, flip count and p-value is identical on both sides:
So the two WARNs, the No baseline refresh is included, deliberately: refreshing here would absorb pre-existing drift this PR did not cause.
|
…hority gate CI's `Engine authority gate` (scripts/check-engine-authorities.sh) rejects raw `obj.keywords.contains(..)` in new engine code, because a raw query silently misses off-zone keyword grants that only `keywords::object_has_effective_keyword_kind(state, id, kind)` can see. `context_free_prop_matches_face` is the structural exception the gate documents. Its whole contract is that NO `GameObject` exists — it evaluates a bare `CardFace` outside the game — so there is no zone, no grant, and no `off_zone_characteristics` to consult, and the printed keyword line is the complete truth. The keyword authorities all require a `GameObject` this function by definition cannot have; a caller holding an `ObjectId` is already directed to the object-based `matches_target_filter` family by the module docs. Annotated with `allow-raw-authority:` and the reason, per the gate's own instructions, rather than reshaping the call to defeat the check. Verified: `./scripts/check-engine-authorities.sh upstream/main` exits 0, as do `cargo fmt --all -- --check`, `check-interaction-bindings.sh --check`, `check-prelowered-ratchet.sh`, `check-skill-doc.sh` and `check-resolution-frame-boundaries.sh` — the other gates in the same CI job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
CI triage for the previous head, fixed at
That function is the structural exception the gate documents: its entire contract is that no I then ran the rest of that job's gates locally — all exit 0:
Everything else was already green on the previous head: both Rust test shards, card data + coverage, WASM, Tauri, frontend, lobby worker, decision-cost perf gate, CodeRabbit. One disclosure: I could not reproduce CI's full |
|
One thing I should surface rather than let you find it in review. Extending
Before, that function honored exactly one property ( The change is one-directional — previously- I kept it rather than special-casing the new path, because forking a second face matcher for deck analysis would recreate the exact duplicate-authority defect this round exists to remove. But it is real scope beyond the cost-reduction axis, so it is now declared under I also corrected two other body sections that had gone stale against the current head: |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — one current-head blocker remains on cdd72457ee3dc41a3686462b79693a8e38c48021.
🔴 Blocker
[MED] FilterProp::AnyOf does not implement three-valued OR. Evidence: crates/engine/src/game/filter.rs:1395-1397 uses try_fold; after a context-free Some(true), a later live-state-only branch yields None, so [Some(true), None] becomes None. The typed-filter caller at filter.rs:1314-1320 admits only Some(true), rejecting that definite static match. The reverse order ([None, Some(true)]) also fails because try_fold stops at the first None. Why it matters: a spell filter with a matching static alternative can be treated as non-matching merely because another alternative needs game state. Suggested fix: implement three-valued OR: short-circuit true; return None only when no branch is true and at least one is unknown; otherwise return false. Add order-sensitive cases for [Some(true), None], [None, Some(true)], all-unknown, and false-plus-unknown.
The raw-keyword authority gate is resolved by the structural annotations and Rust lint is green. The current parse diff reports no card changes; relevant CR citations are verified. Certain CI checks remain pending but are not this review blocker.
Recommendation: fix the tri-state OR and add the order-sensitive regressions, then request re-review.
Addresses the round-3 blocker on phase-rs#6743. `context_free_prop_matches_face` returns `Option<bool>` where `None` means "this property needs live game state and has no face-level answer". The `AnyOf` arm folded with `try_fold`, which short-circuits on the first `None` — so a disjunction mixing a definite match with an unknowable alternative collapsed to unknown in BOTH orders: [Some(true), None] -> None (the true was already found, then discarded) [None, Some(true)] -> None (stopped before reaching the true) `matches_target_filter_against_face_scoped` admits only `Some(true)`, so a spell filter with a definitely-matching alternative was treated as non-matching purely because a sibling alternative referenced live state. Replaced with a proper CR 608.2b Kleene OR: short-circuit on the first definite `true`; return `None` only when nothing is true AND something is unknown; otherwise `false`. Negation already had the correct behavior — `None.map(!)` is `None` — and now has a regression pinning it. Eight tests covering the full truth table, including both orderings the review called out and an end-to-end case through the typed-filter caller (the production entry point that admits only `Some(true)`). Three are proven RED against the old `try_fold`: `any_of_true_then_unknown_is_true`, `any_of_unknown_then_true_is_true`, and `tri_state_or_reaches_the_typed_filter_caller`. The other five pin the rest of the table and pass either way by design. Verified: engine `game::filter` 149 passed, phase-ai cost_reduction 44 passed, `cargo clippy -p engine -p phase-ai --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 Since Replaced with a CR 608.2b Kleene OR: short-circuit on the first definite FilterProp::AnyOf { props } => {
let mut saw_unknown = false;
for inner in props {
match context_free_prop_matches_face(face, inner) {
Some(true) => return Some(true),
Some(false) => {}
None => saw_unknown = true,
}
}
(!saw_unknown).then_some(false)
}Eight regressions covering the full truth table, including both orderings you named:
The last one goes through the production entry point — the typed-filter caller that admits only Proven RED against the old
Verification at this head:
Requesting re-review. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — two current-head blockers remain on d3c45f57fafec1b2b5722babb12f7a1f6021e5d8.
🔴 Blocker
[HIGH] Face-level TargetFilter::Not turns an unknown live property into a match. Evidence: crates/engine/src/game/filter.rs:1314-1320 collapses every property other than Some(true) to false, then :1328-1329 negates that result. Thus a bare face evaluated against Not(Tapped) (where Tapped returns None) is accepted; the same inversion applies to an unanswerable controller scope. Why it matters: face matching claims a static filter matches even though its only predicate requires a live object, violating the fail-closed contract and admitting the wrong card class. Suggested fix: preserve the three-state answer recursively through TargetFilter::{Typed,Or,And,Not} and expose only a definite true at the boolean boundary. Add production-entry regressions for Not(Tapped) and Not around an unknown controller scope.
🔴 Blocker
[MED] The new Kleene-OR comments cite an unrelated rule. Evidence: crates/engine/src/game/filter.rs:1396-1401 and the test heading at :12777 cite CR 608.2b, while docs/MagicCompRules.txt:2789 says that a resolving spell/ability checks target legality. Why it matters: this creates a false rules-audit trail for ordinary Boolean semantics. Suggested fix: remove that citation (a CR rule is not needed for the implementation’s three-valued Boolean behavior), or replace it only with a separately verified rule that actually governs the annotated code.
The prior AnyOf order-sensitive blocker is resolved on this head. Recommendation: preserve unknown through outer filter negation, correct the annotations, and request re-review.
Addresses the two round-4 blockers on phase-rs#6743. [HIGH] `TargetFilter::Not` turned an unanswerable predicate into a match. The `Typed` arm collapsed every non-`Some(true)` property to `false` before returning, so an outer `Not` inverted it: a bare face evaluated against `Not(Tapped)` — whose only predicate needs a battlefield object — was ACCEPTED, admitting the wrong card class through a filter that should have failed closed. The same inversion applied to an unanswerable controller scope, and to the `_ => false` arm covering `SelfRef` and the player-scoped variants. Fixed by threading the third state through the whole walk instead of collapsing early: `target_filter_face_state` returns `Option<bool>` across `Typed`/`Or`/`And`/`Not`, combining with `kleene_and` / `kleene_or`, and `matches_target_filter_against_face_scoped` collapses to a bool exactly once at the public boundary. Unanswerable filter variants now yield `None` rather than a definite `false`, so negating them cannot manufacture a match. One asymmetry is deliberate: under `FaceControllerScope::AssumeOwn` the caller has asserted the face is the querying player's own card, so `ControllerRef::Opponent` is definitely FALSE (not unknown) and its negation is a legitimate definite match. Encoding that as `Some(false)` keeps the fix from over-correcting into "every controller scope is unknowable". [MED] Removed the fabricated `CR 608.2b` citations from the Kleene-OR comment and its test heading. Verified against docs/MagicCompRules.txt:2789: 608.2b governs checking target legality as a spell or ability resolves, and has nothing to do with Boolean disjunction. Three-valued logic here is an implementation property of the `Option<bool>` answer, not a rules behavior, so it carries no CR annotation now. The four other 608.2b references in this file are pre-existing and untouched. Eight production-entry regressions through `matches_target_filter_against_face_scoped`, covering `Not` around: a live-only property, an unknown controller scope, an unanswerable filter variant, a definite false, and a definite true; plus opponent-scope-under-AssumeOwn, AND with an unknown branch, and OR with a definite true beside an unknown. Proven RED against the old collapsed form: `not_around_live_only_property`, `not_around_unknown_controller_scope`, and `not_around_an_unanswerable_filter_variant` all fail when `Not` is restored to negating the boolean; the two definite-answer cases correctly stay green. Verified: engine `game::filter` 157 passed, phase-ai cost_reduction 44 passed, `cargo clippy -p engine -p phase-ai --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>
|
Both fixed at [HIGH]
|
|
Current-head review hold for Next step: wait for the current-head checks and parse-diff artifact, then perform a fresh implementation review before any approval or queue action. |
|
Maintainer update completed for current head |
|
Reviewed the current head The current implementation resolves the prior blockers: both the casting pipeline and deck-time classifier use I am holding rather than approving/enqueuing while the current-head Paired-seed AI gate and Decision-cost perf gate are still in progress. The required Rust/Frontend checks are green. I will make the merge decision from these exact-head results; no contributor change is requested. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved on current head c3f4ff8ba31337f1ca54241503f58b6864ff9f0d.
The review resolves the earlier filter-semantics and cost-modifier-authority findings. The current parse-diff reports no card-parse changes; required Rust and frontend checks are green. The paired-seed AI gate remains in progress and auto-merge will wait for repository requirements.
|
Thanks for the approval. One data point on the pending Paired-seed AI gate, since it is the last thing standing and it is not reporting what it looks like. It did not fail — it was cancelled by the 60-minute job timeout: The same job on the previous head ( For whatever it is worth toward the merge decision, I ran the paired control locally at the true parent (
That result predates this round's changes, but those only ever narrow what the policy credits (spell-filter matching, condition fail-off, zero-multiplier skip, and unknown-preserving negation), so they cannot widen activation. No quick-filter deck clears Entirely your call on how to treat the timed-out gate — I am not requesting anything, just making sure the |
2 participants
Summary
Adds a
cost_reductiondeck-feature axis and its companionCostReductionPolicytophase-ai, so the AI values deploying a CR 601.2f cost reducer before the spells it discounts.features::mana_rampexplicitly deferred this shape in its module docstring — "StaticMode::ModifyCostis deliberately out of scope — cost reducers are a follow-up feature" — and this is that follow-up.The engine already applies the discount at cast time, so the AI was never overcharged; what was missing is any reason to sequence the reducer first.
RampTimingPolicysupplies exactly that signal for permanents that add mana and structurally cannot see a cost reducer, so a deck whose entire acceleration plan is cost reduction read as having no ramp at all.Files changed
crates/phase-ai/src/features/cost_reduction.rs(new)crates/phase-ai/src/features/tests/cost_reduction.rs(new)crates/phase-ai/src/policies/cost_reduction.rs(new)crates/phase-ai/src/policies/tests/cost_reduction.rs(new)crates/phase-ai/src/features/mod.rscrates/phase-ai/src/features/tests/mod.rscrates/phase-ai/src/features/mana_ramp.rscrates/phase-ai/src/policies/mod.rscrates/phase-ai/src/policies/registry.rscrates/phase-ai/src/policies/tests/mod.rscrates/phase-ai/src/config.rscrates/engine/src/game/filter.rscrates/engine/src/game/casting.rscrates/engine/src/types/ability.rsTrack
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: not-applicable — the feature and policy are AI heuristics under
crates/phase-ai/.Disclosure, because this is no longer a one-line engine touch after the review round: addressing the maintainer's "one authority" finding required real
crates/engine/changes.types/ability.rs—BoardWideCostModifier,CostModifierCasterScope,StaticDefinition::board_wide_cost_modifier(). A structural accessor over existing AST; adds no rules behavior.game/casting.rs—collect_battlefield_cost_modifiersrefactored to consume that accessor. Behavior-preserving: the sameMinimum/SelfRef/caster-scope decisions, now read from one place instead of two.game/filter.rs—matches_target_filter_against_face_scoped+context_free_prop_matches_face, extending the CR 205 face authority as the review asked. See Scope Expansion.None of it is parser, effect-resolution, targeting or stack behavior. I am happy to route it through
/engine-implementerif you would rather it went that way.CR references
CR 601.2f— cost determination; reductions are subtracted from the total cost. Governs the whole axis.CR 118.7a— a generic cost reduction affects only the generic component of a cost. Fixes the discount magnitude toamount'sgeneric, not its mana value.CR 113.6— abilities of non-instant/sorcery objects function on the battlefield; aSelfRefreduction is a property of one spell, not a board-wide engine.CR 205— type-line semantics, via the engine authoritymatches_type_filter_against_face.CR 305.1— playing a land is not casting a spell, so lands are excluded from coverage and from "future casts".All five were verified against
docs/MagicCompRules.txtbefore being written.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—1672 passed; 0 failed(lib) plus all integration binaries green, includingevery_policy_penalty_is_tuning_registered_or_explicitly_untuned, which enforces that both newPolicyPenaltiesfields are registered.cargo test -p phase-ai --lib cost_reduction—33 passed; 0 failed.cargo clippy -p phase-ai -p engine --all-targets --features proptest -- -D warnings— exit 0, no diagnostics. Re-run after the rebase onto1604a6f30.cargo fmt --all— clean../scripts/check-parser-combinators.sh— PASS (see Gate A).cargo ai-gate— run on this head and as a paired control on the pre-change parent. Diffing the two full runs, the only difference in the entire output is the nondeterministic OS thread id inside a pre-existing panic line:Every game result, deck verdict, flip count and p-value is byte-identical —
compare: 0 FAIL, 2 WARN, 1 PASS, 0 NEW, 0 REMOVEDon both sides. The two WARNs (enchantress-mirror50→10%,red-mirror70→50%), theenchantress-mirrorsuite FAIL, and theMetallic Rebukepanic (same seed10592737) all reproduce unchanged on the parent commit and are pre-existing and unrelated to this PR. No quick-filter deck clearsCOST_REDUCTION_FLOOR, so the policy is inert across the suite. No baseline refresh is included — refreshing would launder pre-existing drift this PR did not cause.cargo ai-perf-gate— not applicable. This policy adds no board-wide or affordability engine call (max_x_value,feasible_mana_capacity,find_legal_targets,SimulationFilterclone).verdict()runs a card-local static check first and only a confirmed reducer pays for a hand-bounded walk.Mutation evidence that the tests are not vacuous. Three invariants were individually broken in the implementation — the
SelfRefexclusion, the caster-scope check, and the coverage pillar (forced to full) — and 9 tests failed, each in the branch it guards. Restored to green afterwards.Gate A
Gate A PASS head=c1a6f040c1ad69432a6366c314d3717091b571eb base=1604a6f3023413a34740d6d268d77872b3150b0f
Anchored on
crates/phase-ai/src/features/draw_matters.rs:75— the established axis shape this mirrors:detect()folding per-DeckEntryCardFaceAST into counts,pub(crate)parts-based predicates shared with the policy, and acommitmentscalar;:248is the same two-pillarcommitment::geometric_meanformula with a calibration anchor in its docstring.crates/phase-ai/src/policies/draw_payoff.rs:45— the sameTacticalPolicyshape:activation()as a single floor-plus-scale knob returningOption<f32>, and:67the cheap-to-expensiveverdict()ordering where a card-local AST check rejects non-members before any wider scan (MAX_REWARDED_ENGINESis the cap constant this PR'sMAX_REWARDED_FUTURE_CASTS/MAX_REWARDED_DISCOUNTfollow).crates/engine/src/game/casting.rs:7179—collect_battlefield_cost_modifiers, the runtime authority whose eligibility tests the deck-time classifier mirrors exactly:Reducemode only,SelfRefskipped,ControllerRef::Opponentskipped.crates/phase-ai/src/policies/ramp_timing.rs:102— thedefer_to_rampverdict this PR'scost_reduction_defer_to_enginebranch follows.Final review-impl
Final review-impl PASS head=c1a6f040c1ad69432a6366c314d3717091b571eb
Two review rounds are folded into this head.
Round 1 — maintainer review (5 MED + 1 LOW), fixed at
f8d3e4dfd. All fiveMEDs shared one root cause the review named directly: phase-ai MIRRORED
collect_battlefield_cost_modifiers's eligibility instead of sharing it. Fixedthe ownership, not the symptoms:
StaticDefinition::board_wide_cost_modifier()+CostModifierCasterScope(crates/engine/src/types/ability.rs) is now thesingle structural definition — rejects
MinimumandSelfRef(CR 113.6),and names the caster scope so a caller with a
PlayerIdand one without askthe same question of the same code. Both casting and phase-ai consume it.
TypedFilter.propertieshonored. Extended the engine's face authority(
matches_target_filter_against_face_scoped+context_free_prop_matches_face) to every context-free property — mana value(CR 202.3), color (CR 105.2), keyword, supertype, token-ness — composing
through
Not/AnyOf. ReturnsOption<bool>, so a live-only property isunanswerable and fails closed by construction.
dynamic_countresolved through the engine'sresolve_quantity;multiplier 0 earns nothing, >1 scales.
conditionfails off — a candidate in hand has no truthful "as long as"answer, so it earns no credit rather than a guess.
engine's live
matches_target_filterwith the reducer as filter source.DrawPayoff(a regression from theenum insertion);
CostReductionhas its own verified CR 601.2f comment.Round 2 — CI
Engine authority gate, fixed atc1a6f040c. Two rawface.keywords.contains(..)reads incontext_free_prop_matches_face.Annotated
// allow-raw-authority:per the gate's own instructions: thatfunction's contract is that no
GameObjectexists, so there is no zone and nogrant to miss and the printed line is the complete truth.
Round 3 —
FilterProp::AnyOftri-state OR, fixed atc1a6f040c. TheAnyOfarm folded withtry_fold, which short-circuits onOption::None, so adisjunction mixing a definite match with a live-state-only alternative collapsed
to unknown in both orders — and the typed caller admits only
Some(true), so adefinitely-matching spell filter read as non-matching. Replaced with a CR 608.2b
Kleene OR (short-circuit true;
Noneonly when nothing is true and something isunknown; else false), plus eight truth-table regressions including both
orderings and an end-to-end case through the production entry point. Three are
proven RED against the old fold.
Round 4 — face-filter negation + a bad CR citation, fixed at
c1a6f040c.TargetFilter::Notinverted an unanswerable predicate into a match, because therecursion collapsed unknown to
falseinsideTyped(and in the_armcovering
SelfRef/player scopes). Replaced with a real three-state walk —target_filter_face_statereturningOption<bool>throughTyped/Or/And/Not, collapsing to a bool exactly once at the publicboundary — plus eight production-entry negation regressions, three proven RED.
Also removed a fabricated
CR 608.2bcitation: verified atdocs/MagicCompRules.txt:2789, that rule governs target legality on resolution,
not Boolean disjunction, so the three-valued logic now carries no CR annotation.
Tests 33 → 60 (44 phase-ai + 16 engine three-state/negation). Every new regression was proven RED by breaking the specific
invariant it guards — the six mutations were: drop the condition gate, pin the
dynamic multiplier to 1, drop the hand filter-narrowing, and make the color and
mana-value properties unanswerable again.
Claimed parse impact
None. No parser, card-data, or Oracle-text code is touched; the axis reads already-parsed AST.
Scope Expansion
One, disclosed rather than buried, because it reaches beyond this axis.
matches_target_filter_against_facepreviously honored exactly one property (HasSupertype) and returnedfalsefor every other. Extending it — which is what the review asked for ("expose and extend/reuse the engine's full static face matcher for every context-freeTargetFilterproperty") — necessarily changes what its two pre-existing callers see:crates/engine/src/game/effects/search_outside_game.rs:52crates/engine/src/game/effects/create_token_copy_from_pool.rs:112A filter carrying a context-free property — color (CR 105.2), mana value (CR 202.3), keyword,
NotSupertype, token-ness — previously matched nothing through those paths and now matches correctly. The change is strictly toward rules-correctness and is one-directional (previously-falsecases can becometrue; nothing that matched stops matching). Both Rust test shards are green on the head carrying it.I kept it rather than special-casing the new deck-analysis path, because forking a second face matcher would recreate the exact duplicate-authority defect this review round exists to remove. Say the word if you would rather the extension landed as its own PR and this one consumed only the type axis.
Validation Failures
None.
CI Failures
None outstanding. Resolved on earlier heads:
Rust lintfailed at the Engine authority gate step (not clippy) — fixed atc1a6f040c, see Final review-impl above. The other gates in that job were then run locally and all exit 0:check-engine-authorities.sh,cargo fmt --all -- --check,check-interaction-bindings.sh --check,check-prelowered-ratchet.sh,check-skill-doc.sh,check-resolution-frame-boundaries.sh.Paired-seed AI gatereportedcancelledafter 1h0m, so it carried no verdict. Neutrality is evidenced by a paired control instead (below).Contributor trustreportsaction_required(account-reputation scan, not a code signal). No code change affects it.Disclosure: CI's full
--workspaceclippy is not reproducible locally here —openssl-sysfails to build for want of an OpenSSL dev package, in a crate this PR does not touch. The exact CI invocation scoped to the two crates the diff touches (-p engine -p phase-ai --all-targets --features proptest -- -D warnings) exits 0.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes