fix(engine): Licids attach themselves, not their enchant target (#605)#6668
Conversation
…e-rs#605) A Licid activation did nothing at all. "This creature loses this ability and becomes an Aura enchantment with enchant creature. Attach it to target creature" bound the bare "it" attachment anaphor to TargetFilter::ParentTarget, which resolve_object_filter reads as the ability's CHOSEN target -- the enchant recipient. The engine therefore attached the targeted creature to itself; state-based actions undid the nonsense attachment and the activation vanished silently. All 12 Licids share the clause. CR 608.2c: read the whole text and apply the rules of English. An earlier clause in the chain has animated the ability's own source into an Aura, so the chain has made exactly one object attachable and the following "it" names that source. Bind the attachment pronoun to SelfRef for exactly that animate-then-attach class, via a new ParseContext flag source_becomes_attachment_in_chain seeded from the chain's prior clauses. Every other chain keeps its pre-existing parse_target binding: * chains with an earlier typed referent keep ParentTarget (Aura Graft, Ogre Geargrabber, Auriok Survivors) * the Equipment-ETB class keeps ParentTarget (Embercleave) -- it is already rescued at runtime by resolve_parent_target_attachment_from_trigger. Rebinding it would be inert for those cards and an active regression for cards whose source is not attachable at all (Adaptive Armorer, Stonehewer Giant, Quest for the Holy Relic), which would attach a creature/artifact/instant to a permanent -- a state neither attachment_illegality nor the SBA sweep cleans up, since CR 704.5p is unimplemented. Also stamp Duration::Permanent on the animation itself. CR 611.2a: a continuous effect that states no duration lasts until the end of the game. Left duration-less it fell through to the UntilEndOfTurn default in effects/effect.rs, so at cleanup the Licid stopped being an Aura while staying attached -- and its "Enchanted creature can't attack" static, which keys purely on attached_to, would have locked the victim permanently. The global default in effect.rs is deliberately untouched; its None is overloaded at that seam. Verified: a full-corpus parse differential against origin/main changes exactly 12 cards -- the 12 Licids. cargo nextest -p engine 21783/21783 green, clippy -D warnings clean, parser combinator Gates A and G pass. Known follow-ups, unchanged by this commit: "loses this ability" emits no ability-removal modification, and "you may pay {W} to end this effect" lowers to a mandatory PayCost that neither prompts nor ends the effect (see phase-rs#4000). Necromancy has the same animation-duration bug via affected: OriginalSource and is deliberately out of scope here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MnZsf4Z7wgQCsDxbjh3ucK
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe parser now supports Licid animate-then-attach binding and permanent Aura duration. The engine, protocol, UI, AI, replay state, and tests now support paying grouped costs to end continuous effects and generalized illegal-attachment cleanup. ChangesLicid effect lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant EffectChain
participant ParseContext
participant AttachmentParser
participant GameState
EffectChain->>ParseContext: record source animation
ParseContext->>AttachmentParser: provide source attachment flag
AttachmentParser->>GameState: attach source via SelfRef
GameState->>GameState: retain permanent Aura animation
sequenceDiagram
participant Player
participant Client
participant Engine
participant ManaSystem
participant GameState
Player->>Client: select termination offer
Client->>Engine: submit group and cost
Engine->>ManaSystem: pay special-action cost
ManaSystem->>Engine: return payment result
Engine->>GameState: remove matching effect group
GameState->>Engine: emit ContinuousEffectEnded
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Parse changes introduced by this PR · 13 card(s), 2 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the source/recipient attachment repair is correct, but this activation now installs an effect that the printed Licid payment cannot end.
🔴 Blocker
crates/engine/src/parser/oracle_effect/subject.rs:3649 stamps the self Aura/Equipment grant as Duration::Permanent, while the same Calming Licid Oracle text at crates/engine/tests/integration/issue_605_calming_licid.rs:34 says, “You may pay {W} to end this effect.” The parser recognizes that phrase only to avoid marking the generated PayCost optional (crates/engine/src/parser/oracle_effect/assembly.rs:1936-1965); the PayCost resolver then only performs payment (crates/engine/src/game/effects/pay.rs:30-191). It carries no identity for the installed continuous effect and has no cleanup/detach continuation. This makes the newly working Licid state permanent even after its printed termination payment.
Model the ongoing Licid effect with a termination/cleanup authority that is bound to the exact effect created by this activation. The production path must offer the payment, permit decline without payment, and on payment remove that effect, detach the Licid, and restore the resulting characteristics/ability state. Please prove both branches through the real activation/payment pipeline; the present runtime test at issue_605_calming_licid.rs:177-241 only proves persistence across cleanup and never exercises the payment or termination behavior.
🟡 Non-blocking
The required current-head parse-diff artifact is absent: this engine/parser PR claims exactly 12 Licids changed, but there is no <!-- coverage-parse-diff --> comment for d8393191746a51b78bda852141b314353f69248b. Regenerate or otherwise attach the current artifact and reconcile its normalized card set with the claimed 12 before re-review.
✅ Clean
The source-versus-recipient attachment distinction is made at the parser-chain seam, and the existing activation test does exercise the real target/resolve path for that repair.
Recommendation: implement the bound termination path and its discriminating runtime coverage, then provide the current parse-diff artifact for this head.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Requested changes
HIGH — restore locale-key parity
client/src/i18n/locales/en/game.json:125 adds actionButton.endContinuousEffect, but the supported sibling game.json locale files (de, es, fr, it, pl, and pt) do not contain that key. The current frontend locale-parity check therefore fails. Add the corresponding key to every supported locale (with the appropriate translation) before re-review.
MED — regenerate the current-head parse evidence and reconcile the description
The <!-- coverage-parse-diff --> sticky comment was posted at 2026-07-26T13:50:03Z, before this head's 2026-07-26T21:59:04-05:00 commit. It reports 13 cards / 2 signatures against main 6e0701b384a7, while the PR body says the current change is exactly 12 Licids. Regenerate and attach the parse-diff artifact for this exact head (20d8b09e9dcd7c0f57dbcff7d641e74d4d22f4da), then reconcile the normalized card/signature set and the stated scope.
The PR body is also stale about code now present in this head: it describes the pay-to-end path and the relevant attachment/SBA work as unimplemented, while crates/engine/src/game/end_continuous_effect.rs implements the CR 116.2c special action and its termination pipeline. Update the description so it accurately describes the reviewed head rather than treating implemented work as a known follow-up.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/engine/src/parser/oracle_effect/tests.rs (1)
13627-13635: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMissing positive test for the new
end_costfield.All ~23 sites touched by this diff add
end_cost: _to existingEffect::GenericEffectdestructuring patterns (compile-fix for the new field), but none of them — nor any other test in this file — asserts a concreteend_costvalue. Since this cohort ("Parser attachment and termination lowering") is specifically about parsing the Licid pay-to-end-effect cost, this file has no test locking in that extraction actually works (e.g. that a "you may pay {U} to end this effect" clause populatesend_costwith the expectedAbilityCost).Consider adding at least one test asserting
end_cost: Some(...)with the expected cost for a representative Licid-style clause, mirroring the existingGenericEffectassertion style already used throughout this file.Also applies to: 20026-20033, 20197-20205
🤖 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/parser/oracle_effect/tests.rs` around lines 13627 - 13635, Add a positive parser test in the existing GenericEffect assertion areas, such as the checks around the Licid-style branch cases, that parses a representative “you may pay {U} to end this effect” clause and asserts end_cost is Some with the expected AbilityCost. Keep the existing static_abilities, duration, and target assertions intact, and apply the same concrete end_cost validation to the related assertion sites rather than leaving wildcard destructuring.crates/engine/src/parser/oracle_trigger_tests.rs (2)
3405-3417: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd a verified CR annotation to the exact-threshold test.
This new rules-focused test has no
CR <number>: <description>annotation. Verify the applicable rule indocs/MagicCompRules.txtand document it beside the test.🤖 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/parser/oracle_trigger_tests.rs` around lines 3405 - 3417, Verify the applicable Magic Comprehensive Rules section for the exact-threshold scry-bottom trigger behavior, then add a nearby “CR <number>: <description>” annotation to completed_scry_bottom_trigger_preserves_exact_threshold documenting that rule.Source: Path instructions
3405-3417: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssert the lowered effect, not only
execute.is_some().The test name claims exact-threshold behavior, but it passes even if the effect body is lowered incorrectly. Match the expected
Effect::Drawshape so regressions cannot produce a false green.🤖 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/parser/oracle_trigger_tests.rs` around lines 3405 - 3417, Strengthen completed_scry_bottom_trigger_preserves_exact_threshold by replacing the execute.is_some() check with an assertion that the lowered effect matches the expected Effect::Draw shape. Preserve the existing Scry mode and exact Comparator::EQ threshold assertions while validating the trigger’s actual effect body.Source: Path instructions
🧹 Nitpick comments (4)
crates/engine/tests/integration/issue_605_calming_licid.rs (2)
645-664: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead
with_mana_poolcall. Line 649 seeds 2 white and is superseded by the second call at Lines 660-664 (or, if the builder appends, silently makes the pool 4W+2R rather than the documented 2W+2R). Drop the first call and keep the combined seed adjacent to the activations it funds.🤖 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/tests/integration/issue_605_calming_licid.rs` around lines 645 - 664, Remove the initial with_mana_pool call that seeds 2 white mana in ending_one_licids_effect_leaves_the_other_licid_untouched, keeping only the later combined 2W+2R mana setup adjacent to the activations it funds.
956-963: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winV12 pushes each March static into both the live and base lists, contradicting the invariant this same file documents at Lines 1098-1100.
V13b explicitly notes that the Step-1 layer reset rebuilds
static_definitionsfrombase_static_definitions, so pushing to both applies the static twice. March'sSetCardTypes-style animation is idempotent, so the row still passes — but the fixture encodes a shape production never produces, and a future non-idempotent static installed this way would double-apply silently.♻️ Align V12 with the documented base-only pattern
{ let obj = runner.state_mut().objects.get_mut(&march).unwrap(); for def in march_static.statics.iter() { - obj.static_definitions.push(def.clone()); std::sync::Arc::make_mut(&mut obj.base_static_definitions).push(def.clone()); } }🤖 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/tests/integration/issue_605_calming_licid.rs` around lines 956 - 963, Update the March static setup in the shown runner state mutation to append each cloned definition only to base_static_definitions, removing the push into static_definitions. Preserve the subsequent layers_dirty.mark_full() call so the live list is rebuilt from the base definitions according to the documented invariant.Source: Path instructions
crates/engine/src/types/resolved_commands.rs (1)
308-332: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a roundtrip/rejection test for the new
resulting_next_end_effect_group_idhigh-water check.Every other allocator-receipt field in this file (
resulting_next_continuous_effect_id,resulting_next_timestamp, token/attachment/object ids, etc.) has a dedicated test proving both the accept and reject path (seesemantic_commands_roundtrip_and_reject_malformed_payloadsand friends). The newend_permission/resulting_next_end_effect_group_idhigh-water check and the two new command families (ResolvedUncommittedTriggerRemovalCommand,ResolvedStackRemovalCommand) have no equivalent test in this module'stestsblock, so a regression that stops enforcingEndEffectGroupAboveHighWater(or corrupts the new command'scauseinvariant) would go undetected by this file's own test suite.🤖 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/resolved_commands.rs` around lines 308 - 332, Add tests in this module’s tests block covering the new resulting_next_end_effect_group_id high-water validation and the ResolvedUncommittedTriggerRemovalCommand and ResolvedStackRemovalCommand roundtrips. Assert valid payloads are accepted, malformed payloads with an end-effect group at or above the recorded high-water are rejected with EndEffectGroupAboveHighWater, and each new command preserves and validates its cause invariant, following semantic_commands_roundtrip_and_reject_malformed_payloads.crates/engine/src/parser/oracle_trigger.rs (1)
14760-14774: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value"you"-only scope isn't flagged as intentional.
trigger_prefixhardcodestag("you"), so "whenever an opponent/each opponent/a player chooses to put N cards on the bottom while scrying" won't be recognized by this new arm and will fall through to the generic path (orUnimplemented). As per path instructions, every new parser arm must have its pronoun/scope variants covered or explicitly documented as out of scope.Based on path instructions: "For every new arm, verify the plural / possessive / 'an opponent's' / 'your' / 'their' / 'non-X' / 'another' and article-word (a/an/one/two/N/X/each) variants are covered or explicitly out of scope."
🤖 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/parser/oracle_trigger.rs` around lines 14760 - 14774, The trigger_prefix parser only accepts the literal “you” scope and misses required pronoun and scope variants. Update the new trigger arm around trigger_prefix to recognize the applicable opponent, player, each, possessive, and other required scope/article variants, or explicitly document each intentionally unsupported variant according to the path instructions; preserve the existing “when/whenever … chooses to put” structure.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 `@client/src/components/board/ActionButton.tsx`:
- Around line 83-94: Remove the frontend filtering in ActionButton and consume
an engine-provided ordered end-effect offers field directly, rendering it
unchanged. Add and thread this field through every relevant engine-to-client
adapter and verify the round trip with a test, preserving engine ordering and
display data.
In `@crates/engine/src/ai_support/mod.rs`:
- Around line 1023-1042: The two GameAction priority classifiers in
crates/engine/src/ai_support/mod.rs (lines 1023-1042 and 1066-1079) must use
exhaustive matches instead of wildcard fallbacks. Add an explicit
EndContinuousEffect classification to has_meaningful_priority_action while
preserving its required meaningful result, then enumerate every remaining
GameAction variant according to the existing auto-pass semantics in
auto_pass_recommended. Ensure both matches compile-fail when future enum
variants are added.
In `@crates/engine/tests/integration/issue_605_calming_licid.rs`:
- Around line 1513-1552: Replace the isolated ManaRestriction::allows assertions
in spell_restricted_mana_cannot_pay_a_termination_cost with an integration test
using the animated_calming_licid fixture and pay_end_continuous_effect_cost
flow. Seed only spell-restricted {W} mana, assert EndContinuousEffect is not
offered or payable, then replace it with unrestricted {W} mana and assert the
action is offered and succeeds, proving the pipeline supplies
PaymentContext::SpecialAction(EndContinuousEffect).
- Around line 1242-1278: Strengthen both negative cases in
only_mana_termination_costs_are_absorbed: assert parsed.abilities.first() is
present and has a GenericEffect root before checking root_end_cost is None.
Apply the same reach guard to the wrong case, avoiding the current and_then path
that allows an empty ability list to pass silently.
---
Outside diff comments:
In `@crates/engine/src/parser/oracle_effect/tests.rs`:
- Around line 13627-13635: Add a positive parser test in the existing
GenericEffect assertion areas, such as the checks around the Licid-style branch
cases, that parses a representative “you may pay {U} to end this effect” clause
and asserts end_cost is Some with the expected AbilityCost. Keep the existing
static_abilities, duration, and target assertions intact, and apply the same
concrete end_cost validation to the related assertion sites rather than leaving
wildcard destructuring.
In `@crates/engine/src/parser/oracle_trigger_tests.rs`:
- Around line 3405-3417: Verify the applicable Magic Comprehensive Rules section
for the exact-threshold scry-bottom trigger behavior, then add a nearby “CR
<number>: <description>” annotation to
completed_scry_bottom_trigger_preserves_exact_threshold documenting that rule.
- Around line 3405-3417: Strengthen
completed_scry_bottom_trigger_preserves_exact_threshold by replacing the
execute.is_some() check with an assertion that the lowered effect matches the
expected Effect::Draw shape. Preserve the existing Scry mode and exact
Comparator::EQ threshold assertions while validating the trigger’s actual effect
body.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_trigger.rs`:
- Around line 14760-14774: The trigger_prefix parser only accepts the literal
“you” scope and misses required pronoun and scope variants. Update the new
trigger arm around trigger_prefix to recognize the applicable opponent, player,
each, possessive, and other required scope/article variants, or explicitly
document each intentionally unsupported variant according to the path
instructions; preserve the existing “when/whenever … chooses to put” structure.
In `@crates/engine/src/types/resolved_commands.rs`:
- Around line 308-332: Add tests in this module’s tests block covering the new
resulting_next_end_effect_group_id high-water validation and the
ResolvedUncommittedTriggerRemovalCommand and ResolvedStackRemovalCommand
roundtrips. Assert valid payloads are accepted, malformed payloads with an
end-effect group at or above the recorded high-water are rejected with
EndEffectGroupAboveHighWater, and each new command preserves and validates its
cause invariant, following
semantic_commands_roundtrip_and_reject_malformed_payloads.
In `@crates/engine/tests/integration/issue_605_calming_licid.rs`:
- Around line 645-664: Remove the initial with_mana_pool call that seeds 2 white
mana in ending_one_licids_effect_leaves_the_other_licid_untouched, keeping only
the later combined 2W+2R mana setup adjacent to the activations it funds.
- Around line 956-963: Update the March static setup in the shown runner state
mutation to append each cloned definition only to base_static_definitions,
removing the push into static_definitions. Preserve the subsequent
layers_dirty.mark_full() call so the live list is rebuilt from the base
definitions according to the documented invariant.
🪄 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: de4ffccb-17dc-4cee-8c7e-b4cc3b0e3a44
⛔ Files ignored due to path filters (2)
client/src/adapter/generated/interaction/index.tsis excluded by!**/generated/**crates/engine/tests/fixtures/cr733/authority_matrix.json.gzis excluded by!**/*.gz
📒 Files selected for processing (87)
client/src/adapter/types.tsclient/src/components/board/ActionButton.tsxclient/src/i18n/locales/en/game.jsoncrates/engine/src/ai_support/candidates.rscrates/engine/src/ai_support/mod.rscrates/engine/src/database/synthesis.rscrates/engine/src/database/unearth.rscrates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_utils.rscrates/engine/src/game/casting_tests.rscrates/engine/src/game/combat.rscrates/engine/src/game/combat_damage.rscrates/engine/src/game/coverage.rscrates/engine/src/game/dungeon.rscrates/engine/src/game/effects/copy_spell.rscrates/engine/src/game/effects/effect.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/end_continuous_effect.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_tests.rscrates/engine/src/game/interaction.rscrates/engine/src/game/layers.rscrates/engine/src/game/log.rscrates/engine/src/game/mana_abilities.rscrates/engine/src/game/mana_sources.rscrates/engine/src/game/meld_tests.rscrates/engine/src/game/mod.rscrates/engine/src/game/printed_cards.rscrates/engine/src/game/public_state.rscrates/engine/src/game/sba.rscrates/engine/src/game/scenario.rscrates/engine/src/game/trigger_index.rscrates/engine/src/game/trigger_matchers.rscrates/engine/src/game/triggers.rscrates/engine/src/parser/clause_shell.rscrates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_effect/assembly.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/src/parser/oracle_effect/snapshot_tests.rscrates/engine/src/parser/oracle_effect/subject.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_ir/ast.rscrates/engine/src/parser/oracle_modal.rscrates/engine/src/parser/oracle_replacement.rscrates/engine/src/parser/oracle_tests.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/src/parser/oracle_vote.rscrates/engine/src/parser/swallow_check.rscrates/engine/src/types/ability.rscrates/engine/src/types/action_stable_order.rscrates/engine/src/types/actions.rscrates/engine/src/types/events.rscrates/engine/src/types/game_state.rscrates/engine/src/types/interaction.rscrates/engine/src/types/mana.rscrates/engine/src/types/resolved_commands.rscrates/engine/src/types/statics.rscrates/engine/tests/integration/issue_2904_life_of_the_party.rscrates/engine/tests/integration/issue_605_calming_licid.rscrates/engine/tests/integration/issue_6566_granted_leave_exile.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/mjolnir_hammer_double_damage.rscrates/engine/tests/integration/najeela_extra_combat_grant_2898.rscrates/engine/tests/integration/springheart_nantuko_bestow_landfall.rscrates/engine/tests/integration/springheart_realdb_repro.rscrates/engine/tests/integration/std_longtail_e.rscrates/manabrew-compat/src/lib.rscrates/mtgish-import/src/convert/action.rscrates/mtgish-import/src/convert/mod.rscrates/phase-ai/src/features/tests/poison.rscrates/phase-ai/src/policies/anti_self_harm.rscrates/phase-ai/src/policies/effect_classify.rscrates/phase-ai/src/policies/land_animation.rscrates/phase-ai/src/policies/planeswalker_loyalty.rscrates/phase-ai/src/policies/reactive_self_protection.rscrates/phase-ai/src/policies/redundancy_avoidance.rscrates/phase-ai/src/policies/sacrifice_land_protection.rscrates/phase-ai/src/policies/self_cost_value.rscrates/phase-ai/src/policies/self_protection_classify.rscrates/phase-ai/src/policies/tests/poison.rscrates/phase-ai/src/policies/x_cast_gate.rscrates/phase-ai/src/search.rscrates/server-core/src/game_action_payload_guard.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/engine/tests/integration/main.rs
- crates/engine/src/parser/oracle_effect/imperative.rs
|
Addressed the requested Licid termination blocker in
Discriminating coverage uses the real activation/legal-action/payment pipeline for both branches: payment spends the sole white source, ends the exact effect, restores the activated ability, detaches the Licid, and restores attack legality; declining crosses cleanup without spending the termination mana or ending the effect. The suite also covers two independent Licids, wrong-player/affordability rejection, restricted mana, priority timing, replay, and generic CR 704.5p attachment behavior. Local verification on the current merged head: parser gates pass; 28/28 issue #605 tests pass; CR 733 census/replay tests pass; frontend protocol/type-check passes; ESLint has 0 errors; focused server payload-bound and action-order tests pass. The card-data workflow is running now and should refresh the @matthewevans ready for re-review once the refreshed checks settle. |
|
Current-head review follow-up is pushed in
The CodeRabbit notes about the scry-bottom trigger and its Local current-head verification: targeted Clippy with The card-data workflow will refresh the sticky @matthewevans re-review requested. |
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/engine/src/ai_support/mod.rs`:
- Around line 1058-1060: Update the comment near the loop-firewall logic to
retain CR 116.2c and CR 104.4b, and remove CR 732.4 unless you can replace it
with a verified current rule citation that directly supports the claim.
🪄 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: 196d6c34-67b8-476a-9d1f-140af3077ec7
📒 Files selected for processing (36)
client/src/adapter/server-draft-adapter.tsclient/src/adapter/types.tsclient/src/adapter/ws-adapter.tsclient/src/components/board/ActionButton.tsxclient/src/game/sessionCleanup.tsclient/src/i18n/locales/de/game.jsonclient/src/i18n/locales/es/game.jsonclient/src/i18n/locales/fr/game.jsonclient/src/i18n/locales/it/game.jsonclient/src/i18n/locales/pl/game.jsonclient/src/i18n/locales/pt/game.jsonclient/src/network/__tests__/protocol.test.tsclient/src/network/protocol.tsclient/src/providers/GameProvider.tsxclient/src/stores/__tests__/gameStoreCommitGate.test.tsclient/src/stores/gameStore.tsclient/src/stores/multiplayerDraftStore.tsclient/src/stores/replayStore.tsclient/src/test/factories/gameStateFactory.tscrates/engine-wasm/src/lib.rscrates/engine/src/ai_support/mod.rscrates/engine/src/game/ability_rw.rscrates/engine/src/game/coverage.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/triggers.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_ir/ast.rscrates/engine/src/parser/swallow_check.rscrates/engine/src/types/ability.rscrates/engine/src/types/events.rscrates/engine/tests/integration/issue_605_calming_licid.rscrates/engine/tests/integration/main.rscrates/phase-server/src/main.rscrates/server-core/src/protocol.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- crates/engine/tests/integration/main.rs
- crates/engine/src/game/effects/mod.rs
- crates/engine/src/game/triggers.rs
- crates/engine/src/types/events.rs
- crates/engine/src/parser/oracle_ir/ast.rs
- client/src/components/board/ActionButton.tsx
- crates/engine/src/parser/swallow_check.rs
- crates/engine/src/types/ability.rs
- crates/engine/src/game/ability_rw.rs
- crates/engine/src/parser/oracle_effect/lower.rs
- crates/engine/src/game/coverage.rs
- crates/engine/src/parser/oracle_effect/imperative.rs
- crates/engine/tests/integration/issue_605_calming_licid.rs
| // CR 116.2c + CR 732.4 + CR 104.4b: paying to end an effect is | ||
| // optional, so it is meaningful to the loop firewall even though the | ||
| // own-upkeep/draw classifier below deliberately auto-passes it. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Correct the invalid CR citation.
CR 732.4 is not a current Comprehensive Rules provision. Keep CR 116.2c for the pay-to-end special action and CR 104.4b for the optional-action loop distinction, but remove or replace CR 732.4 with the verified rule that actually supports the claim. (magiccomprehensiverules.com)
🤖 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/ai_support/mod.rs` around lines 1058 - 1060, Update the
comment near the loop-firewall logic to retain CR 116.2c and CR 104.4b, and
remove CR 732.4 unless you can replace it with a verified current rule citation
that directly supports the claim.
Source: Path instructions
|
The current-head parse-diff sticky comment has now refreshed from the successful card-data job: baseline main 12fb17a, 13 cards with PayCost removed, and 12 Aura-form Licids with duration changed to permanent. This matches the reconciled PR description. @matthewevans |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved — current head a29df8d2512471b78bd9355b13c3de7ec277db45
The prior requested changes are resolved at the reviewed head.
ActionButtonrenders the engine-projected orderedendContinuousEffectOffersunchanged; the value is threaded through the WASM, WebSocket, P2P, server-draft, snapshot, and cleanup paths.actionButton.endContinuousEffectnow exists in every supported locale.- The priority classifier is exhaustive over
GameAction, including an explicitEndContinuousEffectclassification. - The current parse-diff artifact was updated at
2026-07-27T11:25:07Z; its full artifact confirms exactly 13PayCostremovals (the 12 Aura-form Licids plus Flanking Licid) and exactly the 12 permanent-duration changes claimed in the description.
Manual Quality Gate: PASS. The change is at the engine action/priority authority, and the issue #605 integration suite drives the real activation, legal-action offer, payment, removal, layer/SBA cleanup, and decline paths. CI is green for Rust lint/tests, card-data/coverage, frontend, WASM, and Tauri; the remaining AI checks are in progress and auto-merge will wait for them.
The current CodeRabbit CR 732.4 concern is refuted by the checked-in Comprehensive Rules: docs/MagicCompRules.txt:6383 defines 732.4 for mandatory-action loops, which together with 104.4b directly supports the optional-action distinction in the comment.
Port the Licid pay-to-end action across the current exhaustive Manabrew capability registry. Co-authored-by: mike-theDude <mbriningstool@gmail.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved — current maintainer-port head 34cac82ce1767b735d830a464b2f7cb9b8e940fb
The current-head maintainer port is clean: the end-effect capability remains registered at the engine action/priority authority and its exhaustiveness contract discriminates the new action; no related emitter or sibling registry was missed.
The prior CodeRabbit CR 732.4 concern is refuted by the checked-in Comprehensive Rules (docs/MagicCompRules.txt:6383): it defines the mandatory-action loop rule, which is relevant alongside CR 116.2c and CR 104.4b for the optional-action distinction. CodeRabbit's review of this exact head reports no actionable comments.
No manual Quality Gate assertion is made for this maintainer-port head: the parse-diff sticky artifact was not refreshed after the port. The merge queue will wait for the current pending checks.
3 participants
Fixes #605.
What was broken
Aura-form Licid activations bound the bare
"it"in “Attach it to target creature” to the chosen enchant recipient. The engine therefore attempted to attach the target to itself, and the activation appeared to do nothing.The same activation also lacked two pieces of its ongoing-effect lifecycle:
PayCost.What changed
"it"toSelfRef; Equipment ETBs and non-attachable sources keep their existing bindings.Duration::Permanent.GenericEffect.EndContinuousEffectspecial action at priority; paying through the restriction-aware mana pipeline removes only that group, restores layer-derived characteristics and abilities, and runs attachment SBAs.Scope and parse evidence
There are two intentional card cohorts:
PayCostremoved: those 12 Licids plus Flanking Licid, whose pay-to-end clause belongs to the same CR 116.2c termination class.The sticky
<!-- coverage-parse-diff -->comment is generated by CI for the exact current head and reports these as separate signatures.Verification
cargo fmt --all -- --check-D warningsRuntime coverage drives both branches through the production activation/legal-action/payment pipeline. The pay branch spends the available white mana, removes the exact effect, restores the Licid ability and characteristics, and detaches it; the decline branch crosses cleanup without paying or ending the effect.
Summary by CodeRabbit
New Features
Bug Fixes
Tests