refactor(parser): typed AbilityIr shell + source-addressed builder key (unit 3b, byte-safe subset)#5513
Merged
Merged
Conversation
…te, ordinal) (unit 3b-1) `OracleDocBuilder::items` was keyed `(first_line, ordinal_within_span)`. Two exact, NON-overlapping clauses on one physical line with the same `ordinal_within_span` pass `conflicts_with` (which rejects only OVERLAPPING sibling bytes) and then collide on the map key: `emit` returns `DuplicateItemPosition` and the second clause is lost. A `(first_line, ordinal)` key would also order same-line siblings by ordinal rather than by byte position, so an emitter assigning ordinals out of byte order would have `finish()` return them reversed and `lower_oracle_ir` would lower them reversed -- silently. Add `start_byte` to the key. Inert today (every item spans the whole document, so `start_byte == 0`), but unit 3b emits exact spans, at which point "Destroy target creature. It can't be regenerated." is two clauses on one line. Fixed before it can fire. Discriminating test: `two_exact_clauses_on_one_line_coexist_and_order_by_byte` emits the later clause first and asserts both coexist and come back in byte order. Watched go red against the reconstructed `(first_line, ordinal)` key: 18620 tests run, exactly 1 failed, and it was this test.
…ape hatch (units 3b-2, 3b-3)
The two effect-chain entry points, `parse_effect_chain` and
`parse_effect_chain_with_context`, each opened with a hand-maintained stack of
whole-body `try_parse_* -> return AbilityDefinition` bypasses, then lowered the
IR and ran `finalize_effect_chain` + the owner-library anchor inline. Two latent
hazards lived here:
1. The two stacks are NOT the same set. `parse_effect_chain` runs 8 bypasses;
`parse_effect_chain_with_context` runs the same 8 as a strict prefix plus
`try_parse_exile_pile_shuffle_cloak`. Duplicating them as two `if let`
ladders invited silent divergence. This is now typed data: `ChainLoweringMode
{ Standalone, WithContext }` (exhaustive, no `_` arm, no `Default`), dispatched
by one `try_parse_chain_bypass`. The asymmetry is preserved byte-for-byte --
it is a suspected-accidental pre-existing quirk (ISSUES.md #9), and a parity
migration never absorbs a bug fix.
2. `try_parse_balance_equalization` sets `sub_link = SequentialSibling` on its
ROOT def (CR 608.2e: three independent equalize steps, each resolving even
when an optional parent is declined). No `ClauseIr`/`EffectChainIr` field can
express a root `sub_link` -- `lower_effect_chain_ir` derives it from the
previous clause boundary, and the root has none, so it is unconditionally
`ContinuationStep`. Converting that bypass to return a bare `EffectChainIr`
would silently flip its semantics. `AbilityShellIr` is the typed home for
exactly the root fields no clause can hold; today that is `sub_link` alone
(verified by auditing the return expressions -- not field names -- of all nine
bypasses: they set only kind/effect/sub_ability/duration/player_scope, all
clause-expressible, plus balance's root sub_link).
`lower_ability_ir` is now the single authority for the post-parse sequence:
lower the chain, finalize, anchor, then apply the shell. `finalize` and the
anchor stay here rather than in `lower_effect_chain_ir` so the ~7 production
callers that lower a chain without being a whole ability body are unaffected.
Parity: 191 snapshots byte-identical (0 .snap.new); clippy clean; test-engine
18620 run with the sole failure a pre-existing foreign integration-registration
guard; data/card-data.json byte-identical (H0==H_after==9fdef189), with a
positive control (min_x_value=77 injected into the broad lowering path) proving
the card-data probe discriminates (H_ctrl==48fbb42a).
matthewevans
enabled auto-merge
July 10, 2026 16:14
![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 updates the OracleDocBuilder in doc.rs to key items by (first_line, start_byte, ordinal_within_span) instead of (first_line, ordinal_within_span). This ensures that multiple non-overlapping clauses on the same physical line do not collide and are correctly ordered by their byte position. A new unit test has been added to verify this behavior. Additionally, effect_chain.rs introduces AbilityShellIr and AbilityIr structs to model root-level metadata and encapsulate effect chains with their source text. I have no feedback to provide as there are no review comments to assess.
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✓ No card-parse changes detected. |
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.
Parser IR remediation — unit 3b (byte-safe subset)
Two byte-identical parser migrations from Plan 01 unit 3b. No behavior changes; all three lowered-parity oracles held (60
*_lowered.snap, 21parser/snapshots/…pipeline_snapshot_tests…, and adata/card-data.jsonregen).7c2b0dfabf— unit 3b-1: source-address the builder keyOracleDocBuilder.itemsre-keyed(first_line, ordinal_within_span)→(first_line, start_byte, ordinal_within_span).conflicts_withrejects only overlapping sibling bytes, so two exact non-overlapping clauses on one physical line with the same ordinal passed the conflict check and then collided on the map key (second clause lost); the old key also ordered same-line siblings by ordinal rather than byte position. Inert today (every item spans the whole document,start_byte == 0) — fixed before unit-3b emission can fire it. Discriminating testtwo_exact_clauses_on_one_line_coexist_and_order_by_bytewas watched go red against the reconstructed old key (sole failure among 18,620 tests).20e5cf0eb7— units 3b-2/3b-3: typedAbilityIrshellAbilityShellIr { sub_link: Option<SubAbilityLink> }— the typed home for the one rootAbilityDefinitionfield noClauseIr/ParsedEffectClausecan express (verified by auditing the return expressions of all 9 effect-side bypasses: they set only clause-expressible fields plus balance's rootsub_link).Option<_>so a defaulted shell defers to the lowered stamp instead of overwriting it.AbilityIr { source_text, body: EffectChainIr, shell }— noAbilityDefinitionescape-hatch field.source_text(notOracleUnitSource) because the unit allocator is not threaded throughParseContextyet; minting a span here would fabricate precision.ChainLoweringMode { Standalone, WithContext }(exhaustive, no_, noDefault) types the 8-vs-9 bypass-set asymmetry between the two shims (ISSUES chore: update coverage stats and badges #9), dispatched by onetry_parse_chain_bypass, preserved byte-for-byte — a parity migration never absorbs a bug fix.lower_ability_iris the single post-parse authority (lower → finalize → anchor → shell).finalize_effect_chain+apply_owner_library_reveal_anchor_from_textwere relocated out of both shims intolower_ability_ir(oracle_effect/mod.rs) — the deletion site for unit 7's removal gate moved accordingly.Deferred (out of this byte-safe migration)
3b-3′ / 3b-4 / 3b-5 as originally specified are deferred to a dedicated recognizer→IR bring-up plan. They require converting whole-body def-building recognizers (the 9 bypasses; the 8
TriggerBody::PreLoweredproducers — vote/pile/modal/reanimator) into IR. Those recognizers have noEffectChainIrsibling and deliberately skip the standard lowering transforms, so the conversion is behavioral, not byte-safe — blocked at source:oracle_trigger.rs:1155("chain parsing would fragment it into Unimplemented chunks"). Per the parity migration's #1 rule ("a moved byte is a stop"), it is handed off rather than absorbed. The byte-safe boundary landed here: clause-expressible bodies gotAbilityShellIr; whole-body recognizers stay as-is.