ship/p02 u2 per unit swallow audit#5668
Open
matthewevans wants to merge 12 commits into
Open
Conversation
matthewevans
commented
Jul 12, 2026
matthewevans
commented
Member
- refactor(parser): typed per-unit semantic feature vocabulary (Plan 02 U2, commit 1)
- refactor(parser): typed per-item feature vocabulary + item-scoped evidence view
- refactor(parser): scope the swallow audit to the source unit that raised the clause
- chore(parser): remove the Python swallow audit, a divergent second authority
- fix(parser): audit units must partition the source, or warnings silently vanish
- fix(parser): a card that parses to NOTHING must still be audited
- fix(parser): make ActivateLimit and APNAP fire — their evidence was vacuous
- fix(parser): make the enum the actual wire-format authority, not a claim about one
- test(parser): name the pinned tests for what they pin; keep the census instrument
… U2, commit 1)
Adds `oracle_ir/feature.rs`: the closed `OracleSemanticFeature` set, the
per-unit `UnitSemanticAudit` record, and the item-scoped `ItemDefs` view of
the lowered definitions that supplies an audit's evidence. Additive; the
`swallow_check` cutover commit is the production caller.
Two design deviations from Plan 02 step 1, both forced by measurement:
1. The audit visits the LOWERED definition owned by each item, not sourced IR.
`OracleNodeIr::{Spell,Trigger,Static,Replacement}` — the only IR-carrying node
variants, hence the only ones that could carry nested clause provenance — have
ZERO production constructors. All three `emit()` sites in `DocEmitter` emit
`PreLowered*` or leaf nodes. There is no sourced IR to visit.
2. The audit stays POST-RELATION at its pinned position rather than moving
pre-lowering: pre-lowering audit is blind to relation-synthesized semantics;
the false-positive wave U1 bounded to 31 faces would be caused, not avoided.
`apply_linked_choice_etb_counter` synthesizes a replacement, and takes the
`&mut replacement_ids` track that makes per-item attribution sound across that
synthesis.
Granularity is the item header unit (ordinal 0) — document-unique, Exact-spanned.
Sub-item units are not expressible today: `ClauseIrBuilder` mints clause ids
against a fresh throwaway `OracleDocBuilder`, so every chain restarts at
`OracleItemId(0)` and clause `OracleUnitId`s are not document-unique. Keyed by
`OracleUnitId` so the recognizer bring-up needs no API change here.
…dence view Commit 1 of the swallow-audit cutover. Additive only: this module has zero production call sites, so the card-data export is byte-identical by construction (the parity check for this commit is the absence of a caller, not a diff). Adds `oracle_ir::feature`: - `OracleSemanticFeature` — the closed vocabulary the audit asks its question in, replacing fourteen `detector: "..."` string literals with one typed authority. ONE VARIANT PER EMITTED LABEL. The tempting collapse (three duration detectors into one `Duration`, two optionality detectors into one `Optional`) is wrong twice: `detector` is the wire format of `OracleDiagnostic::SwallowedClause` and ships in exported `parse_warnings`, so collapsing silently breaks every consumer of the coverage report — and it would make per-detector regression attribution impossible, since three distinct detectors would report under one name. - `ItemIdTracks` + `scope_to_item` — clone out the slice of `ParsedAbilities` that ONE document item produced. This is the whole cutover in one function: the audit handed every detector the card-wide `ParsedAbilities`, so a card that dropped an activation limit on line 3 was excused by an unrelated restriction on line 1 — the evidence never had to come from the clause that raised the expectation. Handing the SAME detectors an item-scoped `ParsedAbilities` makes all ~40 `any_*` / `def_tree_has_*` walkers item-scoped without touching one of them. The walkers were never the defect; their scope was. The four recursive categories resolve through the id tracks `lower_oracle_ir` already maintains; every other category is read straight off `item.node` via an exhaustive match (no `_` arm — a new `OracleNodeIr` variant must make a deliberate attribution decision rather than defaulting into invisibility). Soundness of the zip this relies on: of the four relation passes that run before the audit, three are length-preserving and the fourth (`apply_linked_choice_etb_counter`) removes from `result.replacements` and `replacement_ids` at the same index. So `result.<category>[k]` <-> `<category>_ids[k]` holds at the audit point. The audit stays POST-RELATION at its pinned position: pre-lowering audit is blind to relation-synthesized semantics; the false-positive wave U1 bounded to 31 faces would be caused, not avoided. CR annotations traced to govern, not merely grep-verified: CR 614.1a (the word "instead"), CR 602.5b (its own example is "Activate only once each turn"), CR 602.5d/e (activate only as a sorcery/instant), CR 611.2a (a stated duration), CR 603.5 (names both "may" and "unless"), CR 608.2h (information from the game, "such as the number of creatures on the battlefield") + CR 107.3 (X), CR 603.4 (intervening "if"), CR 611.3, CR 101.4 (APNAP), CR 700.2 (modal).
…sed the clause
Commit 2 of the swallow-audit cutover: the SCOPE change, isolated. Evidence-side
JSON introspection is untouched here and dies in the next commit, so this commit's
warning delta is attributable to scope + gate alone.
Before, both halves of the audit were card-wide: every detector got the whole
card's text AND the whole card's `ParsedAbilities`. Evidence therefore never had
to come from the clause that raised the expectation — a card that dropped an
activation limit on line 3 was excused by an unrelated restriction on line 1. Three
detectors (ActivateLimit, Duration_NextTurn, APNAP) were vacuous by construction as
a consequence, firing zero times across 35,396 faces.
Now the audit runs once per source unit, over that unit's own fragment and its own
lowered definitions (resolved through the id tracks `lower_oracle_ir` maintains).
THE UNIT IS THE SPAN, NOT THE ITEM. One line routinely lowers to several items —
Visions of Ruin's "Flashback {8}{R}{R}. This spell costs {X} less to cast this way,
where X is ..." emits a Keyword item AND a ModifyCost static item — and today both
carry the WHOLE LINE as their fragment, because the substrate cannot hand an item a
sub-line fragment. Auditing such items separately is not merely coarse, it is wrong:
each sibling raises the expectations of the entire shared line while supplying
evidence for only its own clause, so the Keyword item would report a swallowed
DynamicQty for a quantity the static represents perfectly. Grouping by span is the
honest granularity the substrate supports, and it splits automatically into finer
units once the recognizer bring-up gives items real sub-line spans.
The card-wide `Effect::Unimplemented` gate is not deleted but RE-SCOPED, which is
what the plan's per-unit suppression rule literally is: an unsupported unit
suppresses only its own expectations. Card-wide it silenced every detector on 2,563
faces.
`line_index` now carries the real line (baseline: 854/854 swallow warnings sat at
0, which does not read as "unattributed" — it reads as line 1). Detectors stay
provenance-agnostic and the unit loop stamps the line onto what they emitted, so a
detector cannot silently forget to attribute itself.
SINK INVARIANT: the diagnostics channel arrives non-empty (it carries the
parse-time diagnostics sealed by `finish()`). The audit never reads it — each unit
emits into a fresh local vec that is stamped and appended — so no predicate here
can match a diagnostic this audit did not emit. The variant sets are disjoint
besides: the audit only ever constructs SwallowedClause.
The audit stays POST-RELATION at its pinned position: pre-lowering audit is blind
to relation-synthesized semantics; the false-positive wave U1 bounded to 31 faces
would be caused, not avoided.
FOUR VACUOUS TESTS REPAIRED. Bronze Horse, Dragon Egg, Siege Behemoth and Atraxa
each asserted that a card does NOT report a swallowed clause. Each passed only
because it parses with an EMPTY MTGJSON keyword list, so its keyword line
("Trample" / "Defender" / "Hexproof" / "Flying, vigilance, deathtouch, lifelink")
fell through to an `Effect::Unimplemented`, which tripped the card-wide gate and
silenced all fourteen detectors on the whole card. The green was the gate, not the
parse: all four cards DO carry exactly those warnings in the shipped card data,
verified against the pre-cutover full-pool export. They are now pinned to their
real behavior with the underlying gap named, so each is a tripwire that flips when
the gap is fixed rather than a green that hides it.
16,220 engine lib tests pass.
…thority
`scripts/swallow_audit.py` re-implemented the swallow heuristics in Python over
`coverage-data.json`, "independently of the Rust runtime". That independence was
the problem: it is a second, unversioned implementation of an audit whose whole
purpose is to be the authority on what the parser dropped. It could agree with
`swallow_check.rs` by luck and disagree by drift, and nothing enforced either.
It is now also simply wrong. The Rust audit is per source unit, typed, and reads
evidence from the lowered definitions a unit produced; the Python script scans a
whole card's text and its serialized JSON for marker substrings — the exact
card-wide string-matching model this plan exists to delete. Keeping it would leave
a tool that reports the old, vacuous answers next to the new, honest ones.
Proven dead before removal: no reference in `.github/`, `Tiltfile*`, `.cargo/`,
or `Makefile*`. Its only mention anywhere was one paragraph in the oracle-parser
skill, removed here.
That skill's "Suppression rule" is corrected in the same commit: it documented the
CARD-WIDE gate ("may skip detectors when a card already has stronger parser
failures"), which silenced every detector on 2,563 faces and is exactly what this
plan re-scoped. Suppression is now per unit and cannot reach a sibling.
…tly vanish
Measured regression in the per-unit cutover, caught by the full-pool delta and
fixed here: 21 faces LOST swallowed-clause warnings that the card-wide audit
correctly raised. Every one was a modal block or a d20 roll table.
Root cause is a substrate lie, not a detector. An item's span under-reports the
text the item consumed. Drown in the Loch is three source lines:
item=0 first_line=0 last_line=0 frag="Choose one —"
item=1 first_line=0 last_line=0 frag="Choose one —"
item=2 first_line=0 last_line=0 frag="Choose one —"
All three items claim line 0 and carry the bare header as their fragment. The two
bullet lines — which hold the card's entire meaning, including the dynamic quantity
"less than or equal to the number of cards in its controller's graveyard" — are
claimed by NOTHING. An audit keyed on item fragments never scans that text, raises
no expectation for it, and drops the warning. Same shape for the "1—9 | ..." roll
tables (Chaos Channeler, Delina, Aberrant Mind Sorcerer).
A warning that disappears because nothing claimed its text is indistinguishable
from a warning that was fixed. It is the single delta direction that can hide a
regression, and it is precisely the direction a per-unit audit is prone to.
So audit units now PARTITION the card: a unit owns every source line from its own
start up to the next unit's start, sliced from the raw `source_text`. Coverage is
total by construction — every line belongs to exactly one unit, continuation lines
included — and no text can go unaudited. Leading lines join the first unit for the
same reason.
Taking the text from `source_text` rather than `fragment()` also drops a second,
quieter discrepancy: fragments are normalized (the card's own name is rewritten to
`~`), while the detectors' marker phrases and the emitted `description` are
raw-text concepts. The card-wide audit read raw text; so does this.
Draft-matters (CR 905) filtering moves from per-unit to per-LINE within the unit: a
unit owns a block now, and a draft line must not take a constructed-play line in
the same block down with it.
Pinned by `modal_bullet_lines_are_audited_not_orphaned`, which is red without this
change — Drown in the Loch reported zero warnings in the pre-fix export.
16,221 engine lib tests pass.
The last unexplained loss in the full-pool delta, and the most dangerous one. Chorus of the Conclave lowers to a completely empty `ParsedAbilities` — no ability, no static, no additional cost, no keyword — and does not even leave an `Effect::Unimplemented` behind to declare the gap. The parser drops the entire card in silence. Producing no items, it produced no audit units, so the unit loop had nothing to iterate and said nothing at all. That is the audit going quiet at exactly the moment the parser failed hardest. The card-wide audit reported this card's swallowed clauses (Optional_YouMay, Condition_If); the per-unit audit reported none. A warning that vanishes because nothing claimed its text is indistinguishable from a warning that was fixed, and here the trigger for vanishing was the parser failing MORE completely. A card with no items now becomes one unit owning all of its text, with no items and therefore NO evidence. Nothing represents anything, so every semantic the text raises is correctly reported as swallowed. Pinned by `a_card_that_parses_to_nothing_is_still_audited`. That test parses with the MTGJSON keyword list POPULATED, which is load-bearing: with it empty the "Forestwalk" line falls through to an ability, the card is no longer item-less, and the condition under test evaporates. The test asserts that premise explicitly before asserting the behaviour, so it cannot silently stop testing anything — which is exactly how the four vacuous tests repaired earlier in this plan came to pass for years while asserting the opposite of what shipped. 16,222 engine lib tests pass.
…acuous
Both detectors fired ZERO times across 35,396 faces while 113 and 56 faces of text
respectively raise their expectations. Neither was dead for want of source text.
Both were vacuous on the EVIDENCE side, so the per-unit re-scope could not reach
them: the fact they demanded as proof was implied by the very clause raising the
expectation.
APNAP. The marker list contained `"player_scope":`. A card reading "Starting with
you, EACH PLAYER votes..." parses `player_scope = EachPlayer` FROM THE VERY CLAUSE
that raises the expectation, so the marker was always present and the detector
always excused itself — even when `starting_with` had been dropped on the floor.
A player scope says WHO acts; it says nothing about the ORDER they act in, which is
the sole subject of CR 101.4.
Replaced with the typed ordering fact, which lives in exactly two homes:
`AbilityDefinition.starting_with` and `Effect::Vote.starting_with` (non-optional, so
the variant's presence IS the fact). NOT `player_scope`. NOT every `ControllerRef`.
ActivateLimit. `any_ability_has_limit` was `any_ability_has_constraint` — ANY
activation restriction counted as evidence of a limit, and its own comment admitted
it ("For Phase 1, treat presence of any non-trivial constraint as covering
activation limits too"). So a card that dropped "activate only once each turn" was
excused by an unrelated sorcery-speed gate ON THE SAME ABILITY. Unit scope cannot
help: the excusing fact sits on the same def.
Replaced with `restriction_is_activation_limit`, an exhaustive match over all 17
`ActivationRestriction` variants with no `_` arm, admitting only the three that are
usage caps (CR 602.5b — whose own example is "Activate only once each turn") and
rejecting the timing windows (CR 602.5d/e) and game-state gates. A new variant must
now state which of the three it is rather than defaulting into counting as a limit
it is not.
A FIFTH VACUOUS TEST, and this one codified the APNAP vacuity directly.
`apnap_accepts_protection_racket_repeat_for_each_opponent_in_turn_order` asserted
that Protection Racket does NOT report a swallowed APNAP — while asserting, three
lines above, that the only thing the card's "in turn order" clause leaves behind is
`player_scope = Opponent`. That IS the vacuity: the test passed for precisely the
reason the detector was broken. The card has no `starting_with` and no `Vote`; its
ordering fact is genuinely dropped. Pinned to its real behaviour with the gap named.
`OracleSemanticFeature::detector_label()` now has its first production callers, so
its `dead_code` allow dies with it.
16,222 engine lib tests pass.
…aim about one Review follow-ups F1 and F3. F1. The label test pinned the ENUM's table — but nothing in the export path called `detector_label()`. All fourteen `SwallowedClause` emit sites built their `detector` from a hand-written string literal. Two independent authorities, no link between them: change a literal and the exported wire format silently rewrites while the test stays green. The docstring meanwhile claimed the function was the "single authority", which was simply false as shipped — the fourteen literals were. The honest options were to weaken the claim or to make it true. Made it true: every emit site now constructs its `detector` through `detector_label()`, and zero string literals remain (`rg 'detector: "' swallow_check.rs` → no matches). The test now pins the wire format because the enum IS the wire format's only source. PROVEN OUTPUT-INERT: a full-pool export before and after this change has the same SHA256 (847bd8be…, 35,396 faces, 98 MB). The labels are byte-for-byte what shipped. Worth naming the pattern, because this unit's own harvest is four tests that did the same thing: a comment asserting a property the artifact does not have. The test was green, the docstring was confident, and the thing they described was not the thing that ran. F3. `scope_to_unit` now states the premise the "unit evidence ⊆ card evidence" argument rests on — the argument that makes an evidence-side LOSS structurally impossible and is how the delta's loss ledger is reasoned about. It holds for the id-tracked and `Vec` categories unconditionally, but NOT for the four `Option`-valued singletons (`modal`, `additional_cost`, `solve_condition`, `strive_cost`): `lower_oracle_ir` assigns those last-write-wins across items, so a card carrying two such items would hand a unit evidence the card-wide view had overwritten — the one shape that can produce an evidence-side loss. No card in the pool does this, so the impact is zero and the argument stands as used; `debug_assert!`s now make a future one fail loudly instead of quietly switching a warning off. 16,222 engine lib tests pass.
…s instrument Review follow-ups F6 and F5. F6. Three test NAMES asserted the opposite of their bodies. Each says "accepts" — the detector does NOT warn — while the body asserts `has_swallowed_detector(..)`, i.e. it DOES: condition_as_long_as_accepts_bronze_horse_and_champions_helm optional_you_may_accepts_atraxa_grand_unifier_from_among apnap_accepts_protection_racket_repeat_for_each_opponent_in_turn_order The Bronze Horse one was the worst of the three because it was HALF true — Champion's Helm really is still accepted, so a reader has no way to know which half the name refers to. This is the same defect as F1 one level down, and it appeared inside the unit whose central finding is five tests that asserted the opposite of what shipped. The lesson did not need restating; it needed applying to the artifacts the fix itself produced. A name is an assertion. When the body changed direction, the name kept making the old claim, and a reader scanning the file would have read the pins as passes. Renamed to state what each pins. Two neutrally-named siblings (Siege Behemoth, Dragon Egg) were not false, but a reader still could not tell which way they bite, so all five now say `..._reports_known_gap` / `..._reports_..._as_swallowed`. Verified the class is closed: zero remaining tests named `*accepts*` assert a firing detector. F5. The pool item-structure census is COMMITTED rather than deleted, per the ruling. It is the only instrument in the tree that can size the substrate defects `audit_units` works around and that Plan 05 inherits — 2,836 multi-item lines across 2,763 faces (~7.8% of the pool) and 37 zero-item faces, which is what makes both substrate fixes in this unit CLASS fixes rather than two anecdotes. It also proved two of `audit_units`' own guards are dead code on the real pool (`first_line` out of range: 0, `fragment == None`: 0) — worth knowing before anyone treats them as load-bearing. `#[ignore]`d, and its hardcoded /tmp path lifted to `ORACLE_POOL_DIR` so it survives a fresh checkout. Deleting it would have thrown away the measurement a Plan 05 named input depends on. Those census figures are the REVIEWER's, cited not recomputed — I did not re-run the instrument, and the handoff attributes them. 16,222 engine lib tests pass.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the parser's swallow audit from a card-wide scope to a per-source-unit (item/line) scope to prevent unrelated card text from masking missing implementations. It introduces a new feature module to define semantic features, track item IDs, and partition documents into audit units, updating the swallow_check logic to run detectors against these unit-scoped definitions. Review feedback suggests grouping audit units using a BTreeMap to guarantee ascending order and robust grouping instead of relying on the implicit ordering of items.
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.
Comment on lines
+248
to
+262
| let mut units: Vec<AuditUnit<'a>> = Vec::new(); | ||
| for item in items { | ||
| let first_line = item.source.span().first_line; | ||
| if first_line >= lines.len() { | ||
| continue; | ||
| } | ||
| match units.last_mut() { | ||
| Some(unit) if unit.first_line == first_line => unit.items.push(item), | ||
| _ => units.push(AuditUnit { | ||
| text: String::new(), | ||
| first_line, | ||
| items: vec![item], | ||
| }), | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
[MEDIUM] Group audit units using
BTreeMapto guarantee ascending order and robust grouping. Evidence:crates/engine/src/parser/oracle_ir/feature.rs:248.
Why it matters: Relying on the implicit ordering ofitemsis fragile and can lead to empty text slices or ungrouped items if the parser's item emission order changes.
Suggested fix: Group items byfirst_lineusing aBTreeMapbefore collecting them intoAuditUnits.
Suggested change
| let mut units: Vec<AuditUnit<'a>> = Vec::new(); | |
| for item in items { | |
| let first_line = item.source.span().first_line; | |
| if first_line >= lines.len() { | |
| continue; | |
| } | |
| match units.last_mut() { | |
| Some(unit) if unit.first_line == first_line => unit.items.push(item), | |
| _ => units.push(AuditUnit { | |
| text: String::new(), | |
| first_line, | |
| items: vec![item], | |
| }), | |
| } | |
| } | |
| let mut groups = std::collections::BTreeMap::new(); | |
| for item in items { | |
| let first_line = item.source.span().first_line; | |
| if first_line < lines.len() { | |
| groups.entry(first_line).or_default().push(item); | |
| } | |
| } | |
| let mut units: Vec<AuditUnit<'a>> = groups | |
| .into_iter() | |
| .map(|(first_line, items)| AuditUnit { | |
| text: String::new(), | |
| first_line, | |
| items, | |
| }) | |
| .collect(); |
…t by array length
Optional hardening from the review, taken.
The label test's expected table is hand-written (15 entries). `detector_label()`'s match
already forces a 16th variant to declare a LABEL — a missing arm is a compile error. But
a 16th variant declaring a DUPLICATE label would sail straight past a 15-entry array that
never mentions it, and a collapsed label is the exact failure this test exists to catch:
it silently rewrites the exported wire format AND destroys per-detector regression
attribution, which is the delta table's key.
So the test now iterates the enum (`strum::EnumIter`, already a dependency) rather than
trusting an array's length. A new variant enters both the count check and the distinctness
check on its own, whether or not anyone remembers this file exists.
The expected table stays hand-written on purpose — it IS the pin. A generated one would
only ever agree with whatever the code happens to say, which is not a test.
RED-PROVEN, not assumed: adding a 16th variant whose label duplicates `DynamicQty` fails
the test ("a variant was added without pinning its exported label above"). Before this
change the same variant passed. The guard bites.
16,222 engine lib tests pass.
Parse changes introduced by this PR✓ No card-parse changes detected. |
The snapshot test landed on main after this branch forked; the per-unit
swallow audit correctly flags the dies-trigger line, whose quoted granted
ability ({G}{W}: Enchanted creature gains indestructible until end of turn)
parses with duration: null — the 'until end of turn' survives only as
description prose. True positive; the duration loss is tracked separately.
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.