Fix Aven Courier - #6686
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds conditional chosen-counter placement, resolution-local counter-kind selection, typed and zone-aware counter discovery, corrected parent-slot resolution, parser support, effect-analysis updates, and Aven Courier integration coverage. ChangesChosen counter contracts and parsing
Population-aware counter discovery
Resolution and placement
Analysis and validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant ChooseCounterKind
participant ResolutionState
participant PutChosenCounter
participant PutCounter
Player->>ChooseCounterKind: select a counter kind during resolution
ChooseCounterKind->>ResolutionState: retain chosen kind locally
ResolutionState->>PutChosenCounter: pass chosen kind to placement
PutChosenCounter->>PutChosenCounter: evaluate target counter condition
PutChosenCounter->>PutCounter: place counter when condition holds
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Implementation review is complete for current head Once those remaining checks and the external review settle without a material finding, approval and auto-merge will resume. |
Parse changes introduced by this PR · 2 card(s), 5 signature(s) (baseline: main
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/game/effects/put_chosen_counter.rs (1)
27-113: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
target_condition_is_satisfiedchecks only the first resolved target, but placement applies to the wholetargetfilter.
target_condition_is_satisfiedtakes.next()fromresolved_object_ids_for_filter(state, ability, target)(Line 37-43) and evaluates the condition against that single object's counter count.resolve()then gates the entire placement on this one check (Line 94-101) and, when satisfied, delegates toEffect::PutCounterusing the sametargetfilter (Line 106-112), which can resolve to multiple objects for a plainTargetFilter::ParentTargetoutside arepeat_foriteration (seetargeting.rs::resolved_targets_parent_target_for_attack_event_returns_all_attackers, which provesParentTargetcan carry several objects).For Aven Courier (single target) this is harmless today, but this effect is explicitly documented as a reusable primitive mirroring
PutCounter's general target-rebinding contract. If reused by a future card with a genuinely multi-objecttargetoutside arepeat_foriteration, the condition would silently gate/apply against only the first sampled object instead of per-object — either wrongly skipping the whole placement because one target fails, or wrongly placing on targets that were never checked.Consider evaluating the condition per resolved object and restricting placement to only the objects that pass (e.g. resolve the id set once, filter by the condition, and hand the delegated
PutCounteran explicit filtered object list/tracked set rather than the rawtargetfilter).🤖 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/put_chosen_counter.rs` around lines 27 - 113, Update target_condition_is_satisfied and resolve so the chosen-counter condition is evaluated independently for every object resolved from target, rather than only the first object. Resolve the target IDs once, retain only objects whose counters satisfy the condition, and delegate PutCounter placement using an explicit filtered target set so objects failing the condition are not modified; preserve the existing no-condition and no-matching-target behavior.
🧹 Nitpick comments (2)
crates/engine/src/parser/oracle_effect/imperative.rs (2)
13435-13466: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the preserved anaphor branch (
it/that permanent/that creature).The two new tests only exercise the new typed-domain path and the target-rejection path. The anaphor→
ParentTargetbranch (lines 3865-3879) is untouched logic but now lives inside a fully rewritten function ("was rewritten from an anaphor-only parser… to a domain-based parser") — a rewrite of this shape carries real regression risk for the previously-shipped Doctor Who cards (The Caves of Androzani II/III) that this diff doesn't currently lock in with a test.✅ Suggested additional test
#[test] fn try_parse_choose_counter_kind_accepts_anaphor_forms() { for input in [ "a counter on it", "a counter on that permanent", "a counter on that creature.", ] { assert_eq!( try_parse_choose_counter_kind(input), Some(ChooseImperativeAst::CounterKind { target: TargetFilter::ParentTarget, }), "anaphor form must still resolve to ParentTarget: {input}" ); } }🤖 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/imperative.rs` around lines 13435 - 13466, Add a regression test alongside the existing try_parse_choose_counter_kind tests covering “a counter on it”, “a counter on that permanent”, and “a counter on that creature.” Assert each parses to ChooseImperativeAst::CounterKind with TargetFilter::ParentTarget, preserving the existing anaphor branch behavior.
3739-3746: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale comment: "Anaphoric form only" no longer describes
try_parse_choose_counter_kind.This comment still claims the callee handles only the anaphoric form ("it"/"that permanent"), but the rewritten function (line 3854 onward) now also parses typed, untargeted domains via
parse_target(e.g. Aven Courier's "a permanent you control"), returningCounterKind{Typed(..)}. Leaving this comment as-is could mislead a future reader into thinking typed-domain support lives elsewhere or is missing.📝 Suggested comment update
- // CR 608.2d + CR 122.1: "choose a counter on it / that permanent" — - // pick one of the distinct counter kinds on the anaphoric object - // (The Caves of Androzani II/III). Anaphoric form only; the declared- - // target form ("a counter on target permanent", Ichormoon Gauntlet) - // is handled by the target parser below. + // CR 608.2d + CR 122.1: "choose a counter on <domain>" — pick one of + // the distinct counter kinds on the anaphoric object (The Caves of + // Androzani II/III) or on a typed, untargeted population (Aven + // Courier's "a permanent you control"). The declared-target form + // ("a counter on target permanent", Ichormoon Gauntlet) is excluded + // and handled by the target parser below.🤖 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/imperative.rs` around lines 3739 - 3746, Update the comment above try_parse_choose_counter_kind to remove the inaccurate “anaphoric form only” claim and the implication that typed, untargeted domains are handled elsewhere; describe that the helper parses both anaphoric and typed untargeted counter domains, while preserving the distinction for declared-target forms handled by the target parser.
🤖 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_effect/imperative.rs`:
- Around line 3858-3863: Update the target-designation guard in the surrounding
parser flow to also recognize the “another target” and “other target” prefixes
accepted by parse_target(), preventing those inputs from falling through to
ChooseCounterKind. Preserve the existing None return behavior for all recognized
target prefixes.
---
Outside diff comments:
In `@crates/engine/src/game/effects/put_chosen_counter.rs`:
- Around line 27-113: Update target_condition_is_satisfied and resolve so the
chosen-counter condition is evaluated independently for every object resolved
from target, rather than only the first object. Resolve the target IDs once,
retain only objects whose counters satisfy the condition, and delegate
PutCounter placement using an explicit filtered target set so objects failing
the condition are not modified; preserve the existing no-condition and
no-matching-target behavior.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_effect/imperative.rs`:
- Around line 13435-13466: Add a regression test alongside the existing
try_parse_choose_counter_kind tests covering “a counter on it”, “a counter on
that permanent”, and “a counter on that creature.” Assert each parses to
ChooseImperativeAst::CounterKind with TargetFilter::ParentTarget, preserving the
existing anaphor branch behavior.
- Around line 3739-3746: Update the comment above try_parse_choose_counter_kind
to remove the inaccurate “anaphoric form only” claim and the implication that
typed, untargeted domains are handled elsewhere; describe that the helper parses
both anaphoric and typed untargeted counter domains, while preserving the
distinction for declared-target forms handled by the target parser.
🪄 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: ff46d803-14ab-4948-b5fe-362fd135119f
📒 Files selected for processing (17)
crates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/coverage.rscrates/engine/src/game/effects/choose_counter_kind.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/effects/put_chosen_counter.rscrates/engine/src/game/quantity.rscrates/engine/src/game/targeting.rscrates/engine/src/parser/oracle_effect/conditions.rscrates/engine/src/parser/oracle_effect/counter.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_ir/ast.rscrates/engine/src/parser/swallow_check.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/issue_6634_aven_courier.rscrates/engine/tests/integration/main.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the chosen-counter path has multi-target, declared-target, and coverage-honesty gaps.
🔴 Blocker
[HIGH] PutChosenCounter predicates only the first resolved target but then applies to the original multi-target filter. Evidence: crates/engine/src/game/effects/put_chosen_counter.rs:37-56 takes .next() from the resolved targets, while :94-113 passes the unfiltered original target onward for counter placement. Why it matters: mixed qualifying and nonqualifying targets can receive a counter contrary to the condition. Suggested fix: resolve and filter every candidate target before placement, with a mixed qualifying/nonqualifying multi-target regression.
[HIGH] Crystalline Giant is marked parse-supported without demonstrated binding of its random counter choice to the ChoiceType::CounterKind required by PutChosenCounter. Evidence: the current parse diff adds Crystalline Giant, but the random-counter choice does not show the required counter-kind binding. Why it matters: coverage can claim support while the runtime has no selected kind to apply. Suggested fix: retain strict-failure coverage until the binding exists, or implement the full random-choice binding with runtime coverage.
🟡 Non-blocking
[MED] The choose-counter parser excludes only the literal target prefix. Evidence: crates/engine/src/parser/oracle_effect/imperative.rs:3858-3889 rejects target but accepts other declared-target forms such as another target / other target as untargeted source populations. Why it matters: those forms bypass the existing target-designation path and receive the wrong binding semantics. Suggested fix: use a shared declared-target-prefix check and add a three-way reach test for target / another target / other target.
The earlier CI-hold comment is stale informational context only; this formal review supersedes it.
Recommendation: request changes — repair all-target predicate handling, declared-target parsing, and Crystalline Giant's counter-kind binding or coverage honesty before another review.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the prior all-target, target-prefix, and Crystalline strict-failure blockers are addressed; one new counter-kind-domain error remains.
🟡 Blocker
[MED] distinct_counter_kinds_among can include an unrelated battlefield object in a negated tracked-set domain. This PR now routes predicate domains through matching_object_ids_in_filter_universe at crates/engine/src/game/quantity.rs:4676-4684. For Not(And(TrackedSet(..), predicate)), filter_candidate_universe builds the Not universe from filter_zone_object_ids(state, filter) (battlefield default) plus the inner tracked-set population (:1936-1949), and the matcher complements against that universe (:2098-2106). A battlefield nonmember can therefore leak into the counter-kind set.
Make the complement universe the tracked/ledger population for this domain, and add a regression fixture with a tracked-set member plus an unrelated battlefield nonmember carrying a distinct counter kind. This is scoped to the newly routed counter-kind path, not a claim about object_count_matching_ids.
Fresh parse-diff and CI artifacts remain pending; this finding is from the current source and does not depend on them.
|
Addressed the requested changes in 1d78dc9:
Verification on this head before push:
|
|
Addressed the current-head tracked-set finding in 73171fe. Added a regression with two tracked exiled members plus an unrelated battlefield nonmember carrying a distinct counter kind; only the nonmatching tracked member contributes. The same test locks the naked-negation boundary. Verification:
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the negated tracked-set population fix still loses a disjunctive tracked-set domain.
🟡 Blocker
[MED] The complement can still include an unrelated battlefield object for Not(And(Or(TrackedSet A, TrackedSet B), predicate)). crates/engine/src/game/quantity.rs:2093-2110 derives a special population only for And; when the inner domain contains Or, the Not path (:2125-2133) falls back to filter_candidate_universe. Its Not universe includes the default battlefield (:1936-1948), so a nonmember's distinct counter kind leaks into the counter-kind result.
Carry the union population through Or before complementing, and add a regression with members in two tracked sets plus an unrelated battlefield nonmember carrying a different counter kind.
🟡 Evidence requested
The current parse diff reports an unexplained Contractual Safeguard change beyond the claimed Aven Courier impact. Restore it, or document and test the intended parser behavior.
|
Addressed the latest changes-requested review in c867657:
Verification: |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
[HIGH] Preserve the selected creature as the authority for “other” in Contractual Safeguard. Evidence: counter.rs:1737 parses the choice as only a TargetFilter, while choose_counter_kind.rs:53 carries only the resolved counter kind forward; filter.rs:4623 makes FilterProp::Another source-relative unless a recipient is supplied. Why it matters: the chosen creature is not the spell source or a per-recipient context here, so it is included in “each other creature you control” and receives the counter. Suggested fix: carry the object selected for the counter-kind choice through the resolution result and bind it as the recipient/exclusion context when resolving the downstream placement.
[MED] Retain TrackedSetFiltered’s nested filter/provenance when deriving a complement population. Evidence: quantity.rs:2062 and quantity.rs:2168 reduce TrackedSetFiltered to raw tracked-set membership, and quantity.rs:4746 uses that reduced population for Not. Why it matters: a complement can be evaluated over objects outside the nested filtered/provenance domain, producing the wrong counter-kind population for composed target filters. Suggested fix: represent and evaluate the full filtered domain when establishing the complement universe rather than discarding its nested predicate.
Please add production-pipeline regressions for both cases: one that resolves Contractual Safeguard with a chosen creature and confirms only the other controlled creatures receive the chosen kind, and one that exercises a complemented TrackedSetFiltered domain with a nested predicate/provenance constraint.
|
Addressed the latest requested changes in 409aa5c.
Verification:
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
[MED] Cover the repaired TrackedSetFiltered complement through a real production counter-kind consumer.
The new regression at crates/engine/src/game/quantity.rs:7521-7641 calls distinct_counter_kinds_among directly. That proves the helper’s local result, but it bypasses both non-concretized production consumers: QuantityRef::DistinctCounterKindsAmong (quantity.rs:3546-3548) and the counter-kind RepeatFor snapshot (effects/mod.rs:8999-9006).
Please add an integration/scenario regression that drives one of those consumers through the real ability-resolution pipeline using a nested Not(And(TrackedSetFiltered { filter, caused_by }, predicate)) population. Include the nested object predicate and producer-action provenance, plus an unrelated object with a distinct counter kind. Assert the externally observable consumer result excludes the outsider (for example, the resolved quantity or the RepeatFor iteration/binding contains only the in-domain kind). A direct helper invocation is not sufficient for this regression.
This is intentionally limited to production-consumer coverage; the prior selected-authority and disjunctive tracked-set blockers are addressed on this head.
|
Addressed the production-consumer coverage request in d37e917. The new integration regression builds a complemented TrackedSetFiltered domain with:
It drives the real resolve_ability_chain repeat_for: DistinctCounterKindsAmong path and asserts the externally visible source receives exactly one +1/+1 counter, proving only the in-domain Lore kind created an iteration. Verification:
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer sign-off: current head rechecked; resolved blockers and the final production-pipeline regression are verified against the live diff.
3 participants
Summary
Fixes Aven Courier by separating its stack-time destination target from its resolution-time counter-kind choice and applying the chosen counter only when the target lacks that kind.
The shared distinct-counter-kind population authority now also preserves explicit-zone, zone-change-ledger, and tracked-set domains through nested boolean filters.
Closes #6634.
Files changed
CR references
Implementation method (required)
Method: /pipeline
Track
Developer
LLM
Model: codex-gpt-5
Thinking: high
Tier: Standard
Verification
Required checks ran clean.
Gate A output is for the current committed head.
Final review-impl is clean for the current committed head.
Anchors cite analogous code at the same seams.
cargo fmt --all -- --check— PASS./scripts/check-parser-combinators.sh— PASScargo clippy --all-targets -- -D warnings— PASScargo test -p engine— unit: 17,797 passed / 6 ignored; integration: 4,083 passed / 2 ignored; doc tests: 7 ignored; 0 failuresFocused quantity population tests — 10 passed; 0 failures
Focused distinct-counter-kind tests — 8 passed; 0 failures
Focused Aven Courier integration tests — 3 passed; 0 failures
Full Oracle export — Aven Courier supported with
gap_count: 0and no parse warningscard-data-validate— PASSFull semantic audit — zero findings for Aven Courier
Gate A
Gate G PASS (router/grant architecture: strict router vs permissive grant boundary intact)
Gate A PASS head=af07b50c0de408c7a4fe3a44a32833180cd66007 base=791927424572cf38371f29d6107ff9d908584b12
Anchored on
Final review-impl
Final review-impl PASS head=af07b50c0de408c7a4fe3a44a32833180cd66007 base=791927424572cf38371f29d6107ff9d908584b12
Claimed parse impact
Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit
New Features
Bug Fixes
Tests