Fix Archnemesis - #6993
Conversation
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the trigger parsing is close, but two correctness gates remain.
🟡 Medium
[MED] CR 608.2k is not provenance for resolving a player anaphor. Evidence: crates/engine/src/parser/oracle_trigger.rs:992-1001,1216-1221 and crates/engine/src/parser/oracle_effect/subject.rs:2127-2141,2661-2675 cite it for binding “that player” after “attack enchanted player.” The verified rule says, “If an ability’s effect refers to a specific untargeted object that has been previously referred to by that ability’s cost or trigger condition, it still affects that object even if the object has changed characteristics.” That is object persistence, not player-anaphora resolution. CR 303.4b supports only that an Aura’s attached object or player is “enchanted.” Why it matters: the annotation asserts rules support the parser’s referent choice when it does not. Suggested fix: remove or replace the unrelated 608.2k citations; retain only verified annotations that describe the implemented behavior.
[MED] The attacker-preservation branch lacks a resolved-behavior regression. Evidence: crates/engine/src/parser/oracle_effect/subject.rs:2661-2675 deliberately excludes that attacking player from the enchanted-player rebinding, while crates/engine/tests/integration/curse_attack_triggers.rs:117-155 only asserts that Curse triggers reach the stack. Why it matters: a stack-presence test cannot show whether Curse of Chaos’s recipient bound to the attacker rather than the enchanted player. Suggested fix: add a discriminating Curse of Chaos resolution test that distinguishes the attacker from the enchanted player and proves the resolved recipient is the attacker.
Evidence status
The current-head parse-diff sticky comment is absent, and Rust lint/tests are still pending for b03d830f720151f19de08c8417f79342ed02dc88. Those are evidence/status gaps, not an additional substantive blocker in this review. The branch is BEHIND only on release/lockfile maintainer churn; no rebase is requested.
Recommendation: address the two findings above, then request re-review on the updated head.
|
Generated for head Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main
|
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested for head 93be65c56943f5c555e154aea92034a53b3acb23.
[HIGH] Event-time anaphora is incorrectly resolved from live attachment state. crates/engine/src/parser/oracle_effect/subject.rs:2206-2209 maps EnchantedPlayer to TargetFilter::AttachedTo, and crates/engine/src/game/targeting.rs:1254-1261 resolves that filter from the Aura's current attached_to when the ability resolves. For Archnemesis's attack trigger, “that player” must remain the defender/attacked player captured by AttackersDeclared (crates/engine/src/types/events.rs:1121-1126); moving or removing the Aura after the trigger is on the stack can otherwise damage/mill the wrong player or no player. Bind this anaphor to the attack event context and add a regression that moves/removes the Aura after trigger creation but before resolution.
[MED] Remove or replace the unverified/unrelated CR 608.2k citations in crates/engine/src/parser/oracle_effect/imperative.rs:8958, lower.rs:8115, and tests.rs:49969. Do not retain a rule citation without first verifying that it governs this anaphora binding.
Please also provide fresh current-head parser evidence after CI finishes. The present checks are queued/in progress and the available baseline sticky evidence is for an older head.
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head ae7a3a5: event-time defending-player capture is covered, feedback is addressed, and the latest CI workflow is green.
|
Maintainer hold — current head Required CI is green and the PR is approved. Because this head changes parser/engine behavior, approval/enqueue remains held until |
Summary
Fixes a parse-fidelity defect on Archnemesis.
Issue: "that player loses 2 life" parses to LoseLife{target: TriggeringPlayer}, which for a YouAttack (AttackersDeclared) event resolves to the attacking player (you) instead of the enchanted/defending opponent — so the controller loses 2 life, not the attacked player.
Files changed
CR references
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Tier: Frontier
Verification
cargo fmt --all— pass (clean, no changes emitted)./scripts/check-parser-combinators.sh— pass (Gate G PASS + Gate A PASS, exit 0, incl. Family-D self-test + cross-product scan). Raw invocation exits 1 ONLY because the WindowsApps python3 stub fails Family-D's self-test = the pre-declared env limitation; masking the stub exposed a real msys2 python3 and the full gate passed.cargo clippy-strict— pass (clean, no warnings/errors)cargo test -p phase-engine— FAIL (18474 passed, 1 failed, 6 ignored). Single failure is an unrelated Windows path-separator census test; see failures[].cargo export-cards data --stats --sidecar-dir client/public --output client/public/card-data.json && cp -> data/card-data.json— pass (card-data.json regenerated against THIS branch; both files fresh & identical 98,176,031 bytes). NOTE: the given recipe omitted --output, which writes the export to STDOUT (not the file) and would have cp'd a stale file; added --output to satisfy the step's stated intent.cargo coverage— pass (Archnemesis supported:true gap_count:0; all parse_details supported)cargo semantic-audit— pass (exit 0; Archnemesis has 0 findings — absent from flagged_cards among 32700 supported cards audited)Scope Expansion
Fixed both facets of the one misparsed clause per the approved plan (wrong effect target AND the missing enchanted-player firing scope); hardened plan Edit 3 with a subject-phrase guard so "that attacking player" on Curse of Chaos/Inertia/Shallow Graves does not regress to AttachedTo.
Validation Failures
See review/cross-check notes.
CI Failures
relfrom PathBuf.display().to_string() (yields backslashes on Windows) and asserts equality against hardcoded forward-slash literals like "game/effects/mod.rs:5999". left/right differ ONLY by \ vs /; the count assertions (producers.len()==5, readers==7, in_test==25) all passed. UNRELATED to Archnemesis and to this branch (none of the pinned files -- game/effects/mod.rs, scoped_library_search.rs, engine.rs:11427 -- are in the branch diff); the test is green in CI/Linux and self-documents its env fragility ('THIS ROW FAILS IN CI BEFORE IT FAILS LOCALLY'). NOT FIXED: it is another agent's actively-maintained file (detailed drift log with commit refs); modifying it is out of scope for card verification and prohibited by CLAUDE.md multi-agent safety. Deterministic separator mismatch, so retry has no value. This is the ONLY reason passed=false; every Archnemesis-specific check is green (coverage supported:true gap:0; semantic-audit 0 findings; fmt/gate/clippy all clean).