feat(engine): derive targeting intent from the announcing effect - #6718
Conversation
Every board-target prompt the manabrew adapter emitted was labelled `TargetingIntent::Hostile` — a hardcoded placeholder, contradicted by the `hostile: false` sitting next to it. Buffing your own creature advertised as an attack on it. The earlier diagnosis blamed the protocol for having no neutral member. That was wrong: the engine has the data. `Effect` is in hand when target slots are built, so CR 601.2c's "one slot per announced target" can carry what that target is for. `TargetSelectionSlot` now stores the announcing link's `EffectKind` plus a `TargetEffectDetail` for the two kinds whose unit tag is ambiguous — a zone change's destination and a pump's direction, both read off the payload at construction and unrecoverable afterwards. Stamping is frame-scoped in `collect_target_slots`, mirroring `current_chooser`, so a chained sub-ability does not inherit the head link's effect. `target_intent` projects slot -> label at the interaction layer, leaving `GameState` storing only game facts. The zone arm delegates to the existing `effect_zone_intent`, so an `EffectZoneChoice` and a zone-change target announcement cannot disagree about what a destination means. Measured over the corpus, this labels 70.3% of targeting links, up from a constant wrong answer. The residue is genuinely undirected pumps — dynamic X and opposing "+2/-2" — which stay on the adverse label for the same asymmetric-loss reason the neutral bucket does: a caution affordance on a real buff is recoverable, marking "-4/-4" harmless is not.
📝 WalkthroughWalkthroughTarget-selection slots now carry effect kind and detail metadata. The engine projects that metadata into interaction intents, and the compatibility layer derives board-target intent and hostility from the projection. Tests and fixtures initialize the expanded slot structure. ChangesTarget intent pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TargetSelectionState
participant InteractionProjection
participant ManabrewCompat
TargetSelectionState->>InteractionProjection: project stamped target effect
InteractionProjection->>ManabrewCompat: provide interaction intent
ManabrewCompat->>ManabrewCompat: derive wire intent and hostility
ManabrewCompat-->>TargetSelectionState: construct board-target prompt
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
crates/manabrew-compat/src/lib.rs (2)
4235-4245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Chooseis grouped under a comment that says these arms are unreachable — butChooseis the documented reachable fallback.The registry entry and this function's own doc comment both describe
Chooseas the ~30% neutral population that does reach a CR 115.1 announcement. Folding it in withKeep/Crew/Saddle/Station/RingBearer/Blight/Payunder "these never reach a CR 115.1 target announcement" makes the one lossy, reachable arm invisible, and any code later added to this list silently inheritsHostileon a rationale that does not apply to it.♻️ Split the reachable neutral arm from the unreachable ones
+ // Reachable and lossy: the neutral pick. `TargetingIntent` has no + // neutral member, so this falls back to `Hostile` — least-wrong, not + // safe. Declared as `local.targeting-intent-neutral-inexpressible`. + InteractionIntentCode::Choose => TargetingIntent::Hostile, // These never reach a CR 115.1 target announcement — they belong to the // board-selection and cost-payment models — but the match stays // exhaustive so a new intent code cannot silently fall into `Hostile`. - InteractionIntentCode::Choose - | InteractionIntentCode::Keep + InteractionIntentCode::Keep | InteractionIntentCode::Crew🤖 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/manabrew-compat/src/lib.rs` around lines 4235 - 4245, Separate InteractionIntentCode::Choose from the grouped unreachable arms in the targeting match. Handle Choose under its own comment and preserve its documented neutral fallback behavior, while keeping Keep, Crew, Saddle, Station, RingBearer, Blight, and Pay grouped as unreachable codes that map to TargetingIntent::Hostile.
4156-4171: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
find_mapover all opportunities silently picks a label when the crate elsewhere fails closed.
interaction_promptandsole_open_opportunityboth refuse when the viewer owns more than one opportunity (local.interaction-simultaneous-decisions-unmapped); this helper instead takes whichever Selection surface it finds first. For today'sTargetSelectionstates that is one opportunity, so behaviour is unchanged — but the inconsistency means a future multi-slot case degrades into a wrongintent/hostilepair rather than a declared gap.♻️ Match the single-opportunity posture used elsewhere
- prepared - .interaction - .opportunities - .iter() - .find_map(|opportunity| { - opportunity.surfaces.iter().find_map(|surface| { + let [opportunity] = prepared.interaction.opportunities.as_slice() else { + return InteractionIntentCode::Choose; + }; + opportunity + .surfaces + .iter() + .find_map(|surface| { if let InteractionPresentationSurface::Selection { intent, .. } = surface { Some(*intent) } else { None } - }) }) .unwrap_or(InteractionIntentCode::Choose)🤖 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/manabrew-compat/src/lib.rs` around lines 4156 - 4171, Update projected_target_intent to fail closed unless the interaction contains exactly one opportunity, matching interaction_prompt and sole_open_opportunity; only inspect that opportunity’s Selection surface, and return the existing Choose fallback for zero or multiple opportunities.crates/engine/src/game/interaction.rs (1)
2748-2752: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffWildcard
_overEffectKindconflicts with the repo's exhaustive-match rule.The doc comment makes a reasonable case (231 variants, neutral fallback), but the practical consequence is that any newly added targeting effect silently ships as
Choose→Hostileat the protocol boundary, with no compiler signal. If the 231-arm match is genuinely unacceptable, consider moving the classification next toEffectKindas an exhaustivedisposition()(grouping the unlabelled majority in one explicit arm list), so additions must opt in.As per coding guidelines: "wildcard
_match arms where the enum is known and an exhaustive match would let the compiler catch missing variants" is a finding.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/interaction.rs` around lines 2748 - 2752, The EffectKind classification match currently uses a wildcard fallback that bypasses exhaustive-match checks. Replace the `_ => InteractionIntentCode::Choose` arm in the surrounding classification function with an exhaustive approach, preferably by adding an exhaustive EffectKind disposition method that explicitly groups neutral variants, and use that classification at the protocol boundary so newly added effects require explicit handling.Source: Coding guidelines
🤖 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/types/game_state.rs`:
- Around line 6185-6214: Add #[serde(default = "...")] to
TargetSelectionSlot::effect_kind, referencing a named default function that
returns the appropriate EffectKind. Define or reuse that named default alongside
the existing TargetEffectDetail default so persisted GameState values missing
effect_kind remain deserializable.
In `@crates/manabrew-compat/src/lib.rs`:
- Line 1413: Preserve the published capability code for the affected registry
entry by reverting the rename from local.targeting-intent-unsourceable to
local.targeting-intent-neutral-inexpressible, and update only its reason text as
needed. If retaining the new code, document an explicit exemption in the entry
explaining why the old code was never emitted or externally consumable.
In `@crates/phase-ai/src/policies/anti_self_harm.rs`:
- Around line 1127-1128: Replace the placeholder NoOp/None target-selection
metadata with the corresponding pending effect attribution across all listed
fixtures: in crates/phase-ai/src/policies/anti_self_harm.rs at 1127-1128 derive
it from make_target_selection_ctx’s effect, at 1772-1773 use the discard/draw
effect, at 1860-1861 the targeted discard effect, at 2732-2733 the rewind
ability, at 3003-3004 the aura target context or an explicitly modeled static
effect, at 3346-3347 the exile trigger, and at 3533-3534 the damage ability; in
crates/phase-ai/src/policies/combat_withdrawal.rs at 241-242 derive it from the
installed ability frame; and in
crates/phase-ai/src/policies/tests/removal_lethality.rs at 242-243 derive it
from the helper’s effect argument.
In `@crates/phase-ai/src/policies/context.rs`:
- Around line 286-287: Update the affected TargetSelectionSlot fixtures in the
Pump-effect cases to use the enclosing Effect::Pump’s concrete EffectKind and
TargetEffectDetail, including the fixed P/T direction, instead of
EffectKind::NoOp and TargetEffectDetail::None. Keep NoOp/None only for genuinely
effectless prompts, covering all listed occurrences.
In `@crates/phase-ai/src/policies/evasion_removal_priority.rs`:
- Around line 638-639: Update the TargetSelectionSlot entries in the enclosing
Destroy effect paths, including the occurrences around effect construction sites
at 638, 713, 781, and 849, so concrete targets use the Destroy effect’s actual
EffectKind and corresponding target-effect detail instead of EffectKind::NoOp
and TargetEffectDetail::None. Preserve NoOp metadata only for genuinely
non-operative targets.
In `@crates/phase-ai/src/policies/hand_disruption.rs`:
- Around line 447-448: Update the TargetSelectionSlot metadata at the affected
slots in the enclosing RevealHand abilities to use the enclosing ability’s
RevealHand effect kind instead of EffectKind::NoOp, while keeping effect_detail
as TargetEffectDetail::None and preserving the TargetSelectionSlot contract.
In `@crates/phase-ai/src/policies/recursion_awareness.rs`:
- Around line 199-200: Update the fixture effect metadata near the
pending-effect handling to derive effect_kind and effect_detail from each
pending effect instead of hardcoding EffectKind::NoOp and
TargetEffectDetail::None. Preserve Destroy for the first fixture and ChangeZone
with an Exile destination for the second, consistent with the
TargetSelectionSlot contract.
In `@crates/phase-ai/src/policies/stack_awareness.rs`:
- Around line 388-389: Update make_target_ctx to derive effect_kind and
effect_detail from its source_effect parameter using the engine’s authoritative
effect-to-kind/detail conversion, instead of always assigning EffectKind::NoOp
and TargetEffectDetail::None. Preserve the resulting metadata in
TargetSelectionSlot for Destroy, DealDamage, Bounce, and other effect types.
In `@crates/phase-ai/src/search.rs`:
- Around line 4494-4495: Update the TargetSelectionSlot metadata in
spell_target_selection_state so the slot describing the constructed
Effect::DealDamage uses EffectKind::DealDamage instead of NoOp, while preserving
the existing effect detail unless the TargetSelectionSlot contract requires it
to change.
In `@crates/phase-ai/src/tactical_gate.rs`:
- Around line 846-847: Update the three gate fixtures’ TargetSelectionSlot
effect metadata to match their pending effects: use the corresponding EffectKind
and TargetEffectDetail for Destroy, Bounce, and DealDamage instead of NoOp/None.
Apply the change at the fixture definitions around the visible
effect_kind/effect_detail fields, preserving the existing target-selection
setup.
---
Nitpick comments:
In `@crates/engine/src/game/interaction.rs`:
- Around line 2748-2752: The EffectKind classification match currently uses a
wildcard fallback that bypasses exhaustive-match checks. Replace the `_ =>
InteractionIntentCode::Choose` arm in the surrounding classification function
with an exhaustive approach, preferably by adding an exhaustive EffectKind
disposition method that explicitly groups neutral variants, and use that
classification at the protocol boundary so newly added effects require explicit
handling.
In `@crates/manabrew-compat/src/lib.rs`:
- Around line 4235-4245: Separate InteractionIntentCode::Choose from the grouped
unreachable arms in the targeting match. Handle Choose under its own comment and
preserve its documented neutral fallback behavior, while keeping Keep, Crew,
Saddle, Station, RingBearer, Blight, and Pay grouped as unreachable codes that
map to TargetingIntent::Hostile.
- Around line 4156-4171: Update projected_target_intent to fail closed unless
the interaction contains exactly one opportunity, matching interaction_prompt
and sole_open_opportunity; only inspect that opportunity’s Selection surface,
and return the existing Choose fallback for zero or multiple opportunities.
🪄 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: 0fae5c3b-aea9-48fc-a072-79499d3cac87
⛔ Files ignored due to path filters (1)
client/src/adapter/generated/interaction/index.tsis excluded by!**/generated/**
📒 Files selected for processing (22)
crates/engine/src/ai_support/candidates.rscrates/engine/src/ai_support/combat_withdrawal.rscrates/engine/src/ai_support/mod.rscrates/engine/src/game/ability_utils.rscrates/engine/src/game/casting.rscrates/engine/src/game/engine_trigger_target_tests.rscrates/engine/src/game/interaction.rscrates/engine/src/game/visibility.rscrates/engine/src/types/game_state.rscrates/engine/src/types/interaction.rscrates/manabrew-compat/src/lib.rscrates/phase-ai/src/policies/anti_self_harm.rscrates/phase-ai/src/policies/combat_withdrawal.rscrates/phase-ai/src/policies/context.rscrates/phase-ai/src/policies/evasion_removal_priority.rscrates/phase-ai/src/policies/hand_disruption.rscrates/phase-ai/src/policies/recursion_awareness.rscrates/phase-ai/src/policies/stack_awareness.rscrates/phase-ai/src/policies/tests/removal_lethality.rscrates/phase-ai/src/search.rscrates/phase-ai/src/tactical_gate.rscrates/phase-ai/tests/scenarios.rs
| /// CR 115.1: The kind of effect that will affect this target — the game | ||
| /// fact of *what the spell or ability does to the thing being chosen*. | ||
| /// Stamped at slot construction from the enclosing ability frame's own | ||
| /// `Effect`, because the slot is otherwise attribution-free: nothing here | ||
| /// references the effect, so a consumer cannot recover it later without | ||
| /// re-walking the effect tree in lockstep with the slot builder. | ||
| /// | ||
| /// This is deliberately the game fact and NOT a presentation intent. | ||
| /// Labelling (e.g. "this prompt is hostile") is a projection-layer | ||
| /// decision made by `target_intent` in `game::interaction`, mirroring how | ||
| /// `WaitingFor::EffectZoneChoice` stores `effect_kind` and lets | ||
| /// `effect_zone_intent` label it. | ||
| pub effect_kind: EffectKind, | ||
| /// CR 115.1: The discriminating fact that `effect_kind` does not carry. | ||
| /// | ||
| /// `EffectKind` is a unit tag, so two effects that do opposite things to a | ||
| /// target can share one variant: `Effect::ChangeZone` is the same kind | ||
| /// whether it exiles or returns to hand, and `Effect::Pump` is the same | ||
| /// kind for "+3/+3" and "-3/-3". Both hold the deciding value in their | ||
| /// payload, which is in hand at slot construction and unrecoverable | ||
| /// afterwards. | ||
| /// | ||
| /// One sum type rather than one `Option<T>` field per lossy kind: the axis | ||
| /// is "what extra fact does this kind need", and a per-kind field would be | ||
| /// the sibling-cluster smell at struct level. `From<&Effect> for | ||
| /// EffectKind` already reads payloads this way for `SetTapState`, so | ||
| /// payload discrimination at this boundary is established practice. | ||
| #[serde(default)] | ||
| pub effect_detail: TargetEffectDetail, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does serde's #[serde(default)] attribute require the field type to implement the Default trait, or does it accept a value literal directly?
💡 Result:
The #[serde(default)] attribute, when used without arguments on a field, requires that the field type implements the Default trait [1][2][3]. When the field is missing during deserialization, Serde will automatically call Default::default() to provide a value [1][2]. If you need a default value that is not provided by the Default trait, you must use the #[serde(default = "path")] syntax, where "path" refers to a function or method that returns the desired value [1][2][3]. This function must be callable as fn() -> T [1][4]. Serde does not support specifying a value literal (such as #[serde(default = 42)]) directly in the attribute [1][3][5]. While there have been community discussions and issues regarding support for literal values [5], the official mechanism remains limited to Default::default() or a custom function path [1][4].
Citations:
- 1: https://serde.rs/field-attrs.html
- 2: https://serde.rs/attr-default.html
- 3: https://www.rustfaq.org/en/how-to-handle-default-values-in-serde/
- 4: https://serde.rs/container-attrs
- 5: Support default literals serde-rs/serde#368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target struct and nearby serde/migration helpers.
rg -n "pub effect_kind: EffectKind|TargetSelectionSlot|#[[:space:]]*serde\\(default" crates/engine/src/types/game_state.rs
# Show the relevant section with line numbers.
sed -n '6170,6235p' crates/engine/src/types/game_state.rs
# Check whether EffectKind has a Default impl or an existing default helper.
rg -n "enum EffectKind|impl Default for EffectKind|fn default_.*effect_kind|EffectKind::NoOp" crates/engine/src/types -g '*.rs'Repository: phase-rs/phase
Length of output: 4485
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding waiting-state enums and any custom serde handling.
sed -n '3400,4485p' crates/engine/src/types/game_state.rs
# Find custom deserialize/default hooks related to game state / waiting states.
rg -n "deserialize_with|default *= *\"|#[[:space:]]*serde\\(.*default|impl .*Deserialize.*GameState|enum WaitingFor|TargetSelectionSlot" crates/engine/src/types/game_state.rs crates/engine/src/types -g '*.rs'Repository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether EffectKind implements Default and whether GameState has custom serde.
rg -n "impl Default for EffectKind|#[[:space:]]*derive\\([^\\)]*Default[^\\)]*\\).*EffectKind|enum EffectKind|impl<'de> Deserialize<'de> for GameState|struct GameState|enum WaitingFor|TargetSelectionSlot" crates/engine/src/types/ability.rs crates/engine/src/types/game_state.rs -g '*.rs'
# Show the WaitingFor definitions and the target-selection variants around them.
sed -n '4380,4475p' crates/engine/src/types/game_state.rs
# Show the EffectKind enum declaration.
sed -n '16300,16640p' crates/engine/src/types/ability.rsRepository: phase-rs/phase
Length of output: 16653
Add a serde default for TargetSelectionSlot::effect_kind. Persisted GameState values that park at target selection will fail to deserialize once this key is missing; use a named default function here to match effect_detail and keep saved in-flight states loadable.
🤖 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/game_state.rs` around lines 6185 - 6214, Add
#[serde(default = "...")] to TargetSelectionSlot::effect_kind, referencing a
named default function that returns the appropriate EffectKind. Define or reuse
that named default alongside the existing TargetEffectDetail default so
persisted GameState values missing effect_kind remain deserializable.
| }, | ||
| UnsupportedCapability { | ||
| code: "local.targeting-intent-unsourceable", | ||
| code: "local.targeting-intent-neutral-inexpressible", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Renaming a published capability code contradicts this registry's own stated policy.
The upstream.multi-destination-partition-missing entry justifies keeping a misleading code precisely because "renaming a published capability code is itself a contract break". A client pinned to local.targeting-intent-unsourceable now looks it up and finds nothing. Either keep the old code and update only reason, or state explicitly here why this one is exempt (e.g. never emitted at a call site, so no client can hold it).
🤖 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/manabrew-compat/src/lib.rs` at line 1413, Preserve the published
capability code for the affected registry entry by reverting the rename from
local.targeting-intent-unsourceable to
local.targeting-intent-neutral-inexpressible, and update only its reason text as
needed. If retaining the new code, document an explicit exemption in the entry
explaining why the old code was never emitted or externally consumable.
| effect_kind: EffectKind::NoOp, | ||
| effect_detail: TargetEffectDetail::None, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve effect attribution across all target-selection fixtures.
These fixtures pair concrete pending effects with TargetSelectionSlot { effect_kind: NoOp, effect_detail: None }, creating state that does not match the engine contract and can conceal incorrect target-intent projections.
crates/phase-ai/src/policies/anti_self_harm.rs#L1127-L1128: derive metadata frommake_target_selection_ctx’seffect.crates/phase-ai/src/policies/anti_self_harm.rs#L1772-L1773: stamp metadata for the discard/draw effect.crates/phase-ai/src/policies/anti_self_harm.rs#L1860-L1861: stamp metadata for the targeted discard effect.crates/phase-ai/src/policies/anti_self_harm.rs#L2732-L2733: stamp metadata for the rewind ability.crates/phase-ai/src/policies/anti_self_harm.rs#L3003-L3004: stamp metadata for the aura target context or explicitly model its static effect.crates/phase-ai/src/policies/anti_self_harm.rs#L3346-L3347: stamp metadata for the exile trigger.crates/phase-ai/src/policies/anti_self_harm.rs#L3533-L3534: stamp metadata for the damage ability.crates/phase-ai/src/policies/combat_withdrawal.rs#L241-L242: derive metadata from the installed ability frame.crates/phase-ai/src/policies/tests/removal_lethality.rs#L242-L243: derive metadata from the helper’seffectargument.
📍 Affects 3 files
crates/phase-ai/src/policies/anti_self_harm.rs#L1127-L1128(this comment)crates/phase-ai/src/policies/anti_self_harm.rs#L1772-L1773crates/phase-ai/src/policies/anti_self_harm.rs#L1860-L1861crates/phase-ai/src/policies/anti_self_harm.rs#L2732-L2733crates/phase-ai/src/policies/anti_self_harm.rs#L3003-L3004crates/phase-ai/src/policies/anti_self_harm.rs#L3346-L3347crates/phase-ai/src/policies/anti_self_harm.rs#L3533-L3534crates/phase-ai/src/policies/combat_withdrawal.rs#L241-L242crates/phase-ai/src/policies/tests/removal_lethality.rs#L242-L243
🤖 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/anti_self_harm.rs` around lines 1127 - 1128,
Replace the placeholder NoOp/None target-selection metadata with the
corresponding pending effect attribution across all listed fixtures: in
crates/phase-ai/src/policies/anti_self_harm.rs at 1127-1128 derive it from
make_target_selection_ctx’s effect, at 1772-1773 use the discard/draw effect, at
1860-1861 the targeted discard effect, at 2732-2733 the rewind ability, at
3003-3004 the aura target context or an explicitly modeled static effect, at
3346-3347 the exile trigger, and at 3533-3534 the damage ability; in
crates/phase-ai/src/policies/combat_withdrawal.rs at 241-242 derive it from the
installed ability frame; and in
crates/phase-ai/src/policies/tests/removal_lethality.rs at 242-243 derive it
from the helper’s effect argument.
| effect_kind: EffectKind::NoOp, | ||
| effect_detail: TargetEffectDetail::None, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the concrete Pump attribution in these fixtures.
Each slot is paired with a pending Effect::Pump, but is stamped as EffectKind::NoOp. Since interaction projection derives target intent from slot metadata, these fixtures can mask incorrect Pump classification. Populate the kind/detail from the enclosing Pump effect, including its fixed P/T direction; reserve NoOp for genuinely effectless prompts.
Based on the PR objective and the TargetSelectionSlot contract.
Also applies to: 351-352, 567-568, 618-619, 670-671
🤖 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/context.rs` around lines 286 - 287, Update the
affected TargetSelectionSlot fixtures in the Pump-effect cases to use the
enclosing Effect::Pump’s concrete EffectKind and TargetEffectDetail, including
the fixed P/T direction, instead of EffectKind::NoOp and
TargetEffectDetail::None. Keep NoOp/None only for genuinely effectless prompts,
covering all listed occurrences.
| effect_kind: EffectKind::NoOp, | ||
| effect_detail: TargetEffectDetail::None, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not label concrete Destroy targets as NoOp.
These target slots are used with Effect::Destroy, so EffectKind::NoOp is incorrect game-state metadata. Populate the slot from the enclosing Destroy effect so target-intent projection and future consumers see the real operation.
Based on the PR objective and the TargetSelectionSlot contract.
Also applies to: 713-714, 781-782, 849-850
🤖 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/evasion_removal_priority.rs` around lines 638 -
639, Update the TargetSelectionSlot entries in the enclosing Destroy effect
paths, including the occurrences around effect construction sites at 638, 713,
781, and 849, so concrete targets use the Destroy effect’s actual EffectKind and
corresponding target-effect detail instead of EffectKind::NoOp and
TargetEffectDetail::None. Preserve NoOp metadata only for genuinely
non-operative targets.
| effect_kind: EffectKind::NoOp, | ||
| effect_detail: TargetEffectDetail::None, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve RevealHand attribution in the target slots.
These slots belong to concrete Effect::RevealHand abilities, but the new metadata says NoOp. Populate the effect kind from the enclosing ability instead of using the neutral sentinel, otherwise projection tests can miss incorrect RevealHand intent handling.
Based on the PR objective and the TargetSelectionSlot contract.
Also applies to: 576-577, 668-669
🤖 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/hand_disruption.rs` around lines 447 - 448,
Update the TargetSelectionSlot metadata at the affected slots in the enclosing
RevealHand abilities to use the enclosing ability’s RevealHand effect kind
instead of EffectKind::NoOp, while keeping effect_detail as
TargetEffectDetail::None and preserving the TargetSelectionSlot contract.
| effect_kind: EffectKind::NoOp, | ||
| effect_detail: TargetEffectDetail::None, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Retain the actual removal operation and destination.
The first fixture targets with Destroy; the second targets with ChangeZone to Exile. Both are currently stamped as NoOp/None, losing the exact facts required for target-intent projection. Derive the metadata from each pending effect, including the Exile destination.
Based on the PR objective and the TargetSelectionSlot contract.
Also applies to: 293-294
🤖 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/recursion_awareness.rs` around lines 199 - 200,
Update the fixture effect metadata near the pending-effect handling to derive
effect_kind and effect_detail from each pending effect instead of hardcoding
EffectKind::NoOp and TargetEffectDetail::None. Preserve Destroy for the first
fixture and ChangeZone with an Exile destination for the second, consistent with
the TargetSelectionSlot contract.
| effect_kind: EffectKind::NoOp, | ||
| effect_detail: TargetEffectDetail::None, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Derive slot metadata from source_effect.
make_target_ctx already receives the concrete effect used to build PendingCast, but the slot is always stamped NoOp/None. That makes Destroy, DealDamage, and Bounce fixtures internally contradictory. Use the engine’s authoritative effect-to-kind/detail conversion here so every caller receives accurate metadata.
Based on the PR objective and the TargetSelectionSlot contract.
🤖 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/stack_awareness.rs` around lines 388 - 389,
Update make_target_ctx to derive effect_kind and effect_detail from its
source_effect parameter using the engine’s authoritative effect-to-kind/detail
conversion, instead of always assigning EffectKind::NoOp and
TargetEffectDetail::None. Preserve the resulting metadata in TargetSelectionSlot
for Destroy, DealDamage, Bounce, and other effect types.
| effect_kind: EffectKind::NoOp, | ||
| effect_detail: engine::types::game_state::TargetEffectDetail::None, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Stamp the damage target slot as DealDamage.
spell_target_selection_state constructs an Effect::DealDamage, but its slot claims NoOp. Populate the metadata from that effect so this production search-path fixture does not bypass target-intent attribution.
Based on the PR objective and the TargetSelectionSlot contract.
🤖 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/search.rs` around lines 4494 - 4495, Update the
TargetSelectionSlot metadata in spell_target_selection_state so the slot
describing the constructed Effect::DealDamage uses EffectKind::DealDamage
instead of NoOp, while preserving the existing effect detail unless the
TargetSelectionSlot contract requires it to change.
| effect_kind: EffectKind::NoOp, | ||
| effect_detail: TargetEffectDetail::None, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the gate fixtures’ effect metadata truthful.
The three fixtures use concrete Destroy, Bounce, and DealDamage effects, but all slots are marked NoOp/None. Derive the corresponding kind/detail from each pending effect so target-selection state remains consistent with the operation being evaluated.
Based on the PR objective and the TargetSelectionSlot contract.
Also applies to: 919-920, 974-975
🤖 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/tactical_gate.rs` around lines 846 - 847, Update the
three gate fixtures’ TargetSelectionSlot effect metadata to match their pending
effects: use the corresponding EffectKind and TargetEffectDetail for Destroy,
Bounce, and DealDamage instead of NoOp/None. Apply the change at the fixture
definitions around the visible effect_kind/effect_detail fields, preserving the
existing target-selection setup.
Parse changes introduced by this PR✓ No card-parse changes detected. |
Resolves the conflict introduced by phase-rs#6718 (derive targeting intent from the announcing effect), which landed after this branch's previous merge. The only collision is the `types::ability` import list in `crates/engine/src/game/ability_utils.rs`: this branch adds `AnnouncedModalChoice`, phase-rs#6718 adds `EffectKind` and `PtValue`. Resolved as the union of both — no logic conflict. The two changes compose at different stages of the announcement pipeline, so neither bypasses the other: - This branch's `resolve_legal_modal_choice` decides which modes are legal when the ability is put on the stack (CR 700.2b / CR 603.3c). - phase-rs#6718 stamps `EffectKind` onto target slots in `collect_target_slots`, which runs after a mode has been chosen. Verified on the merged tree: `cargo check -p engine -p phase-ai --all-targets` is clean, and both round-10 regressions still pass (`prompt_cap_equals_modal_choice_authority`, `prompt_unavailable_modes_equal_modal_choice_authority`). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🤖 AI text below 🤖 Save-compatibility regression, measured: this PR added Measured repro on a real pre-#6718 4p dump saved at Direct Why CI stayed green: no tracked fixture upstream (or in our twelve, save one) pauses at a target-selection prompt, so the suite structurally cannot reach this decode. The fix is a genuine tradeoff, flagged for your call rather than PR'd: (1) |
|
@lgray I'm hesitant to commit to patching the serialization for loading legacy saves. at this point in this the project I'm of the opinion that means "alpha means it may not load 🤷♂️" if you have a use-case then do the save changes locally, but I'm weary of accumulating debt for loading legacy saves and/or being beholden to that as a feature (at this point) |
2 participants
Every board-target prompt the manabrew adapter emitted was labelled
TargetingIntent::Hostile— a hardcoded placeholder, contradicted by thehostile: falsesitting next to it. Buffing your own creature advertisedas an attack on it.
The earlier diagnosis blamed the protocol for having no neutral member.
That was wrong: the engine has the data.
Effectis in hand when targetslots are built, so CR 601.2c's "one slot per announced target" can carry
what that target is for.
TargetSelectionSlotnow stores the announcing link'sEffectKindplus aTargetEffectDetailfor the two kinds whose unit tag is ambiguous — azone change's destination and a pump's direction, both read off the
payload at construction and unrecoverable afterwards. Stamping is
frame-scoped in
collect_target_slots, mirroringcurrent_chooser, so achained sub-ability does not inherit the head link's effect.
target_intentprojects slot -> label at the interaction layer, leavingGameStatestoring only game facts. The zone arm delegates to theexisting
effect_zone_intent, so anEffectZoneChoiceand a zone-changetarget announcement cannot disagree about what a destination means.
Measured over the corpus, this labels 70.3% of targeting links, up from a
constant wrong answer. The residue is genuinely undirected pumps —
dynamic X and opposing "+2/-2" — which stay on the adverse label for the
same asymmetric-loss reason the neutral bucket does: a caution affordance
on a real buff is recoverable, marking "-4/-4" harmless is not.
Summary by CodeRabbit
New Features
Bug Fixes
Tests