Fix Brainspoil - #7130
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 parser now recognizes negated relative attachment clauses and creates negated attachment predicates. Brainspoil integration tests cover Aura targeting exclusions, Equipment exceptions, and destruction behavior with regeneration shields. ChangesBrainspoil targeting behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BrainspoilTest
participant OracleTargetParser
participant Engine
participant Battlefield
BrainspoilTest->>OracleTargetParser: parse negated attachment clause
OracleTargetParser-->>BrainspoilTest: return negated target predicate
BrainspoilTest->>Engine: resolve Brainspoil target
Engine->>Battlefield: check attachments and regeneration shield
Battlefield-->>Engine: return target state
Engine-->>BrainspoilTest: apply destruction behavior
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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_target.rs`:
- Around line 14986-15035: Add full-consumption parser tests alongside
that_isnt_enchanted_negates_aura_attachment,
that_isnt_equipped_negates_equipment_attachment, and
that_isnt_enchanted_or_equipped_negates_attachment_disjunction for “that aren’t
enchanted”, “that aren’t equipped”, and “that aren’t enchanted or equipped”.
Assert the same negated attachment FilterProp structures and consumed lengths as
the singular cases, verifying plural subjects retain the restriction.
In `@crates/engine/tests/integration/brainspoil.rs`:
- Around line 84-109: Replace the direct build_target_slots-only assertions in
the Brainspoil legality test with production-pipeline scenarios that cast
Brainspoil through the scenario runner and target-selection flow. Add cases
confirming the equipped creature is accepted and each Aura-enchanted creature is
rejected, exercising apply/WaitingFor/GameAction/stack resolution as required
while preserving the existing bare and unattached-Aura coverage where
applicable.
🪄 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: f2c50352-0e21-4ab2-94da-b0d014c67ba9
📒 Files selected for processing (3)
crates/engine/src/parser/oracle_target.rscrates/engine/tests/integration/brainspoil.rscrates/engine/tests/integration/main.rs
|
Generated for head Parse changes introduced by this PR · 4 card(s), 3 signature(s) (baseline: main
|
324ac17 to
eda2f51
Compare
eda2f51 to
b21be0f
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocking for current head b21be0f0629ee12722747d951175e2ab3d095797.
The required Rust test shard is red. Job 93266136511 fails the new regression brainspoil_cant_regenerate_rider_bypasses_a_real_regeneration_shield at crates/engine/tests/integration/brainspoil.rs:139: the asserted regeneration shield was not installed (13,284 passed, 1 failed; exit 100). Please fix, remove, or rework that regression so it establishes the actual shield state before asserting Brainspoil's rider behavior, then provide fresh exact-head green CI and a refreshed parse-diff artifact.
The current parse-diff sticky is for older head eda2f515…, and this branch is behind main; do not treat either older evidence or the prior review state as clearance.
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/tests/integration/brainspoil.rs`:
- Around line 149-164: Update both precondition assertions in the Brainspoil
integration test to require each matching Regeneration shield to have
is_consumed set to false, in addition to matching ShieldKind::Regeneration.
Ensure the test setup exercises the failure path where a consumed shield would
otherwise allow Brainspoil to destroy the target.
🪄 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: f5df6b54-e7b6-4703-940b-4f06c8b3293f
📒 Files selected for processing (2)
crates/engine/tests/integration/brainspoil.rscrates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/tests/integration/main.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocking for current head e2aed8f045b0a56614d18746aa3897866220e581.
The new tests still do not discriminate the production behavior completely:
brainspoil.rs:105–130callsbuild_target_slotsdirectly, bypassing the production target-selection path (engine.rs:7843–7849). Exercise Brainspoil through that real selection/resolution pipeline so the test proves the target restriction is enforced where players/AI use it.- The regeneration fixture at
brainspoil.rs:154–169does not require the shield to be unconsumed. Establish and assert a live!is_consumedregeneration shield before testing the rider, then retain the ordinary-destruction control that proves consumption. oracle_target.rs:14986–15035lacks full-consumption tests for plural attachment clauses. Add parser coverage for the plural grammar, including that the complete clause is consumed and produces the intended negated attachment filter rather than falling through to a partial parse.
The prior requested-changes review predates this head. Required CI is still running and the parse-diff sticky is for 5f4b849…, so neither is approval evidence for this revision.
|
Maintainer hold — current head Fresh review confirms the prior requested coverage fixes are clean and no substantive finding remains. Approval is waiting only on external evidence: Rust lint and both Rust test shards are still running, and the available Next step: let the required checks complete successfully and refresh the parse-diff artifact for this exact head against current |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved for exact head 77c9fb34cc3dcb964e20fef152b05722e7a24c5c.
Fresh review found all prior coverage blockers resolved. The current parse-diff artifact matches this head and reports only the intended negated-Aura attachment-filter changes for Akoum, Brainspoil, Time Elemental, and Winds of Rath; all required hosted checks are green.
2 participants
Summary
Fixes Brainspoil's dropped
that isn't enchantedtarget restriction. The parser now reuses the existing negated relative-clause grammar and target legality excludes Aura-enchanted creatures while preserving Equipment targets.Files changed
Track
Developer
LLM
Model: gpt-5.6-sol
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo fmt --all— PASSgit diff --check— PASS./scripts/check-parser-combinators.sh— PASSIndependent
review-impl— PASSFocused
cargo test -p phase-engine --test integration brainspoil— CI-owned: local cargo process could not report completion in the constrained workspace.cargo coverage/cargo semantic-audit— CI-owned: required generated card data could not be retained within available disk space.Gate A
Gate A PASS head=324ac171418e7b1e8dde66743b3b71492660a3ec base=b42dcb14150f1bfff64e7ae87019360c59c0c1af
Anchored on
Final review-impl
Final review-impl PASS head=324ac171418e7b1e8dde66743b3b71492660a3ec
Claimed parse impact
Brainspoil.
Scope Expansion
None.
Validation Failures
None.
CI Failures
Local Developer-track card-data generation, coverage, and semantic audit could not run because bootstrap outputs exhausted the workspace disk. The focused engine test was launched but the local runner did not return its completion status.
Summary by CodeRabbit
Bug Fixes
Tests