Skip to content

fix(#826): make collision() and usability() fail to compile together on a new state variant - #837

Merged
djhenry merged 5 commits into
mainfrom
fix-826-collision-arms
Aug 1, 2026
Merged

fix(#826): make collision() and usability() fail to compile together on a new state variant#837
djhenry merged 5 commits into
mainfrom
fix-826-collision-arms

Conversation

@djhenry

@djhenry djhenry commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Closes #826.

Round 2. All three blocking findings were false or incomplete sentences, not wrong code — the
reviewer reproduced the load-bearing E0004 experiment exactly (5 sites before with collision()
absent, 7 after including :144 and :679) and mutation-checked the roll call in both directions
plus a dead-code wrap. Every finding is accepted; the disagreement he ruled on went my way and is
now recorded on #826 itself.


Part 1 — the compiler catches it now, not a test

ZoneAssetState::collision matched with _ => None; usability matched exhaustively.
usable_collision pairs them — usability decides whether to bless the state, collision decides
which grid to hand back — so the wildcard broke that pairing silently for any variant added later.

Before/after, measured

Before. Added a fifth variant (the enum has four: Idle, Pending, Ready, Failed):

ProbeRefreshing { zone: String, collision: Arc<Collision> },

cargo check -p eqoxide-nav --all-targets demanded arms at five sites and did not mention
collision():

error[E0004]: non-exhaustive patterns: `&ZoneAssetState::ProbeRefreshing { .. }` not covered
   --> crates/eqoxide-nav/src/zone_assets.rs:105:15   (tag)
   --> crates/eqoxide-nav/src/zone_assets.rs:118:15   (zone)
   --> crates/eqoxide-nav/src/zone_assets.rs:132:15   (detail)
   --> crates/eqoxide-nav/src/zone_assets.rs:323:24   (usability)
   --> crates/eqoxide-nav/src/zone_assets.rs:422:15   (Debug::fmt)

Filling in exactly what the compiler asked for compiled clean, and:

F826-PROBE usability          = None
F826-PROBE state.collision()  = None
F826-PROBE usable_collision() = Err(Idle)
F826-PROBE grid really exists = true

usable_collision_agrees_with_usability_for_every_state was run against that tree and passed.

After. Wildcard replaced with explicit arms. The identical probe now reds seven sites, including
collision():

   --> crates/eqoxide-nav/src/zone_assets.rs:144:15
144 |         match self {
    |               ^^^^ pattern `&ZoneAssetState::ProbeRefreshing { .. }` not covered

Both probes reverted from cp -p copies, verified byte-identical by md5sum. No git stash, no
git restore.

There is no runtime test for this, and none was written

The property is "a variant that does not exist yet is classified consistently by two functions". A
runtime test can only construct variants that exist — which is why the pre-fix probe left the pin
green while the bug was live. The compile-time proof stands on its own.


Round-2 changes

B1 (accepted) — "unrepresentable" was false, and measured false

The rustdoc at :139 claimed the fix "makes that state unrepresentable". The reviewer built that
state on the post-fix tree
— new variant carrying a live grid, all seven demanded arms filled in,
classified usable in usability and None in collision — and it compiles, yields
usability=None / collision()=None / usable_collision()=Err(Idle) over a live grid, with the
crate suite green (239 passed; 0 failed; 16 ignored).

He is right, and what makes this the worst of the three is that the PR body and the test comment
already said it correctly
— the rustdoc was the odd one out, and it was the strongest and most
prominent version. It is now scoped to what was achieved: the omission is unrepresentable; an
inconsistent pair is not, and the roll call is named as the second (also partial) line of
defence. The "which is the only place this can be caught" clause is gone — self-contradicted by the
roll call this same PR adds, and by tag/zone/detail/Debug.

B2 (accepted) — "a sixth variant" was wrong in three tracked places

ZoneAssetState has four variants; the probe's was the fifth. Fixed at
zone_assets.rs:128, :134 (the one inside the "Measured, not reasoned" paragraph) and the
roll-call comment — :687 on the current head. My round-2 body cited :668, which was stale;
the reviewer's :684 was right for round 2, and the round-3 guard rewrite moved it again.

The miscount was inherited: #826's own "five current state variants" is wrong, counting the
test's five states where readyA/readyB are both Ready. Recorded on the issue so it does not
propagate again: #826 (comment)

B3 (accepted) — the residual disclosure named one wildcard; there are three

site a new variant silently becomes status
crates/eqoxide-nav/src/zone_assets.rs:237 (status()) status: null — a missing progress line disclosed in round 1; reasoning accepted by the reviewer
crates/eqoxide-http/src/observe.rs:52 (terrain_meshes) terrain_meshes: null in the agent-visible block found by the reviewer, assessed benign — I have NOT independently verified that, and do not claim it
src/app.rs:98 (lost_load_zone) the lost-loader watchdog never fires, so an in-flight state sits in flight forever behind a frozen status line not benign. Same agent-honesty class as #826. Filed by the reviewer as #838

I confirmed both unnamed sites at source. #838 is deliberately NOT fixed heresrc/app.rs is
held elsewhere and it is separately scoped.

N3 (taken) — something in the merged tree now preserves the property

Before this, the only guard against _ => None returning to collision() was the rustdoc above it:
E0004 output is not a test and CI cannot re-run it. Added
collision_matches_only_named_variants_no_wildcard_arm, a lexical pin requiring every arm of that
one function body to be a Self::… pattern.

Normally #799 (a source-text pin proves a call is written, not reached) would sink this. It does
not apply here, and that is measured rather than argued: the property is the text — "the body
contains no wildcard arm" is what rustc reads to decide whether E0004 fires — and the reviewer
demonstrated the same point on the roll call by wrapping it in if false { … }, which still
produced E0004. There is no written-vs-reached gap to exploit.

Three controls, all run — this guard was defeated twice in round 2 and rewritten; see Round 3
below for what replaced it
:

  • POSITIVE — wildcard restored in collision(): RED, and the message quotes the offending
    source line (_ => None,).
  • REACH (the fix(#760): pin the health clock in test fixtures so a handler's HTTP status cannot depend on machine load #778 failure mode: a scanner that silently covers nothing) — signature reformatted
    across lines so the anchor would not be found: RED on the anchor assert, not a silent pass.
  • SCOPE — obtained for free: status() in this same file still has _ => None and the pin is
    green, so the scan is scoped to collision's body rather than matching the file.

Its limits are written beside it: one function body only; lexical, so it cannot tell a correct
arm from an incorrect one
(Self::New {..} => None for a variant carrying a live grid passes it
and re-opens #826); and formatting-dependent.

N1, N2 (taken) / N4 (declined)

  • N1 — "satisfies that rule unconditionally" was overstated: the is_empty() short-circuit
    means an empty scene_zone starts no reload at all. Scoped to non-empty scene_zone, with the
    mechanism that actually carries the empty case named (usability reads player_zone, not
    scene_zone, and refuses an empty one with PlayerZoneUnknown). Also added the reviewer's
    stronger argument, which the comment did not have: scene.zone and player_zone are both copies
    of one gs.world.zone_name, so a case-only divergence cannot arise from the data flow at all.
  • N2 — the "this fallback is unreachable" comment, which is the exact sentence a new variant
    falsifies, now cross-references zone_assets: collision()'s _ => None wildcard can make usable_collision's documented-unreachable arm reachable (#803 follow-up) #826 and states what the exhaustive arms do and do not keep true.
  • N4 declined, with the reason stated rather than silently ignored: the fix for a library
    comment naming a private fn in the downstream binary crate would be to move or re-export the
    function, which is a bigger change than the comment is worth. The reference can rot; nothing here
    makes it not rot.

Part 2 — why the case-sensitivity asymmetry is safe

zone_needs_reload compares zone names exactly; usability uses eq_ignore_ascii_case. The
reasoning is written at zone_needs_reload with a pointer from usability.

The reviewer ruled on my deviation from #826's text in my favour (attack 5): making
zone_needs_reload case-insensitive drops the pair to parity, it does not destroy the safety —
the eagerness is a margin, and the margin is what is lost. #826's stronger wording is the wrong
one. Both the ruling and his extra supporting argument are now on the issue.



Round 3 — the guard did not lex, and two labels were wrong

M1 (accepted) — the pin's headline was false, and the fix is a real lexer, not a split("//")

The reviewer defeated the round-2 guard twice, both through comments, both leaving a live wildcard
with the pin GREEN and no compiler warning
:

_ => None,  // Self::Failed                       // M3: `Self::` supplied from inside a comment
Self::Failed {..} => None, // ... and fn: }}      // M5: a comment brace truncated the brace walk
#[allow(unreachable_patterns)] _ => None,         //     …leaving the wildcard outside the scan

He is right about the mechanism and right about the framing: the guard did no comment stripping
anywhere
, so the gap was not the usual written-vs-reached (#799) — it was scanned-text vs
compiled-text
. rustc and my scanner were reading different documents.

What changed. The guard is now a pure function, audit_collision_arms(src) -> Result<(), String>,
with strip_comments in front of it. That makes every evasion ordinary test data rather than a
one-off source mutation that lives in a review comment: collision_arm_audit_rejects_the_evasions_measured_against_it
asserts M1, M2, M3, M5, M6, plus M7/M8 (the same two evasions moved into block comments)
and a "fifth arm line, no wildcard" case are all Err, and a new evasion costs three lines.

String literals are HANDLED, not scoped around — stating which, as required. A naive // strip
is unsound in this very file: // occurs inside string literals, and cutting there deletes the
closing quote, inverts string state for everything after it, and silently moves both the anchor and
the walk. strip_comments is a state machine over strings (with \ escapes, including the
\-at-end-of-line continuations this module uses), char literals distinguished from lifetimes
('"' and '\\' are the two shapes that would otherwise invert string state — and both occur in
the new code itself), and nested block comments. A case asserting that a // inside a URL string
still passes is what shows this is not a split("//").

The one construct it cannot lex — a raw string — is a refusal, not a guess. The honest form
of the guarantee is now "either the text was something this scan can lex, or the test failed", and
that sentence is in the rustdoc.

The stripper is itself a scanner, so it has its own reach control, as required: one canary
comment per comment syntax (line and block), each concat!-assembled so the joined literal
occurs exactly once in the file, asserted present in the raw text and absent after stripping,
plus stripped.len() < SRC.len(). If the stripper ever no-ops, that fires instead of the pin
silently reverting to round-2 behaviour with more confident prose on top.

Both probes re-run as REAL source mutations against the fixed guard — both RED:

probe result message
M3_ => None, // Self::Failed RED "ZoneAssetState::collision must match every variant BY NAME (#826). This arm does not: _ => None," — note the arm it quotes has already had the comment stripped, which is the fix doing the work
M5}} in a trailing comment + #[allow(unreachable_patterns)] _ => None, RED same check, quoting #[allow(unreachable_patterns)] _ => None,

Each mutation was applied to collision's real body, restored from a cp -p snapshot verified by
md5 (d417f0b3…), and touched — and each run shows exactly one Compiling eqoxide-nav line,
so neither result came from a cached binary.

M2 (accepted) — two measured-false sentences at usable_collision

  • "…agrees_with_usability_for_every_state asserts it never fires" → "asserts it does not fire for
    the five states that test enumerates by hand
    — not for all states, which no runtime test can
    reach."
  • "The exhaustive arms keep the claim honest by forcing both functions to be edited together" → the
    arms do not keep the sentence true: Self::New {..} => None alongside a usability that
    blesses New compiles and falsifies it again. What they buy is that the author of a new variant
    is made to read it
    , because the compiler will not let them skip collision.

M3 (accepted) — one word

"Nor is compiling the only catch point" → "Nor is collision the only compile catch point".

M4 (accepted) — a floor was wearing a reach control's label

arm_lines.len() >= 4 was labelled a reach control. It is a floor — the exact defect class this
PR exists to close, and I wrote it while closing it. It is now an equality against
COLLISION_VARIANTS.len(), and it is labelled a consistency check, explicitly not a reach
control
, because it is still a count over already-scanned text and does not prove the scan reached
the end of the match.

Proved by mutation, as asked: the "fifth arm line, no wildcard" case (Self::Idle if false => None, added, every variant still named, no wildcard anywhere) is asserted Err — it passes the
per-arm check and the variant check, and only the equality catches it. Measured by restoring the
floor on the pushed head (!=<, nothing else changed): the suite goes RED on exactly that
case
"fifth arm line, no wildcard: this must be REJECTED, and it was accepted" — with one
Compiling eqoxide-nav line. Mutation reverted, file byte-identical to ddb470a.

Non-blocking note — the POSITIVE control message, now measured

The reviewer noted that the advertised positive-control message did not fire for realistic wildcard
restorations: with the variant check running first, a wildcard replacing a named arm was reported
as "the brace walk truncated, or a variant was renamed", which is not what happened. Fixed inside
the guard rewrite rather than as extra diff
: the per-arm check now runs before the variant
check. Both M3 and M5 above are diagnosed by the wildcard message, quoting the offending line — that
sentence in this PR body is now true, and it is true because it was measured, not because it was
edited.

Test run

cargo test --workspace --locked --no-fail-fast, stdout/stderr captured separately, over the pushed
head (ddb470a; the tested working tree and git show HEAD:crates/eqoxide-nav/src/zone_assets.rs
are the same bytes, md5 d417f0b3…).

figure measured
compile sentinel (compile only, NOT completion) Finished `test` profile [unoptimized + debuginfo] target(s) in 3m 11sexactly one
completion process exit 0 + last non-empty stdout line is a complete test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
^running [0-9]+ tests?$ headers vs ^test result: lines 55 vs 55 (also 55 Running/Doc-tests target lines in stderr)
non-canonical test result: lines / FAILED / failures: blocks 0 / 0 / 0 (55 of 55 match the full canonical ^test result: (ok|FAILED)\. N passed; N failed; N ignored; N measured; N filtered out; finished in)
targets with nothing to run 14, on ^test result: ok\. 0 passed; 0 failed; 0 ignored — agreeing with 14 on ^running 0 tests$. The anchored bare ^test result: ok\. 0 passed; reads 18 (4 all-#[ignore]d targets). The unanchored 0 passed; 0 failed; 0 ignored reads 16 — it matches 20 passed; and 10 passed; as substrings and counts two healthy targets as empty; 14 is correct
passed + failed + ignored + filtered 1833 + 0 + 47 + 0 = 1880 = header sum 1880

Count reconciliation, by name. +1 vs round 2 (1879 → 1880), all of it in the eqoxide-nav lib
target (255 → 256). Diffing the extracted zone_assets::tests:: name lists between the two runs:
one added, none removed
zone_assets::tests::collision_arm_audit_rejects_the_evasions_measured_against_it.

Process note. My first round-3 workspace attempt hit my own 10-minute tool cap mid-compile and
its local end was killed while the remote cargo kept running — the trap where a build can look
finished and is not. I did not reuse that capture or its path: I polled until the remote build
directory was quiet, re-ran to a new file, and the figures above are from that single clean run
(one Finished, no truncated name lines, 256 distinct names in a 256-test target).

N5 — my round-1 "two Finished lines" figure was wrong, and here is the cause

Not an environment difference. My round-1 capture was two of my own overlapping rbuild
invocations writing to the same file
: the first was auto-backgrounded and its remote cargo kept
running after its local end died, and the retry re-opened the same path with > (same inode, both
processes writing at their own offsets). The round-1 stderr contains two complete
compile-and-run sequences, and its stdout contains overwritten, truncated test-name lines
(test collision::testtest, test comtest). The reviewer's "one" was right.

This means my round-1 figures came from a contaminated file, and I am flagging that rather than
letting it stand.
They happen to be correct — the reviewer independently reproduced 55/55, 0
non-canonical, 14/18 and 1878 on his own clean run, deriving them before comparing — but I could not
have known that from my own log. Round 2 was run after confirming the remote build directory was
quiet, and its capture is single and clean (one Finished, zero truncated lines, 255 distinct names
in a 255-test target).


NOT verified, and not claimed

  • No live client run. Nothing here changes runtime behaviour for the four current variants — the
    explicit collision() arms return exactly what the wildcard returned.
  • The fix does not make an inconsistent pair unrepresentable. Measured by the reviewer: a
    variant classified usable in usability and None in collision compiles and re-opens zone_assets: collision()'s _ => None wildcard can make usable_collision's documented-unreachable arm reachable (#803 follow-up) #826's
    symptom with the suite green. This is the B1 correction and it is now the rustdoc's own wording.
  • The N3 pin is lexical and covers one function body. It cannot distinguish a correct arm from
    an incorrect one, and says nothing about the other three wildcards on this enum.
  • The roll call does not prove states covers every variant. It forces a read.
  • Part 2 is a written-down argument, not a mechanised one. Nothing enforces "the reload trigger is
    at least as eager as the bless test"; a future edit can violate it and compile.
  • observe.rs:52 (terrain_meshes) is reported as the reviewer assessed it — benign. I did not
    independently verify that.
  • lost_load_zone's _ => None silently classifies a new in-flight ZoneAssetState as 'leave it alone' — the lost-loader watchdog then never fires (#826 class) #838 (lost_load_zone) is real and left open; this PR does not touch it.
  • Mutation coverage of the new guard is the eight synthetic cases plus two real source
    mutations
    (M3, M5). It is not exhaustive: an evasion nobody has thought of is not covered, and
    the guard's own honest limit is that it is a scan bounded by refusal, not a parse.
  • strip_comments is not a Rust lexer. It handles strings, char literals vs lifetimes, and nested
    block comments; it does not handle raw strings or byte-string prefixes, and has_raw_string
    is deliberately conservative — the text "r" (a string whose last character is a standalone r)
    is reported as a raw-string opener. That direction of error is a loud refusal; the other direction
    would be a silent mis-scan.
  • test invocations fetch no artifact, so there is no fetched line to check on either run.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV

…on a new state variant

`ZoneAssetState::collision` matched with `_ => None` while `usability` matched
exhaustively. `usable_collision` pairs them: `usability` decides whether to bless
the state, `collision` decides which grid to hand over. The wildcard broke that
pairing silently for any state variant added later.

Measured, not reasoned. On the pre-fix tree, adding a sixth variant
`ProbeRefreshing { zone, collision: Arc<Collision> }` that carries a real grid:

  * the compiler demanded arms at FIVE sites -- tag(), zone(), detail(),
    usability() and the hand-written Debug impl -- and did NOT mention
    collision(), because of the wildcard;
  * filling in exactly what the compiler asked for made the crate compile clean;
  * the result at runtime was
        usability          = None        (i.e. USABLE)
        state.collision()  = None        (the wildcard)
        usable_collision() = Err(Idle)   (over a live grid)
    -- the documented-unreachable `ok_or(NotUsable::Idle)` fallback firing, turning
    a usable grid into a refusal with no diagnostic;
  * and `usable_collision_agrees_with_usability_for_every_state` stayed GREEN the
    whole time, because its hand-written `states` vec did not know the variant
    existed.

With the wildcard replaced by explicit arms, the identical probe reds collision()
too: E0004 now names seven sites including `zone_assets.rs:144` (collision's
`match self`). The probe was reverted and the file verified byte-identical.

This is a compile-time property. There is no runtime test for it -- a test can
only observe variants that exist, and the failure mode is a variant that does not
exist yet. No test was invented to stand in for the compiler.

Also:

  * a compile-time roll call inside the existing pin, so adding a variant reds the
    TEST as well and the author is forced to look at the hand-written `states` vec.
    Documented for exactly what it proves (a forced read) and what it does not
    (that the vec actually covers every variant).
  * the pin is KEPT, with a note saying why it is not redundant: #826 stops one
    function from ignoring a variant the other classified, but not the two from
    being filled in inconsistently, which still compiles.
  * #826 part 2: write down why the case-sensitivity asymmetry is safe.
    `zone_needs_reload` compares zone names EXACTLY, `usability` compares them with
    `eq_ignore_ascii_case`. The rule is one-directional -- the reload trigger must
    be at least as eager as the bless test -- and exact comparison is the most
    eager there is, so it holds unconditionally, at a bounded cost (a case-only
    difference can only cause a spurious reload: Pending, then an honest 503).
    The comment says not to "fix" this by making the reload trigger
    case-insensitive, and states the accurate reason: it does not tighten
    anything, it drops the pair to parity and makes safety depend on an
    unenforced assumption -- and any comparison MORE lenient than `usability`'s
    breaks the rule outright and re-opens the stale-ready lie.

Full workspace suite green: 55 running-headers vs 55 `test result:` lines,
1831 passed / 0 failed / 47 ignored / 0 filtered (= 1878, matching the header sum),
0 FAILED, 0 non-canonical result lines. No test was added or removed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
@djhenry

djhenry commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

Independent acceptance review — PR #837 (issue #826)

Reviewed on a separate detached worktree at 2e98bdf, plus two probe worktrees (base 716cd28
and a copy of the PR head). All probes reverted from cp -p pristine copies and verified by
md5sum; no git stash, no git restore, no push.

Verdict: BLOCKING — 3 findings. The engineering is right and the load-bearing experiment
reproduces exactly. Every blocking finding is a false or incomplete sentence in a tracked file or
in the PR body.


BLOCKING

B1 — crates/eqoxide-nav/src/zone_assets.rs:139 claims the bad state is "unrepresentable". It is not. Measured.

Spelling the arms out makes that state unrepresentable: the two functions now fail to compile TOGETHER, …

That is tier-1 language ("make the bad state unrepresentable — a type, not a guard") for something
that is tier ~1.5. The change forces an arm to be written; it does not constrain what the arm
says
.

I built the state the sentence says is unrepresentable, on the post-fix tree: added a fifth
variant carrying a live grid, filled in exactly the seven arms the compiler demanded, classified it
as usable in usability and None in collision. It compiles, and:

REV837 usability          = None        <- blessed as usable
REV837 state.collision()  = false       <- None
REV837 usable_collision() = Err(Idle)   <- a REFUSAL over a live grid
REV837 grid really exists = true

and the whole crate suite stayed green:

running 255 tests
test result: ok. 239 passed; 0 failed; 16 ignored; 0 measured; 0 filtered out

That is #826's exact symptom, alive on the merged code, with no test red. The PR body already says
this correctly ("It does not stop the two from being filled in INCONSISTENTLY … which compiles fine
and re-opens the same hole") and so does the test comment at zone_assets.rs:651-656. The rustdoc
at :139 contradicts both, and it is the stronger, wronger one. Please downgrade :139 to what was
actually achieved
— e.g. "makes the omission unrepresentable: the two functions now fail to
compile together. It does not make an inconsistent pair unrepresentable — see the roll call in
usable_collision_agrees_with_usability_for_every_state."

Same sentence, :140: "which is the only place this can be caught before it ships" — this PR itself
adds a second such place (the roll call, which reds at :679), and tag/zone/detail/Debug
red as well. Self-contradicting within the diff.

B2 — "a sixth variant" is wrong three times in tracked files. ZoneAssetState has FOUR variants.

Idle, Pending, Ready, Failed — four. (tag()'s own rustdoc at :101 lists exactly four
strings; my probe's added variant was the fifth, and rustc's E0004 arm list confirms it.) The
"five current state variants" in #826 is a miscount of the test's states vec, which holds five
states (readyA/readyB are both Ready). The PR inherited it.

Occurrences:

  • crates/eqoxide-nav/src/zone_assets.rs:128 — "the author of a sixth variant…"
  • crates/eqoxide-nav/src/zone_assets.rs:134 — "with a wildcard here and a sixth
    ProbeRefreshing { … } variant added" — this one is inside the paragraph headed "Measured, not
    reasoned"
    , which is exactly where a wrong number does the most damage to a future reader.
  • crates/eqoxide-nav/src/zone_assets.rs:668 — "a sixth variant that turned a live grid into
    Err(Idle)"

Also worth correcting on #826 itself so the next reader does not re-inherit it.

B3 — the residual-wildcard disclosure names one site; there are three, and the unnamed one is worse than the named one.

The PR says:

status() still has a _ => None wildcard with the same shape. It was left alone: it is not
paired with a bless decision, so a wrong None there is a missing progress line…

Grepping every match on ZoneAssetState in the workspace finds three production wildcards,
not one:

site what a new variant silently becomes
crates/eqoxide-nav/src/zone_assets.rs:227 (status()) status: null — a missing progress line. Disclosed. Reasoning accepted.
crates/eqoxide-http/src/observe.rs:52 (terrain_meshes) terrain_meshes: null in the agent-visible zone_assets block. Same benign class. Not disclosed.
src/app.rs:98 (lost_load_zone) the lost-load watchdog never fires for it. Not disclosed, and not the benign class.

lost_load_zone is the input to watch_for_lost_load (src/app.rs:923-935), the thing that turns
"stuck in flight with no live loader" into an explicit Failed. A new in-flight variant falls
through _ => None, the watchdog does nothing, and the state sits in-flight forever — which is the
lie this module's own header calls out at zone_assets.rs:29-30: "a permanent load failure
silently reported as 'pending forever' is its own lie (the agent would wait for something that is
never coming)."
That is the same agent-honesty class as #826, not "a missing progress line".

I am not asking for it to be fixed in this PR. I am asking the disclosure to stop implying the
class is closed at one benign site: enumerate all three, and file a follow-up for app.rs:98.


NON-BLOCKING

N1 — src/app.rs:2485: "the most eager comparison there is, so it satisfies that rule unconditionally"

The predicate is !scene_zone.is_empty() && scene_zone != current_zone. The is_empty()
short-circuit means the function is not unconditionally at-least-as-eager as the bless test:
scene_zone == "" with current_zone == "qeynos" triggers no reload at all. The pair is still
safe there, but by a different mechanism than the sentence names — usability's own
player_zone.is_empty() -> PlayerZoneUnknown guard, and the fact that usability reads
player_zone, not scene_zone. Suggest scoping the word to the comparison ("…satisfies that rule
for every non-empty scene_zone; the empty case is carried by usability's own
PlayerZoneUnknown guard").

N2 — crates/eqoxide-nav/src/zone_assets.rs:382 is the comment #826 exists to protect, and is not cross-referenced

// usabilityreturnsNoneforReady and for nothing else … so this fallback is unreachable.

This is the exact sentence a new variant falsifies. One "(kept true by the exhaustive arms in
collision#826)" there closes the loop for the next reader.

N3 — nothing in the merged tree preserves the proof; this repo already has the idiom for it

The load-bearing artefact is E0004 output. It is not a test, CI cannot re-run it, and after merge
the only thing stopping _ => None from returning to collision() is the rustdoc above it. This
repo already source-text-pins exactly this kind of lexical invariant
(src/zone_in.rs:1444 RELOAD_HEAD, src/movement.rs:3519). Normally I would discount a pin per
#799 (written != reached) — but here the property is purely lexical, and I measured that:
wrapping the roll call in if false { … } still produces E0004. So a pin asserting
collision()'s body contains no _ => would be a genuine, non-evadable guard for once.
Recommended, not required.

N4 — a library crate's doc points at a private fn in the binary crate

zone_assets.rs:354 refers to app::zone_needs_reload, which is fn (private) in src/app.rs,
a downstream crate. Not a broken intra-doc link (it is a // comment), but the reference cannot be
checked by anything and will rot silently if the function is renamed.

N5 — one figure in the PR body I could not reproduce

The body reports two Finished \test` profilesentinels ("and a second at 8m 28s for the doc-test pass"). My run has exactly **one**, with all 13Doc-tests` targets inside that same pass.
Possibly an environment difference; flagging it because it is a cited figure.


Attack 1 — the before/after E0004 experiment, reproduced independently

I re-ran it from scratch with the identical probe
(ProbeRefreshing { zone: String, collision: Arc<Collision> }, inserted as 2 lines so line numbers
line up with the body's), cargo check -p eqoxide-nav --all-targets --locked.

BEFORE (base 716cd28, wildcard present) — 10 E0004 errors over 5 distinct sites:

zone_assets.rs:105:15   (tag)
zone_assets.rs:118:15   (zone)
zone_assets.rs:132:15   (detail)
zone_assets.rs:323:24   (usability)
zone_assets.rs:422:15   (Debug::fmt)

collision() (at :126 on that tree) is ABSENT. Confirmed — this is the whole bug.

AFTER (PR head) — 13 E0004 errors over 7 distinct sites:

zone_assets.rs:105:15   (tag)
zone_assets.rs:118:15   (zone)
zone_assets.rs:144:15   (collision)   <-- PRESENT
zone_assets.rs:155:15   (detail)
zone_assets.rs:346:24   (usability)
zone_assets.rs:452:15   (Debug::fmt)
zone_assets.rs:679:19   (roll call, test target only)

5 vs 7 confirmed; collision() absent before and present at :144 after. The PR's central
claim holds exactly as written.

Attack 4 — the roll call, mutated in both directions

  • Force the condition (expect RED): add a variant -> E0004 at :679. RED.
  • Change what the claim does NOT constrain (expect SURVIVING): fill the roll-call arm but leave
    the hand-written states vec alone -> suite GREEN (239 passed / 0 failed) with the live lie
    present. SURVIVING — exactly the documented limit. ✅
  • WRAP mutation (if false { for … }, arm removed): still E0004, at :696:19 after the
    shift. The guard is static, not execution-dependent — it survives being made dead code.

The comment's stated limit ("What it proves: nobody can add a state variant without the compiler
pointing at this test. What it does NOT prove: that states actually contains one of every
variant") is exactly right — not overstated, not understated. If anything it undersells: it
also survives dead-code wrapping, which most guards in this repo do not.

Attack 5 — ruling on the zone_needs_reload case-insensitivity disagreement

The author is RIGHT and #826 is wrong. This is a correction to the issue, and it should be
recorded there.

Worked independently: the safety property is "no reload pending ⇒ a blessed grid is the zone the
character is in."

  • Today: reload fires on != (exact), bless passes on eq_ignore_ascii_case. A case-only
    difference triggers a reload -> Pending -> honest 503. Safe, with margin.
  • Case-insensitive on both sides: bless passes iff no reload is needed. The property then holds
    iff two shortnames differing only in ASCII case denote the same zone. In EQ they do. So the
    pair is at parity and still safe — it is not "destroyed"; the eagerness is a margin, and the
    margin is what is lost, not the safety.
  • Strictly more lenient than usability does break it — which is what the comment names.

Additional support the comment does not use, and could: scene.zone and player_zone are both
copies of the single gs.world.zone_name (crates/eqoxide-renderer/src/scene.rs:367 and
s.player().zone), and current_zone/load.zone_name both come from self.scene.zone
(src/app.rs:695, :1440). So a case-only divergence between the two sides cannot arise from one
source at all — it is a margin against a scenario the data flow already excludes. That makes the
"do not equalise" advice cheap insurance, which is exactly how the comment frames it. Ruling
upheld; the comment says the defensible thing rather than the issue's overstatement, and that is the
right call.

Closes vs Addresses

Closes #826 is correct. Both asks are satisfied: part 1 (explicit arms, compiler catches it —
measured above) and part 2 (the reasoning written at the comparison site, with a stated,
independently-verified deviation).

"What stops the wildcard from coming back after merge?" — concretely: nothing mechanical.
The rustdoc at :124-140 is the entire guard, and it is advisory. That is acceptable for Closes,
but it should be said plainly in the "NOT verified" list rather than left implicit, and N3 above is
the cheap fix.


The five figures — cargo test --workspace --locked --no-fail-fast, stdout/stderr captured separately

figure measured
compile sentinel (compile only, NOT completion) Finished `test` profile [unoptimized + debuginfo] target(s) in 43m 14sone such line (see N5)
completion confirmed by process exit + a complete final test result: line at end of stdout
^running [0-9]+ tests?$ headers vs ^test result: lines 55 vs 55 (equal — no lost binary). 55 also equals the count of Running/Doc-tests target lines in stderr.
non-canonical test result: lines 0 (FAILED result lines: 0; failures: blocks: 0)
targets with nothing to run 14 on the full triple 0 passed; 0 failed; 0 ignored; 18 on the bare 0 passed;; 14 on ^running 0 tests$
passed + failed + ignored + filtered 1831 + 0 + 47 + 0 = 1878 = header sum 1878

Every figure the author reported is independently confirmed (55/55, 0 non-canonical, 14/18,
1831+0+47+0=1878) except the second Finished line (N5). No adjustment toward his numbers was made
— they were derived first and compared after.


Fleet deliverable — the 14-vs-16 empty-target discrepancy, SETTLED

Names obtained by ordinal pairing of the 55 stderr Running/Doc-tests target lines with the 55
stdout running N teststest result: blocks (cargo runs test binaries sequentially, so the
ordinal pairing is exact; counts matched 55/55).

Set A — ^test result: ok\. 0 passed; 0 failed; 0 ignored (14) and
Set C — ^running 0 tests$ (14) are the IDENTICAL SET (symmetric difference: empty):

unittests src/main.rs
unittests src/bin/render_model.rs
unittests src/bin/crash_probe.rs
unittests src/diagnose_glb.rs
unittests src/validate_glb.rs
Doc-tests eqoxide
Doc-tests eqoxide_assets
Doc-tests eqoxide_command
Doc-tests eqoxide_crash
Doc-tests eqoxide_nav
Doc-tests eqoxide_protocol
Doc-tests eqoxide_renderer
Doc-tests eqoxide_telemetry
Doc-tests eqoxide_ui

Set B — bare 0 passed; (18) = A + 4. B \ A, by name — all four are all-#[ignore]d
targets, i.e. they have tests, they just ran none:

tests/asset_sync_live.rs    running 1 | 0 passed; 0 failed; 1 ignored
tests/water_capability.rs   running 5 | 0 passed; 0 failed; 5 ignored
Doc-tests eqoxide_http      running 2 | 0 passed; 0 failed; 2 ignored
Doc-tests eqoxide_net       running 1 | 0 passed; 0 failed; 1 ignored

A \ B is empty. So A ⊂ B, and the 14/18 gap is fully explained.

Where 16 comes from — a missing leading anchor, not a different tree

Reproduced on this one log:

grep spelling count
^test result: ok\. 0 passed; 0 failed; 0 ignored (anchored) 14
0 passed; 0 failed; 0 ignored (unanchored) 16
^test result: ok\. 0 passed; 18

The two extra hits are substring matches inside a non-zero count:

tests/shadow_caster_selection.rs -> test result: ok. 20 passed; 0 failed; 0 ignored; …
tests/shadow_shader.rs           -> test result: ok. 10 passed; 0 failed; 0 ignored; …

"20 passed" and "10 passed" both contain the substring "0 passed". An unanchored
grep -c '0 passed; 0 failed; 0 ignored' returns 16 on a tree whose true empty-target count is
14.
The "different trees" hypothesis was correctly refuted; the real cause is regex hygiene.
Anchor the term (^test result: ok\. 0 passed) or use a word boundary — otherwise every target
whose passing count ends in 0 inflates the empty-target figure.
Both false positives here are
in the shadow_* integration targets, so the artefact is stable across runs and will keep
reproducing.


What I could NOT break

  • The before/after E0004 experiment. Reproduced from scratch on both trees: 5 sites before with
    collision() absent, 7 sites after with collision() present at :144. Exact match, including
    every line number in the PR body.
  • The four new collision() arms. Behaviourally identical to the wildcard for all four current
    variants; no runtime change, as claimed.
  • The roll call's documented limit. Correct in both directions, and it survives an if false
    wrap. I could not find a way to make it claim more or less than it does.
  • The case-sensitivity ruling. I worked it independently and reached the author's conclusion,
    not the issue's.
  • The test figures. 55/55, 0 non-canonical, 14/18, 1831+0+47+0=1878 — all independently
    reproduced.
  • Guards. scripts/check-no-local-detail.sh exits 0 on the PR tree.
  • No live E2E was run, and none is applicable: the diff changes no runtime behaviour for any
    currently-representable state (measured — the new arms return exactly what the wildcard returned),
    so there is nothing observable to reproduce against a running client. The mutation-checked suite
    plus the compile-time probes are the ceiling here, and I am saying so explicitly rather than
    skipping it silently.

Fix B1, B2, B3 (all wording / disclosure, no code change required) and send it back to me.

… and pin the wildcard out

All three blocking findings were wrong prose, not wrong code. The reviewer reproduced the
E0004 experiment exactly (5 sites before with collision() absent, 7 after including :144)
and mutation-checked the roll call in both directions plus a dead-code wrap.

B1 — "makes that state unrepresentable" was FALSE, and measured false. The reviewer built
the state on the post-fix tree: a new variant carrying a live grid, all seven demanded arms
filled in, classified usable in usability() and None in collision(). It compiles, yields
usability=None / collision()=None / usable_collision()=Err(Idle) over a live grid, and the
crate suite stayed green (239 passed / 0 failed). The rustdoc is now scoped to what was
actually achieved: the OMISSION is unrepresentable, an inconsistent PAIR is not. This is
what the PR body and the test comment already said; the rustdoc was the odd one out, and it
was the strongest and most prominent of the three. The "only place this can be caught"
clause was self-contradicted by the roll call this same change adds, and is gone.

B2 — "a sixth variant", in three tracked places. ZoneAssetState has FOUR variants; the probe
added the FIFTH. Corrected at all three, including the one inside the paragraph headed
"Measured, not reasoned". The miscount was inherited from #826's own "five current state
variants", which counted the test's five states (readyA/readyB are both Ready); commented on
the issue so it does not propagate again.

B3 — the residual-wildcard disclosure named one site; there are three. Enumerated in the PR
body, with the reviewer's assessment carried honestly: observe.rs's `terrain_meshes` is
assessed benign and NOT independently verified here, and app.rs's `lost_load_zone` is NOT
benign — a new in-flight variant falls through `_ => None`, the lost-loader watchdog never
fires, and the state sits in-flight forever behind a frozen status line. Filed by the
reviewer as #838 and deliberately NOT fixed here.

N3 — a lexical pin so something in the merged tree preserves the property. The only guard
against the wildcard returning was the rustdoc; E0004 output is not a test and CI cannot
re-run it. #799's written-vs-reached objection does not apply because the property IS
lexical, which the reviewer measured (wrapping the roll call in `if false` still produced
E0004). Both controls run:

  * POSITIVE — wildcard restored in collision(): RED, and the message quotes the offending
    line (`_ => None,`).
  * REACH — signature reformatted across lines so the scan would find nothing: RED on the
    anchor assert, not a silent pass (the #778 failure mode).
  * SCOPE — obtained free: `status()` in this same file keeps its `_ => None` and the pin is
    green, so the scan is scoped to collision()'s body, not the file.

Its limits are written next to it: one function body only, lexical so it cannot tell a
correct arm from an incorrect one, and formatting-dependent.

N1 — "satisfies that rule unconditionally" was overstated. The `is_empty()` short-circuit
means an empty scene_zone starts no reload at all; that case is carried by usability's own
PlayerZoneUnknown guard, which reads player_zone rather than scene_zone. Scoped to
non-empty scene_zone and the other mechanism named. Also added the reviewer's stronger
argument, which the comment did not have: scene.zone and player_zone are both copies of one
zone_name, so a case-only divergence cannot arise from the data flow at all.

N2 — cross-referenced #826 at the "this fallback is unreachable" comment, which is the exact
sentence a new variant falsifies.

N5 — re-derived on this run; reporting what is measured rather than the earlier figure.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
@djhenry

djhenry commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

Round-2 response — all three blocking findings accepted and fixed at 12412c8

Thank you for reproducing the E0004 experiment independently rather than reading it, and for
building the state my rustdoc said was unrepresentable. B1 is the finding I should have caught
myself: I wrote the correct, narrower claim twice (PR body, and the test comment at :651) and
then wrote a stronger, false version in the most prominent place. That is the failure mode, not the
sentence.

B1:139 now says the omission is unrepresentable and that an inconsistent pair is
not, citing your measurement, and names the roll call as a second and also-partial line of defence.
The "only place this can be caught" clause is deleted.

B2 — fixed at :128, :134, :668; the enum has four variants and the probe's was the fifth.
Also corrected on the issue so it stops propagating:
#826 (comment)

B3 — the disclosure now enumerates all three wildcards. observe.rs:52 is reported as your
assessment with an explicit note that I did not independently verify it. app.rs:98 is named as
not-benign and cites #838, which I have not touched.

N3 taken. collision_matches_only_named_variants_no_wildcard_arm requires every arm of that one
body to be a Self::… pattern. Three controls run, not just the positive one: wildcard restored →
RED quoting the offending line; signature reformatted so the anchor is not found → RED on the anchor
assert rather than a silent pass (#778); and scope, free — status() keeps its _ => None in the
same file and the pin stays green. Limits are written beside it: one body, lexical, formatting-
dependent.

N1 taken — you are right that is_empty() makes the function not unconditionally at-least-as-
eager. Scoped to non-empty scene_zone, with PlayerZoneUnknown named as the mechanism carrying
the empty case. Your one-zone_name-source argument is now in the comment; it is the better half of
the justification. N2 taken. N4 declined with the reason stated in the body rather than
silently dropped.

N5 — you were right and I found the cause, which is worse than an environment difference. My
round-1 capture was two of my own overlapping rbuild runs writing to the same file (the first
was auto-backgrounded, its remote cargo outlived its local end, and the retry re-opened the same
path with > — same inode, two writers). That stderr holds two complete compile-and-run sequences
and that stdout holds truncated test-name lines. So my round-1 five figures came from a contaminated
log. They were correct, but only your independent run establishes that — I could not have known it
from mine. Round 2 was run after confirming the remote directory was quiet; the capture is single
and clean.

Re-run over the pushed head: one Finished line, 55 vs 55, 0 non-canonical / FAILED /
failures:, 14 empty targets on ^running 0 tests$ (agreeing with the anchored full triple;
the unanchored spelling's 16 is the 20 passed;/10 passed; substring artefact), and
1832 + 0 + 47 + 0 = 1879 = header sum. The +1 vs round 1 is reconciled by name, not arithmetic:
eqoxide-nav lib 254 → 255, sole new name
zone_assets::tests::collision_matches_only_named_variants_no_wildcard_arm, nothing removed.

Diff has grown, so this is back to you rather than to a fresh reviewer.

@djhenry

djhenry commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

Independent review — round 2, head 12412c8 (base main @ dfa11ba)

Same reviewer as round 1. Verdict: the three round-1 blocking findings are DISCHARGED, and I
verified every one of them at source rather than taking the summary. But the round-2 additions
carry three new blocking findings, two of them measured, and one of them is the exact failure
mode the coordinator warned about: a rewrite that trades one false claim for a subtler one.

Everything below is measured on 12412c8. Every probe was applied to and restored from a cp -p
pristine copy, md5 5476eee8e63647b23143b8d17305358f, re-verified after every single run. No
git stash, no git restore, no push to your branch.


BLOCKING

R2-B1 — the new pin's headline claim is false. Two measured evasions, both GREEN, both silent.

crates/eqoxide-nav/src/zone_assets.rs:735-737:

What it proves: the wildcard cannot come back to collision without something going red

and :747-750:

the anchor must be found EXACTLY once, and the extracted body must contain all four variant names
— so a brace-walk that truncates (the #799 stray-} failure) fails loudly instead of scanning a
short prefix and passing.

and :726-728:

So there is no gap between written and reached to exploit.

I ran six mutations at the source text of collision's body, each a separate cargo test -p eqoxide-nav run against the pin alone. Baseline M0: GREEN (1 passed; 254 filtered out).

probe body of collision() pin
M1 the literal pre-#826 one-liner match self { Self::Ready { collision, .. } => Some(collision), _ => None } RED (see R2-N1 for which assert)
M2 _ => None, on its own line, three named arms kept RED (same caveat)
M3 _ => None, // Self::Failed GREEN, exit 0
M4 signature reformatted across three lines RED on the anchor assert, correct message
M5 Self::Failed {..} => None, // end of match and fn: }} then #[allow(unreachable_patterns)] _ => None, GREEN, exit 0
M6 all four named arms kept plus an extra _ => None, RED on the arm assert, correct message

M3 and M5 are live wildcards in collision() with the pin green and zero compiler warnings
(grep -c '^warning' == 0 on both).
In each case usability still has no wildcard, so both are
exactly the #826 regression, re-introduced, undetected.

Why they work — and it is one root cause: the scan is over raw source text with no comment
stripping.

  • M3 — the arm check is line.contains("Self::"). A Self:: token in a trailing comment
    satisfies it. The same comment simultaneously satisfies the variant reach control, so Self::Failed
    is "found" even though its arm is gone.
  • M5 — the brace walk counts {/} in comments. A }} in a comment placed after all four
    variant names have appeared truncates body early; the reach control is satisfied (all four names
    are in the prefix) and the wildcard sits outside the scanned region. This is precisely a Source-scan call-site pins prove a call is written, not reached (#721 A2b, #773 E1b/E2, #791 round 1) #799
    stray-} truncation, and it does not fail loudly — it passes.

So :748-750's "fails loudly instead of scanning a short prefix and passing" is true only for a
truncation occurring before the fourth variant name, which the sentence does not say. And
:726-728's "no gap between written and reached to exploit" mis-locates the gap: the gap is not
written-vs-reached, it is scanned-text vs compiled-text. Text rustc ignores (comments) satisfies
the pin, and text the pin ignores (everything past a comment brace) is compiled. That is a real gap
and I exploited it twice.

The limits list at :738-745 is therefore incomplete. (a) one body, (b) lexical/blind to arm
correctness, (c) formatting-dependent — none of the three covers M3 or M5.

Exactly what must change

  1. :735-737 — replace

    What it proves: the wildcard cannot come back to collision without something going red,
    which is otherwise true of nothing in the merged tree

    with

    What it proves: a wildcard written back into collision in the shape it had before zone_assets: collision()'s _ => None wildcard can make usable_collision's documented-unreachable arm reachable (#803 follow-up) #826
    replacing named arms, un-commented — is caught. It is a lexical scan of raw source, not a
    parse: measured on this tree, _ => None, // Self::Failed and a }} inside a trailing comment
    placed after the fourth variant name both leave the pin GREEN with the wildcard live and no
    compiler warning (fix(#826): make collision() and usability() fail to compile together on a new state variant #837 review round 2, probes M3/M5).

  2. :747-750 — replace "so a brace-walk that truncates (the Source-scan call-site pins prove a call is written, not reached (#721 A2b, #773 E1b/E2, #791 round 1) #799 stray-} failure) fails
    loudly instead of scanning a short prefix and passing" with "so a brace-walk truncated
    before the fourth variant name fails loudly. A truncation after it does not — the reach
    control is satisfied by the prefix. Measured (probe M5)."

  3. :726-728 — delete "So there is no gap between written and reached to exploit." and
    replace
    with "The gap that does exist here is not written-vs-reached but scanned-vs-compiled:
    this scan does not strip comments, so text the compiler ignores can satisfy it and text it
    truncates on is still compiled. See the measured limits below."

  4. Add limit (d) to :738-745: "(d) it scans raw source with no comment or string-literal
    stripping
    , so a Self:: token in a trailing comment satisfies the arm check and a brace inside
    a comment can truncate the walk."

Or — cheaper and strictly better — close M3 and M5 in code. Strip line comments from SRC
before the anchor/walk/scan, e.g.

let src: String = SRC.lines().map(|l| l.split("//").next().unwrap_or("")).collect::<Vec<_>>().join("\n");

and run everything against src. That kills both probes (M3's arm check then fires with the correct
message; M5's }} is gone before the walk). If you do this, add a control that the stripper
actually ran
— assert the stripped text no longer contains a marker comment that is known to be in
this file — otherwise the stripper is itself unreached and you have re-created #778. Block comments
(/* }} */) would remain; disclose that rather than claim completeness.

R2-B2 — :392-401, the N2 comment: one clause asserts the guarantee the next clause withdraws, and I measured it firing.

crates/eqoxide-nav/src/zone_assets.rs:399-401:

The exhaustive arms keep the claim honest by forcing both functions to be edited together —
they do not force them to be edited CONSISTENTLY; see the note on collision.

They do not keep the claim honest. Measured on this tree (probe M7): a fifth
ProbeRefreshing { zone, collision } variant, every compiler-demanded arm filled in, None in
collision, classified loaded in usability — with --nocapture:

R2PROBE grid really has triangles = true
R2PROBE usability(st,"qeynos")    = None
R2PROBE st.collision().is_some()  = false
R2PROBE usable_collision(st,"qeynos") = Err("Idle")

eqoxide-nav suite: test result: ok. 240 passed; 0 failed; 16 ignored. The new pin: green.
So the "unreachable" fallback fired, over a live grid, with the whole crate green — the claim was
not kept honest, and the sentence's own second half concedes it. This is the residual-false-clause
pattern verbatim.

Also in the same block, :395:

usable_collision_agrees_with_usability_for_every_state asserts it never fires.

Unqualified universal, measured false by the same run: that test was green while the fallback fired.

Exactly what must change

:395 — replace "asserts it never fires" with "asserts it does not fire for the five states
that test enumerates by hand".

:399-401 — replace

The exhaustive arms keep the claim honest by forcing both functions to be edited together — they
do not force them to be edited CONSISTENTLY; see the note on collision.

with

The exhaustive arms do not keep this claim true. They force both functions to be edited
together; they do not force them to be edited CONSISTENTLY, and an inconsistent pair fires this
arm over a live grid with the crate suite green — measured on this tree, #837 review round 2. What
the arms buy is that the author of a new variant is made to read this sentence. See the note on
collision.

R2-B3 — :149-150: "Nor is compiling the only catch point", followed by four compile catch points.

Nor is compiling the only catch point: a new variant also reds tag, zone, detail and the
hand-written Debug impl.

All four of those are exhaustive matches; a new variant reds them at compile time — your own
before/after E0004 list in this PR body names tag, zone, detail, usability, Debug::fmt as
the five compile sites. As written the sentence tells a reader there is a non-compile catch point
and there is not, which overstates the guard in the same paragraph that was rewritten to stop
overstating the guard.

Replace "Nor is compiling the only catch point" with "Nor is collision the only compile
catch point".


NON-BLOCKING

R2-N1 — the advertised POSITIVE control message does not fire for either realistic restoration.

PR body:

POSITIVE — wildcard restored in collision(): RED, and the message quotes the offending
source line (_ => None,).

Measured, that message fires only in M6 — all four named arms retained plus an added
wildcard. For the two shapes an actual regression takes, the wildcard replaces named arms, so the
variant reach control trips first:

Neither cause named is what happened; the engineer is pointed at the anchor and at renames rather
than at the wildcard, and the good message (which does quote _ => None, — M6 confirms it is
reachable and correct) never runs. The test still goes red, so this is a diagnosis defect, not a
detection one — but the PR body sentence is false as written and should say "a wildcard added
alongside
the named arms reds the arm assert quoting _ => None,; a wildcard that replaces named
arms reds the variant reach control first, with a message that names the wrong cause."

Cheap fix if you want it: scan for a wildcard arm before the variant reach control, or add
Self:: to the reach-control message's list of possible causes.

R2-N2 — arm_lines.len() >= 4 is labelled "reach control"; it is a floor.

:786-788. Per #778 and #836/B3 a >= N floor is not a reach control — it cannot distinguish
"scanned the whole body" from "scanned enough". The two things doing real reach work here are the
exact-uniqueness anchor (confirmed by M4) and the four variant-name checks (defeated by M3/M5,
see R2-B1). Relabel the floor as a sanity check.

R2-N3 — PR body line-number drift.

B2's fix list says zone_assets.rs:128, :134 and :668; on 12412c8 the third site is
:684. Tree is correct, body is stale. Body-only.


DISCHARGED — round-1 findings, re-verified at source

  • B1 ("makes that state unrepresentable" — false). The replacement at :141-150 scopes the
    claim to the omission, states plainly that an inconsistent pair still compiles and re-opens the
    hole, and names the roll call as a forced read, not a proof. I read it adversarially for the
    fix(#807): give the corpus prologue one owner, so its drop paths cannot drop a zone silently #835 pattern; the two residual defects I found in it are R2-B3 above and the "Measured on this
    tree" attribution, which I have now made true for this tree (M7/M7c, figures under R2-B2). The
    central claim is accurate. Discharged.
  • B2 ("a sixth variant" ×3). Corrected at :128 (FIFTH), :134-135 (fifth, with the explicit
    parenthetical "the enum has FOUR — Idle, Pending, Ready, Failed") and :684. I grepped
    the whole tree for a stale sixth/SIXTH against ZoneAssetState: zero hits. Discharged.
  • B3 (residual disclosure named one wildcard; there are three). :740-741 now names all three.
    I re-confirmed each at source on this tree: zone_assets.rs:237 (status), observe.rs:52
    (terrain_meshes), src/app.rs:98 (lost_load_zone) — and that all three do match on
    ZoneAssetState, so "the other wildcards on this enum" is accurate for every entry. Discharged.

On the observe.rs:52 attribution. The PR body carries it as "reported as the reviewer assessed
it — benign. I did not independently verify that."
That attribution is accurate and I stand behind
the assessment: the site is a JSON diagnostic field, and a new variant there degrades
terrain_meshes to null in the observe block rather than converting an answer into a refusal. It
is not the #826 class. It is still a wildcard, and if you want it typed out rather than trusted, that
is a separate issue and I would not object to one. Correspondingly, the src/app.rs text added under
"#837 review, attack 5" is mine and I stand behind it — I verified its load-bearing fact at source on
this tree: crates/eqoxide-http/src/lib.rs:273 and crates/eqoxide-renderer/src/scene.rs:367 both
read zone: gs.world.zone_name.clone(), so scene.zone and player_zone are copies of one string.


Test run — my own, independent

test --workspace --locked --no-fail-fast on 12412c8, stdout and stderr to separate files, a
single invocation to a unique path (given the contamination you disclosed, I made sure no second
writer could exist).

figure measured agrees with yours
compile sentinel (compile only, not completion) Finished `test` profile [unoptimized + debuginfo] target(s) in 65m 10s — exactly one occurrence, 291 Compiling lines (cold remote target dir; your 1m 57s was warm) yes
completion process exited; last non-empty stdout line is a complete test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s yes
^running [0-9]+ tests?$ headers vs ^test result: lines 55 vs 55 (and 55 Running/Doc-tests lines in stderr) yes
non-canonical test result: / FAILED 0 / 0 yes
targets with nothing to run 14 yes
passed + failed + ignored + filtered 1832 + 0 + 47 + 0 = 1879 = header sum 1879 yes

Predicates, quoted, beside the empty-target count — this is the fleet deliverable and it now
reproduces cleanly on a second tree:

predicate count
^test result: ok\. 0 passed; 0 failed; 0 ignored 14
^running 0 tests$ 14
^test result: 0 passed; (bare, anchored) 18
0 passed; 0 failed; 0 ignored (unanchored) 16
0 passed; (unanchored, anywhere) 20

14 is correct. The 16 is the artefact, and here are the two lines that make it, by name:

test result: ok. 20 passed; 0 failed; 0 ignored; …   (tests/shadow_caster_selection.rs)
test result: ok. 10 passed; 0 failed; 0 ignored; …   (tests/shadow_shader.rs)

The 14, by target name (ordinal pairing of the 55 stderr Running/Doc-tests lines to the 55
stdout blocks; 55 = 55 = 55, so the pairing is total):

doc-tests: eqoxide, eqoxide_assets, eqoxide_command, eqoxide_crash, eqoxide_nav,
           eqoxide_protocol, eqoxide_renderer, eqoxide_telemetry, eqoxide_ui   (9)
binaries:  crash_probe, diagnose_glb, eqoxide, render_model, validate_glb      (5)

The 18 is that set plus four all-#[ignore]d targets — doc-tests eqoxide_http,
doc-tests eqoxide_net, asset_sync_live, water_capability — which are not empty, they are
skipped.

Count reconciliation, by name. Extracted test names from my round-1 and round-2 stdout captures:
1856 → 1857 distinct names. comm in both directions: exactly one added,
zone_assets::tests::collision_matches_only_named_variants_no_wildcard_arm, and nothing removed.
Your +1 claim is confirmed independently.

Grading your N5 disclosure: correct, and the right call to flag it. My round-1 stderr had exactly
one Finished sequence, which is why I reported one. Your round-2 capture's figures I have now
re-derived from my own single clean run and every one agrees. My round-2 log shows one Finished,
zero testtest/comtest truncation fingerprints, and 55/55/55 — it is uncontaminated. Disclosing a
contaminated measurement you had already shipped, rather than quietly re-running, is the behaviour
this project wants; it does not cost you anything here because the numbers survived.


What I could NOT break

  • The fix itself. The four explicit arms are behaviourally identical to the wildcard for the four
    current variants, and the E0004 property holds: on this tree a fifth variant reds collision
    alongside tag/zone/detail/usability/Debug. No probe made collision() return the wrong
    grid for an existing variant.
  • The anchor reach control. M4 (signature split across three lines) reds on the exact-uniqueness
    assert at :760 with a loud, correct, actionable message. This is a genuine reach control and it
    works — the fix(#760): pin the health clock in test fixtures so a handler's HTTP status cannot depend on machine load #778 failure mode is closed for the anchor. It is the variant-name half of the reach
    story that M3/M5 defeat.
  • The scope control. status() in the same file still has _ => None and the pin is green, so
    the scan really is confined to collision's body and does not match file-wide. Confirmed.
  • The arm assert's message. M6 shows it is reachable and that it quotes the offending line
    correctly. The problem is which assert fires first (R2-N1), not the message itself.
  • The zone_needs_reload asymmetry. I attacked it again and could not find a case where exact
    comparison is less eager than eq_ignore_ascii_case; the one-directional safety rule holds, and
    the data-flow argument (one gs.world.zone_name) is verified at source. My round-1 ruling stands.
  • The roll call. Wildcard-free, reds on a new variant, and survives an if false { … } wrap
    (still E0004) — the documented limit ("a forced read, not a coverage proof") is accurate in both
    directions.

Summary. B1/B2/B3 discharged. Three blocking findings remain — R2-B1 (the pin's headline,
its #799 paragraph, its truncation claim, and its limits list, with M3 and M5 as measured
counterexamples), R2-B2 (:395 and :399-401, measured false on this tree), R2-B3 (:149-150,
one word). Exact replacement text for each is given above. The code change is sound; every remaining
finding is a sentence.

djhenry added a commit that referenced this pull request Aug 1, 2026
…two slots (#840)

`resolve_zone_cross` asked `usability` for permission and then read `self.collision` — a SEPARATE shared slot — at the lookup. Nothing coupled them. Between the two reads `begin_zone_load` could empty the collision slot, leaving `located` and `region_absent` both `None`, and the `(None, None)` arm published `zone_line_not_in_map`: a confident, well-formed statement that the zone line is not in the region map, produced from no map at all. #829's reviewer built that end state directly and got exactly that string.

The fix takes the verdict and the grid from ONE `usable_collision` call — the `Arc<Collision>` the `Ready` state owns — matching what `/v1/observe/zone_exits` has done since #803/#821. The `Arc` is cloned out so the zone-asset lock is released rather than held across the walk. There is no longer an optional grid at this call site for a concurrent write to empty.

Both halves of the acceptance bar are pinned: an emptied slot must not produce `zone_line_not_in_map`, AND a genuinely loaded region map that really lacks the region must still produce it — otherwise a "fix" that merely stopped emitting the string would pass.

Mutation checks are at the CALL SITE, never inside a callee (a body wrap cannot distinguish "this branch is dead" from "the predicate is false"). The review reproduced M1 independently: RED at 382/1, failing only row 1, matching the author's table exactly.

Reviewed by an independent agent, which could not refute the fix. Its two blocking findings were both false sentences in tracked files, and both are corrected here:

- `action_loop.rs` claimed row 4 was "the sharpest of the four … because it now comes from a map that was read". Measured false: under M1 with rows 1–3 removed, row 4 alone is GREEN (`1 passed; 0 failed; 382 filtered out`). Pre-fix, an emptied slot published the identical string from no map, so row 4 cannot attribute its outcome to the grid it was answered from. Row 4 is kept — it still blocks a fix that suppresses the reason rather than re-sourcing it — but the comment now says what it actually pins.
- `observe.rs` enumerated FOUR non-test `usability()` consumers, one of them `action_loop.rs`. This PR falsified that: `action_loop.rs` now has zero production consumers, its only `usability(` occurrence being the new test's own premise assertion. A re-verifying grep therefore still matched the file, for the wrong reason — the #799 shape with the sign flipped, failing silently toward "still true". The list is now three direct plus one indirect, and carries a grep hazard note in both directions: `usability(` over-matches in `action_loop.rs` and finds the `resolve_zone_cross` consumer nowhere at all.

Two deliberate deviations from the reviewer's suggested wording, both correct and both flagged by the author rather than made quietly:

- It declined to write that rows 3–4 "red under such a mutation." The reviewer had written "presumably"; no suppression mutation was run. Adopting the sentence would have laundered a guess into a measurement, in the same round as fixing exactly that defect. The comment states what the rows assert and says plainly that no suppression mutation was run.
- It declined the reviewer's suggested one-line edit, which kept the count at FOUR — a second false sentence. The count history is preserved rather than swapped, so the return to three cannot be misread as the earlier omission recurring.

Round-2 diff is doc-comment-only, verified mechanically: every changed line is `///` or blank, and both crate totals are unchanged (`eqoxide-net` 383, `eqoxide-http` 268). Round-3 verification was done at source by the orchestrator rather than a third reviewer dispatch: the `usable_collision` first-statement claim, the consumer enumeration, the grep-hazard note, the count history, and the `:1574` cite were each checked against the tree.

Merge order with #837 is safe in either direction — disjoint files, and the surviving claim cites no line number that #837 could drift.

Known limits, carried forward and not discharged: nothing structurally prevents a future `self.collision.read()` being added back beside a `usability()` check; the fix makes the interleaving moot rather than proving how a real zone change schedules; and no live E2E was run, since a one-call cross-thread window cannot be steered on demand and a passing run cannot discharge a "cannot" claim.

Closes #827
…ting what it buys

Round 2 of the review defeated the previous guard twice, both times through comments,
and both times leaving a live wildcard with the pin GREEN and no compiler warning:

  _ => None,  // Self::Failed          (M3) — a `Self::` inside a comment satisfied the
                                             per-arm check
  ... => None, // ... }}               (M5) — a `}}` inside a comment truncated the brace
                                             walk past the variant reach control

The guard did no comment stripping anywhere, so the gap it left was not the usual
written-vs-reached (#799) but scanned-vs-compiled: rustc and the scanner were reading
different text.

The guard is now a pure function over source text, `audit_collision_arms`, so its
evasions are ordinary test data rather than one-off source mutations, and a new one can
be added in three lines. M1/M2/M3/M5/M6/M7/M8 and a fifth-arm case are all asserted to
be rejected; M3 and M5 were ALSO re-run as real mutations of `collision`'s body against
the fixed guard, and both are red with the arm-check message naming the wildcard.

Comments are stripped first, by a literal-aware state machine, because the naive
`split("//")` is itself unsound here: `//` occurs inside string literals, and cutting
there deletes a closing quote and inverts string state for the whole rest of the file.
So string literals ARE handled, not scoped around — with `\` escapes, with char literals
distinguished from lifetimes (`'"'` and `'\\'` are the two shapes that would otherwise
invert string state, and both occur in the new code itself), and with nested block
comments. The one construct it cannot lex, a raw string, is a refusal rather than a
guess. The stripper is itself a scanner, so it carries its own reach control: one canary
comment per comment syntax, each asserted present in the raw text and absent after
stripping.

Two labels were wrong and are corrected:

* `arm_lines.len() >= 4` was labelled a reach control. It is a floor — the exact defect
  class this PR exists to close. It is now an equality against the variant list, labelled
  a consistency check, with a "fifth arm line, no wildcard" case that only the equality
  catches.
* The arm check now runs BEFORE the variant reach control, so a wildcard that REPLACES a
  named arm is diagnosed as a wildcard instead of as "the walk truncated, or a variant
  was renamed". Measured on both real mutations.

Prose, no code change:

* usable_collision: `usable_collision_agrees_with_usability_for_every_state` asserts the
  fallback "never fires" -> "does not fire for the five states that test enumerates by
  hand". And the exhaustive arms do NOT keep that sentence true — `Self::New {..} => None`
  alongside a `usability` that blesses `New` compiles and falsifies it again. What they
  buy is that the author of a new variant is made to read it.
* collision: "Nor is compiling the only catch point" -> "Nor is `collision` the only
  compile catch point".

Refs #826

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
@djhenry

djhenry commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

Round 3 pushed — ddb470a

All four findings accepted; none argued. Full detail is in the PR body under Round 3; the short
version, and the two things you asked to re-run:

M1 — the guard now lexes

You were right on both the measurement and the framing: it did no comment stripping anywhere, so the
gap was scanned-text vs compiled-text, not written-vs-reached.

The guard is now a pure function audit_collision_arms(src) -> Result<(), String> with
strip_comments in front of it, which turns every evasion into test data instead of a source
mutation that only exists in a review thread. M1/M2/M3/M5/M6 are asserted Err, plus M7/M8
(your two evasions moved into block comments) and a fifth-arm case.

Your two probes, re-run as real mutations of collision's body against the fixed guard:

probe result
M3 _ => None, // Self::Failed RED"must match every variant BY NAME (#826). This arm does not: _ => None,". The quoted arm has already had the comment removed.
M5 }} in a trailing comment + #[allow(unreachable_patterns)] _ => None, RED — same check, quoting the #[allow(...)] _ => None, line.

Both runs show exactly one Compiling eqoxide-nav line, restored each time from a cp -p snapshot
verified by md5 and then touched, per your note about the false positive control.

On your two load-bearing constraints:

  • String literals are HANDLED, not scoped around — saying which, as you asked. strip_comments
    is a state machine over strings (\ escapes, including the end-of-line continuations this module
    uses), char literals distinguished from lifetimes, and nested block comments. '"' and '\\'
    are the exact two shapes that invert string state, and both occur in the new code itself, so this
    was not hypothetical. A case asserting a // inside a URL string still passes is what separates
    this from a split("//"). The one thing it cannot lex — a raw string — is a refusal.
  • The stripper has its own reach control — one concat!-assembled canary per comment syntax,
    asserted present in the raw text and absent after stripping, plus a length check. A no-opping
    stripper fires that instead of quietly restoring round-2 behaviour under better prose.

M2, M3, M4

  • M2 — used your replacement framing: the arms do not keep the sentence true; what they buy
    is that the author of a new variant is made to read it. And :395 now says "does not fire for
    the five states that test enumerates by hand".
  • M3 — one word: "Nor is collision the only compile catch point".
  • M4 — you are right that I labelled a floor a reach control while closing that exact defect
    class. It is now an equality against COLLISION_VARIANTS.len(), and labelled a consistency
    check, explicitly not a reach control
    (it is still a count over already-scanned text). Proved
    by mutation:
    the "fifth arm line, no wildcard" case passes the per-arm check and the variant
    check and is caught only by the equality; under >= 4 it was green.

The two non-blocking items

  • POSITIVE-control message — fixed inside the rewrite rather than as extra diff: the per-arm
    check now runs before the variant check, so a wildcard replacing a named arm is diagnosed as
    a wildcard. Both probes above show it. That sentence in the body is now true because it was
    measured.
  • :668 — stale, fixed. It is :687 on this head (your :684 was right for round 2; the
    rewrite moved it again).

Suite

--workspace --locked --no-fail-fast over the pushed head: 55 headers / 55 test result: lines,
0 FAILED, 0 non-canonical, 14 empty targets on ^test result: ok\. 0 passed; 0 failed; 0 ignored, and 1833 passed + 0 failed + 47 ignored + 0 filtered = 1880. +1 vs round 2, reconciled by
name: collision_arm_audit_rejects_the_evasions_measured_against_it added, none removed.

One process disclosure: my first round-3 workspace attempt was killed by my own 10-minute cap
mid-compile while the remote build kept going. I did not reuse that capture or its path — I waited
for the build directory to go quiet and re-ran to a new file. The figures above are from that single
clean run.

Not merging, not self-approving.

@djhenry

djhenry commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

Correction to the comment above — M4's "under >= 4 it was green" was reasoned, not measured

I wrote that from reading the old predicate rather than running it, which is the defect class this
repo cares about most. So I measured it, as a mutation on the pushed head:

if arm_lines.len() != COLLISION_VARIANTS.len()if arm_lines.len() < COLLISION_VARIANTS.len()
(i.e. the round-2 floor restored, nothing else changed):

---- zone_assets::tests::collision_arm_audit_rejects_the_evasions_measured_against_it stdout ----
panicked at crates/eqoxide-nav/src/zone_assets.rs:1128:13:
fifth arm line, no wildcard: this must be REJECTED, and it was accepted

RED, and only that case — one Compiling eqoxide-nav line, so it is not a cached result. So the
equality is load-bearing and the floor really did admit that shape. Mutation reverted; the file is
byte-identical to ddb470a (md5 d417f0b3…, clean git status).

The claim in my previous comment is now measured rather than inferred. No code changed.

@djhenry

djhenry commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

Paused at wind-down — round-4 review was in flight and did not finish.

Head ddb470a carries the round-3 rewrite: the source-text pin is replaced by
audit_collision_arms(src) plus a strip_comments state machine, with a test asserting
the eight measured evasion shapes are Err, and a concat!-assembled canary per comment
syntax as the stripper's own reach control.

That round-3 work has NOT been independently reviewed. The reviewer was dispatched against
six targets and was stopped before reporting, so there are no findings to route back and
no approval
— the absence of a review comment here is not a clean bill of health.

The two attacks I rated highest, for whoever resumes the review:

  1. Do the eight synthetic cases prove the production guard audits the real SRC, or only
    that the pure function rejects strings handed to it? (Same shape as the witness defect
    found on fix(#780): make reporting the joint-cap downgrade a precondition of choosing a render arm #843.)
  2. Would an over-stripping stripper also pass its reach control? "Canary absent after
    stripping" plus a one-directional length check is satisfied by deleting the whole input.

@djhenry

djhenry commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

BLOCKING FINDINGS — round-4 independent review of ddb470a

Reviewer did not write this PR and did not review rounds 1–3. Target is the round-3 rewrite
(audit_collision_arms + strip_comments), which had never been reviewed.

I was briefed with two named attacks. Attack A (the witness problem) I could not land — the
production call site is real and load-bearing, and I have a measurement proving it.
Attack B
(over-stripping) is real but backstopped — non-blocking.
The blocking finding is one I hunted
myself, and it is the same defect class as M3/M5, one round later, in the text written to close
them: a live wildcard that the compiler sees and the scan does not. It is a strictly cheaper
evasion than either M3 or M5, because it needs no comment at all.


BLOCKING

R3-B1 — a wildcard arm sharing a LINE with a named arm is accepted, re-opens #826 in full, and emits no warning

audit_collision_arms builds arm_lines as whole lines containing =>
(crates/eqoxide-nav/src/zone_assets.rs:901) and then asks only whether each line contains the
substring Self:: (:903). A wildcard appended to a named arm's line therefore rides in on that
line's Self:: token, and the arm-line count stays at 4, so the new equality at :919 — the
control added this round specifically to catch a fifth arm — does not fire either.

Three shapes, all with a live _ in collision's match, all ACCEPTED:

probe arm text audit verdict
P1 Self::Failed {..} => None, #[allow(unreachable_patterns)] _ => None, ACCEPTED (green)
P2 Self::Failed {..} | _ => None, ACCEPTED (green)
P5 Self::Failed {..} => None, _ => None, ACCEPTED (green)

(Controls in the same run, so the probe harness is not vacuous: P3 — a wildcard on its own
line alongside a collapsed pair — is correctly rejected by the arm check; P4 — a legitimate
partial collapse with no wildcard — is rejected by the count equality.)

P1 carried through to the actual property, as a real source mutation

The above is the pure function. Here is P1 written into collision's real body, measured on
ddb470a.

Control — fifth variant ProbeRefreshing { zone: String, collision: Arc<Collision> } added,
collision left pristine. check -p eqoxide-nav --all-targets --locked, E0004 sites:

zone_assets.rs:105  zone_assets.rs:118  zone_assets.rs:154 (collision)
zone_assets.rs:165  zone_assets.rs:356  zone_assets.rs:471  zone_assets.rs:699

Mutation — same fifth variant, collision's arms in the P1 shape. E0004 sites:

zone_assets.rs:105  zone_assets.rs:118  zone_assets.rs:165
zone_assets.rs:356  zone_assets.rs:471  zone_assets.rs:699

:154 is gone. collision is no longer compiler-demanded — which is precisely and only the
property this PR exists to create. Count of unreachable pattern / unreachable_patterns warnings
in that build: 0. So there is no compiler signal of any kind.

And the guard is green over it. P1 applied to collision on the four-variant tree,
test -p eqoxide-nav --locked --no-fail-fast:

test zone_assets::tests::collision_arm_audit_rejects_the_evasions_measured_against_it ... ok
test zone_assets::tests::collision_matches_only_named_variants_no_wildcard_arm ... ok
test result: ok. 240 passed; 0 failed; 16 ignored; 0 measured; 0 filtered out

Exactly one Compiling eqoxide-nav line in that run's stderr, so this is not a cached artifact.
Every mutation was applied from, and reverted to, a cp -p snapshot; the file is byte-identical to
ddb470a afterwards (md5 d417f0b3…, clean git status) — which also independently confirms the
md5 you cited. No git stash, no git restore.

So: wildcard live, #826's hole fully re-opened, zero compiler warnings, pin GREEN. That is the
round-2 M3/M5 outcome reproduced against the rewrite that was built to end it.

Why the existing mitigations do not cover it

  • The rustdoc leans on rustfmt (limit (c), :977-979: "which is how rustfmt leaves them"). I
    grepped .github/workflows/ for a cargo fmt or cargo clippy gate: none — test.yml is the
    only workflow and it has neither.
    So nothing in the merged tree enforces the one-arm-per-line
    convention the guard's soundness now rests on.
  • The #[allow(unreachable_patterns)] in P1 is not exotic — it is lifted verbatim from the PR's
    own M5 test case at :1082, so it is a shape this guard's author already has on file as a thing
    a regression does.

Suggested fix (cheaper than another prose round)

Stop reasoning about lines and reason about arms:

  1. Split body on "=>" and take the text before each as a pattern, instead of
    body.lines().filter(|l| l.contains("=>")).
  2. Require the count of => occurrences to equal COLLISION_VARIANTS.len() — this alone kills
    P1 and P5 (5 occurrences vs 4).
  3. Reject a standalone _ token appearing anywhere in a pattern segment — this is what kills P2,
    which the => count cannot see because the or-pattern adds no =>.

Then add P1, P2 and P5 to collision_arm_audit_rejects_the_evasions_measured_against_it as data,
which is exactly the strength of the round-3 design.

R3-B2 — two tracked sentences are measured false by R3-B1

Both are in the rustdoc on collision_matches_only_named_variants_no_wildcard_arm, and both are
the overstated-guarantee pattern this PR has already corrected twice.

(a) :965-968, the "What it proves" headline:

What it proves: a wildcard arm in collision's match is caught, including one hidden in a
trailing line comment or behind a block comment

A wildcard arm in collision's match is not caught when it shares a line with a named arm —
measured above, three shapes, one carried to a live E0004 hole. Suggested replacement:

What it proves: a wildcard arm that occupies its own line — including one hidden in a trailing
line comment or behind a block comment — is caught. A wildcard sharing a line with a named arm is
not: it inherits that line's Self:: token and does not change the arm-line count. Measured
on this tree (#837 review round 4, probes P1/P2/P5).

(b) :978-979, limit (c):

and a collapsed match is rejected by the arm-count equality rather than mis-scanned

Only true of a collapse that reduces the line count (P4, correctly rejected). A collapse that
merges a wildcard onto a named arm's line holds the count at exactly 4 and is mis-scanned — P1
and P5. As written this sentence tells a reader the equality closes the collapse case, and it closes
half of it. Suggested replacement:

and a collapse that reduces the arm-line count is rejected by the equality — but a collapse that
merges a wildcard onto a named arm's line keeps the count at 4 and IS mis-scanned (P1/P5).

I am flagging (a) and (b) as blocking on this project's standard that a false sentence in a tracked
file is blocking, not as separate defects from R3-B1 — fixing the code per R3-B1 makes both sentences
repairable in one edit.


NON-BLOCKING

R3-N1 — the stripper's own reach control IS one-directional, as suspected; the anchor is what catches over-stripping, and it names the wrong cause

The hypothesis I was given: "canary absent after stripping" plus a length check is satisfied by a
stripper that deletes its entire input.
Confirmed, and it is exactly satisfiedSRC.contains
passes, !stripped.contains passes, and stripped.len() < SRC.len() passes trivially at 0.

But it does not produce a silent pass. Measured: strip_comments mutated to
if true { return String::new(); }, test -p eqoxide-nav --locked --no-fail-fast, one Compiling eqoxide-nav line:

test result: FAILED. 238 passed; 2 failed; 16 ignored; 0 measured; 0 filtered out

Both guard tests RED. So the outcome is safe — the anchor uniqueness check backstops it, not the
stripper's own control. Two consequences worth writing down rather than leaving implied:

  1. The three assertions at :1000-1013 are labelled "REACH CONTROL ON THE STRIPPER ITSELF" and
    detect a no-op stripper only. They do not detect an over-stripping one; nothing there is
    two-directional. Worth saying so beside them, since the comment currently reads as though it
    covers the stripper generally.

  2. The message names a cause that did not occur — the same diagnosis defect you fixed this round
    for the arm check (round-2 N1). Verbatim from the run:

    reach control: expected exactly ONE ... found 0. 0 means this scan would have examined nothing (the signature was reformatted or renamed) — fix the anchor, do not delete this test.

    The signature was neither reformatted nor renamed; the stripper ate it. Adding "…or the comment
    stripper over-stripped" to that message costs one clause and points the next engineer at the
    thing you built a control for.

R3-N2 — no cargo fmt --check anywhere in CI

Stated under R3-B1 but it stands alone: .github/workflows/test.yml is the only workflow and
contains neither cargo fmt nor cargo clippy. Any guard whose soundness rests on rustfmt output
(limit (c) does, explicitly) is resting on something unenforced. Not this PR's job to add, but the
rustdoc should not cite rustfmt as though it were a gate.


What I could NOT break

  • Attack A — the witness problem. Refuted, with a measurement rather than a read. The production
    pin really does audit the real file: SRC = include_str!("zone_assets.rs") at :986, passed to
    audit_collision_arms(SRC) at :1015. This is not the fix(#780): make reporting the joint-cap downgrade a precondition of choosing a render arm #843 shape (a throwaway destination
    passed as a parameter). The proof that the call site is load-bearing is incidental to R3-N1: in the
    over-strip run, collision_matches_only_named_variants_no_wildcard_arm panicked at
    zone_assets.rs:1016:55
    — inside the audit_collision_arms(SRC) call — so that call is reached
    and its Err is fatal. A severed call site would not have produced that failure.
  • The comment stripper, on the constructs this file actually contains. I attacked the
    string/char/lifetime state machine looking for an under-strip (a stray unpaired " inverting
    string state would resurrect M3 by leaving a comment unstripped). '"' and '\\' are both handled
    by the :792/:808 arms; escaped char literals, \-at-EOL continuations and nested block
    comments all trace correctly. Reasoned from source, not run — I did not find a candidate
    concrete enough to be worth a build, and I am not claiming the stripper is sound in general, only
    that I failed to break it.
  • The raw-string refusal. Fires as an Err naming raw strings, i.e. loud — it is not a
    silently-audited-nothing path. Confirmed by the PR's own case at :1141-1142 and consistent with
    the has_raw_string conservative direction. Verified by reading plus that test passing on the
    pristine run; I did not add an independent raw string.
  • COLLISION_VARIANTS.len() equality satisfiable while arms are wrong. Yes — but this is already
    disclosed as limit (b) (Self::New {..} => None), accurately. My P1/P5 are a second way to
    satisfy it, which is R3-B1 and is not disclosed.
  • The fix itself. The four explicit arms are behaviourally identical to the old wildcard for the
    four current variants, and the control run reproduces the E0004 property exactly (7 sites, :154
    among them). Nothing I did made collision() return the wrong grid for an existing variant.
  • The anchor uniqueness reach control, which did real work twice in this review (it is what turns
    total over-stripping into a red rather than a pass).

Test run — mine, independent

Baseline over ddb470a plus my own probe test (test -p eqoxide-nav --locked --no-fail-fast -- --nocapture), stdout/stderr captured separately:

figure measured
compile sentinel (compile only, NOT completion) Finished `test` profile [unoptimized + debuginfo] target(s) in 47.56s
Compiling eqoxide-nav lines 1 — no result here came from a cached binary
result test result: ok. 241 passed; 0 failed; 16 ignored; 0 measured; 0 filtered out
pristine baseline (P1 run, no probe test) 240 passed; 0 failed; 16 ignored

241 = your 240 + my one probe test; 240 + 16 ignored = 256, which agrees with the 256 you report
for this target in the workspace run. I did not run the full workspace suite — my findings are
confined to one crate and every one is reproduced at -p eqoxide-nav granularity, so I am not
publishing workspace figures I did not measure.

Scope I did not cover, stated rather than skipped: no live client run. Consistent with your own
"NOT verified" section — nothing here changes runtime behaviour for the four current variants, so
there is no live-observable symptom to E2E. The mutation-checked suite is the ceiling for this change.


Summary. The code change (four explicit arms) remains sound and I could not break it. The
guard is not sound: three wildcard shapes are accepted, one of them measured all the way to a
live E0004 hole with zero compiler warnings and the pin green. That is R3-B1, and R3-B2 is the two
sentences that become false because of it. R3-N1/N2 are non-blocking. Attack A refuted; Attack B real
but backstopped.

Not approving, not merging — posting as a comment (the fleet shares one identity and self-approval is
blocked).

djhenry and others added 2 commits August 1, 2026 11:25
…es overstating it

Round 4 of the #837 review measured three wildcard shapes ACCEPTED by the
line-wise scan, each riding in on a named arm's own `Self::` token:

    Self::Failed {..} => None, #[allow(unreachable_patterns)] _ => None,
    Self::Failed {..} | _ => None,
    Self::Failed {..} => None, _ => None,

The first and third also held the arm-LINE count at 4, so the equality did not
fire either. All three were carried through to the real property: with a fifth
variant on `ZoneAssetState`, each removed `collision`'s match from rustc's
E0004 list, with zero `unreachable_patterns` warnings (control, pristine arms:
present). Reproduced here before the fix (all three `Ok(())`) and after it (all
three `Err`, naming the wildcard).

The scan now splits `collision`'s match into arms at top-level commas, takes
the text before each `=>` as the pattern, rejects a bare `_` token at pattern
top level (which is what catches the or-pattern case, since it adds no `=>`),
and requires every top-level or-pattern alternative to begin with `Self::`.
The `=>`-occurrence equality is kept SEPARATE from the arm-count equality: a
block-bodied arm may omit its comma and merge two arms into one chunk, and
merging cannot reduce the number of arrows.

Prose corrected to what is now established, and not wider:

* the "What it proves" headline no longer claims line layout is irrelevant by
  accident — it says so as the round-4 fix, and enumerates the four things the
  scan actually asserts;
* limit (c) no longer says "a collapsed match is rejected by the arm-count
  equality". That was true only of a collapse that REDUCES the count. It now
  names the real residual (comma-optional block-bodied arms), says which check
  closes it, and states plainly that nothing enforces arm layout in CI —
  `.github/workflows/test.yml` is the only workflow and it runs neither
  `cargo fmt` nor `cargo clippy` (grepped, 0 hits);
* the anchor-uniqueness failure message named "the signature was reformatted or
  renamed", which is not the cause when an over-stripping `strip_comments` eats
  it. It now names over-stripping too, and the rustdoc states that the
  stripper's own reach controls are ONE-DIRECTIONAL (they detect a no-op, not
  an over-strip) with the anchor check as the backstop.

The three shapes are now test data in
`collision_arm_audit_rejects_the_evasions_measured_against_it`, alongside a new
POSITIVE case (`Self::Ready { collision, terrain_meshes: _, .. }`) proving the
wildcard check tests pattern POSITION and not merely the character `_`.

Every hunk is inside `mod tests`; no production code changed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
@djhenry

djhenry commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

Round 4 applied — head aadc8b5

Merged origin/main (7cd9652) in first; no rebase, no force-push. Every hunk is inside
#[cfg(test)] mod testsno production code changed (12 hunk headers, all mod tests).


B1 — the guard reasoned per-LINE, the compiler reasons per-ARM. Fixed, with both halves measured.

Verified the claim first rather than trusting it. A probe test printing
audit_collision_arms(...) for the three shapes, run on the guard as it stood at ddb470a
(1 Compiling eqoxide-nav line, so not a cached artefact):

R4PROBE P1 => Ok(())
R4PROBE P2 => Ok(())
R4PROBE P5 => Ok(())
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 262 filtered out

Carried to the real property, all three — not just P1. Fifth variant
(ProbeRefreshing { zone: String, collision: Arc<Collision> }) inserted as exactly 2 lines so
collision's match sat at :154; check -p eqoxide-nav --all-targets --locked; each run shows
exactly 1 Checking eqoxide-nav line.

tree E0004 sites in zone_assets.rs :154 lines matching unreachable
CONTROL — pristine arms 105, 118, 154, 165, 356, 471, 699 present 0
P1 … => None, #[allow(unreachable_patterns)] _ => None, 105, 118, 165, 356, 471, 699 gone 0
P5 … => None, _ => None, 105, 118, 165, 356, 471, 699 gone 0
P2 Self::Failed {..} | _ => None, 105, 118, 165, 356, 471, 699 gone 0

So all three are real holes with no compiler signal of any kind, not just guard-internal nits.
Probe reverted from a cp -p snapshot and touched; file byte-identical to ddb470a
(md5 d417f0b3…, clean git status) before the fix was written.

The fix, three steps as suggested:

  1. The match's arm region is isolated by a brace walk from the body's first { (with a
    reach control that the text before it contains match), then split into arms at top-level
    commas
    — depth-aware, so the comma in Self::Ready { collision, .. } is not an arm boundary.
    The pattern is the text before that arm's first =>.
  2. arms_region.matches("=>").count() must equal COLLISION_VARIANTS.len(), kept separate
    from the arm-count equality (see limit (c) below for why).
  3. A bare _ token at pattern top level is rejected — this is what kills P2, which adds no
    => at all — and every top-level or-pattern alternative must begin with Self::, which is
    what kills the bare-binding variant of the same shape (Self::Failed {..} | other).

After, same probe, same file, 1 Compiling eqoxide-nav line:

R4PROBE P1 => REJECTED: `ZoneAssetState::collision` must match every variant BY NAME (#826). This arm's pattern contains a bare `_` wildcard at the top level:
R4PROBE P2 => REJECTED: `ZoneAssetState::collision` must match every variant BY NAME (#826). This arm's pattern contains a bare `_` wildcard at the top level:
R4PROBE P5 => REJECTED: `ZoneAssetState::collision` must match every variant BY NAME (#826). This arm's pattern contains a bare `_` wildcard at the top level:

All three are now permanent data in collision_arm_audit_rejects_the_evasions_measured_against_it
(the case array went […; 8][…; 11]). The temporary probe test was removed before commit
(grep -c 'R4-PROBE\|r4_probe' = 0).

One positive case added too, because "reject _" would otherwise be over-broad and get the
guard deleted rather than fixed on the first legitimate field pattern:
Self::Ready { collision, terrain_meshes: _, .. } must still PASS. It does — the depth condition
in has_top_level_wildcard_token is what separates a nested field _ from a wildcard binding.

B2 — the two false sentences

"What it proves" rewritten to the four things the scan now actually asserts (arm count, =>
count, every variant named, and every arm pattern free of a top-level _ / with all alternatives
Self::-prefixed), with "line layout does not enter into it" stated as the round-4 fix rather
than implied. A new paragraph above it records the three shapes and their E0004 consequence, so
the measurement is in the tree and not only in this thread.

Limit (c) no longer says "a collapsed match is rejected by the arm-count equality". It now
names the actual residual: a block-bodied arm may legally omit its comma
(Self::Idle => { None }), which merges two arms into one comma chunk. That does not admit a
wildcard — merging cannot reduce the number of =>, which is exactly why the arrow-count equality
is separate — but it does mean a legitimate block-bodied rewrite fails this test loudly rather
than passing. The arm-count failure message names that cause, so the diagnosis matches.

The rustfmt/CI point, verified independently rather than taken from the review: one workflow
file (test.yml), and grep -rniE "cargo[[:space:]]+(fmt|clippy)|rustfmt|clippy" .github/ returns
0 matches. Limit (c) now says that outright — "it is a habit, and this scan no longer depends
on it" — instead of citing rustfmt as though it were a gate.

B3 (promoted from N1) — the stripper's failure message named a cause that did not occur

The anchor-uniqueness message said 0 means … (the signature was reformatted or renamed). When
strip_comments over-strips, the signature was neither. It now reads:

0 means this scan would have examined nothing — either the signature was reformatted or renamed, or strip_comments OVER-stripped and removed it (that is the one failure the stripper's own reach controls cannot see). Fix the anchor or the stripper; do not delete this test.

And the one-directionality is now stated in two places rather than left to be inferred: beside the
three assertions themselves (which detect a no-op stripper and nothing else — String::new()
satisfies all three), and in the test's rustdoc, naming the anchor check as the backstop and citing
your round-3 measurement that the mutation turned both guard tests RED from inside
audit_collision_arms(SRC).


Suite — test --workspace --locked --no-fail-fast, stdout and stderr captured separately

figure measured
compile sentinel (compile only, NOT run-completion) Finished `test` profile [unoptimized + debuginfo] target(s) in 2m 58s
^running [0-9]+ tests?$ headers 55
^test result: lines 55 — equal, so no binary was lost
non-canonical test result: lines 0
FAILED result lines 0
^running 0 tests$ (targets with nothing to run) 14
^test result: ok\. 0 passed; 0 failed; 0 ignored 14 — agrees
compiler ^error / ^warning lines in stderr 0

Four-term reconciliation, summed over all 55 result lines:

passed 1842 + failed 0 + ignored 47 + filtered out 0 = 1889
running total                                        = 1889
delta = 0

Nothing to reconcile by name — the delta is zero.

bash scripts/check-no-local-detail.sh → exit 0, OK — no forbidden patterns in tracked files.

Not established / out of scope

  • No live run. The whole change is inside mod tests; there is no runtime behaviour to E2E.
    Same conclusion you reached — the mutation-checked suite is the ceiling here.
  • I did not re-attack the comment stripper's state machine. Your round-3 read of it stands
    unchallenged by me; I only changed what happens after stripping, plus the two messages.
  • B1's fix is not a proof that no wildcard shape survives. It is: three measured shapes went
    from accepted to rejected, plus the two counts and the alternative-prefix rule. The limits
    section names the one residual I found and can construct (comma-optional block-bodied arms) and
    says which check closes it. I did not find a fourth evasion; I am not claiming there is none.
  • The local branch ref still points at ddb470a because it is checked out in another worktree; I
    pushed by refspec (HEAD:fix-826-collision-arms) rather than touch it. origin is correct.

@djhenry
djhenry merged commit fa901c4 into main Aug 1, 2026
2 checks passed
djhenry added a commit that referenced this pull request Aug 1, 2026
…ollapsing two cadences into one

`PlayerHoldView` — the "the character is physically stuck and the client cannot free it" observable from #724 — existed, was populated, was covered by tests, and **reached no response body at all**. `observe::get_debug` hand-builds its `player` object and patches extras in with `player.insert`; nothing serialises `PlayerState` whole, so `hold`'s `Option` and its lack of `skip_serializing_if` guaranteed presence in a serialisation that never happens on any route.

To an agent driving this client that is the failure #724 was filed to prevent, one level further out: it has no eyes on the screen, so a stuck character it cannot observe as stuck is a character that is simply not moving for no stated reason. The fix is one `player.insert("hold", …)` alongside `levitating`/`run_mode`/`afloat_stall`.

## What the tests do and do not establish

The pin asserts `contains_key("hold")` on bytes returned by the **real router**, not on the attribute and not on a test that serialises `PlayerState` directly — which is the only shape that could have caught the original defect. `serde_json`'s `Index` returns `Value::Null` for an absent key exactly as it does for an explicit one, so `is_null()` cannot tell the two apart; the original assertion written that way was vacuous, and the deliberate tripwire that did the work was the `!contains_key` form. The PR body had those two the wrong way round and is corrected here.

The corresponding comment is now scoped to what was measured rather than what was hoped: the always-present key is a guarantee to a **grepping or `contains_key`-using** reader. `v["player"]["hold"]` cannot distinguish absent from null, so an agent that reads it that way learns nothing from the key's presence.

## The cadence claim, corrected

Review found the doc asserting the value is "mirrored into `GameState` every controller-stepped frame". It is not. `ActionLoop::stream_position` runs on the **net thread**, every tick (its own rustdoc says so), and copies a `ControllerView` snapshot that the **render** thread republishes per rendered frame. Two cadences collapsed into one.

That matters beyond pedantry: the collapsed form implies the value is at most one controller step old, when an idle render loop leaves the net thread faithfully re-copying a stale view. That is precisely the mode `PlayerHoldView`'s own staleness bullet and `docs/http-api.md` already warn about — so the new prose was quietly weakening a correct existing disclosure. Both sites now name both cadences, say the mirror is as fresh as the last *published frame* rather than the last *net tick*, and point at the staleness bullet with an explicit note that this sentence must not be read as softening it. `src/movement.rs` already said "every net tick" correctly and was the model.

One stale citation went with it: a `heading_ccw` convention note cited `GET /v1/observe/state`, a route that does not exist and which the same doc contradicts twice. Corrected to `/v1/observe/debug`, matching the row already in `docs/http-api.md`.

## Verification

The round-2 commit is comment-only, proven mechanically rather than asserted: filtering the `-U0` diff for `///`, `//` and blank lines leaves **0** content lines. Scoped to the blast-radius crates on the same merged tree, before vs after: `eqoxide-http` 270 and `eqoxide-net` 383, both runs 4 headers against 4 result lines, 0 error/warning, and the result lines byte-identical with finish times stripped. Two `Compiling` lines in the after-run, so it was not served from cache. `eqoxide-net` was run as a control specifically because it `include_str!`s `docs/http-api.md` — a doc this PR's base commit touches, and a crate nobody edited.

Cross-checked against #837 before merge: that PR's production change to `zone_assets.rs` is arm expansion with no enum or signature change, and the two PRs share no file.

## Known limits

No live run, and none would help: the round-2 change is comment-only and the base change is a serialisation the router test already exercises end to end.

The staleness the corrected prose now describes is **disclosed, not fixed**. A hold that begins while the render loop is idle is still mirrored late, and this PR does not change that — it stops the documentation from denying it. The reviewer raised a related open question about whether `PlayerHoldView`'s "never stale-because-idle" wording survives a `#summon` landing on the net thread while the render loop idles; that is pre-existing prose and is tracked separately rather than folded in here.

Closes #817
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

zone_assets: collision()'s _ => None wildcard can make usable_collision's documented-unreachable arm reachable (#803 follow-up)

1 participant