fix(parser): keep multi-zone search-and-put as one compound #6200
Conversation
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
There was a problem hiding this comment.
Code Review
This pull request introduces a helper function is_search_result_put_onto_battlefield_restatement to handle bare "put it onto the battlefield" restatements after a search compound, preventing incorrect clause splitting for multi-zone searches (e.g., "The Hunger Tide Rises"). It also refactors the follow-up continuation parser to use this helper and adds unit tests. The review feedback correctly identifies a violation of Rule R1, pointing out that the new helper function uses verbatim string matching on Oracle phrases instead of nom combinators, and provides a robust refactoring suggestion using nom combinators.
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.
| fn is_search_result_put_onto_battlefield_restatement(lower: &str) -> bool { | ||
| let bare = strip_search_result_subject(lower.trim().trim_end_matches('.')); | ||
| matches!( | ||
| bare, | ||
| "put that card onto the battlefield" | ||
| | "put it onto the battlefield" | ||
| | "puts that card onto the battlefield" | ||
| | "puts it onto the battlefield" | ||
| | "put them onto the battlefield" | ||
| | "put those cards onto the battlefield" | ||
| | "put that card onto the battlefield tapped" | ||
| | "put it onto the battlefield tapped" | ||
| | "puts that card onto the battlefield tapped" | ||
| | "puts it onto the battlefield tapped" | ||
| | "put them onto the battlefield tapped" | ||
| | "put those cards onto the battlefield tapped" | ||
| ) | ||
| } |
There was a problem hiding this comment.
[HIGH] Avoid verbatim string matching for Oracle phrases. Evidence: crates/engine/src/parser/oracle_effect/sequence.rs:173. Why it matters: Verbatim string matching on full Oracle phrases bypasses the robust nom-based parser, creating fragile matches that fail on minor punctuation or spacing variations. Suggested fix: Use nom combinators to parse the restatement phrase.
fn is_search_result_put_onto_battlefield_restatement(lower: &str) -> bool {
let bare = strip_search_result_subject(lower.trim().trim_end_matches('.'));
(
alt((tag::<_, _, OracleError<'_>>("put "), tag("puts "))),
alt((tag("that card"), tag("it"), tag("them"), tag("those cards"))),
tag(" onto the battlefield"),
opt(tag(" tapped")),
)
.parse(bare)
.is_ok()
}References
- R1. Nom combinators on the first pass — no exceptions. Every new parser dispatch under crates/engine/src/parser/ must use nom 8.0 combinators... verbatim string equality on full Oracle phrases is the single most prohibited pattern in the codebase. (link)
- Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates to prevent combinatorial explosion and improve maintainability.
matthewevans
left a comment
There was a problem hiding this comment.
The new is_search_result_put_onto_battlefield_restatement helper recognizes complete Oracle phrases with a matches! string list. This is parsing dispatch, and the project requires new parser grammar to be expressed with nom combinators from the first line (compose the verb/pronoun/tapped axes rather than enumerating full sentences).
Please replace this helper’s verbatim phrase matching with a nom parser, then keep the two callers delegated to that shared combinator. The multi-zone origin fix and its regression coverage are otherwise pointed at the right seam.
Parse changes introduced by this PR · 15 card(s), 5 signature(s) (baseline: main
|
|
Maintainer fixup pushed to The search-result battlefield restatement is now parsed through a shared, all-consuming nom combinator, preserving the existing verb/pronoun/tapped variants without verbatim Oracle-text dispatch. Refreshed CI is the remaining verification gate. |
matthewevans
left a comment
There was a problem hiding this comment.
Blocked: the regression does not exercise the search-resolution pipeline.
🔴 Blocker
[HIGH] The new coverage stops at parse_effect_chain and a direct parse_followup_continuation_ast call. Evidence: crates/engine/src/parser/oracle_effect/tests.rs:12199 and crates/engine/src/parser/oracle_effect/sequence.rs:8160; neither test runs a search choice through GameScenario / GameRunner and observes the selected library-or-graveyard card move once to the battlefield. Why it matters: the reported defect is the search/pick resolution path, while the refreshed parse artifact changes 15 cards across five signatures; an AST-only assertion cannot prove that the production continuation consumes the result without a duplicate move. Suggested fix: add a registered integration regression that uses the exact The Hunger Tide Rises chapter-IV Oracle text, selects a candidate from each relevant source zone, and asserts the selected object reaches the battlefield exactly once.
✅ Clean
The prior nom-combinator finding is resolved at sequence.rs:173; the current all-consuming verb/pronoun/tapped parser is at the correct shared seam. The fresh parse-diff (updated 2026-07-19T18:31:09Z) maps to the intended search-and-put class, including Agency Outfitter, Finale of Devastation, and Invasion of Ikoria.
Recommendation: request changes for the discriminating runtime regression, then re-review the new head.
7333cc4 to
86e03da
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Blocked — the maintainer conflict port is complete, but this current head has two substantive review blockers.
🔴 Blockers
[HIGH] Restore a composable parser for the search-result restatement. sequence.rs:173-190 dispatches on a 12-item list of complete English sentences. That is fragile to the independent verb/pronoun/tapped axes and regresses the all-consuming nom approach this seam needs. Please replace it with a shared composable parser (including the optional tapped suffix) and add focused coverage for the independent axes.
[HIGH] Exercise the real Saga path. The new integration test manually parses a constant into a spell ResolvedAbility (issue_5977_hunger_tide_chapter_iv.rs:35-38) and enters resolution through raw resolve_ability_chain (:78-80). It therefore does not prove The Hunger Tide Rises' chapter IV trigger/ability pipeline. Please drive the actual card through the normal GameScenario/GameRunner action path, then make the sacrifice and search choices there.
The port itself was deliberately limited to retaining both integration modules in main.rs; the current parse-diff/CI evidence must also be refreshed for this new head.
Recommendation: request changes.
6d2addf to
04617ac
Compare
Co-authored-by: andriypolanski <andriypolanski@users.noreply.github.com>
matthewevans
left a comment
There was a problem hiding this comment.
Approved after current-head review: the multi-zone search result stays one compound resolution, tracked-set detection covers all dynamic filter-property quantity axes, and the Hunger Tide regression drives the real chapter/choice pipeline.
Closes #5977
Summary
Discord report: The Hunger Tide Rises chapter IV lets you sacrifice creatures, but the follow-up "search your library and/or graveyard … and put it onto the battlefield" misbehaves during the search/pick step.
Root cause: the bare-
andclause splitter peeled"and put it onto the battlefield"off the multi-zone search sentence into a siblingChangeZone { target: ParentTarget }, whileSearchDestinationalready pushed the correctChangeZone { origin: None, target: Any }for library-and/or-graveyard tutors. The follow-up absorber only recognized library-origin puts (origin: Some(Library)), so the strayParentTargetput survived and broke the search→put chain.Fix: keep
"search … and put it onto the battlefield"as one compound (like"search … and exile them"), and extend the SearchDestination restatement absorber to cover multi-zone puts withorigin: None.Changes
crates/engine/src/parser/oracle_effect/sequence.rs—is_search_result_put_onto_battlefield_restatementhelper; bare-andsuppressor; absorber acceptsorigin: None | Some(Library).crates/engine/src/parser/oracle_effect/tests.rs— Hunger Tide Rises chapter IV chain regression.crates/engine/src/parser/oracle_effect/sequence.rs(tests) — clause-split + follow-up absorption regressions.Test Plan
cargo fmt --all -- --checkcargo test -p engine --lib hunger_tide_rises_chapter_iv_sacrifice_search_put_chaincargo test -p engine --lib bare_and_keeps_multi_zone_search_put_onto_battlefield_compoundcargo test -p engine --lib multi_zone_search_put_restatement_absorbed_after_origin_none_destinationcargo test -p engine --lib assassins_trophy_its_controller_may_search_chainParentTargetput