Add Opposition Agent#5888
Conversation
github-actions
Bot
added
the
needs-maintainer
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements comprehensive support for Opposition Agent's mechanics, including search-control, found-card replacement, exile play permissions, and nested replacement resume. The changes introduce the SearchFound replacement event, track library search control, manage turn control lifecycles, and update the parser to support these patterns. The review feedback highlights several critical areas for improvement: correcting a stable-sorting contradiction in turn control migration where max_by_key behavior conflicts with vector insertion order, optimizing a double lookup on state.objects, and ensuring strict adherence to the repository style guide by adding mandatory CR annotations to rules-touching functions and replacing the reveal boolean field with a typed enum. Additionally, the reviewer advises against swallowing unexpected errors during batch completion.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| return; | ||
| }; | ||
| if let Ok(batch) = process_search_found_batch(state, batch, events) { | ||
| let _ = finish_search_found_batch(state, batch, events); |
There was a problem hiding this comment.
The EngineError returned by finish_search_found_batch is silently swallowed here using let _ =. Swallowing unexpected errors can mask critical bugs and leave the engine in an invalid state. We should propagate or expect the result, aligning with other batch completion handlers in run_batch_completion.
finish_search_found_batch(state, batch, events)
.expect("SearchFound batch completion must resolve");References
- Avoid swallowing all errors (e.g., using
.ok()) when only specific errors are expected to be handled or ignored. Propagate unexpected errors (such as invariant violations or invalid actions) to prevent masking critical bugs.
| // Legacy saves have no creation timestamp. Zero is the deterministic oldest | ||
| // timestamp; insertion at the front makes any other timestamp-zero legacy | ||
| // schedule later in vector order win the stable max-by-key tie. | ||
| state.scheduled_turn_controls.insert( | ||
| 0, | ||
| ScheduledTurnControl { | ||
| target_player, | ||
| controller, | ||
| timestamp: 0, | ||
| lifecycle: ScheduledTurnControlLifecycle::Active, | ||
| grant_extra_turn_after: false, | ||
| window: crate::types::ability::ControlWindow::NextTurn, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Rust's Iterator::max_by_key returns the first element that achieves the maximum value. Since the newly materialized legacy entry is inserted at the front (index 0), it will actually win any tie with other timestamp-zero entries, directly contradicting the comment's claim that 'any other timestamp-zero legacy schedule later in vector order' would win. Pushing the entry to the end of the vector instead of inserting it at the front resolves this contradiction and is also more efficient.
// Legacy saves have no creation timestamp. Zero is the deterministic oldest
// timestamp; pushing to the end makes any other timestamp-zero legacy
// schedule earlier in vector order win the stable max-by-key tie.
state.scheduled_turn_controls.push(
ScheduledTurnControl {
target_player,
controller,
timestamp: 0,
lifecycle: ScheduledTurnControlLifecycle::Active,
grant_extra_turn_after: false,
window: crate::types::ability::ControlWindow::NextTurn,
},
);| if state | ||
| .objects | ||
| .get(&object_id) | ||
| .is_some_and(|object| object.zone == Zone::Exile) | ||
| { | ||
| if let Some(object) = state.objects.get_mut(&object_id) { |
There was a problem hiding this comment.
The function grant_search_found_play_permission performs a double lookup on state.objects by calling get and then get_mut for the same object_id. This can be optimized into a single get_mut call to improve efficiency.
if let Some(object) = state.objects.get_mut(&object_id) {
if object.zone == Zone::Exile {| fn library_search_control_effect( | ||
| state: &GameState, | ||
| searcher: PlayerId, | ||
| ) -> Option<PlayerControlEffect> { |
There was a problem hiding this comment.
The rules-touching function library_search_control_effect lacks a mandatory CR <number>: <description> comment, violating Rule R6 of the repository style guide. Please add an appropriate CR annotation (e.g., referencing CR 723.1a or CR 723.5).
References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
| #[serde(default)] | ||
| pub reveal: bool, |
There was a problem hiding this comment.
The struct PendingSearchFoundBatch introduces a new bool field reveal, violating Rule R2 of the repository style guide ('No bool fields — parameterize with existing typed enums'). Please replace this with a typed enum (e.g., RevealMode or similar) to better express the design space.
References
- No bool fields — parameterize with existing typed enums. (link)
| pub(crate) fn complete_replaced_selection( | ||
| state: &mut GameState, | ||
| player: PlayerId, | ||
| survivors: Vec<ObjectId>, | ||
| events: &mut Vec<GameEvent>, | ||
| ) -> Result<(), EffectError> { |
There was a problem hiding this comment.
The rules-touching function complete_replaced_selection lacks a mandatory CR <number>: <description> comment, violating Rule R6 of the repository style guide. Please add an appropriate CR annotation (e.g., referencing CR 701.23a).
References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
Parse changes introduced by this PRBaseline pending for |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — narrow this card-specific PR and use the engine authority.
🔴 Blocker
[HIGH] The change adds 5,465 unrelated validation lines from crates/engine/src/database/card_db.rs:72 through the generic validator beginning near :410 to a one-card Opposition Agent PR. Why it matters: the unrelated blast radius cannot be reviewed or safely attributed to this fix. Suggested fix: split the generic validator work out and retain only the scoped Opposition Agent change.
[HIGH] The CI Engine authority gate fails on raw .keywords.iter() uses at crates/engine/src/database/card_db.rs:418,699,1222,2790. Why it matters: the PR violates the repository's keyword-access authority. Suggested fix: route those checks through the established authority rather than direct iteration.
Recommendation: request-changes.
2 participants
Summary
Adds engine support for Opposition Agent.
Files changed
crates/engine/src/analysis/crates/engine/src/game/crates/engine/src/parser/crates/engine/src/types/crates/engine/tests/integration/opposition_agent.rscrates/phase-ai/src/policies/CR references
Implementation method (required)
Method: /engine-implementer
Track
Developer
LLM
Model: gpt-5
Thinking: high
Tier: Standard
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— passed on the final committed head.Parser Gate G and Gate A — passed on the final committed head.
Final
review-impl— passed on the final committed head.Tilt
clippy— the narrowed task diff reached terminalokbefore the final upstream-only sync; the exact-head rerun was blocked by the local filesystem reachingNo space left on device, so exact-head completion is CI-owned.Tilt
card-data— exact-head rerun was blocked by the same localNo space left on devicecondition; completion is CI-owned.cargo coverage— earlier task snapshot exited 0; 31,351/35,397 cards supported (88.57%).cargo semantic-audit— earlier task snapshot exited 0; 32,396 cards audited.Tilt
test-engineandtest-ai— full final-head completion is CI-owned.Strict no-MTGish path gate — passed for branch, index, worktree, and untracked files on the final committed head.
Validator-scope gate — passed; the four database/export files called out in review are absent from the PR diff, along with the introduced raw keyword iteration sites.
Gate A
Gate A PASS head=93b61c791a09ffc5b37e0317b390e7c31cfcbb07 base=7403be8a5e14c33e3e416531a45497187d32aa31
Anchored on
crates/engine/src/parser/oracle_replacement.rs:1270— existing nom-based replacement parser that lowers a complete Oracle pattern into a typedReplacementDefinitionwith an execute ability.crates/engine/src/parser/oracle_replacement.rs:1385— existing nom-based replacement parser that composes subject and consequence alternatives into typed replacement parameters.Final review-impl
Final review-impl PASS head=93b61c791a09ffc5b37e0317b390e7c31cfcbb07
Claimed parse impact
Validation Failures
The final local
clippyandcard-datareruns could not complete because the workspace filesystem had only hundreds of MB free and Rust failed writes withNo space left on device. Repository policy forbids deleting Tilt build artifacts while Tilt is active, so those exact-head checks are CI-owned. No code diagnostic remains known.CI Failures
The complete final-head
clippy,card-data,test-engine, andtest-aichecks are CI-owned due the local storage constraint. A prior intermediate test run observed the unrelated token-preset expected-count mismatch caused by an uncommittedcrates/engine/data/known-tokens.tomlchange in the shared workspace; that file is excluded from this PR.