[codex] fix(parser): parse repeated named control conditions - #5121
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Reviewed current head I inspected the parser/test diff and did not find a code blocker in the repeated named-control parser path, but I’m holding approval/enqueue for live evidence: this parser PR does not yet have the required |
Parse changes introduced by this PR · 2 card(s), 2 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The new repeated named-control helper cites the wrong Comprehensive Rule for static-condition users. Evidence: crates/engine/src/parser/oracle_nom/condition.rs adds parse_repeated_named_control_presence with CR 201.2 + CR 603.4, but docs/MagicCompRules.txt defines CR 603.4 only as the intervening-if triggered-ability rule; the parse-diff shows this helper is also lowering Liu Bei's static "as long as" condition, where CR 603.4 does not apply. Why it matters: this repo treats verified CR comments as review evidence, and a wrong CR citation is worse than no citation around rules-sensitive parser code. Suggested fix: keep the CR 201.2 named-object grounding here and either remove CR 603.4 from this new helper comment or split the wording so triggered/static uses are not both justified by the intervening-if rule.
# Conflicts: # docs/parser-misparse-backlog.md
|
Updated this branch with current origin/main (5c943f9). Addressed the requested CR citation fix by replacing CR 603.4 with verified CR 608.2c on the repeated named-control helper comments while keeping CR 201.2 for card-name grounding. Resolved the backlog merge conflict by preserving upstream's Kookus removal and this PR's High Marshal Arguel / Liu Bei removals. Local checks: fetched docs/MagicCompRules.txt for CR verification; cargo fmt --all; git diff --check. |
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The repeated named-control helper still cites an inaccurate CR after the latest update. Evidence: crates/engine/src/parser/oracle_nom/condition.rs now cites CR 201.2 + CR 608.2c above the new dispatch ordering and parse_repeated_named_control_presence; the local CR text for 608.2c is the instruction-following rule during spell/ability resolution, while this helper is shared static-condition parsing and the parse-diff includes Liu Bei's static as long as condition. Why it matters: CR comments are review evidence in this repo, and replacing the previous triggered-ability-only citation with a resolution-only citation still misstates the rule grounding for this parser path. Suggested fix: remove CR 608.2c from the two new repeated named-control comments and keep the verified CR 201.2 name grounding, unless a more precise static-condition rule is verified from docs/MagicCompRules.txt.
I did not push this as a maintainer fixup because GitHub reports maintainerCanModify: false for this fork PR.
# Conflicts: # docs/parser-misparse-backlog.md
|
Updated this branch with latest origin/main (2c94c74) after #5123/#5109 landed. Addressed the new review by removing CR 608.2c from the two repeated named-control comments and keeping only the verified CR 201.2 name grounding. Resolved the backlog conflict by preserving both the already-merged Sift Through Sands removal and this PR's High Marshal Arguel / Liu Bei removals. Local checks: cargo fmt --all; git diff --check. |
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer sweep approval for current head 2c94c7470e2374b541afeea7a72ef30419abfba1.
I rechecked the stale requested-change item against the current diff. The branch is current with origin/main, hard-stop classification is clean, the prior CR-citation blocker is addressed by removing the inaccurate CR 608.2c citation from the repeated named-control helper, and the parse-diff is scoped to the expected High Marshal Arguel / Liu Bei named-control changes. Required Rust/card-data checks are still pending, so auto-merge is enabled for the merge queue to wait on them.
Summary
Fixes the Root #30 subcluster for repeated named-control conditions where each list item carries its own type phrase.
Root cause:
parse_control_named_pairhandledyou control [type] named A and B, where the type applies to both names, but the parser had no combinator for the sibling shapeyou control [type] named A and/or [type] named B. As a result, High Marshal Arguel absorbeda land named ...into the first named filter, and Liu Bei treated theor a permanent named ...clause as part of the first literal name.This PR adds a prefix-dispatched nom parser for repeated named-control items before the shared-type named-pair parser. Each item is parsed through
parse_type_phrase, keeps its own type filter, and only treatsand/oras a name boundary when the following text is another valid[type] named ...item, preserving comma-bearing names likeGuan Yu, Sainted Warrior.Backlog update: removes
High Marshal ArguelandLiu Bei, Lord of Shufromdocs/parser-misparse-backlog.mdRoot #30, changing the branch-local count from 12 to 10.Parsed-card diff audit
Targeted baseline/after exports:
/tmp/phase-root30-namelists-baseline.json/tmp/phase-root30-namelists-after.jsonHigh Marshal Arguel|Liu Bei, Lord of ShuRaw parsed card keys changed exactly:
high marshal arguelliu bei, lord of shuExpected parse movements:
High Marshal Arguel: the second condition changes from an enchantment nameda land named temple of aclazotzto a land namedtemple of aclazotz.Liu Bei, Lord of Shu: the condition changes from one permanent namedguan yu, sainted warrior or a permanent named zhang fei, fierce warriorto anOrof two permanent-named predicates.Sibling scan against current MTGJSON found exactly these two repeated named-control condition texts:
jq -r '.data | to_entries[] | .key as $name | .value[] | select(.text? and (.text | test("you control .*named .* (and|or) .*named"; "i"))) | $name + " :: " + (.text | gsub("\\n"; " / "))' data/mtgjson/AtomicCards.jsoncoverage-parse-diffreported no support-signature clusters (oracle_changed: 0), so the expected raw AST repairs are confined to the two audited cards.Validation
cargo fmt --all./scripts/check-parser-combinators.shgit diff --checkcargo test -p engine --features cli --lib repeated_named_control_presence -- --nocapturecargo test -p engine --features cli --lib inverted_liu_bei_condition_lowers_to_named_or_condition -- --nocapturecargo test -p engine --features cli --lib test_you_control_named_pair -- --nocapturecargo run -p engine --features cli --bin oracle-gen -- data --filter "High Marshal Arguel|Liu Bei, Lord of Shu" --output /tmp/phase-root30-namelists-after.jsoncargo run -p engine --features cli --bin card-data-validate -- /tmp/phase-root30-namelists-after.jsoncargo run -p engine --features cli --bin coverage-parse-diff -- /tmp/phase-root30-namelists-baseline.json /tmp/phase-root30-namelists-after.json --markdown /tmp/phase-root30-namelists-parse-diff.md --json /tmp/phase-root30-namelists-parse-diff.jsonTilt was not running in this checkout (
No tilt apiserver found: tilt-default), so I used the direct cargo fallback allowed by the repo instructions.