fix(engine): fuse battlefield-entry record+emit into one authority - #7012
Merged
matthewevans merged 3 commits intoAug 5, 2026
Merged
Conversation
Six sibling sites emitted a `ZoneChanged { from: None, to: Battlefield }` event
carrying a placeholder `turn_zone_change_index` of `0`. The batched-trigger
collector dedups on `(definition_ref, turn_zone_change_index)` read off the
event, so a later same-turn entry aliased onto the first occurrence's key, was
treated as already-collected, and its batched enters-the-battlefield fire was
swallowed. Measured on the pre-fix tree: two same-turn conjures produce dedup
keys `[0]` and one trigger fire; post-fix they produce `[0, 1]` and two fires.
Within a single event the placeholder was additionally wrong in a second way:
every entrant recorded index `0`, so `self_ref_own_departure_successor`
subscripted another object's ledger row. That case is fixed here too
(`[0, 0]` -> `[0, 1]`), though it is not the case that lost a trigger.
CR 603.2c: an ability triggers once per occurrence, and repeatedly when one
event contains multiple occurrences. CR 603.6a: each time an event puts one or
more permanents onto the battlefield, all permanents are checked for matching
ETB triggers.
Adds `zones::record_and_emit_entry_from_no_zone`, which fuses the snapshot, the
`record_zone_change` ledger write (which performs the CR 608.2i battlefield-entry
bookkeeping itself) and the event emission into one indivisible call. The defect
class is literally "emit without record", so fusing is what narrows it: a
record-only helper plus a separate emit helper is today's shape, and today's
shape is what produced the defect.
Sites routed: conjure, counters (InjectPredefinedTokenAbilities + both
FinalizeTokenEntry / FinalizeCopyTokenEntry arms), token_copy x2, gift_delivery,
incubate. Deletes the private clone `counters::push_token_entry_events` and both
`token.rs` halves (`record_committed_token_entry`,
`push_token_entry_events_for_record`), folding them into the authority;
`push_committed_token_entry_events` now returns `Option<ZoneChangeRecord>`. That
return is consumed at exactly two of its eight call sites -- `gift_delivery.rs:175`
and `token_copy.rs:866`, both `.expect(...)` (`rg -n -A8
'push_committed_token_entry_events\(' crates/engine/src`) -- which preserves the
panic those routes already had when a just-created token is missing; the flush
route discards it. That sentence is about THIS emitter. It does not describe
`zones::record_and_emit_entry_from_no_zone` one level down, whose own four direct
callers are `conjure.rs:218` and `incubate.rs:123` (both `.expect`, i.e. they
PANIC on `None`), `token.rs:1881` (`is_some()`), and `counters.rs:530` (discards).
An earlier revision copied this sentence into that function's doc, where it named
the wrong callers and framed the `None` arm as inert everywhere; that doc is
corrected here and now carries its own query.
The object-gone arm is load-bearing through its EFFECT (nothing recorded,
nothing emitted), not through the returned value. Each site's co-located
`record_battlefield_entry` call is deleted in the same edit, because
`record_zone_change` already performs it and leaving both double-counts
`battlefield_entries_this_turn`.
The invariant is structural, not enumerated: `TokenCreated` is emitted IF AND ONLY
IF the authority recorded the entry. `push_committed_token_entry_events` gates the
push on `record.is_some()`, and `record_and_emit_entry_from_no_zone` returns `None`
exactly when the object is gone -- verified to have exactly one `?`, on
`state.objects.get`, with no second `None` source -- so the authority's own verdict
is the predicate. `restrictions::record_token_created` applies that same existence
test to `created_tokens_this_turn` and `players_who_created_token_this_turn`, so
event and ledger cannot disagree on any route THAT REACHES THIS EMITTER -- which is
the class boundary, stated because the measurement was made against it. Two routes
do not reach it: `incubate.rs` and `counters.rs`'s `InjectPredefinedTokenAbilities`
arm call `record_token_created` and then the authority DIRECTLY, so they write both
ledgers and emit no `TokenCreated` at all. That is pre-existing and unchanged here --
`b654513cb` behaves identically -- but it means Incubator tokens do not fire
`TriggerMode::TokenCreated`, even though CR 701.53a describes incubating as creating
a token. Routing them through this emitter would reorder the ETB events the
`InjectPredefinedTokenAbilities` arm exists to control, so it is recorded as
follow-up rather than done here.
A token creation writes THREE ledgers, not two, and gating the event on the
authority's verdict at first only moved which one it disagreed with. The third is
`last_created_token_ids`, the `TargetFilter::LastCreated` anaphora slot
(`game/filter.rs`'s `contains(&object_id)`, `game/targeting.rs`'s `clone()`). On
the object-gone path it produced
`(TokenCreated=0, created_tokens_this_turn=0, last_created_token_ids=1)`: a "the
token you created" reference naming an object that never finished entering. Adds
`token::record_last_created_token`, which carries the same existence predicate.
That population is stated WITH the query that produced it AND with the tip it was
run at, because this ledger has now been mis-swept twice from a too-narrow grep and
mis-STATED twice from an unlabelled tip. Counts are ripgrep JSON MATCH EVENTS
(`rg --pcre2 -U --json ... | grep -c '"type":"match"'`). An event is a matched
CHUNK and ripgrep emits one per LINE, so a line carrying two matches counts once.
Both populations below are unaffected -- `--count-matches` sums to the same 39 and
11 -- but two positive CONTROLS quoted later in this message ARE affected, and both
figures are given where they appear. With
`M=(push|extend|clear|insert|append|retain|splice|truncate|remove|drain|resize|pop)`,
`rg -n --pcre2 -U "state\s*\.\s*last_created_token_ids\s*(\.\s*M\s*\(|=[^=])"
crates/engine/src` returns, RUN AT THIS COMMIT, 39 hits, of which 3 are prose in
comments this commit itself added; the remaining 36 code hits plus one more bound
as `s.` (`engine.rs`'s per-turn `.clear()`) are 37 = 20 production writers and 17
inside `#[cfg(test)]`. Of those 20, exactly ONE publishes a single just-created id
-- the `push` inside `record_last_created_token` -- and the other 19 are 4 clears
and 15 bulk republishes.
RUN AT `b654513cb`, the same query returns 39 hits with NO comment among them, for
23 production writers, of which FIVE published the id of the single token the
caller had just created. Those five are now the authority's five production CALL
SITES (`counters.rs` x3, `token_copy.rs`, and the liminal finalizer in `token.rs`)
-- calls, not writers, which is why they are absent from this commit's 20. An
earlier revision of this message stated that BASE-tip call-site count of 5 in the
present tense against the HEAD-tip writer population of 20, and partitioned 20 as
5 + 15, which omits the clears and contradicts this commit's own
`record_last_created_token` doc. Both halves are now labelled with their tip and
their population.
The raw totals above are POSITIVE CONTROLS, not invariants, and they churn in a
specific way worth naming: the query matches its own documentation, so writing
prose about this ledger moves the number. The previous revision's "36" was already
stale when it shipped -- invalidated by the three comment lines the same commit
added. What does not churn is the classification, and that half is now enforced
executably rather than in prose (see the third census anchor below).
The slot has a SECOND publish destination, and the first version of this guard was
defeated one line below itself by it. `PendingCopyTokenResolution::created_ids` is
the copy-batch buffer, and `token_copy::drain_copy_token_resolution` ends with
`state.last_created_token_ids = pending.created_ids;` -- an ASSIGNMENT, not an
append. So two sites that guarded the ledger write and then pushed the same id into
the buffer as a separate statement republished the withheld id AND overwrote the
guarded ledger with the unguarded list. Same discipline:
`rg -n --pcre2 -U "pending\s*\.\s*created_ids\s*(\.\s*M\s*\(|=[^=])"
crates/engine/src` returns, RUN AT THIS COMMIT, 11 match events, of which 1 is
prose in a comment this commit added, leaving 10 production writers and 0 inside
`#[cfg(test)]`; exactly ONE of the 10 is a single-id publish, the `push` inside
`record_last_created_copy_batch_token`. RUN AT `b654513cb` the same query returns
11 with no comment among them, i.e. 11 production writers, of which TWO published a
single just-created id (`counters.rs`'s `FinalizeCopyTokenEntry` arm and
`token_copy::apply_remaining_token_modifications_after_counter_pause`). The two
tips reading 11 is a COINCIDENCE, not stability -- two single-id pushes were
removed while one authority push and one doc comment were added -- and an earlier
revision of this message quoted the BASE number as though it described this tip.
Those two sites now make ONE call, `token::record_last_created_copy_batch_token`,
which performs both writes under ONE evaluation of the predicate. After that, the
only CODE occurrence of `pending.created_ids.push(` in `crates/engine/src` is the
one inside that authority; the query's second hit is this commit's own doc comment
quoting the defect.
The `-U` is load-bearing, not decoration: `token_copy.rs:321-323` and `:327-329`
split `pending` / `.created_ids` / `.extend(...)` across three lines, so the
line-oriented form of the same query reports 9 match events at BOTH tips --
deriving this population from `grep 'last_created_token_ids.push('` is exactly how
the earlier sweeps missed sites.
CLASS BOUNDARY, named because four prior sweeps each claimed more than they
measured -- and then the first revision of THIS paragraph did it a fifth time.
CLOSED: "a single just-created object id reaching `last_created_token_ids` or
`PendingCopyTokenResolution::created_ids` without the existence predicate", measured
by the two queries above over `crates/engine/src`. Its `.push` / `.insert` half is
pinned executably by the third census anchor below; the half no text scanner can
classify -- `.extend(std::iter::once(id))` and its three siblings, whose ARGUMENT
decides whether they publish one id -- is pinned as a whole population by that
anchor's conjunct 4 rather than argued away.
NOT closed, and unchanged from `b654513cb`: this tip's 15 bulk republishes and 4
clears. An earlier revision of this paragraph said each of the 15 assigns "a clone
of ledger 3, a `CopyTokenApplyStatus` built one line after
`.expect("token just created")`, or the buffer itself". Measured FALSE:
`token.rs:892`, `token.rs:1023` and `token_copy.rs:617` assign a batch seeded from
an `initial_created_ids` parameter and extended IN THE LOOP with ids minted in the
same call (`token.rs:976`), which is none of those three. What the paragraph needed
was a reachability claim, and this is it: each of the 15 assigns a clone of ledger 3,
a `CopyTokenApplyStatus`, the buffer itself, OR a batch accumulated inside one
uninterrupted call -- and on that fourth path the id cannot have died between mint
and publish. The counter-replacement pause `return false`s at `token.rs:941`, BEFORE
`created_ids.push(obj_id)` at `:976`; the only other production code between them is
`effects::attach`, which performs no object removal
(`rg -n --pcre2 -U 'objects\s*\.\s*(remove|retain)\s*\('
crates/engine/src/game/effects/attach.rs` exits 1 with no output, against 1 match
for the identical query over `game/zones.rs`; and `attach.rs`'s own five
`check_state_based_actions` calls all sit past ITS `#[cfg(test)]` at line 1440 --
naming `attach.rs` again because the preceding clause mentions `zones.rs` and "that
file" read as the wrong referent); and
no state-based-action check or zone move occurs anywhere in the loop body
(`awk 'NR>=774 && NR<=1042'` over `token.rs` for
`check_state_based_actions|change_zone|objects\.remove|objects\.retain` prints
nothing). Also NOT closed: an id that existed when published and died afterwards,
which stays in ledger 3 until the per-turn `.clear()`. That one is pre-existing
behaviour and a different question from this fix.
`record_last_created_copy_batch_token` is deliberately NOT folded into
`record_last_created_token`, and the distinction is behavioural rather than
stylistic: that function's other three callers (`counters.rs`'s
`FinalizeTokenEntry` and `EmitCommittedCopyTokenEntry` arms, and
`finalize_committed_liminal_token_entry_from_action`) do not mirror into the batch
buffer, and mirroring there would put a plain or already-batched token into a copy
batch's `created_ids`, which the drain then assigns wholesale onto ledger 3 --
silently widening what "the tokens created this way" names. The mirror of that
argument is why the predicate stays out of `restrictions::record_token_created`,
whose production call sites are a strict SUPERSET of the anaphora slot's, so
folding the write in would widen `LastCreated` to `incubate.rs`,
`gift_delivery.rs` and the predefined-token arms, which do not claim it.
The 3-tuple coherence test fails at the pre-fix tip with `left: (0, 0, 1)` against
`right: (0, 0, 0)`, and it was NOT SUFFICIENT. Every fixture in `counters.rs`
builds `GameState::new_two_player(42)`, whose resolution stack carries no
`ResolutionFrame::CopyToken`, so `state.active_copy_token_mut()` returns `None` and
the copy-batch branch is never entered in ANY of the five arms. The test was
sensitive to the lines that changed without reaching the branch where the guard
could be defeated; sensitivity is not coverage. The new
`a_vanished_token_never_reaches_the_anaphora_slot_through_the_copy_batch_buffer`
closes that: it pushes a real `CopyToken` frame seeded with an earlier batch's
SURVIVOR id, dispatches both routes through `apply_pending_counter_post_action`,
and measures its 3-tuple AFTER `drain_pending_copy_token_resolution` performs the
wholesale assignment -- the state a `TargetFilter::LastCreated` reference actually
reads. Reachability is DEMONSTRATED, not argued: deleting the `if let
Some(pending)` body fails the PRESENT row with `left: (0, 0, 1)` against
`right: (1, 1, 1)`. Two-sided on one named assertion, the gone row's `(0, 0, 1)`:
MUTANT-DROP (delete the guard's early return -- the exact shape that shipped)
gives `(1, 1, 1)`, MUTANT-TRIVIALIZE (keep every branch, force `contains_key` to
`true`) gives `(1, 1, 1)`, and both leave the PRESENT row passing. The survivor
column stays 1 under every mutant, which is what makes the two zeros a measurement
of the guard rather than of a drain that never ran.
This replaces per-call-site guarding, which was the wrong instrument: three
successive sweeps of the eight call sites each missed a route, and each shipped a
claim that the sweep was complete. Guarding inside the shared emitter makes the
class unrepresentable instead of enumerated, and deletes the caller-side guards in
the process. An earlier revision of this message claimed a specific net production
line count here; that figure was a delta against an unpublished intermediate
candidate and is not reproducible from this commit, so it is dropped rather than
restated. The figure for this commit's own range is whatever
`git diff --numstat b654513..HEAD -- 'crates/engine/src/*'` reports at this commit.
The census enforces both halves of the pair AND the anaphora slot behind them, so
none of the three is left to enumeration: it pins production
`ZoneChanged { from: None }` constructions to the record+emit authority, production
`GameEvent::TokenCreated` constructions to this emitter, and production single-id
publishes into either anaphora container to the two `record_last_created_*`
functions -- each with a function-scope conjunct so a clone written inside the
owning file that also deleted the original's hit cannot pass.
Without the guard the emit is not merely a ledger disagreement. `match_token_created`
reads the controller inside `if let Some(token_controller) = state.objects.get(..)`,
and `valid_card_matches` short-circuits `None => true` without reading state, so a
vanished token never has the CR 111.2 controller filter applied and the matcher
returns true -- a trigger fire for a NON-MATCHING controller. Measured: with
`valid_card = None` the gone path matches where the present path does not; with
`valid_card = Typed{Creature}` it does not, which is the negative control.
The class-wide fix is to resolve the controller through last-known information the way
`valid_card_matches_with_lki` already does for the card filter; that touches a shared
matcher and is recorded as follow-up rather than done here.
Because the predicate sits in the shared emitter, it covers all five pause-deferred
routes -- `counters.rs`'s `FinalizeTokenEntry`, `FinalizeCopyTokenEntry`,
`ApplyCopyTokenModificationsAndFinalize` and both `FinalizeCommittedLiminalTokenEntry`
arms -- including `token.rs`'s `flush_pending_token_battlefield_entry`, which earlier
revisions left unguarded and merely disclosed. It also covers the non-deferred
creation paths, a wider blast radius than a per-caller guard: that is deliberate, and
the whole suite plus an equivalence probe asserting
`record.is_some() == state.objects.contains_key(..)` at every invocation found nothing
reachable that depends on the old behaviour. That probe was executed at the round-6
candidate, where the suite was 22979 tests; at this tip the battery is 23049 passed
and 15 ignored (18496 + 12 + 9 + 4532 + 0). The number is stated against the
candidate it was measured at rather than presented as a fact about this commit --
the difference is six tests added by later review rounds, plus the 64 upstream added
between `4b34e5465` and this commit's base `b654513cb`, which the rebase brought in. The two `.expect(...)`
call sites still panic identically -- the panic is on the returned `Option`, which is
unchanged. `phase-engine` contains no production `catch_unwind`:
`rg -c 'std::panic::catch_unwind\(' crates/engine/src` reports 2, both
`#[cfg(test)]`-scoped (`game/layers.rs:20642` under the `cfg(test)` at :8573, and
`parser/oracle_static/tests.rs:1230`). The catchers that DO exist are consumer-side
in `phase-ai`, and there are FIVE, not the two an earlier revision listed:
`rg -n 'std::panic::catch_unwind\(' crates/phase-ai/src/` gives `bin/ai_tune.rs:769`,
`bin/ai_commander.rs:832` and `:833`, and `duel_suite/run.rs:584` and `:604`. None
of the five can observe the difference: `events` is a call-local `Vec<GameEvent>`
dropped during the unwind, so whether a `TokenCreated` was pushed before the panic
is unobservable from any catcher. The conclusion is unaffected; only the
enumeration was incomplete.
STILL OPEN, recorded rather than fixed: `match_token_created` resolves the token's
controller by reading the live object, so for a gone object it skips the CR 111.2
controller filter entirely. Suppressing the emit makes event and ledger agree; the
deeper fix is to emit with a last-known-information controller the way
`valid_card_matches_with_lki` already does for the card filter. That touches a shared
matcher, so it is follow-up -- and the emitter is now its single adoption point.
No CR settles whether a token that never successfully entered should fire a creation
trigger -- CR 704.3 checks state-based actions only when a player would get priority,
so nothing can remove the token between its creation and the CR 603.6a check, and the
arm is a defensive engine artifact of deferring the emit past a replacement pause. The
requirement on it is internal agreement between event and ledger, not a rules verdict.
The entry-ledger annotations cite CR 608.2i, not CR 403.3. CR 403.3 is definitional
("Permanents exist only on the battlefield"); it does not describe an entry-time
characteristics snapshot kept for later look-back queries. All seven production
consumers of `battlefield_entries_this_turn` are conditions, quantities, or a static
analysis probe evaluated during resolution -- none is a trigger condition -- so
CR 603.10's look-back-for-triggering rule does not apply either. CR 608.2i ("Some
effects look back in time and require information about previous game states") is
what the pre-existing `battlefield_entry_record_for` already cites. The retag covers
every site in this commit's touch-set, including the one survivor inside it
(`zones::retag_battlefield_entry_snapshots`); the remaining pre-existing CR 403.3
entry-ledger citations are in files this commit does not modify -- `restrictions.rs`
and `types/game_state.rs`, which carry 8 and 5 CR 403.3 citations respectively, both
counts unchanged from `b654513cb` -- and are recorded as follow-up. Those per-file
totals include non-entry-ledger uses (definitional sites and test assertions): an
earlier revision of this message enumerated five coordinates as though they were the
exhaustive entry-ledger subset, and they are not, so the follow-up triages the 13
rather than assuming each needs the retag. Separately, `effects/token.rs`'s two CR 111.10 citations for the token
CREATION EVENT are retagged to CR 111.1 ("Some effects put tokens onto the
battlefield"). That retag is deliberately narrow: CR 111.10 is the predefined-token
characteristics catalog, which the file's ~58 other CR 111.10 citations use
correctly, and a blanket retag would corrupt them.
`game/restrictions.rs` is deliberately untouched: it is the ledger authority
(one `GameEvent` emission in 5166 lines, never a `ZoneChanged`), so the fused
emitter belongs beside the existing emitter in `game/zones.rs`.
Adds a structural census test asserting every production no-origin-zone
`ZoneChanged` construction in `crates/engine/src` lives in the single authority,
so a SEVENTH clone trips CI rather than silently shipping a placeholder index.
It is a source-text tripwire against an accidental clone, not a proof that one
cannot be written, and its residual ceilings are enumerated in its own module
header. It scans each literal's own brace extent and classifies the `from`
FIELD -- both the spelling `from: None` and field-init shorthand -- because an
earlier form keyed on the substring `from: None` inside a fixed six-line window
and BOTH halves were evadable by code that compiles (`let from = None;` plus
`from,`; and a multi-line `record:` initializer written first, which pushes
`from: None` past the window). Review found THREE further evasions of that
brace scan, and unlike the first two they are FAIL-OPEN: a `}` inside a `//`
comment, inside a string literal, or inside a NESTED block comment closed the
literal early and TRUNCATED the captured body, so every field after it went unseen
and a real construction scored zero. The first two are closed by skipping comments
and string / raw-string / char literals. The third survived that very rewrite,
because block-comment state was a BOOL: Rust block comments nest, so
`/* outer /* inner */ } */` left comment state at the inner `*/` and the `}` was
counted -- and the rewrite's residual list, copied from its predecessor rather than
read off the scanner, did not name it. It is closed with a depth counter.
A FOURTH fail-open truncation, found in review round 12, closes here too: RAW BYTE
and RAW C strings (`br#"..."#`, `cr#"..."#`). `b` and `c` are alphanumeric, so the
guard that stops an identifier's trailing `r` opening a raw string rejected these,
and the `"` after it fell through to the `#`-blind escaped-string scan, which stops
at the first embedded `"` and lets the following `}` close the literal. Measured
against a verbatim extraction of the shipped scanner, before and after the one-line
`literal_prefix_start` fix: `br#"a " } b"#` scores `(0, 0)` -> `(1, 1)` and
`cr#"..."#` the same, while the byte-identical control `r#"a " } b"#` scores
`(1, 1)` under BOTH scans. All three forms lex as valid Rust
(`rustc --edition 2021 --crate-type=lib`, rc 0). Latent, not live: `br` / `cr`
occur ZERO times in `crates/engine/src`
(`rg --pcre2 -U --json '(?<![A-Za-z0-9_])[bc]r#*"' crates/engine/src`), against
non-empty positive controls of 141 and 31 match EVENTS for `(?<![A-Za-z0-9_])r#*"`
and `(?<![A-Za-z0-9_])b"` under the identical query shape -- 156 and 35 individual
matches under `--count-matches`. The two gaps have DIFFERENT causes, measured with
ripgrep 15.2.0 rather than assumed, because an earlier revision of this paragraph
gave both the same wrong one: `b"` genuinely hits twice on some lines (35 matches
on 31 lines), so per-LINE event grouping explains 31 vs 35; `r#*"` does NOT (156
matches on 156 distinct lines, no line carrying two), and its 141 comes from `-U`
MULTILINE mode merging ADJACENT matches into one event -- the same query without
`-U` reports 156 events. Either figure satisfies what a control has to be, which
is non-empty.
That is the third hole found one at a time in one function, so the residual list's
PROVENANCE is fixed rather than its contents. The previous revision said the list
was "derived from the branches the scanner actually has", which cannot work: a
branch the scanner does NOT have is invisible to that derivation, and round 12
found `br#"..."#` sitting in exactly that gap. The basis is now EXTERNAL -- the
Rust Reference's enumeration of tokens whose interior text is not code (line
comment, nesting block comment, char, byte, string, byte string, C string, raw
string, raw byte string, raw C string, plus lifetimes as the deliberate
non-literal), each mapped in a table to the branch that consumes it. That table is
checkable against the Reference rather than against this file's own history.
All six evasions are permanent anti-vacuity arms, each paired with a control that
is the SAME literal differing only in the evading construct -- balanced braces for
the comment/string arms, a NON-nested block comment for the nesting arm, an
unprefixed `r#"..."#` for the byte/C raw arms -- so each pair measures its specific
evasion and not comments or raw strings in general. Each control is asserted BEFORE
its evasion so a run against the unfixed scanner shows the control PASSING in the
log rather than aborting before it is reached.
Two of the residual list's supporting MEASUREMENTS were also wrong, in the lane's
signature way -- a query whose shape cannot express the thing counted -- and both
are corrected. (1) The nested-block-comment absence was measured with the
line-oriented `rg -n --pcre2 '/\*.*/\*'`, which returns 0 even on a genuine
multi-line nested comment; it is now
`rg --pcre2 -U --multiline-dotall --json '/\*(?:(?!\*/).)*?/\*' crates/engine/src`
-> 0 match events, with a synthetic positive control (the `printf` that builds it is
in the doc) that the same query scores 1 on. The verdict is unchanged -- there is
no live instance -- but the evidence now supports it. (2) "No anchor literal in
`engine/src` contains a string at all" was false. Measured over the exact population
this scanner walks: of 326 `ZoneChanged` and 38 `TokenCreated` anchor lines, 20 and
10 respectively DO carry a string inside the literal's extent, and ZERO carry one
spanning a line break -- which is the truncating form the residual is actually
about. The old claim was false in the direction that stops a reader looking for the
multi-line case.
`FN_PREFIX_ALLOW_SET` also gains `pub(in crate::game) `: the census's own abort
told the reader to extend the allow-set with exactly the prefix another arm
hard-coded as its "unrecognised" fixture, so obeying the error turned one red test
into another. The prefix counts that back this up are re-measured AT THIS COMMIT'S
TREE and shipped with the command that produces them (a `find ... -exec awk` one-
liner printed in `FN_PREFIX_ALLOW_SET`'s doc, implementing exactly the rule
`top_level_fn_headers` applies): 14021 bare, 1813 `pub(crate) `, 1492 `pub `, 734
`pub(super) `, 24 `pub(in crate::game) `, and exactly FIVE distinct prefixes. An
earlier revision shipped the BASE-tip `1808 pub(crate) ` beside the HEAD-tip
`14008 bare`, a pair that existed at no commit; the four absolute counts are now
labelled as positive controls that churn on any top-level `fn` addition, while the
two facts the arms depend on -- five distinct prefixes, and `const ` not among them
(`rg -n --pcre2 '^const\s+fn\s' crates/engine/src` exits 1) -- are re-derivable
from the same commands.
A THIRD ANCHOR closes the enforcement gap this commit was otherwise shipping. The
two anchors above pin the two halves of the entry EVENT; the defect that actually
shipped in review rounds 9 and 10 was neither, and it had no tripwire at all. It
was a single just-created id published into the anaphora slot without the existence
predicate -- first into `last_created_token_ids` directly, then, once that was
guarded, into `pending.created_ids` one line BELOW the guard, which the drain's
wholesale assign republished onto ledger 3. Four enumeration sweeps each declared
that class closed, and its closure then rested on a query written in a doc comment
-- i.e. on the ENUMERATION this commit argues against, enforcing its own newest
authority. `every_single_id_anaphora_publish_lives_in_an_authority` is that query
made executable, and the word "executable" is doing real work, so here is exactly
how much of the query it covers. The query's mutator set is twelve verbs. Two of
them -- `.push(`, `.insert(` -- introduce exactly one element whatever their
argument, and CONJUNCTS 1-3 pin those to the production multiset
`{effects/token.rs: 2}`, `(2, 0)` bidirectionally, with both hits' enclosing
functions resolved to `record_last_created_token` and
`record_last_created_copy_batch_token`. Six -- `.clear(`, `.retain(`, `.truncate(`,
`.remove(`, `.drain(`, `.pop(` -- cannot introduce an element at all, which is a
property of the method rather than of this tree. The remaining four -- `.extend(`,
`.append(`, `.resize(`, `.splice(` -- introduce one id or many depending on an
ARGUMENT no text scanner can read: `.extend(std::iter::once(id))` publishes exactly
one id and is byte-indistinguishable in shape from `.extend(other_vec)`. CONJUNCT 4
therefore does not classify them; it pins their entire production population to
`(5, 0)` and the multiset `{counters.rs: 2, token.rs: 1, token_copy.rs: 2}`, so a
sixth in ANY OF THE EIGHT LISTED VERBS cannot arrive without a human reading its
argument. That scope is stated because an earlier revision claimed it without one:
the set then held four verbs, and `extend_from_slice`, `clone_from`, `resize_with`
and `extend_from_within` each compile, each publish exactly one id, and each scored
`(0, 0)` under it. They are listed now; any `std` mutator still unlisted is a named
fail-OPEN in the residual list, not a denied one. That converts an unbounded
fail-OPEN into a bounded fail-CLOSED, which is the discipline this file already
applies to an unreadable `/* ... */` tail and to an unrecognised `fn` prefix.
Conjunct 4 is a SEPARATE pin rather than four more verbs in conjunct 1, and the
reason is measured: folding them in takes the pin to `(7, 0)`, puts `counters.rs`
and `token_copy.rs` -- whose ABSENCE is conjunct 1's entire claim -- back into the
multiset, and adds a non-authority function
(`apply_create_token_after_replacement_with_created_ids`) to conjunct 2's list. Two
pins state the two facts; one pin would state neither. What is NOT covered, and is
named in the file's residual list with its own measured query and positive control:
an indexed assignment (`...[0] = id`), which shares the `[` tail with 26 indexed
READS (25 of them `#[cfg(test)]` fixtures, so counting `[` would move the pin to
`(3, 25)`, the production multiset to
`{database/encore_tests.rs: 1, effects/token.rs: 2}`, and the files touched from one
to seven, churning on every new test that reads the ledger), and a publish through
an alias or the UFCS `Vec::push(&mut state.last_created_token_ids, id)` form.
It shares `cfg_test_scoped_lines`, the file walker and the `top_level_fn_headers` /
`enclosing_fn` resolver with the other two anchors, but deliberately NOT
`literal_body`: it keys on a method CALL, not a struct-literal body, so it has no
brace scan and inherits none of the four fail-open truncations above. It is not
line-oriented either -- `call_tail` reads the next code token across line breaks,
because `token_copy.rs:321-323` writes `pending` / `.created_ids` / `.extend(...)`
on three lines and a line-oriented rule is exactly what under-counted this surface
before. Its one comment rule, `code_span`, is subtractive-only and each half is
guarded so it cannot delete code: a leading `/* ... */` is dropped only up to the
first `*/` (so `engine.rs`'s `/*tapped=*/` argument-label shape keeps its call), and
a trailing `//` only when no `"` precedes it (so a URL cannot hide a publish). Both
guards have their own control arm.
The anti-vacuity set is eight arms: the round-9 shape at both scopes; the round-10
multi-line buffer shape; a negative control over every non-publish tail form that
occurs in the crate, whose message now says only what is true of each form (a bulk
assign, `.clear`, `.clone`, `.len`, indexing and `.contains` cannot introduce an id;
`.extend` CAN, and is uncounted there solely because its argument is unreadable --
an earlier revision asserted the universal "none of them introduces an id", which
was false for exactly the verb the anchor was built for); a full-line prose negative
control; `.insert(` as the other single-element write; a fail-CLOSED arm proving an
unreadable `/* ... */` tail is COUNTED rather than skipped; a partition arm that
asserts `.extend(std::iter::once(id))` scores `(0, 0)` under the single-id
classifier and `(1, 1)` under conjunct 4's, and `.push(id)` the reverse, so the two
populations can never double-count or absorb each other's churn; and a
comment-position arm covering the trailing-`//` and full-line-`/*` prose shapes plus
the two controls that prove `code_span` cannot eat code.
The anchor was verified to go RED on the defect it exists to catch, by mutating a
scratch copy of `crates/engine/src` and re-running the extracted classifier:
reintroducing round 10's unguarded `pending.created_ids.push(id)` beside the guard
at `counters.rs` fails the pin, the same at `token_copy.rs` fails it, a bare
`state.last_created_token_ids.push(id)` (the round 8/9 shape) fails it, and a clone
written INSIDE `token.rs` that also deletes one authority's own push -- which keeps
the per-file multiset at 2 and passes conjunct 1 -- is caught by the function-scope
conjunct instead. Four mutants, four reds, each on the conjunct it is meant to
exercise.
Test provenance: the integration suite is salvaged from a halted predecessor
implementation. Its assertions carry inline REVERT-PROBE anchors naming, per
assertion, the mutation that should break it. Seven of the twenty-five anchors
were executed at this tip and carry verbatim failure points; the rest are stated
but not run here. The module header now defines that convention explicitly -- RUN
is an instruction to the reader, MEASURED means executed with a transcribed
failure point -- and names both sets by test, because the gap between "stated"
and "executed" is what let two anchors name failure points their recipes never
reached. It also records the recipe-scope failure mode behind that: an
authority-wide revert degrades the fixture's priming producer too and can trip an
upstream guard before the named assertion, which a site-isolated revert does not.
Every assertion added or altered in each review round's own delta was proved
two-sided per-assertion, at the tip that round produced: MUTANT-DROP (delete the
fix) and MUTANT-TRIVIALIZE (keep the shape, make the recorded index a meaningless
constant) must each fail that specific assertion, not merely the suite. That is a
claim about the assertions each round touched, not about all twenty-five at
once. The distinctness check between a realized copy entry and a
same-turn token batch reads both indices off their own emitted events, because
the dedup guard keys on the event-borne index; an earlier form compared a ledger
POSITION against event-borne STORED indices and so passed under both of its own
mutants. `ledger_index` stays positional on
purpose -- its other three callers compare it against the same entry's stored
index, which is the invariant "the recorder reported the slot it assigned", and
unifying it on the stored field would turn those into tautologies.
Assisted-by: ClaudeCode:claude-opus-5
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBattlefield-entry recording is centralized for no-origin entries. Token creation and deferred-entry paths use guarded shared helpers for zone-change events and creation ledgers. Integration tests cover authoritative indices, exact-once recording, deferred entries, and vanished objects. ChangesBattlefield-entry authority
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant CreationEffect
participant TokenEntryHelper
participant ZoneChangeLedger
participant EventStream
CreationEffect->>TokenEntryHelper: commit token entry
TokenEntryHelper->>ZoneChangeLedger: record no-origin battlefield entry
ZoneChangeLedger-->>TokenEntryHelper: return zone-change record
TokenEntryHelper->>EventStream: emit ZoneChanged and TokenCreated when object exists
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
needs-maintainer
Contributor
Author
|
🤖 AI text below 🤖 The automated review was rate-limited before it could start, so the green check above reflects an un-run review rather than a clean one. Re-requesting now that the window has reset. @coderabbitai review |
Contributor
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/engine/tests/integration/token_zone_change_index.rs (1)
48-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExecute at least one discriminating revert probe per site class, then record the measured failure point.
The header lists the whole
*_after_a_token_batch_fires_the_batched_triggerfamily as predictions. That family is exactly the set of seven new site discriminators this change adds (S1, S2, S3a unattached, S3a attached, S3b, S4, S5, S6). Each per-test doc carriesREVERT-PROBE (discriminating, RUN), andRUNis defined here as an instruction, not a result. So the central claim — that each routed site now ships a recorder-assigned index rather than the0placeholder — currently rests on unexecuted probes.The header itself names the failure mode this created before: two gift-token anchors named failure points their recipes never reached. Run each site-isolated mutant, confirm the named assertion is the FIRST failure, and replace the prediction with a
MEASUREDline carryingleft/right. Prefer the site-isolated recipe over the authority-wide one so the priming batch stays on the real authority and no upstream reach-guard dominates.As per path instructions: "A test must exercise the FAILURE path the fix prevents and drive the engine through its production pipeline".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/tests/integration/token_zone_change_index.rs` around lines 48 - 58, Execute the discriminating, site-isolated revert probes for every *_after_a_token_batch_fires_the_batched_trigger discriminator in the listed test family, driving each through the production pipeline and confirming the named assertion is the first failure. Replace the corresponding prediction entries in the module header with MEASURED records containing the observed left/right values, and use site-isolated recipes that keep the priming batch on the real authority.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/game/effects/incubate.rs`:
- Line 99: Remove the incorrect CR 608.2i citations from the documented
explanations at crates/engine/src/game/effects/incubate.rs:99-99 and 114-121,
crates/engine/src/game/effects/conjure.rs:204-216,
crates/engine/src/game/effects/gift_delivery.rs:163-167, and
crates/engine/src/game/effects/token_copy.rs:853-858 and 967-971. Preserve valid
trigger citations such as CR 603.2c and CR 603.6a, without changing the related
bookkeeping or trigger implementation.
In `@crates/engine/tests/integration/battlefield_entry_authority_census.rs`:
- Around line 936-941: Update the token access in the parsing logic around the
name extraction to safely handle `fn` being the final whitespace-delimited
token. Route the missing `tokens[at + 1]` case through the existing `Err` path
instead of indexing directly, while preserving the current name extraction
behavior when the token exists.
---
Nitpick comments:
In `@crates/engine/tests/integration/token_zone_change_index.rs`:
- Around line 48-58: Execute the discriminating, site-isolated revert probes for
every *_after_a_token_batch_fires_the_batched_trigger discriminator in the
listed test family, driving each through the production pipeline and confirming
the named assertion is the first failure. Replace the corresponding prediction
entries in the module header with MEASURED records containing the observed
left/right values, and use site-isolated recipes that keep the priming batch on
the real authority.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f91e5374-02f2-445f-aba5-7fc3460c81a7
📒 Files selected for processing (11)
crates/engine/src/game/effects/conjure.rscrates/engine/src/game/effects/counters.rscrates/engine/src/game/effects/gift_delivery.rscrates/engine/src/game/effects/incubate.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/effects/token_copy.rscrates/engine/src/game/zones.rscrates/engine/tests/integration/battlefield_entry_authority_census.rscrates/engine/tests/integration/loop_shortcut_offer_writer_census.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/token_zone_change_index.rs
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
requested changes
Aug 5, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Request changes — reviewed against head 3b2fbdb.
-
[HIGH] Supply the parse-diff artifact for this exact head. This PR changes engine authority in crates/engine/src/game/zones.rs:1390, but it has no coverage-parse-diff sticky comment bound to the current head. Please rerun or restore CI so the complete parse-diff sticky artifact identifies 3b2fbdb; without it, I cannot assess card-level parser coverage impact.
-
[LOW] Fail closed for a malformed top-level function header. In crates/engine/tests/integration/battlefield_entry_authority_census.rs:936, tokens[at + 1] can panic after accepting a column-zero line whose final token is bare fn. Use tokens.get(at + 1) and return the existing line-numbered Err diagnostic when the name token is absent.
I am not requesting the CR-annotation change raised in the automated review.
…try census `top_level_fn_headers` read `tokens[at + 1]` with no bounds check, so a column-0 header whose final whitespace token is the bare `fn` aborted the census with an index-out-of-bounds panic instead of its designed `Err` diagnostic — the wrong failure mode for what is really an unhandled header shape. Route the missing name token through the existing line-numbered `Err` path. Reachability measured before fixing: zero lines in `crates/engine/src` currently have this shape, so the panic was latent, not live. That zero carries a positive control — the same detector returns 2 on a synthetic corpus containing the shape. Test `bare_trailing_fn_header_is_an_error_not_a_panic` is non-vacuous and two-sided discriminating, both demonstrated rather than asserted: - NON-VACUOUS: fixtures use the prefixes `""` and `"pub "`, both present in `FN_PREFIX_ALLOW_SET`, so they clear the earlier unrecognised-prefix `Err` arm and actually reach the name-token read. An unknown-prefix fixture would return `Err` upstream and assert nothing about this line. - DROP (guard removed): arm 1 fails with `index out of bounds: the len is 1 but the index is 1`, reproducing the reported defect. - TRIVIALIZE (always `Err`): arm 2 fails on the happy path, so a blanket `Err` cannot satisfy the test. Raised independently in review by the maintainer and by the automated reviewer. Assisted-by: ClaudeCode:claude-opus-5
…CR 608.2i half The automated review read `CR 400.7 + CR 608.2i + CR 603.2c` on this header and asked for CR 608.2i to be deleted here and at five sibling sites, on the grounds that it "does not define battlefield-entry bookkeeping". The citation is correct and stays; the paragraph under it was the actual gap. `restrictions::record_zone_change` writes BOTH ledgers in one call: it appends the CR 400.7 zone-change row (whose length is the index allocator) and, when `to_zone == Zone::Battlefield`, calls `record_battlefield_entry` for the CR 608.2i entry row. Every routed site here reaches the battlefield, so both rules describe writes the call actually performs. Unlike its five siblings, this paragraph explained only the index half — its body was entirely zone-change index and batched-trigger dedup and never mentioned the entry ledger, leaving the CR 608.2i half of its own citation unexplained. Per the repo convention that a CR annotation's description is mandatory and grep output must be self-documenting, state the entry-ledger half explicitly instead of dropping a true citation. Comment-only; no behaviour change. Assisted-by: ClaudeCode:claude-opus-5
Contributor
Author
|
🤖 AI text below 🤖 Thanks — both items addressed. Pushed as two commits on top of the reviewed tip so the diff since your review is exactly 2. [LOW] Fail closed for a malformed top-level function header — fixed (
|
| probe | result |
|---|---|
| non-vacuity | fixtures use prefixes "" and "pub ", both in FN_PREFIX_ALLOW_SET, so they clear the earlier unrecognised-prefix Err arm and actually reach the name-token read |
| DROP (guard removed) | arm 1 fails: index out of bounds: the len is 1 but the index is 1 — reproduces the reported defect |
TRIVIALIZE (always Err) |
arm 2 fails on the happy path, so a blanket Err cannot satisfy the test |
1. [HIGH] parse-diff artifact for this exact head
I believe this one is already satisfied and was a page-timing artifact rather than a missing run — flagging the evidence rather than asserting it:
The coverage-parse-diff sticky comment for 3b2fbdbc2b253443f432ef214de8dd08dfc5d450 exists and is bound to that exact head — #7012 (comment) — stamped verbatim:
Generated for head
3b2fbdbc2b253443f432ef214de8dd08dfc5d450.Parse changes introduced by this PR
✓ No card-parse changes detected.
It was created at 2026-08-05T00:17:22Z; your review was submitted at 00:18:08Z, 46 seconds later — so it would not have been on the page when you loaded it. No rerun was needed for that head. Since I have now pushed, CI will regenerate the sticky against the new head ee4446fdb; if it does not appear there, say so and I will rerun it.
On the CR 608.2i annotations
Noted that you are not requesting that change, so I have not made it — the citation stays at all six sites.
For the record, since it may come up again from the automated reviewer: the citation is load-bearing because restrictions::record_zone_change writes both ledgers in one call —
let turn_zone_change_index = state.zone_changes_this_turn.len(); // CR 400.7 row + index
state.zone_changes_this_turn.push_back(record);
if to_zone == Zone::Battlefield { record_battlefield_entry(state, object_id); } // CR 608.2i row— and every routed site reaches the battlefield, so both rules describe writes the code actually performs.
One genuine gap did fall out of that review, and it is the only reason the second commit exists: unlike its five siblings, the incubate.rs paragraph explained only the index half, so its own CR 608.2i half sat unexplained. ee4446fdb is comment-only and states the entry-ledger half. Happy to drop that commit if you would rather the annotations be untouched in this PR — it changes no behaviour.
On the automated nitpick re: revert probes in token_zone_change_index.rs
Not actioned, because the file already carries the disclosure it asks for. The module header defines the vocabulary in bold — "RUN in an anchor tag is an INSTRUCTION to you, not a claim that anyone ran it. Only anchors that also say MEASURED were executed" — then lists the MEASURED anchors with their left/right values and separately enumerates "STATED BUT NOT EXECUTED here — treat as predictions until run", which explicitly includes the *_after_a_token_batch_fires_the_batched_trigger family the comment names. Executing that whole family is a reasonable enhancement, but it is a heavy lift and the current labelling is not overclaiming. Say the word if you want it done in this PR.
Verification at ee4446fdb
cargo fmt --all --checkclean.battlefield_entry_authority_census— 7/7 pass (the 6 existing plus the new one).- Gate A re-run with the base passed explicitly:
Gate A PASS head=ee4446fdb… base=b654513cb…. (The pre-commit hook's own run defaults to this fork'smain, which is stale at4b34e5465, i.e. a wider and more conservative range.) - Not rebased: every number in the main commit message is labelled against base
b654513cb, and a rebase would invalidate that prose without changing the tree.
Contributor
Author
|
🤖 AI text below 🤖 One addendum against my own comment above, so it is on the record rather than found later. I said CR 608.2i is the established citation for this ledger. That is true of the ledger's references, but not yet of its definition. Three definitional sites still read
So anyone grepping It is not new breakage: all three are verbatim present at the PR base Deliberately not fixing it here: you have said you are not requesting the CR-annotation change, and finishing that migration would pull two untouched files into the diff. Flagging it as the concrete follow-up those disclosed counts refer to. Say the word if you would rather it land in this PR. |
Contributor
Author
|
🤖 AI text below 🤖 Both requested items are now in place at head [HIGH] parse-diff artifact — regenerated and now bound to the current head, stamped verbatim:
Worth noting for the future, since it likely explains what you hit: this is a sticky comment — the bot rewrites one comment in place (still #7012 (comment), created [LOW] census fail-closed — fixed in CI at this head: 12 pass, 1 skipping, 0 failing. The only judgement call left for you is the second commit, |
matthewevans
approved these changes
Aug 5, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Approved at current head ee4446f: focused engine bug fix; current-head parse-diff is no-change and required CI is green.
Merged
4 tasks
This was referenced Aug 6, 2026
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
2 participants
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.
🤖 AI text below 🤖
Summary
Six battlefield-entry sites emitted
ZoneChanged { from: None, to: Battlefield }carrying a placeholderturn_zone_change_indexof0, so the CR 603.2c batched-trigger collector — which dedups on(definition_ref, turn_zone_change_index)read off the event — aliased every later same-turn entry onto occurrence0and swallowed its enters-the-battlefield fire. This fuses the record and the emit into one authority,zones::record_and_emit_entry_from_no_zone, so the emitted record carries this turn's real index.The fix is one guard in the shared authority rather than six guards at six call sites: three successive sweeps of the call sites each missed a route and each shipped a claim that the sweep was complete, so the class was made unrepresentable instead of enumerated.
Files changed
11 files, +4193 / −284 (production
engine/srcis +902 / −229; the rest is the behavioural suite and the structural census).crates/engine/src/game/zones.rs— the fusedfrom: None → Battlefieldrecord+emit authoritycrates/engine/src/game/effects/counters.rs— routes 3 sites; deletes the private clonepush_token_entry_events; adds the event/ledger coherence testscrates/engine/src/game/effects/token.rs— foldsrecord_committed_token_entry+push_token_entry_events_for_recordinto the authority; hoists the object-existence guard into the shared emittercrates/engine/src/game/effects/token_copy.rs— routes 2 sitescrates/engine/src/game/effects/gift_delivery.rs— routes 1 sitecrates/engine/src/game/effects/conjure.rs— routes 1 sitecrates/engine/src/game/effects/incubate.rs— routes 1 sitecrates/engine/tests/integration/token_zone_change_index.rs— the behavioural suite (28 tests)crates/engine/tests/integration/battlefield_entry_authority_census.rs— new structural census (6 tests, three anchors)crates/engine/tests/integration/loop_shortcut_offer_writer_census.rs— visibility only (2 lines), so the census can reuse its classifiercrates/engine/tests/integration/main.rs—modregistrationTrack
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
CR 111.1, CR 111.2, CR 301.5, CR 400.7, CR 603.2c, CR 603.6a, CR 608.2i, CR 614.12a, CR 616.1, CR 702.174a, CR 704.3, CR 704.5f, CR 707.2
This list is derived from the diff, not recalled — it is every distinct
CRannotation on an added line inb654513cb..HEAD. Each verified two-step againstdocs/MagicCompRules.txt: the number RESOLVES and its text SUPPORTS the site. The two load-bearing ones are CR 603.2c ("it can trigger repeatedly if one event contains multiple occurrences") and CR 603.6a ("Each time an event puts one or more permanents onto the battlefield"), which are exactly the rules the measurements exhibit.Two further numbers, CR 111.10 and CR 403.3, also occur on added lines but ONLY inside negative citations — text of the form "NOT CR 111.10, which is …" and "CR 608.2i, not CR 403.3: …" — explaining why each is the wrong rule for the site. They are not annotations and are excluded from the list above. (An earlier revision of this body asserted "CR 403.3 is deliberately absent, 0 occurrences on added lines". That was literally false — the correct claim is no CR 403.3 annotation, which is what the retag actually achieved.)
CR 403.3 is definitional — "Permanents exist only on the battlefield" — and does not describe an entry-time characteristics snapshot kept for later look-back queries. CR 608.2i ("Some effects look back in time and require information about previous game states") is what the pre-existing
battlefield_entry_record_foralready cites, so the new annotations match it. CR 603.10's look-back-for-triggering rule does not apply either: all seven production consumers ofbattlefield_entries_this_turnare conditions, quantities, or a static-analysis probe evaluated during resolution — none is a trigger condition.Verification
All four run locally at the committed head
3b2fbdbc2:cargo fmt --all -- --check— rc=0, no diffcargo clippy --workspace --all-targets -- -D warnings— rc=0, cleancargo test -p phase-engine— rc=0, 23049 passed, 0 failed, 15 ignored, 5 binaries (18496 + 12 + 9 + 4532 + 0)cargo check --workspace --all-targets— rc=0The test-count delta is arithmetically closed rather than asserted: 23049 − 22979 (the round-6 candidate this change's equivalence probe was executed at) = 70 = 6 tests added by later review rounds + 64 brought in by rebasing onto
b654513cb. The 64 splits lib +31 / integration +33, matching the per-binary deltas.Base note. This branch is based on
b654513cb, andupstream/mainhas advanced 3 commits since (currently460e204ce).git merge-treeagainst that newer tip is clean; the only file both sides touch is the integration suite'smodregistration. The branch was deliberately not re-rebased, because every count in the commit message is labelled againstb654513cband a second rebase would invalidate them again for no reviewable gain. Note also thatGate A's default base ismerge-base origin/main HEAD— on a fork whosemainis stale that resolves to the old pre-rebase base, so the line below was produced by passing the true base explicitly.Each verdict is read off the gate's exit code, not off a log grep; the greps are evidence about the verdict, not the verdict. Counts were taken binary-safe (
-aP) on non-empty stdout, because the localgrepis ugrep 7.5 and prints nothing rather than0for-con a file it classifies as binary — a silent blank would otherwise read as a clean zero.The census's six tests are confirmed by name in the run log, which is the discriminating check — a bare pass count is equally consistent with the anchors silently failing to register while unrelated tests appear:
every_from_none_battlefield_entry_construction_lives_in_the_authorityevery_token_created_construction_lives_in_the_single_emitterevery_single_id_anaphora_publish_lives_in_an_authoritythe_census_resolver_keys_on_cfg_scope_and_on_the_literal_from_fieldthe_token_created_resolver_keys_on_cfg_scope_and_on_field_completenessthe_anaphora_publish_resolver_keys_on_cfg_scope_and_on_the_call_tailGate A
Gate A PASS head=3b2fbdbc2b253443f432ef214de8dd08dfc5d450 base=b654513cb391203fa1add4dbc797dc21aa9f429e
Anchored on
crates/engine/src/game/zones.rs(move_to_zone) — the pre-existingSome(from)counterpart at the same seam: it callsrestrictions::record_zone_change, writes the returned index into the record, and only then pushesGameEvent::ZoneChanged. The new authority is that same fused shape for thefrom: Nonecase; the new function's doc cross-references it explicitly.crates/engine/tests/integration/loop_shortcut_offer_writer_census.rs(cfg_test_scoped_lines,rs_files) — the pre-existing structural-census pattern in this repo, and the in-repo precedent for a hand-rolled source scanner living in a test. The new census calls both rather than reimplementing a scope classifier or a file walker; the only change to that file is widening those two topub(super)(2 lines, no behaviour). Itsclassify's measured comment-line exclusion rule is inherited by citation.Final review-impl
Final review-impl PASS head=3b2fbdbc2b253443f432ef214de8dd08dfc5d450
Claimed parse impact
None.
Scope Expansion
One deliberate widening, declared rather than claimed away — and three adjacent defects disclosed rather than fixed.
DECLARED WIDENING. The object-existence guard sits in the shared emitter
token.rs::push_committed_token_entry_events, not at each call site, so it covers all eight callers — including the non-deferred creation paths, which is a wider blast radius than the per-caller guard it replaced. That is deliberate, for the reason in the Summary. It is measured, not assumed: an equivalence probe assertingrecord.is_some() == state.objects.contains_key(..)at every invocation across the whole suite found nothing reachable that depends on the old behaviour.Deliberately NOT repaired here, disclosed rather than fixed:
match_token_createdskips the CR 111.2 controller filter for a gone object. It resolves the controller by reading the live object, andvalid_card_matchesshort-circuitsNone => truewithout reading state, so a vanished token can match for a NON-MATCHING controller. Suppressing the emit makes event and ledger agree; the class-wide fix is to resolve the controller through last-known information the wayvalid_card_matches_with_lkialready does for the card filter. That touches a shared matcher, so it is follow-up — and the emitter is now its single adoption point.TokenCreatedat all.incubate.rsandcounters.rs'sInjectPredefinedTokenAbilitiesarm callrecord_token_createdand then the authority DIRECTLY, so they write both ledgers and never reach this emitter;TriggerMode::TokenCreatedtherefore cannot fire for Incubator tokens even though CR 701.53a describes incubating as creating a token. Pre-existing and unchanged —b654513cbbehaves identically. Routing them through the emitter would reorder the ETB events theInjectPredefinedTokenAbilitiesarm exists to control.counters.rssacrifice_athas zero test coverage. Pre-existing; untouched by this change; disclosed so it is not mistaken for coverage this PR added.Also ledgered as follow-up, found while auditing this change and out of scope for it: the remaining pre-existing CR 403.3 citations in files this commit does not modify (8 in
game/restrictions.rs, 5 intypes/game_state.rs) need the same entry-ledger triage; andtrigger_matchers.rscarries one wrong CR number at its token-created matcher.Validation Failures
None.
CI Failures
None.
Reviewer orientation (disclosed up front)
Five things a reviewer will notice, named here rather than left to be discovered:
battlefield_entry_authority_census.rsscansengine/srcfor struct-literal extents, skipping strings, char literals, raw strings and nested block comments. That is unusual, and the in-repo precedent isloop_shortcut_offer_writer_census.rs, whose classifier and file walker this census reuses rather than duplicates. The census is described honestly as a source-text tripwire against an accidental clone, not a proof that one cannot be written; its residual ceilings — each fail-OPEN or fail-CLOSED, each measured — are enumerated in the module header.std.is_ambiguous_mutatorbounds eight named mutators; anystdmutator not listed is a named fail-OPEN, recorded in the residual list rather than denied. An earlier revision claimed a new ambiguous publish "cannot arrive without a human reading its argument" — that was a false universal, andextend_from_slice,clone_from,resize_withandextend_from_withineach disprove it. They are listed now. This was disclosed by us, not found in review.counters.rsis +516 / −53. The bulk is the event/ledger coherence suite, not the routing; the routing itself is three call sites.ZoneChangedconstructions,TokenCreatedconstructions, and single-id anaphora publishes each get an exact pinned production multiset. The third was added after review observed that the first two pin the entry event while the defect that motivated the guard was in the anaphora publish.Review lineage (disclosed)
This is candidate 16 of its lineage, across 17 review rounds; every round's independent
/review-implfindings were fixed and each fix then MEASURED rather than asserted. The rounds that changed shipped behaviour or shipped claims:from: Noneinside a fixed six-line window, and two compiling evasions slipped through: field-init shorthand, and a multi-linerecord:initializer written first, which pushesfrom: Nonepast the window. It now scans each literal's own brace extent and classifies thefromfield. Both evasions are pinned as permanent anti-vacuity arms.TokenCreated→ single emitter), keyed on field COMPLETENESS because the emitter is written entirely in field-init shorthand.counters.rsconstructed aResolutionFrame::CopyToken, so the buffer wasNonein all five arms and the criterion could not discriminate. Replaced with a demonstrated reachability proof.b654513cbre-based every BASE-relative claim in the message.git rebaseexiting 0 says the code merged; it says nothing about whether the prose still describes the tree. Six numbers were re-measured and corrected; several suspected drifts were disproved by measurement and left alone. The audit also caught one genuine CR two-step failure in this diff — an annotation citing CR 603.7 (delayed triggered abilities) and CR 701.36a (populate) for a function that publishes a token into two ledgers. Both resolved but neither text supported the site; corrected to CR 111.1 + CR 707.2.Summary by CodeRabbit
Bug Fixes
Tests