From 897e35454945399637b17f90d077013771f9d9d1 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Tue, 14 Jul 2026 05:02:14 -0700 Subject: [PATCH 1/3] fix(parser): route every keyword-line router slot through the strict parser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plan 02 step 5 item 11. The strict, remainder-checking keyword parser was wired at only 2 of ~14 router entries (priorities 9 and 13). The other 16 call sites — priorities 0 and 1b, priority 8f, the flashback / suspend / specialize / buyback / escalate / commander-ninjutsu / d20 intercepts, and the two routing classifiers — still called the PERMISSIVE surface, which takes the leading keyword and discards the remainder. At a router boundary that discarded remainder is unparsed card text: the line is consumed with no keyword and no diagnostic, and the card renders as fully supported. This migrates all 16 onto strict surfaces. Priority 1b was SHADOWING the strict router. It runs long before priorities 9/13, so whenever MTGJSON names the keyword — the common case — 1b decided the line first. "Cycling {2} if you control an artifact" with MTGJSON ["cycling"] committed a bare Cycling, ate the conditional, and raised nothing; the same line with an empty MTGJSON list was already honest. The strict wiring downstream was therefore unreachable for exactly the cards MTGJSON knows about. Priority 8f carried two defects, both fixed here: (a) Class-A silent swallow — `i += 1; continue;` sat OUTSIDE both `if let Some` blocks, so a candidate line neither parser could parse vanished outright. (b) No word boundary — a bare `alt((tag("kicker"), ...))` accepted any line merely starting with those letters, so "Kickerfoo {2}" matched and then vanished via (a). `is_kicker_family_line` now shares `is_keyword_cost_line`'s boundary rule (end / space / tab / em-dash). Building blocks, rather than 16 one-off fixes: - parse_router_keyword_fragment — the remainder-CHECKING sibling of parse_granted_keyword_fragment (which is core-with-remainder-discarded). - parse_router_keyword_list — the keyword-LIST sibling, sharing the list walk with extract_granted_keyword_list via a typed KeywordRemainderPolicy { DiscardRemainder | RequireAllConsuming } rather than a second copy or a bool. parse_router_keyword_line cannot serve here: it parses ONE keyword and takes no MTGJSON names, so it cannot see a bare-keyword line ("Flying, vigilance"). - matches_keyword_prefix_at_word_boundary — one boundary rule, shared. - parse_kicker_cost_payload now rejects a cost that is not fully typed, reusing the existing ability_cost_is_fully_typed authority. parse_oracle_cost NEVER fails, so without this the additional-cost path would rebuild a cost out of the untyped tail and re-consume the very line the keyword parser had just declined. The permissive symbols are no longer imported into oracle.rs at all. Gate G in scripts/check-parser-combinators.sh accordingly collapses from an exact-count ratchet (13/2/1) to the plain invariant Plan 02 step 7 specifies: zero permissive keyword-parser symbols in any router context, at any count. SKILL.md §3a's migration-status paragraph is deleted with it. Coverage may legitimately DECREASE: silently-swallowed lines become honest, exact-unit Effect::Unimplemented. That is the point of the change. Witnesses (crates/engine/src/parser/oracle_tests.rs), each watched RED on pre-fix code and GREEN after, each paired with a positive control so the strict assertions cannot pass vacuously: - Kickerfoo {2} -> pre-fix: keywords=[] abilities=0 Unimplemented=[] (vanished); now exactly one exact-unit Unimplemented. - priority 1b does not shadow the strict router (both MTGJSON-named and silent). - crew / semicolon-list / kicker / suspend / specialize / escalate / commander-ninjutsu / buyback semantic tails are no longer eaten. CR 702.33 Kicker, CR 702.33c Multikicker, CR 702.56 Replicate, CR 702.187 Mayhem — all grep-verified against docs/MagicCompRules.txt with positive and negative controls. --- .claude/skills/oracle-parser/SKILL.md | 21 +- crates/engine/src/parser/oracle.rs | 156 +++++++----- crates/engine/src/parser/oracle_keyword.rs | 171 +++++++++++-- crates/engine/src/parser/oracle_tests.rs | 277 +++++++++++++++++++++ scripts/check-parser-combinators.sh | 107 +++----- 5 files changed, 566 insertions(+), 166 deletions(-) diff --git a/.claude/skills/oracle-parser/SKILL.md b/.claude/skills/oracle-parser/SKILL.md index 6fd4c0e56b..13472e1308 100644 --- a/.claude/skills/oracle-parser/SKILL.md +++ b/.claude/skills/oracle-parser/SKILL.md @@ -466,6 +466,8 @@ the wrong one at a router boundary is the silent-swallow bug class. |---|---|---| | `parse_keyword_line_core()` | Remainder-**preserving** core: `Option<(Keyword, &str /* unconsumed */)>`. The single authority both wrappers are built on. | Internal — call a wrapper, not this. | | `parse_router_keyword_line()` | **STRICT / all-consuming.** Candidate recognizer → reminder strip → core → `all_consuming` permitted tail (P/R/M modifiers). Returns a typed `RoutedKeywordLine` only when the line parses *completely*. | The **only** surface that may license a router to CONSUME a whole line. | +| `parse_router_keyword_fragment()` | **STRICT.** One keyword phrase: core + all-consuming permitted tail + modifiers. The primitive the other strict surfaces are built on. | Any router intercept parsing a single keyword phrase (flashback, suspend, specialize, buyback, escalate, commander ninjutsu, …). | +| `parse_router_keyword_list()` | **STRICT.** The keyword-LIST sibling: comma parts, MTGJSON validation, protection expansion — with every part all-consuming. | Router slots and routing classifiers facing a keyword *list* (priorities 0 and 1b, `is_semicolon_keyword_line`, `is_spell_resolution_instruction_line`). `parse_router_keyword_line` cannot serve here: it parses ONE keyword and takes no MTGJSON names, so it cannot see a bare-keyword line ("Flying, vigilance"). | | `parse_granted_keyword_fragment()` / `extract_granted_keyword_list()` | **PERMISSIVE.** Takes the leading keyword and **discards the remainder** — by design. | **Embedded grant contexts only**: static/token/vote/class-level/effect-payload payloads ("…gains vanishing 3 if …"), where a trailing clause belongs to the *enclosing* sentence. | **Consume-on-success (the rule).** A candidate recognizer (`is_keyword_cost_line`) @@ -485,16 +487,15 @@ strict parser fails the build. `KNOWN_NOUN_PARAM_LEAKS` is a **ratchet** listing families whose noun/filter parameter still absorbs a trailing clause — entries are deleted as each is fixed, never added to. -**Migration status (accurate as of Plan 02 step 5).** The strict router is wired at -priority `9` (spell) and priority `13` (permanent). The remaining keyword-cost router -entries — priority `0`, `1b`, `8f`, the flashback/suspend/buyback/escalate/commander-ninjutsu -intercepts, and the two line classifiers — **still call the permissive surface** and are -therefore still capable of swallowing a semantic tail. That surviving set is enumerated -and frozen by **Gate G** in `scripts/check-parser-combinators.sh`: it counts the permissive -calls inside `parse_oracle_ir`, `is_semicolon_keyword_line`, and -`is_spell_resolution_instruction_line` and fails if the count rises (a new permissive -router call) *or* falls without lowering the gate's expected floor. Gate G is a ratchet, -not a blessing — when every count reaches 0, the boundary is fully enforced. +**The boundary is fully enforced.** Every router slot and routing classifier now parses +through a strict surface; the permissive symbols are not even imported into `oracle.rs`. +**Gate G** in `scripts/check-parser-combinators.sh` is the plain whole-file invariant: a +permissive keyword-parser symbol appearing anywhere inside `parse_oracle_ir`, +`is_semicolon_keyword_line`, or `is_spell_resolution_instruction_line` fails the build, at +any count. Note the ordering consequence this closed: priority `1b` runs long before the +strict routers at `9`/`13`, so while it was permissive it claimed keyword lines first +whenever MTGJSON named the keyword — which silently shadowed the strict wiring downstream +for exactly the cards MTGJSON knows about. ### `is_static_pattern()` — `oracle_classifier.rs` Gates Priority `7`. Returns false for `target`-leading lines, then matches diff --git a/crates/engine/src/parser/oracle.rs b/crates/engine/src/parser/oracle.rs index b1e22bac7f..fd461d8e76 100644 --- a/crates/engine/src/parser/oracle.rs +++ b/crates/engine/src/parser/oracle.rs @@ -68,8 +68,8 @@ use super::oracle_ir::feature::ItemIdTracks; use super::oracle_ir::relation::{DocumentRelationIr, LinkedChoiceKind}; pub use super::oracle_keyword::keyword_display_name; use super::oracle_keyword::{ - extract_granted_keyword_list, is_keyword_cost_line, parse_granted_keyword_fragment, - parse_kicker_additional_cost_line, parse_router_keyword_line, + is_keyword_cost_line, is_kicker_family_line, parse_kicker_additional_cost_line, + parse_router_keyword_fragment, parse_router_keyword_line, parse_router_keyword_list, }; use super::oracle_level::parse_level_blocks; use super::oracle_modal::{ @@ -2148,6 +2148,11 @@ fn is_standalone_spell_keyword_action_line(line: &str) -> bool { parsed } +/// A classifier MAY probe with the STRICT parser; it may NOT probe with a helper +/// that discards the remainder. This one gates a routing decision (priority 0 and +/// the spell-resolution guard), so a permissive probe would report "this whole line +/// is keywords" about a line carrying an unparsed semantic clause — and the router +/// would then consume it. fn is_semicolon_keyword_line(line: &str, mtgjson_keyword_names: &[String]) -> bool { let mut saw_multiple_parts = false; let mut parts = line @@ -2158,13 +2163,13 @@ fn is_semicolon_keyword_line(line: &str, mtgjson_keyword_names: &[String]) -> bo return false; }; - if extract_granted_keyword_list(first, mtgjson_keyword_names).is_none() { + if parse_router_keyword_list(first, mtgjson_keyword_names).is_none() { return false; } for part in parts { saw_multiple_parts = true; - if extract_granted_keyword_list(part, mtgjson_keyword_names).is_none() { + if parse_router_keyword_list(part, mtgjson_keyword_names).is_none() { return false; } } @@ -2201,8 +2206,12 @@ fn is_spell_resolution_instruction_line( return false; } + // Strict probe: a line is only "not a spell resolution instruction, it's a + // keyword line" when it parses COMPLETELY as keywords. A permissive probe here + // reports true for "Cycling {2} if you control an artifact" and the conditional + // tail is never parsed by anything. if !is_ability_activate_cost_static(&lower) - && extract_granted_keyword_list(line, mtgjson_keyword_names).is_some() + && parse_router_keyword_list(line, mtgjson_keyword_names).is_some() { return false; } @@ -3705,19 +3714,18 @@ pub(crate) fn parse_oracle_ir( .map(|s| s.trim()) .filter(|s| !s.is_empty()) .collect(); + // Consume-on-success: EVERY part must parse completely as keywords. The + // permissive form accepted a part carrying a semantic clause ("cycling + // {2} if you control an artifact"), consumed the whole line, and dropped + // that clause with no keyword and no diagnostic. if parts.len() > 1 { - let all_keywords = parts.iter().all(|part| { - extract_granted_keyword_list(part, mtgjson_keyword_names).is_some() - }); - if all_keywords { - for part in &parts { - if let Some(extracted) = - extract_granted_keyword_list(part, mtgjson_keyword_names) - { - for __item in extracted { - emitter.keyword_at(item_line, __item); - } - } + let routed: Option>> = parts + .iter() + .map(|part| parse_router_keyword_list(part, mtgjson_keyword_names)) + .collect(); + if let Some(routed) = routed { + for keyword in routed.into_iter().flatten() { + emitter.keyword_at(item_line, keyword); } i += 1; continue; @@ -3761,8 +3769,10 @@ pub(crate) fn parse_oracle_ir( // CR 702.122 + CR 602.5b: Crew with a trailing "Activate only once each // turn." cadence sentence. Must run before the generic keyword-only - // extraction below — that path parses "Crew N" via `parse_granted_keyword_fragment` - // and would consume the line, dropping the cadence sentence. + // extraction below: that path would emit a bare `Crew N` and leave the cadence + // sentence to be re-parsed as its own unit. This intercept models both in one + // keyword. (Both surfaces are strict about the tail — `parse_crew_keyword` ends + // in `all_consuming`, and priority 1b now routes through the strict list.) if lower_starts_with(&lower, "crew ") { if let Some(crew_kw) = parse_crew_keyword(&lower) { emitter.keyword_at(item_line, crew_kw); @@ -3774,9 +3784,17 @@ pub(crate) fn parse_oracle_ir( // Priority 1b: keyword-only line — extract any keywords for the union set // Guard: "{Keyword} abilities you activate cost {N} less" is a static ability, // not a keyword line. Don't let keyword extraction consume it. + // Consume-on-success. This slot runs LONG before the strict routers at + // priority 9 / 13, so whenever MTGJSON names the keyword — the common case — + // it is THIS slot, not those, that decides the line. On the permissive + // surface it consumed "Cycling {2} if you control an artifact" as a bare + // Cycling and dropped the condition, which made the strict wiring downstream + // unreachable for exactly the cards MTGJSON knows about. Only a completely + // parsed keyword list may consume the line now; anything else falls through + // and becomes an honest, exact-unit `Effect::Unimplemented`. let is_ability_cost_static = is_ability_activate_cost_static(&lower); if !is_ability_cost_static { - if let Some(extracted) = extract_granted_keyword_list(&line, mtgjson_keyword_names) { + if let Some(extracted) = parse_router_keyword_list(&line, mtgjson_keyword_names) { if let Some(cost) = parse_kicker_additional_cost_line(&line, &lower) { merge_kicker_additional_cost(&mut result.additional_cost, cost); additional_cost_line.get_or_insert(item_line); @@ -4660,7 +4678,7 @@ pub(crate) fn parse_oracle_ir( if lower_starts_with(&lower, "flashback") { if line.contains('\u{2014}') { let lower_clean = lower.trim_end_matches('.').trim(); - if let Some(kw) = parse_granted_keyword_fragment(lower_clean) { + if let Some(kw) = parse_router_keyword_fragment(lower_clean) { emitter.keyword_at(item_line, kw); i += 1; continue; @@ -4668,17 +4686,21 @@ pub(crate) fn parse_oracle_ir( } else if let Some((flashback_part, reduction_part)) = split_flashback_trailing_self_spell_cost_reduction(&line, &lower) { + // ATOMIC + consume-on-success. The split PROMISES two semantic halves. + // The previous form advanced `i` unconditionally, so a line whose + // keyword half parsed but whose cost-reduction half did not (or vice + // versa) was consumed with the other half silently dropped. Both must + // parse, or the line falls through and stays honestly red. let flashback_lower = flashback_part.to_lowercase(); - if let Some(kw) = parse_granted_keyword_fragment(&flashback_lower) { + if let (Some(kw), Some(def)) = ( + parse_router_keyword_fragment(&flashback_lower), + parse_flashback_trailing_self_spell_cost_reduction(reduction_part), + ) { emitter.keyword_at(item_line, kw); - } - if let Some(def) = - parse_flashback_trailing_self_spell_cost_reduction(reduction_part) - { emitter.static_at(item_line, def); + i += 1; + continue; } - i += 1; - continue; } } @@ -5079,7 +5101,7 @@ pub(crate) fn parse_oracle_ir( // Spells (instants/sorceries) with Suspend would otherwise be caught by // the is_spell branch and produce an Unimplemented effect. if lower_starts_with(&lower, "suspend ") { - if let Some(kw) = parse_granted_keyword_fragment(&lower) { + if let Some(kw) = parse_router_keyword_fragment(&lower) { emitter.keyword_at(item_line, kw); i += 1; continue; @@ -5089,7 +5111,7 @@ pub(crate) fn parse_oracle_ir( // Digital-only Specialize: "specialize {cost}" — MTGJSON may omit the keyword // when it appears as a standalone rules line; intercept before dispatch fallback. if lower_starts_with(&lower, "specialize ") { - if let Some(kw) = parse_granted_keyword_fragment(&lower) { + if let Some(kw) = parse_router_keyword_fragment(&lower) { emitter.keyword_at(item_line, kw); i += 1; continue; @@ -5101,7 +5123,7 @@ pub(crate) fn parse_oracle_ir( // is intercepted as a keyword, not parsed as an effect. // MTGJSON keywords array only says "Harmonize" (no cost), so we extract cost here. // Format: "Harmonize {cost} (reminder text)" — space-separated. - // Note: When MTGJSON provides "Harmonize" in keywords, extract_granted_keyword_list at + // Note: When MTGJSON provides "Harmonize" in keywords, the strict keyword list at // priority 1b already handles this. This is a fallback for test/edge cases. if lower_starts_with(&lower, "harmonize ") { if let Some(harmonize_kw) = parse_harmonize_keyword(&line) { @@ -5123,41 +5145,53 @@ pub(crate) fn parse_oracle_ir( } } - // Priority 8f: Kicker / Multikicker / Replicate cost lines — must run BEFORE Priority 9 - // (spell catch-all) so these keyword declarations on spell cards don't become Unimplemented. + // Priority 8f: CR 702.33 Kicker / CR 702.33c Multikicker / CR 702.56 Replicate / + // CR 702.187 Mayhem cost lines — must run BEFORE Priority 9 (spell catch-all) so + // these keyword declarations on spell cards don't become Unimplemented. // We cannot use is_keyword_cost_line here because it would also catch "flashback" // etc. whose specific em-dash parsers run between Priority 9 and Priority 13. // Note: "mayhem" IS in is_keyword_cost_line and is handled at Priority 1b via MTGJSON // keywords when present; this guard catches it when keywords[] is empty. - if alt(( - tag::<_, _, OracleError<'_>>("kicker"), - tag("multikicker"), - tag("replicate"), - tag("mayhem"), - )) - .parse(lower.as_str()) - .is_ok() - { + // + // Two defects fixed here (task #123), both of which made this the worst site: + // (a) CLASS-A SILENT SWALLOW. `i += 1; continue;` used to sit OUTSIDE both + // `if let Some` blocks, so a candidate line that NEITHER parser could + // parse was consumed with no keyword, no additional cost, and no + // diagnostic — it vanished and the card rendered as fully supported. + // The line is now consumed only if something was actually recorded. + // (b) NO WORD BOUNDARY. The dispatch was a bare `alt((tag("kicker"), …))`, + // so it matched any line merely STARTING with those letters — + // "Kickerfoo {2}" was accepted and then vanished via (a). + // `is_kicker_family_line` shares `is_keyword_cost_line`'s boundary rule. + if is_kicker_family_line(&lower) { + let mut recorded = false; if let Some(cost) = parse_kicker_additional_cost_line(&line, &lower) { merge_kicker_additional_cost(&mut result.additional_cost, cost); + additional_cost_line.get_or_insert(item_line); + recorded = true; } - if let Some(kw) = parse_granted_keyword_fragment(&lower) { + if let Some(kw) = parse_router_keyword_fragment(&lower) { emitter.keyword_at(item_line, kw); + recorded = true; } - i += 1; - continue; + if recorded { + i += 1; + continue; + } + // Nothing parsed: fall through to the spell catch-all / priority 15 so the + // line becomes an honest, exact-unit `Effect::Unimplemented`. } // CR 702.27a: Buyback em-dash form — "Buyback—Sacrifice a land." (Constant // Mists) etc. MTGJSON omits the Buyback keyword when the cost is non-mana, - // so `extract_granted_keyword_list` bails and the line would otherwise fall through + // so the priority-1b keyword list bails and the line would otherwise fall through // to the spell-effect catch-all and produce `Unimplemented`. Intercept here // before the spell catch-all, mirroring the Flashback em-dash intercept above. // structural: not dispatch — em-dash char presence gates the cost sub-parser, // which uses nom combinators in `parse_buyback_cost` / `parse_oracle_cost`. if lower_starts_with(&lower, "buyback") && line.contains('\u{2014}') { let lower_clean = lower.trim_end_matches('.').trim(); - if let Some(kw) = parse_granted_keyword_fragment(lower_clean) { + if let Some(kw) = parse_router_keyword_fragment(lower_clean) { emitter.keyword_at(item_line, kw); i += 1; continue; @@ -5173,7 +5207,7 @@ pub(crate) fn parse_oracle_ir( .is_ok() { let lower_clean = lower.trim_end_matches('.').trim(); - if let Some(kw) = parse_granted_keyword_fragment(lower_clean) { + if let Some(kw) = parse_router_keyword_fragment(lower_clean) { emitter.keyword_at(item_line, kw); i += 1; continue; @@ -5508,7 +5542,7 @@ pub(crate) fn parse_oracle_ir( // CR 702.49d: Commander ninjutsu is not in MTGJSON keywords — extract explicitly. if lower_starts_with(&lower, "commander ninjutsu ") { - if let Some(kw) = parse_granted_keyword_fragment(&lower) { + if let Some(kw) = parse_router_keyword_fragment(&lower) { emitter.keyword_at(item_line, kw); i += 1; continue; @@ -5518,7 +5552,7 @@ pub(crate) fn parse_oracle_ir( // CR 702.138a: Escape is extracted by the generic keyword-cost guards — // the `is_spell` guard above (Priority 9) for instants/sorceries and the // `is_keyword_cost_line` guard below (Priority 13) for permanents — via the - // `escape—` branch registered in `parse_granted_keyword_fragment`, alongside its + // `escape—` branch registered in `parse_keyword_line_core`, alongside its // evoke/embalm/eternalize/escalate em-dash siblings. No dedicated intercept // is needed here. @@ -5540,17 +5574,18 @@ pub(crate) fn parse_oracle_ir( if let Some((flashback_part, reduction_part)) = split_flashback_trailing_self_spell_cost_reduction(&line, &lower) { + // ATOMIC + consume-on-success — see the identical split above (priority 7 + // guard). Both halves must parse or the line is not consumed. let flashback_lower = flashback_part.to_lowercase(); - if let Some(kw) = parse_granted_keyword_fragment(&flashback_lower) { + if let (Some(kw), Some(def)) = ( + parse_router_keyword_fragment(&flashback_lower), + parse_flashback_trailing_self_spell_cost_reduction(reduction_part), + ) { emitter.keyword_at(item_line, kw); - } - if let Some(def) = - parse_flashback_trailing_self_spell_cost_reduction(reduction_part) - { emitter.static_at(item_line, def); + i += 1; + continue; } - i += 1; - continue; } } // Consume-on-success. The previous form advanced `i` OUTSIDE the @@ -5632,8 +5667,11 @@ pub(crate) fn parse_oracle_ir( } // Try as keyword — the ability-word prefix ("Void Shields —") was // stripped, so the remainder may be a keyword line that Priority 1b - // missed because it ran on the unprefixed original line. - if let Some(kw) = parse_granted_keyword_fragment(&effect_lower) { + // missed because it ran on the unprefixed original line. Strict: the + // stripped remainder must be a COMPLETE keyword declaration, or the line + // continues to the static/effect paths and ultimately an honest + // `Effect::Unimplemented`. + if let Some(kw) = parse_router_keyword_fragment(&effect_lower) { if !matches!(kw, Keyword::Unknown(_)) { emitter.keyword_at(item_line, kw); i += 1; diff --git a/crates/engine/src/parser/oracle_keyword.rs b/crates/engine/src/parser/oracle_keyword.rs index 2794861342..20ee7dc6ff 100644 --- a/crates/engine/src/parser/oracle_keyword.rs +++ b/crates/engine/src/parser/oracle_keyword.rs @@ -295,13 +295,90 @@ pub(crate) fn parse_kicker_additional_cost_line(raw: &str, lower: &str) -> Optio }) } +/// `parse_oracle_cost` NEVER fails — it hands back `AbilityCost::Unimplemented` +/// for text it cannot type. So "the cost parser returned" is not evidence the cost +/// parsed. Without the `ability_cost_is_fully_typed` guard, a kicker line with a +/// semantic tail ("Kicker {2} if you control an artifact") yields a `Some(_)` cost +/// built from the untyped remainder, and the priority-8f router then consumes the +/// line AND fabricates a cost — strictly worse than declining, because the card +/// renders as supported. Declining lets the line fall through to an honest, +/// exact-unit `Effect::Unimplemented`. fn parse_kicker_cost_payload(input: &str) -> Option { let stripped = strip_reminder_text(input); let cost_text = stripped.trim().trim_end_matches('.').trim(); if cost_text.is_empty() { return None; } - Some(parse_oracle_cost(cost_text)) + let cost = parse_oracle_cost(cost_text); + ability_cost_is_fully_typed(&cost).then_some(cost) +} + +/// Which keyword-parse contract a keyword-LIST consumer needs for each part. +/// +/// This is the list-level counterpart of the `parse_granted_keyword_fragment` / +/// `parse_router_keyword_line` split, expressed as a typed axis rather than two +/// near-duplicate list walks. The list semantics (comma parts, MTGJSON validation, +/// protection expansion, `instances_function_separately`) are identical in both +/// modes; only the per-part remainder contract differs, so THAT is the parameter. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum KeywordRemainderPolicy { + /// EMBEDDED GRANT. The trailing clause belongs to the ENCLOSING sentence + /// ("… gains vanishing 3 if that creature doesn't have vanishing"), so the + /// part parser must take the leading keyword and leave the rest alone. + /// Discarding the remainder is CORRECT here. + DiscardRemainder, + /// ROUTER. The line IS the unit. Any unconsumed semantic prose means the + /// router may not consume the line — it must fall through to ordinary + /// parsing and become an honest, exact-unit `Effect::Unimplemented` rather + /// than vanishing with no keyword and no diagnostic. + RequireAllConsuming, +} + +impl KeywordRemainderPolicy { + /// The single place the policy turns into an actual per-part parse. + fn parse_part(self, lower: &str) -> Option { + match self { + Self::DiscardRemainder => parse_granted_keyword_fragment(lower), + Self::RequireAllConsuming => parse_router_keyword_fragment(lower), + } + } +} + +/// PERMISSIVE keyword-list extraction — see [`KeywordRemainderPolicy::DiscardRemainder`]. +/// +/// Valid ONLY in embedded grant contexts (static/token/vote/class-level payloads). +/// A router that consumes a line on this result silently swallows whatever the +/// keyword did not explain. Routers must call [`parse_router_keyword_list`]. +pub(crate) fn extract_granted_keyword_list( + line: &str, + mtgjson_keyword_names: &[String], +) -> Option> { + parse_keyword_list_with_policy( + line, + mtgjson_keyword_names, + KeywordRemainderPolicy::DiscardRemainder, + ) +} + +/// STRICT keyword-list extraction — see [`KeywordRemainderPolicy::RequireAllConsuming`]. +/// +/// The list-level sibling of [`parse_router_keyword_line`], and the ONLY keyword-list +/// surface a router or a routing classifier may use. Every comma part must parse to a +/// typed keyword with an all-consuming permitted (`P`/`R`/`M`) tail; one part carrying +/// a semantic clause declines the WHOLE line. +/// +/// `parse_router_keyword_line` cannot serve this role: it parses a single keyword and +/// takes no MTGJSON names, so it cannot see a bare-keyword line ("Flying, vigilance") +/// or the MTGJSON-authoritative parts that carry no Oracle parameter. +pub(crate) fn parse_router_keyword_list( + line: &str, + mtgjson_keyword_names: &[String], +) -> Option> { + parse_keyword_list_with_policy( + line, + mtgjson_keyword_names, + KeywordRemainderPolicy::RequireAllConsuming, + ) } /// Try to extract keywords from a keyword-only line (comma-separated). @@ -312,15 +389,19 @@ fn parse_kicker_cost_payload(input: &str) -> Option { /// Returns only keywords not already covered by MTGJSON names — these are typically /// parameterized keywords where MTGJSON lists the name (e.g. "Protection") but /// Oracle text has the full form (e.g. "Protection from multicolored"). -pub(crate) fn extract_granted_keyword_list( +/// +/// `policy` decides the per-part remainder contract — the ONE axis on which the +/// grant and router surfaces differ. +fn parse_keyword_list_with_policy( line: &str, mtgjson_keyword_names: &[String], + policy: KeywordRemainderPolicy, ) -> Option> { let line_without_reminder = strip_reminder_text(line); let line = strip_keyword_activation_cost_prefix(line_without_reminder.trim()); if mtgjson_keyword_names.is_empty() { - return parse_mtgjson_missing_standalone_keyword_line(line); + return parse_mtgjson_missing_standalone_keyword_line(line, policy); } if mtgjson_keyword_names.iter().any(|n| n == "mobilize") { @@ -393,7 +474,7 @@ pub(crate) fn extract_granted_keyword_list( // place printed multiplicity survives — emit one Keyword per occurrence // so the runtime's per-instance trigger loop fires correctly. Synthesis // reconciles the deduped MTGJSON copy against these. - if let Some(kw) = parse_granted_keyword_fragment(&lower) { + if let Some(kw) = policy.parse_part(&lower) { if kw.instances_function_separately() { new_keywords.push(kw); } @@ -403,14 +484,14 @@ pub(crate) fn extract_granted_keyword_list( // Prefix match: Oracle text has more detail (e.g. "protection from red"). // Extract the full parameterized keyword. - if let Some(kw) = parse_granted_keyword_fragment(&lower) { + if let Some(kw) = policy.parse_part(&lower) { new_keywords.push(kw); continue; } } // Not an MTGJSON match — try parsing as any keyword (for keyword-only line validation) - if let Some(kw) = parse_granted_keyword_fragment(&lower) { + if let Some(kw) = policy.parse_part(&lower) { if !matches!(kw, Keyword::Unknown(_)) { // Keywords not in MTGJSON (e.g., firebending) must be extracted here. // They also validate the line as a keyword line. @@ -476,9 +557,12 @@ fn strip_activation_cost_dash(rest: &str) -> Option<&str> { .map(|(keyword_text, _)| keyword_text.trim_start()) } -fn parse_mtgjson_missing_standalone_keyword_line(line: &str) -> Option> { +fn parse_mtgjson_missing_standalone_keyword_line( + line: &str, + policy: KeywordRemainderPolicy, +) -> Option> { let lower = line.to_lowercase(); - let keyword = parse_granted_keyword_fragment(&lower)?; + let keyword = policy.parse_part(&lower)?; match keyword { Keyword::ForMirrodin => Some(vec![keyword]), // CR 702.89a: Umbra armor (printed as "umbra armor"/"totem armor") is a @@ -2006,6 +2090,62 @@ pub(crate) fn parse_router_keyword_line(line: &str) -> Option }) } +/// The remainder-CHECKING sibling of [`parse_granted_keyword_fragment`], which is +/// literally `parse_keyword_line_core(t).map(|(kw, _discarded_remainder)| kw)`. +/// +/// Same core, same typed keyword — but any unconsumed SEMANTIC prose rejects the +/// fragment instead of being thrown away. Only a permitted (`P`/`M`) tail may +/// remain. This is the primitive every router-context keyword parse is built on: +/// the whole-line router ([`parse_router_keyword_line`]) adds candidate recognition +/// and reminder-text handling on top; the list router +/// ([`parse_router_keyword_list`]) applies it per comma part. +/// +/// Takes ALREADY-LOWERCASED text, matching `parse_granted_keyword_fragment`'s +/// contract, so it is a drop-in at every site that used the permissive surface. +pub(crate) fn parse_router_keyword_fragment(lower: &str) -> Option { + let (keyword, unconsumed) = parse_keyword_line_core(lower)?; + let (modifiers, _had_terminal_punctuation) = parse_permitted_keyword_tail(unconsumed)?; + apply_keyword_line_modifiers(keyword, &modifiers) +} + +/// A candidate prefix matches only at a WORD BOUNDARY: the prefix must be followed +/// by end-of-line, whitespace, or an em-dash. +/// +/// Extracted from [`is_keyword_cost_line`]'s guard so that every candidate +/// recognizer shares one boundary rule. Without it, a bare `tag("kicker")` accepts +/// "Kickerfoo {2}" — the router claims a line it cannot parse and (before this +/// unit) consumed it with no keyword and no diagnostic. +fn matches_keyword_prefix_at_word_boundary(lower: &str, prefix: &str) -> bool { + tag::<_, _, OracleError<'_>>(prefix) + .parse(lower) + .is_ok_and(|(rest, _)| { + rest.is_empty() + || rest.as_bytes().first() == Some(&b' ') + || rest.as_bytes().first() == Some(&b'\t') + || tag::<_, _, OracleError<'_>>("\u{2014}").parse(rest).is_ok() + }) +} + +/// CR 702.33 Kicker / CR 702.33c Multikicker (a kicker variant, not a sibling rule) +/// / CR 702.56 Replicate / CR 702.187 Mayhem — the four keyword ADDITIONAL COSTS +/// that declare themselves on their own Oracle line. +/// +/// The priority-8f candidate set. Deliberately NOT merged into +/// [`KEYWORD_COST_PREFIXES`]: these are keyword ADDITIONAL COSTS whose lines also +/// feed `parse_kicker_additional_cost_line`, and `is_keyword_cost_line` additionally +/// gates `is_spell_resolution_instruction_line` — widening it there would change +/// which lines count as spell-resolution text. +pub(crate) const KICKER_FAMILY_PREFIXES: [&str; 4] = + ["kicker", "multikicker", "replicate", "mayhem"]; + +/// Whether a line is a priority-8f kicker-family candidate, guarded at a word +/// boundary. Candidate recognition ONLY — never evidence that the line parses. +pub(crate) fn is_kicker_family_line(lower: &str) -> bool { + KICKER_FAMILY_PREFIXES + .iter() + .any(|prefix| matches_keyword_prefix_at_word_boundary(lower, prefix)) +} + /// Bare-integer-count keywords whose `FromStr` arm does `p.parse().unwrap_or(N)` /// (or wraps the integer in `QuantityExpr::Fixed`) over the parameter string — /// see the arms in `types/keywords.rs`. For these the generic normalizer must @@ -2588,18 +2728,9 @@ pub(crate) const KEYWORD_COST_PREFIXES: [&str; 95] = [ /// precisely the bug this unit removes. Only `parse_router_keyword_line` returning /// `Some` licenses a router to consume the line. pub(crate) fn is_keyword_cost_line(lower: &str) -> bool { - KEYWORD_COST_PREFIXES.iter().any(|kw| { - tag::<_, _, OracleError<'_>>(*kw) - .parse(lower) - .is_ok_and(|(rest, _)| { - rest.is_empty() - || rest.as_bytes().first() == Some(&b' ') - || rest.as_bytes().first() == Some(&b'\t') - || tag::<_, _, OracleError<'_>>("\u{2014}") - .parse(rest) - .is_ok() - }) - }) + KEYWORD_COST_PREFIXES + .iter() + .any(|kw| matches_keyword_prefix_at_word_boundary(lower, kw)) // CR 702.29e: Typecycling — first word ends in "cycling" but isn't "cycling" itself || lower .split_whitespace() diff --git a/crates/engine/src/parser/oracle_tests.rs b/crates/engine/src/parser/oracle_tests.rs index 653fd20248..b4a5845a6a 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -21328,3 +21328,280 @@ fn ill_gotten_gains_return_clause_binds_bounded_multi_target() { return_clause.multi_target ); } +// =========================================================================== +// Task #123 — router wiring: every router slot and routing classifier parses +// keyword lines through a STRICT (remainder-checking) surface. +// +// The invariant under test is CONSUME-ON-SUCCESS: a router may advance past a +// line only when the line parsed COMPLETELY. A keyword-cost line carrying a +// semantic clause it cannot model must fall through and become an honest, +// exact-unit `Effect::Unimplemented` — never vanish with no keyword and no +// diagnostic, which renders the card as fully supported. +// +// Each swallow witness below is paired with a POSITIVE control on the same +// family, because "everything is now Unimplemented" would satisfy the swallow +// assertions vacuously. The controls prove the valid line still routes. +// =========================================================================== + +/// The hostile suffix: semantic prose no permitted (`P`/`R`/`M`) tail admits. +const T123_SEMANTIC_TAIL: &str = " if you control an artifact"; + +fn t123_parse(text: &str, kws: &[&str], types: &[&str]) -> ParsedAbilities { + let kwv: Vec = kws.iter().map(|s| s.to_string()).collect(); + let tyv: Vec = types.iter().map(|s| s.to_string()).collect(); + parse_oracle_text(text, "Witness", &kwv, &tyv, &[]) +} + +fn t123_unimplemented_count(parsed: &ParsedAbilities) -> usize { + parsed + .abilities + .iter() + .filter(|a| matches!(&*a.effect, Effect::Unimplemented { .. })) + .count() +} + +/// A swallowed line: consumed, nothing recorded, nothing flagged. The exact +/// failure this unit exists to remove. +fn t123_assert_not_swallowed(label: &str, parsed: &ParsedAbilities) { + assert!( + t123_unimplemented_count(parsed) > 0, + "[{label}] the line was CONSUMED with no Effect::Unimplemented — a silent \ + swallow. keywords={:?} abilities={} statics={} triggers={}", + parsed.extracted_keywords, + parsed.abilities.len(), + parsed.statics.len(), + parsed.triggers.len(), + ); +} + +/// THE 8f witness. `Kickerfoo {2}` is a fabricated keyword line: no such +/// keyword exists, so nothing can parse it. +/// +/// Before this unit, priority 8f dispatched on a bare `tag("kicker")` with NO +/// word-boundary guard, so "kickerfoo" matched the "kicker" prefix; then +/// `i += 1; continue;` sat OUTSIDE both `if let Some` blocks, so the line was +/// consumed with no keyword, no additional cost, and no diagnostic. It vanished +/// entirely, and the card rendered as fully supported. +/// +/// Red-first: on pre-fix code this test FAILS with 0 abilities / 0 keywords / +/// 0 Unimplemented — the line is simply gone. +#[test] +fn t123_kickerfoo_synthetic_line_is_honestly_unimplemented_not_swallowed() { + let parsed = t123_parse("Kickerfoo {2}", &[], &["Instant"]); + + t123_assert_not_swallowed("kickerfoo", &parsed); + assert_eq!( + t123_unimplemented_count(&parsed), + 1, + "expected exactly ONE exact-unit Unimplemented for the one bogus line" + ); + assert!( + parsed.extracted_keywords.is_empty(), + "a fabricated keyword line must not manufacture a keyword, got {:?}", + parsed.extracted_keywords + ); +} + +/// Priority 8f, defect (a): a REAL kicker prefix at a word boundary, with a +/// semantic tail neither the additional-cost parser nor the keyword parser can +/// model. Pre-fix this produced `Kicker(generic: 0)` — the tail eaten AND the +/// cost fabricated as 0 (`parse_oracle_cost` never fails, so the cost slot was +/// built out of untyped prose). +#[test] +fn t123_kicker_with_semantic_tail_is_not_consumed() { + let text = format!("Kicker {{2}}{T123_SEMANTIC_TAIL}"); + let parsed = t123_parse(&text, &[], &["Instant"]); + + t123_assert_not_swallowed("kicker+tail", &parsed); + assert!( + !parsed + .extracted_keywords + .iter() + .any(|k| matches!(k, Keyword::Kicker(_))), + "must not commit a Kicker built from an unmodelled tail, got {:?}", + parsed.extracted_keywords + ); +} + +/// Positive control for 8f: the valid lines still route to typed keywords with +/// their real costs, and raise no Unimplemented. Without this, the two tests +/// above would pass even if 8f had simply been deleted. +#[test] +fn t123_kicker_family_valid_lines_still_route() { + for (text, label) in [ + ("Kicker {2}", "kicker"), + ("Multikicker {1}{R}", "multikicker"), + ("Replicate {2}", "replicate"), + ] { + let parsed = t123_parse(text, &[], &["Instant"]); + assert_eq!( + t123_unimplemented_count(&parsed), + 0, + "[{label}] a VALID keyword line must not become Unimplemented: {parsed:#?}" + ); + } + + // The cost must be the printed one, not a fabricated empty cost. + let replicate = t123_parse("Replicate {2}", &[], &["Instant"]); + assert!( + replicate + .extracted_keywords + .iter() + .any(|k| matches!(k, Keyword::Replicate(_))), + "Replicate {{2}} must yield a Replicate keyword, got {:?}", + replicate.extracted_keywords + ); +} + +/// THE SHADOW witness (priority 1b). This is the finding the census did not +/// record: priority 1b runs LONG before the strict routers at priority 9/13, so +/// whenever MTGJSON names the keyword — the common case — it is 1b, not those, +/// that decides the line. +/// +/// Pre-fix, 1b's permissive list parser consumed this whole line as a bare +/// `Cycling`, dropped the conditional, and raised nothing. The strict wiring +/// downstream never even saw it. +/// +/// The `mtgjson EMPTY` half is the discriminator: it proves the difference is +/// caused by 1b's MTGJSON-gated path and not by some unrelated slot. +#[test] +fn t123_priority_1b_does_not_shadow_the_strict_router() { + let text = format!("Cycling {{2}}{T123_SEMANTIC_TAIL}"); + + // MTGJSON names the keyword -> pre-fix, priority 1b claimed and swallowed it. + let named = t123_parse(&text, &["cycling"], &["Creature"]); + t123_assert_not_swallowed("1b cycling+tail (mtgjson named)", &named); + assert!( + !named + .extracted_keywords + .iter() + .any(|k| matches!(k, Keyword::Cycling(_))), + "must not commit a Cycling built from an unmodelled tail, got {:?}", + named.extracted_keywords + ); + + // MTGJSON silent -> 1b declines, and the line was ALREADY honest pre-fix. + // Both paths must now agree; that agreement is the point of the migration. + let unnamed = t123_parse(&text, &[], &["Creature"]); + t123_assert_not_swallowed("1b cycling+tail (mtgjson silent)", &unnamed); +} + +/// Positive control for 1b: bare keyword lines and correctly-parameterized +/// keyword lines still route. This is the path a naive "just make it strict" +/// change would destroy, since `parse_router_keyword_line` alone cannot see a +/// bare-keyword list. +#[test] +fn t123_priority_1b_valid_keyword_lines_still_route() { + let bare = t123_parse("Flying, vigilance", &["flying", "vigilance"], &["Creature"]); + assert_eq!( + t123_unimplemented_count(&bare), + 0, + "a bare keyword list must still route: {bare:#?}" + ); + + let cycling = t123_parse("Cycling {2}", &["cycling"], &["Creature"]); + assert_eq!( + t123_unimplemented_count(&cycling), + 0, + "a valid parameterized keyword line must still route: {cycling:#?}" + ); + assert!( + cycling + .extracted_keywords + .iter() + .any(|k| matches!(k, Keyword::Cycling(_))), + "Cycling {{2}} must yield a Cycling keyword, got {:?}", + cycling.extracted_keywords + ); +} + +/// Priority 1b, Crew. The strict `parse_crew_keyword` intercept correctly +/// DECLINES "Crew 2 " (its cadence tail is `all_consuming`), and +/// the line then fell into 1b's permissive list — which ate the tail. The leak +/// was never in the Crew parser; it was downstream, exactly as the charter says. +#[test] +fn t123_crew_with_semantic_tail_falls_through_to_unimplemented() { + let text = format!("Crew 2{T123_SEMANTIC_TAIL}"); + let parsed = t123_parse(&text, &["crew"], &["Artifact"]); + + t123_assert_not_swallowed("crew+tail", &parsed); + assert!( + !parsed + .extracted_keywords + .iter() + .any(|k| matches!(k, Keyword::Crew { .. })), + "must not commit a Crew built from an unmodelled tail, got {:?}", + parsed.extracted_keywords + ); + + // Positive control: the plain Crew line, and the modeled cadence sentence, + // both still route (this is the NOT-A-DEFECT path — do not regress it). + let plain = t123_parse("Crew 2", &["crew"], &["Artifact"]); + assert!( + plain + .extracted_keywords + .iter() + .any(|k| matches!(k, Keyword::Crew { .. })), + "plain Crew 2 must still route, got {:?}", + plain.extracted_keywords + ); +} + +/// Priority 0, the semicolon keyword line. Every part must parse completely; +/// one part carrying a semantic clause declines the WHOLE line. +#[test] +fn t123_semicolon_keyword_line_with_semantic_tail_is_not_consumed() { + let text = format!("Defender; cycling {{2}}{T123_SEMANTIC_TAIL}"); + let parsed = t123_parse(&text, &["defender", "cycling"], &["Creature"]); + + t123_assert_not_swallowed("semicolon+tail", &parsed); + + // Positive control: a clean semicolon keyword line still routes. + let clean = t123_parse("Defender; reach", &["defender", "reach"], &["Creature"]); + assert_eq!( + t123_unimplemented_count(&clean), + 0, + "a clean semicolon keyword line must still route: {clean:#?}" + ); +} + +/// The advance-on-partial family: each of these router intercepts used to call +/// the permissive fragment parser and advance `i` on a parse that had discarded +/// the remainder. Every one of them ate its tail AND fabricated its cost. +/// +/// Driven as a table because the defect — and the fix — are one shape, not six. +#[test] +fn t123_advance_on_partial_intercepts_no_longer_eat_their_tails() { + let cases: [(&str, &str, &[&str]); 5] = [ + ("suspend", "Suspend 3—{1}{R}", &["Sorcery"]), + ("specialize", "Specialize {2}", &["Creature"]), + ("escalate", "Escalate {2}", &["Instant"]), + ( + "commander ninjutsu", + "Commander ninjutsu {1}{U}", + &["Creature"], + ), + ("buyback", "Buyback {3}", &["Instant"]), + ]; + + for (label, valid_line, types) in cases { + // Swallow witness: valid keyword + a clause nothing can model. + let hostile = format!("{valid_line}{T123_SEMANTIC_TAIL}"); + let parsed = t123_parse(&hostile, &[], types); + t123_assert_not_swallowed(label, &parsed); + + // Positive control: the same line WITHOUT the tail must still route, + // or the assertion above is passing for the wrong reason. + let clean = t123_parse(valid_line, &[], types); + assert_eq!( + t123_unimplemented_count(&clean), + 0, + "[{label}] the valid line must still route to a keyword: {clean:#?}" + ); + assert!( + !clean.extracted_keywords.is_empty(), + "[{label}] the valid line must yield a keyword, got {:?}", + clean.extracted_keywords + ); + } +} diff --git a/scripts/check-parser-combinators.sh b/scripts/check-parser-combinators.sh index 104615cc33..65e9759e4d 100755 --- a/scripts/check-parser-combinators.sh +++ b/scripts/check-parser-combinators.sh @@ -67,14 +67,16 @@ fi # THE BOUNDARY. There are two keyword-parsing surfaces and they are NOT # interchangeable: # -# parse_router_keyword_line() STRICT. All-consuming: it returns a typed -# keyword only when the candidate line parses -# completely (keyword + permitted P/R/M tail -# to eof). This is the ONLY surface that may -# license a router to CONSUME a whole line. +# parse_router_keyword_line() STRICT, whole line. All-consuming: returns a +# parse_router_keyword_list() STRICT, keyword list (comma parts + MTGJSON). +# parse_router_keyword_fragment() STRICT, one keyword phrase. +# These return a typed keyword ONLY when the text +# parses completely (keyword + permitted P/R/M +# tail). They are the ONLY surfaces that may +# license a router to CONSUME a line. # -# parse_granted_keyword_fragment() PERMISSIVE. By design it takes the leading -# extract_granted_keyword_list() keyword and DISCARDS the remainder — correct +# parse_granted_keyword_fragment() PERMISSIVE. By design they take the leading +# extract_granted_keyword_list() keyword and DISCARD the remainder — correct # for an EMBEDDED grant ("...gains vanishing 3 # if ..." inside a static/token/vote payload), # and invalid at a whole-line router boundary, @@ -84,76 +86,33 @@ fi # A router that advances a line on a permissive parse is a SILENT SWALLOW: no # keyword recorded, no diagnostic, and the card renders as fully supported. # -# THIS GATE IS A RATCHET, NOT A BLESSING. Plan 02 step 5 wired the strict router -# into priorities 9 and 13 but left the remaining router entries on the permissive -# surface. Those survivors are enumerated below as an EXACT expected count per -# router context. The gate fails if the count goes UP (a new permissive router call -# — the regression this exists to stop) and equally if it goes DOWN without updating -# the number (so it can never quietly drift out of truth). +# STATUS: MIGRATION COMPLETE (task #123). Plan 02 step 5 wired the strict router into +# priorities 9 and 13; task #123 migrated the remaining 16 permissive calls (priorities +# 0, 1b, 8f, the flashback/suspend/specialize/buyback/escalate/commander-ninjutsu/d20 +# intercepts, and the two routing classifiers) onto the strict surfaces. The permissive +# symbols are now ABSENT from oracle.rs entirely — not merely unused, but not imported. # -# DEFINITION OF DONE: entries are DELETED from this allowlist as each router entry -# converts to parse_router_keyword_line. When every EXPECTED_* reaches 0, this -# becomes the plain "no permissive symbol in a router context" gate that Plan 02 -# step 7 actually asks for, and the migration-status paragraph in SKILL.md §3a goes -# with it. Same instrument as KNOWN_NOUN_PARAM_LEAKS in oracle_keyword.rs. -# -# OWNER OF THE RESIDUAL: task #123 (P02-U5 router wiring). Every surviving call -# below is that task's work-list. Do not "fix" one in isolation without lowering the -# corresponding floor here in the same commit. -# -# THE ALLOWLIST — 16 permissive calls, each with why it is still here (task #123): -# -# parse_oracle_ir (13): -# :3720,:3725 priority 0, semicolon keyword line — commits on Some(vec![]), -# which extract_granted_keyword_list returns on MTGJSON metadata -# alone, with the parameterized Oracle line never parsed. -# :3789 priority 1b, keyword-only line — same Some(vec![]) leak; this -# is also where a "Crew N " loses its tail after the -# strict Crew intercept correctly declines it. -# :4673,:4682 flashback em-dash / cost-reduction split — non-atomic: advances -# even when only one half of the split parsed. -# :5092,:5102 suspend / specialize — advance-on-partial. -# :5154 priority 8f kicker/multikicker/replicate/mayhem — HIGHEST SEVERITY. -# Class-A: `i += 1; continue;` sits OUTSIDE both `if let Some` blocks, -# so a line neither parser can handle is consumed with NO keyword and -# NO Unimplemented. Also dispatches on a bare alt(( tag("kicker"), … )) -# with no word-boundary guard, unlike is_keyword_cost_line. -# :5170,:5186 buyback em-dash / escalate — advance-on-partial. -# :5521 commander ninjutsu — advance-on-partial. -# :5554 priority 13 residual — advance-on-partial. -# :5646 d20 table / ability-word retry — advance-on-partial. -# -# is_semicolon_keyword_line (2) :2161,:2167 -# is_spell_resolution_instruction_line (1) :2205 -# Classifiers, not routers, but they gate routing decisions on a permissive parse. -# Step 5 item 11: a classifier MAY probe with the strict parser; it may NOT call a -# helper that discards the remainder. +# This gate is therefore no longer a ratchet with an allowlist. It is the plain +# invariant Plan 02 step 7 asks for: NO permissive keyword-parser symbol may appear in +# a router context, at any count, ever. A reintroduction fails the build. # # SPAN EXTRACTION: from the `fn NAME(` signature at column 0 to the next `}` at # column 0. Brace COUNTING would be wrong here — oracle.rs is saturated with mana # symbols ("{T}", "{2}{B}") inside string literals, and a naive counter reads # those as scope. rustfmt guarantees a top-level fn closes with `}` at column 0, # so the anchor is exact. Full-line comments are excluded so that prose NAMING a -# permissive symbol (there are several) is not miscounted as a call. +# permissive symbol is not miscounted as a call. # --------------------------------------------------------------------------- ORACLE_RS='crates/engine/src/parser/oracle.rs' PERMISSIVE_SYMS='parse_granted_keyword_fragment|extract_granted_keyword_list' # Pre-rename spellings. These must not come back under any name, in any context. LEGACY_SYMS='parse_keyword_from_oracle|extract_keyword_line' -# NOT listed above: `parse_crew_keyword`. Plan 02 step 5 item 11 groups it with the -# remainder-discarding helpers, but that describes the PRE-step-5 code. As it stands -# it is strict: its cadence tail is `all_consuming(tag("activate only once each -# turn"))`, so "Crew 2 if you control an artifact" returns None rather than eating -# the suffix, and its call site advances only inside `if let Some`. The line then -# falls through to priority 1b — and it is 1b's `extract_granted_keyword_list` that -# would swallow the tail, which is exactly the call this gate is counting. Listing -# parse_crew_keyword here would encode a false claim about the code. - -# Expected surviving permissive calls per router context (see ratchet note above). -EXPECTED_parse_oracle_ir=13 -EXPECTED_is_semicolon_keyword_line=2 -EXPECTED_is_spell_resolution_instruction_line=1 +# NOT a router-context symbol: `parse_crew_keyword`. Plan 02 step 5 item 11 groups it +# with the remainder-discarding helpers, but that describes the PRE-step-5 code. As it +# stands it is strict: its cadence tail is `all_consuming(tag("activate only once each +# turn"))`, so "Crew 2 if you control an artifact" returns None rather than eating the +# suffix, and its call site advances only inside `if let Some`. arch_fail=0 @@ -174,19 +133,13 @@ for ctx in parse_oracle_ir is_semicolon_keyword_line is_spell_resolution_instruc continue fi actual="$(printf '%s\n' "$span" | grep -cE "$PERMISSIVE_SYMS" || true)" - eval "expected=\$EXPECTED_$ctx" - if [ "$actual" -gt "$expected" ]; then - echo "✗ (G) $ctx: $actual permissive keyword-parser calls, expected $expected." >&2 - echo " A NEW permissive call entered a router context. Routers must consume a" >&2 - echo " line only via all-consuming parse_router_keyword_line(); the permissive" >&2 - echo " surface discards the remainder and silently swallows semantics — no" >&2 - echo " keyword, no diagnostic, and the card renders as fully supported." >&2 - arch_fail=1 - elif [ "$actual" -lt "$expected" ]; then - echo "✗ (G) $ctx: $actual permissive calls, expected $expected — a router entry was" >&2 - echo " migrated to the strict parser. That is the GOAL (task #123): lower" >&2 - echo " EXPECTED_$ctx to $actual here, in the same commit, and delete that" >&2 - echo " entry from the allowlist comment above. The floor must never drift." >&2 + if [ "$actual" -ne 0 ]; then + echo "✗ (G) $ctx: $actual permissive keyword-parser call(s), expected 0." >&2 + echo " Routers must consume a line only via the STRICT surfaces" >&2 + echo " (parse_router_keyword_line / _list / _fragment). The permissive" >&2 + echo " surface discards the remainder and silently swallows semantics —" >&2 + echo " no keyword, no diagnostic, and the card renders as fully supported." >&2 + echo " This migration is COMPLETE (task #123); do not reintroduce it." >&2 arch_fail=1 fi done From 66a1bc5da2606525f924ad93d244ed727652fad3 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Tue, 14 Jul 2026 07:50:53 -0700 Subject: [PATCH 2/3] test(parser): pin the bare metadata-named keyword line as the overreach control MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The router migration converts swallowed keyword lines into honest Effect::Unimplemented. The line it must NOT move is the most common keyword line in the corpus: a bare, MTGJSON-named keyword. Measured: such a line parses to an EMPTY extracted_keywords — the keyword rides the card's MTGJSON metadata, not the parse output. So "the strict parser produced no keywords" is the CORRECT outcome here, not evidence of a swallow. A refusal keyed on that emptiness would have satisfied every swallow witness in this unit and false-redded every vanilla evergreen creature in the pool. Green in both worlds, pre- and post-migration, by construction. --- crates/engine/src/parser/oracle_tests.rs | 28 ++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/crates/engine/src/parser/oracle_tests.rs b/crates/engine/src/parser/oracle_tests.rs index b4a5845a6a..3654b6d008 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -21515,6 +21515,34 @@ fn t123_priority_1b_valid_keyword_lines_still_route() { ); } +/// THE OVERREACH CONTROL, and the reason this unit does NOT refuse a line merely +/// because its parsed keyword list came back empty. +/// +/// A bare, MTGJSON-named keyword line parses to an EMPTY `extracted_keywords`: the +/// keyword rides the card's MTGJSON metadata, and the router's job on this line is +/// simply to consume it without raising anything. So "the strict parser produced no +/// keywords" is NOT evidence of a swallow here — it is the normal, correct outcome +/// for the single most common keyword line in the corpus. A refusal keyed on that +/// emptiness would satisfy every swallow assertion above and false-red every vanilla +/// evergreen creature in the pool. +/// +/// Green in BOTH worlds, pre- and post-fix. That is the point: it is the line the +/// migration must NOT move. +#[test] +fn t123_metadata_named_bare_keyword_line_still_routes() { + let parsed = t123_parse("Flying", &["flying"], &["Creature"]); + + assert_eq!( + t123_unimplemented_count(&parsed), + 0, + "a bare metadata-named keyword line must not become Unimplemented: {parsed:#?}" + ); + assert!( + parsed.abilities.is_empty() && parsed.statics.is_empty() && parsed.triggers.is_empty(), + "a bare metadata-named keyword line must be consumed cleanly, got {parsed:#?}" + ); +} + /// Priority 1b, Crew. The strict `parse_crew_keyword` intercept correctly /// DECLINES "Crew 2 " (its cadence tail is `all_consuming`), and /// the line then fell into 1b's permissive list — which ate the tail. The leak From ec38d8e6f33621abe557ebe6806dc8e244105197 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Tue, 14 Jul 2026 07:50:53 -0700 Subject: [PATCH 3/3] docs(parser): update the 1b router-table row to the strict list parser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The migration commit rewrote §3a's surface table and deleted the migration-status paragraph but left the priority-1b row in the router registry naming the permissive helper it no longer calls. --- .claude/skills/oracle-parser/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude/skills/oracle-parser/SKILL.md b/.claude/skills/oracle-parser/SKILL.md index 13472e1308..198fbe0bd4 100644 --- a/.claude/skills/oracle-parser/SKILL.md +++ b/.claude/skills/oracle-parser/SKILL.md @@ -416,7 +416,7 @@ Unlabeled handlers interleaved between labeled slots are shown as `—` rows. | `0` | Semicolon-separated keyword line ("Defender; reach"); colon guard excludes activated abilities | per-part keyword extraction | `oracle.rs` | | `1` | Modal block: "Choose one —" header + mode lines, or Spree + `+` lines (consumes multiple lines) | `parse_oracle_block()` + `lower_oracle_block()` | `oracle_modal.rs` | | — | "Equip {cost}" / "Equip — {cost}" (not "Equipped …"); "Crew N" with trailing cadence sentence | `try_parse_equip()`, `parse_crew_keyword()` | `oracle.rs` | -| `1b` | Keyword-only line (guard: "{kw} abilities you activate cost {N} less" is a static, not a keyword line) | `extract_granted_keyword_list()` (permissive — see §3a) | `oracle_keyword.rs` | +| `1b` | Keyword-only line (guard: "{kw} abilities you activate cost {N} less" is a static, not a keyword line) | `parse_router_keyword_list()` (strict — see §3a) | `oracle_keyword.rs` | | `2` | "Enchant {filter}" | skip (handled externally) | — | | — | Commander-permission / deck-construction copy-limit sentences (skip); named equip " — Equip {cost}" | `try_parse_equip()` | `oracle.rs` | | `11` | Planeswalker loyalty `+N:` / `−N:` / `0:` / `[+N]:` (runs here despite the label) | `try_parse_loyalty_line()` | `oracle.rs` |