Add Lady Loki, Agent of Chaos - #6945
Conversation
github-actions
Bot
added
the
needs-maintainer
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe change adds target-aware quantity resolution, parses qualified target card references, and preserves parent object targets through paused damage continuations. New tests cover Lady Loki’s parser and runtime behavior. ChangesTarget-scoped damage resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TriggeredSpell
participant LadyLokiAbility
participant ExileUntilEffect
participant DamageEachPlayer
participant QuantityResolver
participant CastFromZone
TriggeredSpell->>LadyLokiAbility: trigger on first matching spell
LadyLokiAbility->>ExileUntilEffect: exile cards until a nonland card
ExileUntilEffect->>DamageEachPlayer: resolve damage effect
DamageEachPlayer->>QuantityResolver: resolve target-scoped mana values
DamageEachPlayer->>CastFromZone: offer optional free cast of exiled card
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
crates/engine/tests/integration/lady_loki_agent_of_chaos.rs (1)
159-167: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the hit's zone instead of discarding the binding.
let _ = hit;discards the id. The test then has no reach guard proving the dig actually exiled a nonland card with mana value 1. If the dig failed and the amount fell back to another path that also produced 2, the test would pass for the wrong reason.💚 Replace the discard with a reach guard
- let _ = hit; - let mut runner = scenario.build(); let outcome = runner.cast(spell).x(3).resolve(); + + // Reach guard: the dig exiled the nonland hit, so MV(hit) = 1 was read. + assert_eq!( + outcome.zone_of(hit), + engine::types::zones::Zone::Exile, + "the nonland hit must be exiled by the dig" + );As per path instructions: "require a paired positive reach-guard proving the input actually reached the code under 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/tests/integration/lady_loki_agent_of_chaos.rs` around lines 159 - 167, Replace the discarded hit binding in the test setup with a positive reach guard that asserts the card identified by hit is in the exile zone after the dig. Keep the existing result assertion, ensuring the test proves the nonland mana-value-1 card reached the code path under test.Source: Path instructions
crates/engine/src/game/effects/deal_damage.rs (2)
3081-3088: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTighten the chain assertion to an exact match.
The assertion uses
any(...). A regression that re-stashes the paused first opponent (P1) alongside P2 would still pass. Assert the full summary instead, so the test pins both the amount and the exact remaining-player set.💚 Exact-match assertion
- let summary = collect_chain_summary(&cont.chain); - assert!( - summary.iter().any( - |(_, target, amount)| *target == TargetRef::Player(PlayerId(2)) && *amount == 3 - ), - "remaining opponent must be stashed with |MV(spell) − MV(hit)| = 3 (a \ - target-dropped pre-resolve gives 4): {summary:?}" - ); + let summary = collect_chain_summary(&cont.chain); + assert_eq!( + summary, + vec![(source, TargetRef::Player(PlayerId(2)), 3)], + "only the remaining opponent must be stashed, with \ + |MV(spell) − MV(hit)| = 3 (a target-dropped pre-resolve gives 4)" + );As per path instructions: "Test adequacy is the highest-frequency contributor finding — scrutinize 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/engine/src/game/effects/deal_damage.rs` around lines 3081 - 3088, Update the assertion around collect_chain_summary to require an exact summary rather than merely finding a matching entry: verify the remaining stashed player set contains only PlayerId(2) and that its damage amount is 3, so an extra PlayerId(1) entry fails the test.Source: Path instructions
1028-1037: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a negative case that proves the guard blocks propagation.
cast_tail_with_parent_targetscopies the parent's object targets into every stashed tail whenshould_propagate_parent_targetsreturns true. The new regression test at Line 2964 only covers the true branch. No test covers the false branch on a pause path. If the predicate ever diverges fromresolve_ability_chain, a plain multi-targetDealDamagetail would silently gain the parent's object referents and damage the wrong recipients after a replacement pause.Add a paired case: a parent
DealDamagewith two object targets and a sub whosetarget_choice_timingisResolution, then assert the stashed tail keepstargets.is_empty().As per path instructions: "For every negative assertion … require a paired positive reach-guard proving the input actually reached the code under 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/game/effects/deal_damage.rs` around lines 1028 - 1037, Add a paired regression test near the existing propagation test for cast_tail_with_parent_targets: use a parent DealDamage with two object targets and a sub whose target_choice_timing is Resolution, then assert the stashed tail’s targets remain empty. Include a positive reach-guard confirming the replacement pause reaches the stashed-tail path before the negative assertion, and leave the true-branch coverage intact.Source: Path instructions
crates/engine/src/parser/oracle_nom/quantity.rs (1)
4628-4639: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompose the
nonprefix as its own axis instead of enumerating two literals.
parse_card_type_qualifierenumeratesnonlandandnoncreatureas separate literal arms, while delegating bare types toparse_core_type. Thenonprefix is an independent axis. Composing it overparse_core_typecovers the whole class (nonartifact card's mana value,nonenchantment card's mana value, …) with the same node count.♻️ Compose the prefix axis
fn parse_card_type_qualifier(input: &str) -> OracleResult<'_, ()> { terminated( - alt(( - value((), tag("nonland")), - value((), tag("noncreature")), - value((), tag("permanent")), - value((), parse_core_type), - )), + value( + (), + ( + opt(tag("non")), + alt((value((), tag("permanent")), value((), parse_core_type))), + ), + ), tag(" "), ) .parse(input) }As per coding guidelines: "Compose
alt()per axis; never enumerate the cartesian product as separatetag("full string")arms."🤖 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_nom/quantity.rs` around lines 4628 - 4639, Update parse_card_type_qualifier to parse the optional “non” prefix separately and compose it with parse_core_type, while retaining the standalone “permanent” qualifier and trailing-space requirement. Remove the separate nonland and noncreature literal arms so all non-prefixed and non-prefixed core card types are handled through the shared parser.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.
Nitpick comments:
In `@crates/engine/src/game/effects/deal_damage.rs`:
- Around line 3081-3088: Update the assertion around collect_chain_summary to
require an exact summary rather than merely finding a matching entry: verify the
remaining stashed player set contains only PlayerId(2) and that its damage
amount is 3, so an extra PlayerId(1) entry fails the test.
- Around line 1028-1037: Add a paired regression test near the existing
propagation test for cast_tail_with_parent_targets: use a parent DealDamage with
two object targets and a sub whose target_choice_timing is Resolution, then
assert the stashed tail’s targets remain empty. Include a positive reach-guard
confirming the replacement pause reaches the stashed-tail path before the
negative assertion, and leave the true-branch coverage intact.
In `@crates/engine/src/parser/oracle_nom/quantity.rs`:
- Around line 4628-4639: Update parse_card_type_qualifier to parse the optional
“non” prefix separately and compose it with parse_core_type, while retaining the
standalone “permanent” qualifier and trailing-space requirement. Remove the
separate nonland and noncreature literal arms so all non-prefixed and
non-prefixed core card types are handled through the shared parser.
In `@crates/engine/tests/integration/lady_loki_agent_of_chaos.rs`:
- Around line 159-167: Replace the discarded hit binding in the test setup with
a positive reach guard that asserts the card identified by hit is in the exile
zone after the dig. Keep the existing result assertion, ensuring the test proves
the nonland mana-value-1 card reached the code path under test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d316f225-37bf-4bb9-a19b-238ca6be8e18
📒 Files selected for processing (6)
crates/engine/src/game/effects/deal_damage.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/quantity.rscrates/engine/src/parser/oracle_nom/quantity.rscrates/engine/tests/integration/lady_loki_agent_of_chaos.rscrates/engine/tests/integration/main.rs
|
Generated for head Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main
|
|
Maintainer hold for current head |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — current head 6d705c61c44a93d86223a23d68f5e64e8631eb55 has an unaccounted parser blast radius.
The current parse-diff reports O-Kagachi Made Manifest changing from unsupported where_x_binding to supported Pump (+target's mana value/+0) alongside the intended Lady Loki change: #6945 (comment). The new generic that [type] card qualifier is the shared parser seam (crates/engine/src/parser/oracle_nom/quantity.rs:4617-4668), and O-Kagachi uses it after a player chooses a nonland card in a graveyard, returns that chosen card, then gets +X/+0 for its mana value (data/mtgish-cards.json:28199).
Either add a production-pipeline O-Kagachi scenario proving the real choose → return → mana-value pump chain (including a discriminating nonzero mana value), or narrow/justify this additional parse change so coverage stays honest. Lady Loki-only tests do not establish the selected-authority/target threading for O-Kagachi's distinct choice path.
I also checked current CodeRabbit feedback: its only review is against older head 269497e4... and there are no unresolved current-head threads, so no separate current CodeRabbit reach-guard request applies.
JacobWoodson
changed the title
…ghten Lady Loki tests
Addresses the PR review parser blast-radius: the opt(parse_card_type_qualifier)
"that card" arms widened the seam so a BARE "that card" also lowered to
ObjectScope::Target. That flipped O-Kagachi Made Manifest's "...where X is the
mana value of that card" from an honest where_x_binding gap into a dishonest
Pump (+target's mana value/+0) -- but O-Kagachi's "that card" is a card the
defending player CHOSE from a graveyard, not a threaded target, so no target is
ever wired for the pump to read.
Narrow both the possessive and prepositional arms to REQUIRE the type qualifier
(parse_card_type_qualifier), keeping Lady Loki's type-qualified "that nonland
card's mana value" (bound to the ExileFromTopUntil producer) while reverting
O-Kagachi to its baseline where_x_binding -- verified end-to-end via
parse_oracle_text. Also compose the non prefix as its own opt(tag("non"))
axis over parse_core_type so every "non<type>" qualifier is covered by one node
set instead of enumerated literals.
Tests:
- rename/extend the positive scope test to cover the composed non-axis
- add bare_of_that_card_mana_value_is_not_target_scope honesty guard (O-Kagachi
prepositional form), paired with the positive reach-guard
- Lady Loki X-spell test: replace `let _ = hit;` with an Exile reach guard
- exact-match the DamageEachPlayer pause chain summary
- add the false-branch pair proving a Resolution-timed sub does not inherit
parent targets on the pause path
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/parser/oracle_nom/quantity.rs`:
- Around line 4632-4636: Correct the documentation above the non qualifier
parser by removing the inaccurate CR 205.2b citation or replacing it with a
verified rule citation whose text explicitly defines the non qualifier behavior;
leave the parser implementation and canonical parse_core_type delegation
unchanged.
- Around line 4645-4647: Extend parse_core_type to recognize the vanguard card
type so parse_card_type_qualifier supports complete card-type coverage. Add
regression cases for both “that vanguard card’s mana value” and “mana value of
that vanguard card,” covering the possessive and prepositional forms while
preserving existing card-type parsing.
🪄 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: a5559522-fee0-4131-845e-35f82f1f7ef4
📒 Files selected for processing (5)
crates/engine/src/game/effects/deal_damage.rscrates/engine/src/game/quantity.rscrates/engine/src/parser/oracle_nom/quantity.rscrates/engine/tests/integration/lady_loki_agent_of_chaos.rscrates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/engine/tests/integration/main.rs
- crates/engine/tests/integration/lady_loki_agent_of_chaos.rs
- crates/engine/src/game/effects/deal_damage.rs
- crates/engine/src/game/quantity.rs
| opt(tag("non")), | ||
| alt((value((), tag("permanent")), value((), parse_core_type))), | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Support the missing vanguard card type.
parse_card_type_qualifier claims complete card-type coverage but delegates to parse_core_type, which excludes vanguard. Therefore, that vanguard card's mana value and mana value of that vanguard card do not parse. Add vanguard to the shared parser and add possessive and prepositional regression cases. Vanguard is a card type under CR 205.2a and CR 300.1. (media.wizards.com)
As per path instructions, strict fidelity to the Comprehensive Rules is non-negotiable.
Also applies to: 10887-10896
🤖 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_nom/quantity.rs` around lines 4645 - 4647,
Extend parse_core_type to recognize the vanguard card type so
parse_card_type_qualifier supports complete card-type coverage. Add regression
cases for both “that vanguard card’s mana value” and “mana value of that
vanguard card,” covering the possessive and prepositional forms while preserving
existing card-type parsing.
Source: Path instructions
|
@matthewevans thanks for the catch on the parse blast radius. Addressed in O-Kagachi (the blast radius): the Both the possessive and prepositional arms now require the type qualifier. Lady Loki's type-qualified "that nonland card's mana value" (bound to the So the regenerated parse-diff should now show only Lady Loki. No separate O-Kagachi choose→return→pump scenario is added, since that path (a defending-player choice from a graveyard) isn't implemented and this keeps coverage honest rather than claiming it. Also in this commit:
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head review for 5ec8afdd28365abe6fb7b716bf847349a799a948 — changes requested.
[MED] The parser coverage artifact is not bound to this review head. Evidence: the full <!-- coverage-parse-diff --> marker was updated at 2026-08-04T02:48:54Z, but its body says it was generated for c869f45cfab59ab48a3013749994e71cbefb616f, rather than 5ec8afdd28365abe6fb7b716bf847349a799a948; parser files changed in the intervening merge. Why it matters: the current card-level parse blast radius is therefore unverified. Suggested fix: regenerate the parse-diff artifact for the current head before coverage acceptance.
[MED] The new qualifier documentation has false rules provenance and an inaccurate completeness claim. Evidence: crates/engine/src/parser/oracle_nom/quantity.rs:4632-4637 attributes the non prefix to CR 205.2b and says the delegated parser covers the full core set. CR 205.2b instead concerns multi-card-type applicability; the canonical parse_core_type intentionally omits Vanguard because CoreType does not model the out-of-scope Vanguard avatars (crates/engine/src/parser/oracle_nom/primitives.rs:345-354). Why it matters: this repository requires every CR annotation to describe the code it annotates. Suggested fix: remove the CR 205.2b attribution and describe the qualifier as covering the supported CoreType vocabulary; do not add new Vanguard support in this PR.
… qualifier doc The parse_card_type_qualifier doc attributed the `non` prefix to CR 205.2b and claimed complete coverage of the card-type set. Both are inaccurate: - CR 205.2b concerns multi-card-type applicability, not a `non` qualifier. Drop the citation; the `non` prefix is a plain grammatical axis with no dedicated CR. - parse_core_type intentionally omits `vanguard` (CoreType models no Vanguard variant), so the qualifier does NOT cover the full CR 300.1 card-type set. Describe coverage as exactly the supported CoreType vocabulary and note vanguard is out of scope and not added by this PR. Per-repo rule: every CR annotation must describe the code it annotates. No behavior change; documentation only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Maintainer fixup for current-main line movement. Co-authored-by: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com>
|
Maintainer fixup pushed at |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved for current head 70332cfbe082743e64ce7fdce9be763b4c254692.
The current parse-diff is bound to this head and confines parser impact to Lady Loki (one card, two signatures). Current CI is green. The remaining open CodeRabbit Vanguard thread is refuted by the current code: parse_card_type_qualifier explicitly consumes only the supported CoreType vocabulary, and CoreType intentionally has no Vanguard variant; expanding that engine model is outside this PR's Lady Loki seam.
matthewevans
removed
the
needs-maintainer
2 participants
Summary
Adds engine support for Lady Loki, Agent of Chaos.
Files changed
CR references
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Tier: Frontier
Verification
cargo fmt --all— clean./scripts/check-parser-combinators.sh (Gate A)— clean (after env fix: hardcodedpython3resolved to the broken Windows App Store stub causing a false 'cross-product detector RED' failure; reran with msys64 python3 on PATH -> Gate G PASS + Gate A PASS)cargo clippy-strict— incomplete - still compiling in background (cold build; foreground attempt timed out at 10m, background run still 'Checking phase-engine', no result yet)cargo test -p phase-engine— not run (blocked on clippy-strict completing in the && chain)./scripts/gen-card-data.sh— not run (blocked on prior steps)cargo coverage— not runcargo semantic-audit— not runRe-verified at chunk-1 checkpoint with freshly regenerated card-data: all listed cards supported:true gap:0, semantic-audit clean. The run-time 'partial' was a stale-card-data artifact, not a code defect.
Scope Expansion
None. (Scope narrowed vs plan: the R6 zones/LKI change for EventSource-across-exile proved unnecessary — move_to_zone reuses the object id on Stack->Exile, so the exiled spell's off-stack MV is read directly.)
Validation Failures
See review/cross-check notes.
CI Failures
tilt get uiresource clippyexit 1), so the direct-cargo fallback path was correctly selected. Also,python3/pythonon PATH resolve to the Windows App Store stub (Permission denied, exit 126); a working interpreter exists at C:/msys64/mingw64/bin/python3.exe (3.9.7). Gate A only passes when that msys64 python3 is prepended to PATH.Summary by CodeRabbit
Bug Fixes
Tests