ship/t123 router wiring - #5813
Merged
Merged
Conversation
matthewevans
commented
Jul 14, 2026
Member
- fix(parser): route every keyword-line router slot through the strict parser
- test(parser): pin the bare metadata-named keyword line as the overreach control
- docs(parser): update the 1b router-table row to the strict list parser
…parser
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.
…ch control 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.
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.
matthewevans
enabled auto-merge
July 14, 2026 15:43
Contributor
There was a problem hiding this comment.
Code Review
This pull request completes the migration of the Oracle parser's routing boundaries to a fully strict, remainder-checking architecture (Task #123). It replaces all remaining permissive keyword-parsing surfaces in oracle.rs with strict counterparts (parse_router_keyword_list and parse_router_keyword_fragment), preventing silent swallow bugs where unparsed semantic tails were discarded. Key improvements include enforcing word boundaries on kicker-family keywords, ensuring kicker costs are fully typed, and updating the CI gate in check-parser-combinators.sh to strictly disallow permissive symbols in router contexts. Comprehensive unit tests have been added to verify these strict parsing behaviors. No review comments were provided, so there is no feedback to address.
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.
Parse changes introduced by this PR · 26 card(s), 11 signature(s) (baseline: main
|
parthmishra
pushed a commit
to parthmishra/phase
that referenced
this pull request
Jul 14, 2026
…gn invariants (phase-rs#5816) The add-* skills lagged five invariants that have landed on main, and one of them (add-static-ability) stated the opposite of what the engine now does. - add-static-ability: continuous effects apply to OBJECTS (CR 613.1), not only battlefield permanents. New "Zone Boundary" section: the layer pass materializes keywords ONLY into the zones it also resets ({Battlefield, Hand, Stack} — layer_pass_materializes_keywords); library / graveyard / exile are owned by off_zone_characteristics and computed on demand. A grant into an unowned zone is a silent no-op that still reports as supported (the PR phase-rs#5803 defect class). - add-keyword: document the strict keyword router (PR phase-rs#5813 / task phase-rs#123). Router slots and routing classifiers must parse through parse_router_keyword_line / _list / _fragment (all-consuming); the permissive parse_granted_keyword_fragment / extract_granted_keyword_list are for EMBEDDED GRANT contexts only. A line with an unconsumed semantic tail must DECLINE, not commit. Gate G in scripts/check-parser-combinators.sh is the whole-file invariant. New router-registry checklist item (is_keyword_cost_line + ROUTER_KEYWORD_CASES set-equality test). - add-trigger: new "Purged Source" section (CR 608.2h). LKI look-back via subject_filter_matches_with_lki / source_controller_or_lki; attachments do NOT survive on the live object (CR 704.5m/n sever them — only LKISnapshot::attachments holds the pre-sever set) while attack/block history DOES (durable id-keyed ledgers on GameState). Names the non-vacuity vector: a ceased-to-exist token (CR 111.7), not a printed creature. - casting-stack-conditions + add-engine-effect: the three X carriers (GameObject::cost_x_paid = CR 107.3m cast-X, GameState::announced_source_x = CR 107.3a/d announce-X, ResolvedAbility::chosen_x = the published channel, CR 107.3i) and publish_announced_x as the single publish authority. Re-measuring X at resolution is rules-wrong (CR 107.3c), not merely late. - add-replacement-effect: cross-line "instead" lowers to a BRANCH (ClauseDisposition::ReplaceMeaning → else_ability), never two independent effects — CR 614.1a + CR 614.6, the phase-rs#44/phase-rs#79 double-execution class. - project-reference: card-export measurement hazards — export-cards rewrites the tracked parser input oracle-subtypes.json (non-idempotent), gen-card-data dirties known-tokens.toml, and the export is nondeterministic on ~20 faces (the noise floor of any whole-pool ledger). - card-test: gen-test-fixture.py --check verifies COVERAGE only (a key-set comparison), never the stored parse VALUES — a parser change can leave the integration fixture stale while the gate stays green. Also fixes stale anchors found while verifying the self-check blocks (all five now pass; two were red before this change): - add-static-ability / add-keyword cited crates/engine/src/parser/oracle_static.rs, which is a directory now (oracle_static/mod.rs, oracle_static/grammar.rs, oracle_static/keyword_grant.rs). - casting-stack-conditions cited pay_and_push and pay_ability_cost in casting.rs; they live in casting_costs.rs and costs.rs. - add-replacement-effect cited "MTG Rule 614.16" for as-enters choices; 614.16 is the token/counter-creation replacement rule. The correct citations are CR 614.1c + CR 614.12a. Every symbol grep-verified against the tree at 5f4fbcd; every CR number grep-verified against docs/MagicCompRules.txt. scripts/check-skill-doc.sh and Gate G both pass. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
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.