test(engine): positively discriminate the assimilate target filter - #7108
Merged
Conversation
Tests 3a and 3b asserted only that an illegal candidate stayed in its
graveyard, which is equally consistent with "the filter rejected it" and
"the ETB production never fired at all". Seed a legal AND an illegal
candidate in one fixture and declare the illegal one FIRST, so the filter
is observable across all three reachable states:
correct the legal set is a singleton, so
auto_select_targets_for_ability assigns it and no prompt is
raised; the legal card enters, the illegal one stays
over-match two legal candidates, auto-selection declines, a required
slot IS created, and pick_slot_target fills it from the
DECLARED order -- taking the bait, flipping both legs
under-match empty legal set, so CR 603.3d removes the trigger before
any slot exists and the POSITIVE leg fails
Declaration order is a regression-only instrument: it converts the
over-match case from pick_slot_target's no-declared-target panic into a
clean assertion failure. Each test holds two filter legs constant and
varies one, so 3a isolates the Creature type leg and 3b the ownership leg
(CR 109.4: a graveyard card has no controller, so "an opponent's
graveyard" rides as ownership). InZone { Graveyard } is isolated by
neither and keeps only its parser-level coverage.
Both tests are now revert-failing. 3b previously passed with the lowering
reverted, because nothing moved at all, and its doc comment claimed only
forward-guard status; that claim is updated rather than left stale.
Correct Recruit's mis-cited keyword-action rule. CR 701.9 is Discard;
Recruit is CR 701.70a. The "standalone keyword action" comment is purely
a 701.70a claim, so it is replaced. The discard-result comment genuinely
involves both rules -- the discard is 701.9a, the nonland contingency is
701.70a -- so it gains 701.70a instead. Two further Recruit-tagged 701.9a
sites are deliberately left alone: they assert snapshot and provenance
semantics already covered by 701.9a + 608.2c + 400.7, and assert nothing
that 701.70a supplies.
Relocate the parser-only fail-closed test beside its sibling shape test
in the parser module, and drop the two imports it was the last user of.
Replace the ephemeral BASE_SHA run identifier in three doc comments with
a stable PR reference. BASE_SHA is an implementation-run artifact whose
meaning changes every run and is undefined to a reader of the repository.
Two comment incoherences introduced by the preceding commit, which moved the parser-only fail-closed test out of the integration suite and into `parser/oracle_effect/tests.rs`. `borg_queen_assimilate.rs`'s module doc pointed at the parser file but named only `borg_queen_assimilate_lowers_to_reanimate_then_retype_chain`, so the relocated `assimilate_without_a_graveyard_target_stays_unimplemented` was referenced from nowhere in the tree — the one thing the relocation was supposed to preserve was the ability to find it. Both are now named, and labelled by what they assert (positive lowering / fail-closed negative) rather than listed. The relocated test also kept its `3c.` ordinal, which indexed the integration file's 3a/3b/3c scheme. In its new home that prefix points at nothing. Dropped; the doc comment already states what the test discriminates. Comment-only: 0 non-comment lines change. Verified via `git diff -U0` with comment lines filtered out, so this cannot alter parse output, and `crates/engine/src/parser/oracle_effect/tests.rs` is gated behind `#[cfg(test)] mod tests;` and is never compiled into the `oracle-gen` binary that generates card data.
matthewevans
enabled auto-merge
August 8, 2026 20:05
Contributor
📝 WalkthroughWalkthroughThe parser tests now cover fail-closed assimilate phrasing and valid graveyard lowering. Integration tests verify creature-type and opponent-ownership filtering. Recruit comments include CR 701.70a references. ChangesAssimilate regression coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/parser/oracle_effect/imperative.rs`:
- Line 9816: Remove the unrelated CR 608.2c citation from the comment describing
Recruit as a standalone keyword action, retaining only CR 701.70a. Leave the
ordered-resolution comment near the later instruction-resolution logic
unchanged.
🪄 Autofix
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: 961600e6-7470-4725-8201-574f3b60ed3d
📒 Files selected for processing (3)
crates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/tests/integration/borg_queen_assimilate.rs
| } | ||
|
|
||
| // CR 701.9a + CR 608.2c: Recruit is a standalone keyword action. Keep this | ||
| // CR 701.70a + CR 608.2c: Recruit is a standalone keyword action. Keep this |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the unrelated CR citation.
Line 9816 documents recognition of Recruit as a standalone keyword action. It does not document written instructions resolving in order. Remove CR 608.2c and retain CR 701.70a. The ordered-resolution comment at Lines 11945-11948 can retain CR 608.2c.
Based on learnings, cite CR 608.2c only when the comment documents written instructions resolving in order.
🤖 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` at line 9816, Remove
the unrelated CR 608.2c citation from the comment describing Recruit as a
standalone keyword action, retaining only CR 701.70a. Leave the
ordered-resolution comment near the later instruction-resolution logic
unchanged.
Sources: Coding guidelines, Path instructions, Learnings
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up items from #7096 (the
assimilatekeyword action). Test-strengthening plus a CR-citationcorrection; no production behavior changes.
What changed
1. The assimilate target-filter tests now discriminate positively. Tests 3a/3b previously declared
zero targets and asserted only that nothing moved. That passes for the right reason and also for two
wrong ones, so it could not distinguish a correct filter from a filter that matches nothing. Both now
declare a bait object first and assert the split:
3a discriminates a creature card from a land in the same graveyard; 3b discriminates an opponent's
graveyard from the controller's own.
Declaration order is load-bearing here, and the shipped doc comment says so explicitly, because the
target-slot machinery has three states and only the middle one is sensitive to order:
auto_select_targets_for_abilityreturnsSome->AutoAssigned, no prompt, order irrelevantNeedsPlayerChoice->pick_slot_targettakes the first-declared legal object — order decidesbuild_target_slotserrors, so no slot is ever built; CR 603.3d removes the ability from the stackThat third row is why declaration order is documented as a regression-only instrument rather than a
guarantee: an under-matching filter never reaches
pick_slot_targetat all, so the negative leg alonecannot catch it — the paired positive leg is what does.
2. Recruit CR citations corrected.
imperative.rscitedCR 701.9a(Discard) for Recruit. Recruitis
CR 701.70a, verified againstdocs/MagicCompRules.txt:3877:The two sites get different treatment, which that rule text justifies directly:
701.9areplaced by701.70a;is
701.9a, and the nonland contingency that makes the discard result load-bearing is701.70a.Every other
CR 701.9ain the file was checked and is a genuine Discard site, left alone.3. A parser-only test moved to the parser suite. The
assimilate-without-a-graveyard-targetfail-closed test asserted on
parse_effectoutput and did not need the runtime harness, so it nowlives in
parser/oracle_effect/tests.rsbeside the existing shape test. Engine integration test countdrops 8 -> 7, and two now-dead imports were removed. The second commit fixes two comment incoherences
this relocation introduced (the integration module doc named only one of the two parser tests, and the
relocated test kept a
3c.ordinal indexing a scheme in a different file).Validation
test-engine: green on exactly these bytes (23251/23251 on the first candidate; re-verifiedgreen after the comment fixes, with build start timestamps confirmed to post-date every edit).
oracle-gen+coverage-reportprojection from a pinned
AtomicCards.jsonproduced a byte-identicalcard-data.json, and anegative control (one card's
parse_detailsmutated) produced 2 clusters, proving the empty resultdiscriminates rather than being vacuous.
docs/MagicCompRules.txtwith a positive control, per theproject's mandatory-verification rule.
Validation gap, stated explicitly
Local
clippycould not return a verdict on this change. The shared checkout currently fails tocompile
phase-engine (lib)because of an unrelated concurrent change(
crates/engine/src/types/ability_visit.rs:visit_ability_def_scopedreferenced at four sites withno definition — another agent mid-refactor, not touched here). That failure stops upstream of the
test "integration"target where this PR's doc comments live, so it is neither a pass nor a fail forthis change. CI builds from the committed tree, which does not contain that broken file, so CI's
clippy run is the authoritative — and uncontaminated — gate here.
The residual risk is confined to doc lints.
doc_lazy_continuation(which did fire on this fileearlier in development, so the lint demonstrably reaches this surface) requires a preceding markdown
list item; the comment edits in the second commit are plain prose with no list marker, and one of them
removes a
3c.token that was never a valid ordered-list marker.Summary by CodeRabbit
Bug Fixes
Tests
Documentation