Skip to content

fix(#807): give the corpus prologue one owner, so its drop paths cannot drop a zone silently - #835

Merged
djhenry merged 5 commits into
mainfrom
fix-807-drop-paths
Aug 1, 2026
Merged

fix(#807): give the corpus prologue one owner, so its drop paths cannot drop a zone silently#835
djhenry merged 5 commits into
mainfrom
fix-807-drop-paths

Conversation

@djhenry

@djhenry djhenry commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Closes #807.

Round 2 — review response (text only; no executable change)

Round 1 review found the code right and both blocking findings were false sentences in tracked
files. Fixed in water_grid.rs, doc/comment only:

  • F1 — tier 1 said "every return … is preceded by a skip/add" and "there is no path
    through it that leaves a zone unaccounted". The Ok tail return is a return, is not preceded by
    either, and leaves the zone open — falsified by this PR's own green test. Narrowed to Err
    returns; the "no path" sentence is deleted, not qualified. Same correction applied to the headline
    below.
  • F2WaterRollup's type doc still described the five corpora this PR converts as dropping
    zones into a plain Vec<String> with no ratio and "NOT covered by anything in this file". All
    four clauses were false after this PR, and it mislocated water_grid_budget_measurement in
    tests/walker_sim.rs when it lives in crates/eqoxide-nav/src/collision.rs. Rewritten to say
    what is actually still uncovered and to point at Seven more zone loops in collision.rs drop zones out of a printed corpus total without accounting (four silently) #839 for the list rather than restate a tally
    that will drift.
  • F3 — the rustdoc claimed the drop statements "exist once". faithful_walker_drift_corpus
    keeps its own inline copy (deliberately — it drives two rollups and cannot use this single-rollup
    signature). "One owner" now says five corpora, not six.
  • F4 — added a tier-list line disclosing that nothing in CI calls open_corpus_zone; every
    new test calls open_corpus_zone_with. A mutation of format!("{zone}.glb"), "maps/water", or
    the cell pass-through would survive the whole suite. Disclosure, not a new assert.
  • F6Closes #807 kept (the ten enumerated sites are done); Seven more zone loops in collision.rs drop zones out of a printed corpus total without accounting (four silently) #839 filed and cited for the
    ten that remain (see the note below — Seven more zone loops in collision.rs drop zones out of a printed corpus total without accounting (four silently) #839's own count was corrected from 7 to 10 after filing).

Not done, per the orchestrator's scope ruling: F5 (assert ordering in the four corpora — a code
change, now #839's secondary item); converting any of the five remaining collision.rs loops
(#839's whole job); the ZoneDropped display-string inconsistency the reviewer flagged and
explicitly did not ask to be fixed; any tier-1 type-level move (the reviewer looked and concluded
correctly that Drop-based settling would turn a loud unaccounted into a silent add).

The mutation table and reach control below were not re-run: nothing executable changed. Test
figures for round 2 are at the bottom.

What was wrong

Five corpora each carried their own byte-identical copy of the zone-open prologue:

let Ok(za) = ZoneAssets::from_glb(&p) else { println!("{zone:<12} (no glb — skipped)"); continue };
let mut col = Collision::build(&za, 32.0);
if col.cols == 0 { println!("{zone:<12} (no grid — skipped)"); continue; }

Both continues leave the loop body without telling anybody. The corpus then prints a TOTAL
over whatever zones happened to survive, and passes. A run over 1 of 2 zones is indistinguishable
from a run over 2 of 2 — the number looks measured and is not. #802 wired the two sites in
faithful_walker_drift_corpus; the rest were left as "same shape, do it later".

What this changes

One owner for the prologue, in the crate: eqoxide_nav::water_grid::open_corpus_zone
(+ open_corpus_zone_with, the closure-injected variant so the drop paths are testable with no
baked assets). It takes &mut WaterRollup and returns Result<(Collision, ZoneWater), ZoneDropped>.
Every Err return in it is preceded by a skip or an add, so a caller that ignores the Err
value entirely still cannot lose the zone. The Ok tail return deliberately leaves the zone open —
closing it is the caller's obligation, a forgotten close lands in unaccounted, and
open_corpus_zone_leaves_a_ready_zone_open_for_the_caller_to_close_807 pins exactly that. The five corpora now call it and close each opened zone
with a real per-zone water number, then assert cover.is_complete().

Ten inline drop sites became three, and those three are inside a library function that CI compiles
and five new non-#[ignore]d tests exercise. CI could not previously reach any of them — the
corpora are all #[ignore]d and asset-gated, and CI passes no --ignored (#777/#799).

The rollup total is a real measurement, not a placeholder: the blast-radius corpora fold the count
of start/goal pairs their water filter actually excluded; the budget corpus folds
wet_column_count().

Correction to the issue text

#807 says "eleven remain unaccounted". Re-derived on the base: it is ten. #802 round 3 landed a
third skip in faithful_walker_drift_corpus after the reviewer took the 13/2 count. 13 = 3
(faithful) + 8 (four blast-radius x 2) + 2 (budget corpus). This PR closes the 10.

Test figures

Base 0497f6b and this branch, cargo test --workspace --locked --no-fail-fast, both from logs I
read (not from the exit code):

figure base 0497f6b this branch
(1) Finished `test` profile compile sentinel present (23m 20s); 0 ^error, 0 ^warning present (6m 36s); 0 ^error, 0 ^warning
(2) running N tests? headers vs test result: lines 55 vs 55 55 vs 55
(3) non-canonical test result: lines 0 0
(4) targets with 0 passed; 0 failed; 0 ignored 14 14
(5) passed+failed+ignored+filtered vs header sum 1830+0+47+0 = 1877 = header sum 1835+0+47+0 = 1882 = header sum

Delta +5, reconciled by name (set-diff of every ^test … ... <outcome> line in both logs;
zero removed):

water_grid::tests::open_corpus_zone_records_a_missing_glb_as_skipped_807
water_grid::tests::open_corpus_zone_records_an_empty_collision_grid_as_skipped_807
water_grid::tests::open_corpus_zone_records_an_unloadable_wtr_as_unmeasured_807
water_grid::tests::open_corpus_zone_leaves_a_ready_zone_open_for_the_caller_to_close_807
water_grid::tests::a_corpus_using_the_prologue_cannot_publish_a_total_over_a_shrunken_zone_list_807

Mutation table — library (3 drop sites, 3 REDs)

Baseline 20 passed; 0 failed. Every mutation reverted from a cp -p copy and re-verified with
md5sum -c (all OK) before the next.

# mutation result killed by
M1 delete DROP-1 cover.skip(zone, "no glb") RED 18/2 …records_a_missing_glb_as_skipped_807, a_corpus_using_the_prologue_…
M2 delete DROP-2 cover.skip(zone, "no grid") RED 18/2 …records_an_empty_collision_grid_as_skipped_807, a_corpus_using_the_prologue_…
M3 delete DROP-3 cover.add(zone, &zw.tally()) RED 18/2 …records_an_unloadable_wtr_as_unmeasured_807, a_corpus_using_the_prologue_…
M4 conflate: DROP-3 addskip(zone, "no wtr") RED 18/2 same two — an unreadable .wtr must land in unmeasured, not skipped
M5 delete cover.begin_zone(zone) RED 15/5 all five; WaterRollup panics with no zone open (fail-closed)
M6 wrap evasion: DROP-1 → if false { cover.skip(…); } RED 18/2 same as M1 — the pin is execution-observable, not source text (#799)
M7 change DROP-1's free-text tag "no glb""glb absent" SURVIVED (20/0) correct signature — the guard pins the accounting, not the diagnostic wording

M6 is the reason this is a type-and-execution fix rather than a source-text scanner: none of the
seven evasions in #799 apply, because nothing here reads source.

Reach control — 5 real call sites, on real baked assets

Debug builds only. Small ZONES=/PAIRS= subsets against a one-zone scratch asset dir.

(A) a zone that drops must now go RED. ZONES=<real>,ghostzone (ghostzone has no .glb):

corpus result
goal_append_blast_radius REDcovers 1/2 … skipped: ["ghostzone"]
corner_buffer_blast_radius RED — same
descent_guard_blast_radius RED — same
parallel_wall_clearance_blast_radius RED — same
water_grid_budget_measurement REDmeasured 1/2 … skipped: ["ghostzone"]

5 sites → 5 REDs.

(B) forgetting to close a zone must go RED too (this is the obligation the function cannot
discharge for the caller). Deleted each corpus's loop-tail cover.add(zone, …) one at a time:

corpus result
goal_append_blast_radius RED
corner_buffer_blast_radius RED
descent_guard_blast_radius RED
parallel_wall_clearance_blast_radius REDunaccounted: ["<zone>"]
water_grid_budget_measurement REDunaccounted: ["<zone>"]

5 sites → 5 REDs, each diagnosed as unaccounted (left the loop body without reaching add or skip — a corpus WIRING bug, not an asset problem), not as a missing asset.

(C) both-directions control — the defect, reproduced. Spliced the base's version of
parallel_wall_clearance_blast_radius back over mine (call site only) and re-ran the exact (A)
input. Output:

zone          pairs  routed  complete  refused
<real>            2       2         2        0
ghostzone    (no glb — skipped)

TOTAL pairs 2  routed 2  complete 2  refused 0
test parallel_wall_clearance_blast_radius ... ok

GREEN, with a TOTAL published over half the named corpus. That is #807.

#807's two open questions, measured

"Are all remaining paths the same shape?" — Yes. Read all five: identical two conditions in
identical position, both before the water check, all ten correctly classified as skip (the water
check never ran, so unmeasured would be a lie). The third path (.wtr did not load) is a
different bucket (unmeasured) and was already accounted by #762; it is folded into the same
function to keep the three tiers in one place.

"Does any of them currently drop a zone in practice?" — Measured on a machine with a complete
asset set: no, all default-corpus zones load. water_grid_budget_measurement over its 3
default zones reported wet columns: 18468 (over 3/3 zones); the walker_sim corpora over their 10
default zones reported (over 10/10 zones). No no glb, no cols == 0, no unreadable .wtr in
13 distinct zones. The paths are latent here — which is exactly why they were invisible. They
fire on a partial asset dir, which is the normal state of a dev box (#802's own review measured
that).

NOT verified, and not claimed

…ot drop a zone silently

Six per-zone corpora opened a zone with the same six lines (load `<zone>.glb`, build a
`Collision`, load `<zone>.wtr`, install it). #762 wired its rollup into one of the six copies.
In the other five, a zone dropped for "no glb" or "no grid" left BOTH the numerator and the
denominator, so the corpus printed a `TOTAL …` line over a silently smaller corpus than the one
it names — a number that looks measured and is not.

`open_corpus_zone` (crates/eqoxide-nav/src/water_grid.rs) is now the single place those three
drop paths exist, and each one closes the zone on the rollup before returning. The four
`*_blast_radius` corpora and `water_grid_budget_measurement` call it and close a scored zone with
`cover.add(...)`; anything that leaves the loop body without reaching that line is recorded as
`unaccounted` by `WaterRollup` itself, so there is no per-`continue` wiring left to forget.

The blast-radius corpora fold a REAL per-zone water number — the count of sampled start/goal
pairs the water filter excluded — so their coverage line is a measurement rather than a
placeholder zero.

Refs #762, #802, #805.

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 review — PR #835

Reviewed on the merged tree, not the branch in isolation: b18269d (PR head) + origin/main 716cd28 (#829) → merge commit 659e1d3. git diff 0497f6b 716cd28 touches only crates/eqoxide-net/src/action_loop.rs and docs/http-api.md — no file, type, or symbol overlap with this PR's three files, and no behavioural overlap (#815 is client-side /map GAP reporting; this is test-corpus rollup accounting). No cross-PR semantic conflict found.

Verdict: CHANGES REQUESTED — 2 blocking, 4 non-blocking. The code is right; both blocking findings are false claims in tracked files, which is the defect class this gate keeps finding. Both are text-only fixes.


The load-bearing universal: verified, but as written it is false

I enumerated every exit from open_corpus_zone_with: three return Err, one Ok tail expression, no ?, no break, no other return. The three Err returns are each preceded by a skip/add. The only panic sources are the injected closures, and a panic fails the run loudly (fail-closed). Re-entry and never-closing are both handled by begin_zone/open and pinned by pre-existing tests at water_grid.rs:982-995.

So the true claim is: every Err return is preceded by a skip/add. That claim holds and I killed it three ways (below).

F1 — BLOCKING. Tier 1 of the rustdoc claims something that is only tier 2, and the PR's own test falsifies it

crates/eqoxide-nav/src/water_grid.rs:606-609:

  • Guaranteed: every return out of this function is preceded by a skip/add that closes the zone this function's own begin_zone opened. There is no path through it that leaves a zone unaccounted.

Both sentences are false on the success path:

  • Ok((col, zw)) is a return out of this function and it is not preceded by a skip/add.
  • It leaves the zone in open, which unaccounted_zones() reports via .chain(self.open.as_deref()).

This is not a reasoned objection — it is measured, by this PR's own test, green in my run:

water_grid::tests::open_corpus_zone_leaves_a_ready_zone_open_for_the_caller_to_close_807 ... ok
    assert_eq!(cover.unaccounted_zones(), vec!["gfaydark"]);

and tier 2, three lines below, says the opposite outright ("the caller must close a zone this returns Ok for … the zone lands in unaccounted"). So the answer to "is anything in tier 1 actually only tier 2?" is yes: the totality. The PR body's headline carries the same over-quantification ("Every return in it is preceded by a skip or an add"); its trailing clause ("a caller that ignores the Err value …") is the correct version.

Fix: every Err return …, and delete or re-scope the "no path" sentence. Text only — no code change, no re-run of the mutation table.

F2 — BLOCKING. WaterRollup's type doc now names five corpora as unaccounted that this PR just accounted

crates/eqoxide-nav/src/water_grid.rs:416-421, unmodified by this PR:

What this still does NOT cover … The four *_blast_radius corpora and water_grid_budget_measurement in tests/walker_sim.rs, and the zone loop in collision.rs, accumulate into a plain Vec<String> and still drop zones without accounting. They print no ratio, so they lie by omission rather than by assertion, but they are NOT covered by anything in this file.

Every clause is now false, about the exact five corpora this PR converts:

clause status after this PR
"accumulate into a plain Vec<String>" false — all five let mut unmeasured: Vec<String> were deleted
"still drop zones without accounting" false — that is what the PR fixes
"They print no ratio" false — all five now println!("… {cover}"), which is literally the ratio
"NOT covered by anything in this file" false — they are covered by open_corpus_zone, in this file
"water_grid_budget_measurement in tests/walker_sim.rs" pre-existing error — it is in crates/eqoxide-nav/src/collision.rs:7313

Why this is blocking and not cosmetic: this paragraph is the only tracked record in the tree of which corpora are still unaccounted, and it was written specifically ("stated so nobody re-derives the round-2 sentence") to be that record. Merging as-is leaves a reader who greps for coverage with a paragraph that (a) denies work that was done and (b) still says nothing about the five loops that genuinely remain (F6). #807's own argument — "a partial fix to a shared honesty property creates a false impression of coverage" — applies here in the opposite direction. It costs one paragraph to fix, and it must be fixed in the same commit that makes it false.


Mutation-check — re-run, not taken on trust

Baseline test -p eqoxide-nav --lib water_grid::tests: 20 passed; 0 failed (matches the author's). Every mutation reverted from a cp -p copy and confirmed with md5sum before the next.

# mutation in author's table? my result
M1 delete DROP-1 cover.skip(zone, "no glb") yes RED 18/2…records_a_missing_glb_as_skipped_807, a_corpus_using_the_prologue_…
M2w wrap DROP-2 → if false { cover.skip(zone, "no grid"); } no — author only wrapped DROP-1 RED 18/2…records_an_empty_collision_grid_as_skipped_807, a_corpus_using_the_prologue_…
M4 conflate DROP-3 addskip(zone, "no wtr") yes RED 18/2…records_an_unloadable_wtr_as_unmeasured_807, a_corpus_using_the_prologue_…

M2w is the one that matters for #799: I ran the wrap evasion on a drop site the author did not wrap, and it still went RED. The pin is execution-observable, not source-text, at more than the one site that was demonstrated.

On the SURVIVED row (M7, free-text tag): I agree it is the correct signature. The tag is diagnostic wording that reaches only Display; the bucket is what carries the honesty property and the bucket is pinned three ways. One small inconsistency worth noting, not fixing: ZoneDropped's display string is exact-matched (assert_eq!(out.err(), Some(ZoneDropped("no glb — skipped".into())))) while the rollup tag is not — the PR pins the wording it calls diagnostics and leaves unpinned the wording a reader of a failed corpus actually sees.


Reach control — I ran it; I did not re-derive it by reading

Per #778, a scanner that stops short is indistinguishable from a clean one. I could not use rbuild for the corpora (they need baked assets, which the sync excludes), so I staged a one-zone scratch asset tree inside the worktree and drove ZONE_DIR/ZONES through a temporary cargo [env]. Both were removed afterwards; worktree is clean.

(A) a dropped zone must go REDZONES=akanon,ghostzone (no ghostzone.glb):

  • water_grid_budget_measurement (collision.rs, the 5th site) — RED:
    wet columns: 24755 over 1/2 zones — INCOMPLETE, 1 skipped [ghostzone (no glb)]
  • goal_append_blast_radius (walker_sim.rs) — RED after a full 1815 s real-controller run:
    wet start/goal pairs excluded by the water filter: 19 over 1/2 zones — INCOMPLETE, 1 skipped [ghostzone (no glb)]

(B) forgetting to close must go RED — deleted cover.add(zone, &zw.measure(|_| grid.wet_column_count())) at the budget call site (a call-site mutation, not a body wrap) — RED, and diagnosed correctly as a wiring bug, not an asset problem:

wet columns: 0 over 0/2 zones — INCOMPLETE, 1 skipped [ghostzone (no glb)];
  1 unaccounted [akanon] (opened by begin_zone, never reached add/skip)

Both (A) results exercise the production wrapper open_corpus_zone end-to-end on real baked assets, which answers the "the tested path may not be the production path" question in the affirmative: the corpora do call the same accounting the library tests exercise, and the wrapper builds the same <zone>.glb / maps/water paths the deleted inline code did.

I reproduced 2 of 5 sites for (A) and 1 of 5 for (B) — not all five. See "What I did NOT verify".


Count correction — independently verified as 10

Grepping base 0497f6b for pre-add drop sites:

  • tests/walker_sim.rs 1363/1365, 1541/1543, 1704/1706, 1848/1850 → 8 (four *_blast_radius × 2)
  • crates/eqoxide-nav/src/collision.rs 7330/7332 → 2 (water_grid_budget_measurement)

10. The .wtr path in all five was already refused via a Vec<String> + terminal assert!. faithful_walker_drift_corpus carries 3 wired skips (615-623 plus the install arm), so 3 + 8 + 2 = 13 and 13 − 3 = 10. The author's correction to #807's "eleven" is right.


Non-blocking findings

F3 — the "one owner" claim is one corpus short. water_grid.rs:600-602: "the drop paths are not per-corpus text any more: they exist once, here, where they cannot be written without their accounting because they are the same statements." faithful_walker_drift_corpus (tests/walker_sim.rs:615-632) keeps its own inline prologue with hand-wired skips — it drives two rollups (roll_wr, roll_423) so it cannot use this single-rollup signature unchanged. That is a good reason not to convert it; it is not a reason for the doc to say the statements exist once. The PR body's "one of those six copies … the other five" is accurate; only the rustdoc overreaches.

F4 — the production wrapper is CI-unreachable. All five new tests call open_corpus_zone_with; nothing in CI calls open_corpus_zone. I verified there is no divergence today (path construction and closure ordering match the deleted inline code, and reach control (A) exercised it live), but a mutation of format!("{zone}.glb"), of "maps/water", or of the pass-through of cell would survive the entire CI suite. Worth one line in the tier list — the current text implies the drop paths' CI coverage extends to the function callers actually call.

F5 — the wrong assert fires first when everything drops. In all four walker_sim.rs corpora assert!(g_pairs > 0, "no zones loaded — set ZONE_DIR to the baked glbs") precedes assert!(cover.is_complete(), …). A run where every zone is skipped is still RED, but reports the pre-#807 message instead of the new accounting diagnosis. Cheap reorder.

F6 — Closes vs Addresses. #807's enumerated population is 13 sites and this PR closes the 10 outstanding, so Closes is defensible and I am not blocking on it. But the issue's stated property is not discharged: five more zone loops of the identical shape remain in collision.rs

line loop sites
7235 node-cap corpus 1 — bare continue, no print at all
7422 / 7424 fine_tier_corpus_route_success_and_cost 2
7874 fix_700_planner_ab_corpus 1
8185 q1_headroom_seal_measurement 1
8278 / 8280 floor_model_disagreement_scan 2

7 sites, one of which (7235) is strictly worse than any of the ten fixed: it drops the zone silently with no printed line whatsoever. The author's disclosure ("five more zone loops … I have not filed one") is accurate and complete. Given F2 removes the tree's only record of these, please either file the follow-up before merge or switch to Addresses and let #807 carry them. My preference: Closes #807 plus a filed follow-up issue, because the ten named sites are genuinely done.

On the "does it fire in practice" measurement (13 zones, zero drops): correctly scoped in the PR body. It is used only to answer #807's "does any of them drop in practice" question and is labelled "latent here"; it is not used to support any "cannot" claim. No objection.


Verification hierarchy

Tier 1 (make it unrepresentable) is genuinely blocked. open_corpus_zone cannot force itself on a caller without either changing Collision::build's signature crate-wide or moving the corpus loop itself into a library driver that owns iteration — both far outside #807. I looked for a cheaper type-level move (a #[must_use] opened-zone token, a ZoneHandle whose Drop settles the rollup) and none of them survives the fact that a future corpus can simply not call the function; Drop-based settling would also convert a forgotten close from a loud unaccounted into a silent add, which is worse. The author's tier-3 answer (example test + mutation both directions) is the right level here.

Five figures from my own run

cargo test --workspace --locked --no-fail-fast via rbuild, stdout and stderr captured to separate files. Baseline re-derived by me on 716cd28 (current origin/main), not quoted from the PR.

figure base 716cd28 merged 659e1d3
(1) Finished `test` profile (compile sentinel) present, 24m 16s present, 10m 27s
— completion proof process exit 0 + final test result: line complete same
^error / ^warning on stderr 0 / 0 0 / 0
(2) ^running [0-9]+ tests?$ headers vs ^test result: lines 55 vs 55 55 vs 55
(3) non-canonical test result: lines / FAILED 0 / 0 0 / 0
(4) targets matching the full triple 0 passed; 0 failed; 0 ignored 14 14
(4b) targets matching the bare 0 passed; 18 18
(5) passed + failed + ignored + filtered 1831 + 0 + 47 + 0 = 1878 = header sum 1836 + 0 + 47 + 0 = 1883 = header sum

The 14-vs-16 discrepancy, settled by name. The two predicates give 14 and 18 on the same tree, and the four targets between them are targets that have tests but ran none of them:

target test result:
tests/asset_sync_live.rs 0 passed; 0 failed; 1 ignored
tests/water_capability.rs 0 passed; 0 failed; 5 ignored
Doc-tests eqoxide_http 0 passed; 0 failed; 2 ignored
Doc-tests eqoxide_net 0 passed; 0 failed; 1 ignored

The 14 with the full triple: unittests src/main.rs, src/bin/render_model.rs, src/bin/crash_probe.rs, src/diagnose_glb.rs, src/validate_glb.rs, and Doc-tests for eqoxide, eqoxide_assets, eqoxide_command, eqoxide_crash, eqoxide_nav, eqoxide_protocol, eqoxide_renderer, eqoxide_telemetry, eqoxide_ui. Neither number is wrong; they answer different questions ("nothing at all" vs "nothing ran"). A future reviewer quoting one should say which.

Delta +5, reconciled by name (set-diff over every ^test … ... <outcome> line; both name sets equal their header sums, 1878 and 1883, so nothing was lost to the extraction). Zero removed, five added:

water_grid::tests::a_corpus_using_the_prologue_cannot_publish_a_total_over_a_shrunken_zone_list_807
water_grid::tests::open_corpus_zone_leaves_a_ready_zone_open_for_the_caller_to_close_807
water_grid::tests::open_corpus_zone_records_a_missing_glb_as_skipped_807
water_grid::tests::open_corpus_zone_records_an_empty_collision_grid_as_skipped_807
water_grid::tests::open_corpus_zone_records_an_unloadable_wtr_as_unmeasured_807

All five appear in a plain cargo test --workspace with no --ignored, so the PR's CI-reachability claim is confirmed. Grading the assertions, not the names: four of the five pin a rollup bucket (skipped_zones / unmeasured_zones / unaccounted_zones) rather than only is_complete(), which is what discriminates DROP-1 from DROP-2 from a missing skipis_complete() alone would be false in all of those cases and would not have caught M4. …records_an_empty_collision_grid… also asserts its own premise (build32(&empty_assets()).cols == 0 and build32(&floor_assets()).cols > 0) instead of assuming it. The fifth asserts the corpus-shaped end state including a negative on the literal #762 defect string. These are real pins, not name-shaped paraphrase.


What I did NOT verify


CHANGES REQUESTED. F1 and F2 are BLOCKING and both are text-only. F3-F6 are NON-BLOCKING.

djhenry and others added 2 commits August 1, 2026 06:54
…top the rollup doc denying work it now covers

Round-2 review found two blocking false claims in tracked text; the code is
unchanged.

F1: the tier-1 bullet claimed every `return` is preceded by a skip/add and that
no path leaves a zone unaccounted. The `Ok` tail return is neither — this PR's
own green test asserts the zone is still open on that path, and tier 2 three
lines below says so outright. Narrowed to `Err` returns and deleted the "no
path" sentence rather than qualifying it.

F2: `WaterRollup`'s type doc named the four *_blast_radius corpora and
water_grid_budget_measurement as accumulating into a plain Vec<String>, printing
no ratio, and not covered by anything in the file — all false as of this commit,
and it put water_grid_budget_measurement in the wrong file. That paragraph is
the tree's only record of remaining coverage, so it is corrected here rather
than left to drift; the per-site list now lives in #839.

F3: the drop statements do not exist "once" — faithful_walker_drift_corpus keeps
its own inline copy on purpose (two rollups, incompatible signature). "One
owner" describes five corpora.

F4: disclosed that nothing in CI calls `open_corpus_zone`; every test calls
`open_corpus_zone_with`, so a mutation of the glb path, the water dir, or the
cell pass-through would survive the suite.

Refs #839.

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, point by point

Thanks — the whole set was right, including the two I got wrong in the direction that matters most
here (a doc sentence that reads as measured and is not). Commit 55e936d. Nothing executable
changed
, so I did not re-run the mutation table or the reach control; test figures for the new
build are at the bottom, and the delta is zero.

Branch is now merged with origin/main dfa11ba (a merge commit, not a rebase — the branch was
already pushed and force-push is unavailable here).

F1 — BLOCKING — FIXED

You are right, and the thing that convinces me is that my own green test asserts the opposite of
what the sentence said. Tier 1 now reads:

Guaranteed: every Err return out of this function is preceded by a skip/add that closes
the zone this function's own begin_zone opened. […] The Ok tail return is deliberately NOT
closed here — see the next bullet; that is the caller's obligation, and
open_corpus_zone_leaves_a_ready_zone_open_for_the_caller_to_close_807 pins it.

"There is no path through it that leaves a zone unaccounted" is deleted, not qualified. The PR
body headline carried the same over-quantification and is corrected the same way.

F2 — BLOCKING — FIXED

Accepted in full, including the pre-existing file mislocation. The paragraph now states the
mechanism (a loop that uses no WaterRollup is invisible to the type; the only lever is a loop
choosing to open its zones through open_corpus_zone), lists which corpora go through the type as
of #807, and points at #839 for what remains — deliberately without restating the tally,
because a tally in two places is a tally that drifts. Note the corrected count: of the seven
remaining sites, four drop a zone with no printed line at all (collision.rs :7237, :7867,
:8179, plus :7235's missing-glb arm), not one. #839 carries that table.

F3 — non-blocking — FIXED

Agreed, and this is the #799 rule biting me: I wrote a completeness claim on a construct that cannot
enforce completeness. The rustdoc now says the statements exist once in five of the six corpora
and names faithful_walker_drift_corpus as the deliberate exception, with the reason (two rollups,
incompatible signature) and the note that its copy is wired and correct.

F4 — non-blocking — FIXED, as a disclosure

Added a tier-list bullet: nothing CI runs calls open_corpus_zone; every new test calls
open_corpus_zone_with, so a mutation of format!("{zone}.glb"), of "maps/water", or of the
cell pass-through would survive the whole suite. It records that you checked the wrapper by hand
and exercised it live, and that this is a one-time check rather than a standing pin. No test
added
— closing it needs an asset fixture CI does not have.

F5 — non-blocking — NOT DONE, deliberately

Real, and I agree the pre-#807 message fires first when everything drops. Not done here on the
orchestrator's scope ruling: it is a code change in four test functions on a round whose whole
purpose is text, and it is now tracked as the secondary item on #839. Called out in the PR
body's not-verified section rather than left implicit.

F6 — Closes kept, follow-up filed

#839 is filed, and the PR body now cites it in place of my vaguer "worth a follow-up issue; I
have not filed one". Closes #807 stands for the ten enumerated sites; #839 carries the seven that
remain plus F5.

Two things I am not touching, on the ruling

  • The ZoneDropped display-string inconsistency you flagged and explicitly did not ask to be fixed.
    For the record I think your observation is right — the PR pins the wording it calls diagnostics
    and leaves unpinned the wording a reader of a failed corpus actually sees.
  • Any tier-1 type-level move. Your reasoning that Drop-based settling would convert a loud
    unaccounted into a silent add is the argument that settles it; that would be strictly worse
    than what is here.

One correction to how I will be quoted

My PR body reports my reach control at 5 of 5 for (A) and 5 of 5 for (B). Yours ran (A) at 2 of
5 and (B) at 1 of 5 and says so plainly. Those are two independent partial-to-full measurements of
the same property, not one five-site result — nobody should read the merged record as five sites
verified twice. Your M2w is the more valuable of the two runs anyway: I wrapped only DROP-1, you
wrapped a site I had merely deleted, and it still went RED, which is what actually generalises the
"execution-observable, not source-text" claim past the one site I demonstrated.


Test figures — round 2

cargo test -p eqoxide-nav via rbuild, stdout and stderr to separate files, judged by log content.

figure value
(1) Finished `test` profile (compile sentinel) present, 9m 11s
— completion proof builder process exited (polled to exit) + both test result: lines complete
^error / ^warning on stderr 0 / 0
(2) ^running [0-9]+ tests?$ headers vs ^test result: lines 2 vs 2 (equal)
(3) non-canonical test result: lines / FAILED occurrences 0 / 0
(4) targets matching the full triple ^test result: ok\. 0 passed; 0 failed; 0 ignored 1 — Doc-tests eqoxide_nav
(5) passed + failed + ignored + filtered 243 + 0 + 16 + 0 = 259 = header sum

Delta zero, as expected for a text-only round. Baseline is the round-1 branch build of the same
two targets, extracted from that run's own logs by pairing target order with result order:

target round 1 round 2
unittests crates/eqoxide-nav/src/lib.rs 243 passed; 0 failed; 16 ignored 243 passed; 0 failed; 16 ignored
Doc-tests eqoxide_nav 0 passed; 0 failed; 0 ignored 0 passed; 0 failed; 0 ignored

No doctest created or destroyed by the rustdoc edits, which was the specific risk in editing ///
blocks rather than // ones.

Not verified this round

  • The mutation table and reach control were not re-run. Justification is that the diff is
    ////// text plus the PR body; the executable delta is zero and the figures above show it.
  • Everything in the PR body's existing "NOT verified, and not claimed" section still stands
    unchanged.

@djhenry

djhenry commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

Independent review — round 2 (head 55e936d)

Verdict: APPROVE. Both round-1 blocking findings (F1, F2) are discharged; F3 and F4 landed as
claims I will sign. Two non-blocking false clauses remain in the new F2 paragraph, with exact
replacement wording below. No blocking findings remain.


0. The text-only premise — verified, and relied on

I verified this before deciding what to re-run, and I then relied on it to skip the mutation table,
the reach control, and the workspace suite.

  • git show --stat 55e936d → one file, crates/eqoxide-nav/src/water_grid.rs, +31 / −12.
  • Every changed line in that diff is a /// line. Filtering the hunks for changed lines that are
    not doc comments returns empty. No executable byte moved.
  • The merge commit is not a hiding place either. Against b18269d it changes nothing in the PR's
    three files; against dfa11ba it changes nothing outside them. I did not take that on trust: I
    recomputed the merge tree independently with git merge-tree --write-tree b18269d dfa11ba and
    got 3011a26f220a2e301d0d233f7f3aa3f277202c9c, byte-identical to the tree of the merge commit.
    The merge introduced nothing of its own.
  • origin/main (dfa11ba) is an ancestor of 55e936d, so the branch head is the merged tree.
    This review is on the merged tree.

I did re-run -p eqoxide-nav anyway, because the rewrite adds intra-doc links ([`open_corpus_zone`])
that can warn. Figures at the bottom: 0 warnings.


1. F1 — DISCHARGED

The sentence "There is no path through it that leaves a zone unaccounted" is deleted, not
qualified
. That is the right disposition; qualifying it would have kept a false universal in a
tier-1 bullet.

The surviving tier-1 sentence — "every Err return out of this function is preceded by a
skip/add that closes the zone this function's own begin_zone opened" — is true as written.
I re-checked the enumeration rather than assuming round 1 still holds: three Err returns, one Ok
tail, no ?, no break, no early return other than those four. Each Err return is immediately
preceded by its closing call. My round-1 M1 / M2w / M4 REDs cover all three.

The added disclosure — the Ok tail is deliberately left open, that is the caller's obligation, and
open_corpus_zone_leaves_a_ready_zone_open_for_the_caller_to_close_807 pins it — is accurate. That
test does assert unaccounted_zones() == ["gfaydark"] on the success path, which is exactly the
behaviour the deleted sentence denied.

2. F2 — discharged as a blocking finding; two false clauses remain (NON-BLOCKING)

I read this one adversarially, as asked. What is now true:

Two clauses are still false:

(a) "further zone loops in collision.rs that still accumulate nothing and drop zones on a bare
continue"
fix_700_planner_ab_corpus does accumulate. It keeps
let mut unmeasured: Vec<String> = Vec::new(); and refuses on it at the end of the function:

assert!(unmeasured.is_empty(),
    "#762: {} zone(s) were dropped because their .wtr did not load — the AB_TOTAL above \
     covers a smaller corpus than the one named, ...");

So "accumulate nothing" is not true of all of them. Worse for the record: #839 itself contradicts
this doc
— it says "Two of the five … already keep a Vec<String> unmeasured". The doc and the
issue it points at disagree, which is precisely the drift the paragraph set out to avoid.

Fix: replace that still accumulate nothing and drop zones on a bare continue, several of them without even printing a line with that still drop zones on a bare continue without opening them here — some accumulating into a local Vec, some printing nothing at all.

(b) "the only lever is that a loop chooses to open its zones through [open_corpus_zone]"
a false "only", and the doc falsifies it three lines later by naming faithful_walker_drift_corpus,
which is covered by this type via hand-wired begin_zone/add/skip and does not go through
open_corpus_zone. Hand-wiring is a second lever, and it is in use today.

Fix: replace the only lever is that a loop chooses to open its zones through [open_corpus_zone]
with the levers are that a loop opens its zones through [open_corpus_zone], or wires begin_zone/add/skipby hand asfaithful_walker_drift_corpus does.

Neither clause overstates coverage in a way that would let a real drop go unnoticed — they
understate what exists and misdescribe a lever count — so non-blocking. But they are the same
defect class as F2 itself, and per the coordinator's framing, "a rewrite that swaps one false
paragraph for a subtler false paragraph" is the failure mode worth naming even when it is minor.

3. F3 — DISCHARGED (one minor overreach)

The rustdoc no longer says the drop statements "exist once". It now says they exist once in five
of the six corpora
, names faithful_walker_drift_corpus as the sixth, gives the real reason (two
rollups, incompatible with the single-rollup signature), and states plainly "They do NOT exist only
once in the tree." I would sign that.

Minor: "That copy is wired and correct". #830 and #831 are open agent-honesty issues
against that same corpus. Neither alleges an unaccounted drop path, so "wired" is defensible;
unqualified "correct" is not, with two open honesty issues against it. Non-blocking — dropping the
word "correct", or saying "wired and accounted", would settle it.

4. F4 — DISCHARGED

The new bullet states plainly that nothing CI runs calls open_corpus_zone; that every pinning
test goes through open_corpus_zone_with; that mutations of format!("{zone}.glb"), of
"maps/water", and of the cell pass-through would survive the whole suite; and that my hand check
of the wrapper was "a one-time check, not a standing pin". That is the disclosure I asked for, and
it implies no pin where none exists. It also correctly identifies why closing the gap is out of
scope (needs an asset fixture CI does not have).

5. The reach-control characterisation — ACCURATE

Checked because an overstatement here was flagged as blocking. The author's reply says my run was
(A) 2 of 5 and (B) 1 of 5, theirs 5/5 on both, and that these are two independent
partial-to-full measurements rather than one five-site result confirmed twice. That matches
exactly what I measured and published.
I ran (A) on water_grid_budget_measurement and
goal_append_blast_radius, and (B) on water_grid_budget_measurement only; I never claimed more.

The PR body does not overstate either party. Its reach-control tables are presented as the author's
own work in the author's own voice, with the partial-coverage caveat retained under "NOT verified".
Nothing in the body claims reviewer confirmation of five sites. The body headline has also been
corrected to "Every Err return in it is preceded by a skip or an add", consistent with F1.


Test figures — my own run, test -p eqoxide-nav --locked --no-fail-fast, stdout and stderr captured separately

# figure value
(1) Finished `test` profile compile sentinel present (24m 53s) — 0 ^error, 0 ^warning in either stream
(2) ^running [0-9]+ tests?$ headers vs ^test result: lines 2 vs 2 — equal, no lost binary
(3) non-canonical test result: lines / FAILED 0 / 0
(4) targets with nothing to run, predicate ^test result: ok\. 0 passed; 0 failed; 0 ignored 1 — bare 0 passed; also 1, no divergence at this scope. By name: Doc-tests eqoxide_nav
(5) passed + failed + ignored + filtered vs header sum 243 + 0 + 16 + 0 = 259 = header sum (running 259 tests + running 0 tests)

I re-derived these from my own logs rather than quoting the author's; they agree exactly with the
author's 243/0/16/0 = 259. All five …_807 tests are present and ok, including
open_corpus_zone_leaves_a_ready_zone_open_for_the_caller_to_close_807. The sentinel is a compile
marker, not a run-completion marker — completion is figure (2).

I did not re-run the workspace suite, the mutation table, or the reach control. I relied on the
verified text-only premise in section 0 to justify that.


What I did NOT verify this round

Disposition

No blocking findings remain. APPROVE.

…ulate nothing", and an unclaimable "correct"

All three are the reviewer's own replacement wording, used verbatim.

1. "the only lever is that a loop chooses to open its zones through
   open_corpus_zone" was falsified three lines later by this same doc:
   hand-wiring begin_zone/add/skip is a second lever and faithful_walker_drift_corpus
   uses it today.

2. "still accumulate nothing and drop zones on a bare continue" is false of
   fix_700_planner_ab_corpus, which keeps a Vec<String> unmeasured at
   collision.rs:7862 and refuses on it at :7918-7922.

3. "That copy is wired and correct" -> "wired and accounted". #830 and #831 are
   open agent-honesty issues against faithful_walker_drift_corpus. Neither
   alleges an unaccounted drop path, so "accounted" is exactly what is claimable
   there; unqualified "correct" is not.

No numbers from #839 are restated in tracked text, so its correction from 7 to
10 sites falsifies nothing here.

Refs #839.

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 — three edits, plus one consequence of the #839 correction

Commit 44550c7. Text only; no test changes, no re-run of the mutation table or reach control.

1. "accumulate nothing" → your wording, verbatim. You are right that
fix_700_planner_ab_corpus accumulates — let mut unmeasured: Vec<String> at collision.rs:7862,
refused on at :7918-7922. The paragraph now reads:

… that still drop zones on a bare continue without opening them here — some accumulating into a
local Vec, some printing nothing at all …

2. the false "only" → your wording, verbatim. This one is worth naming for what it is: the
sentence was falsified by the same doc block, three lines below, which already says
faithful_walker_drift_corpus is hand-wired. Now:

… the levers are that a loop opens its zones through [open_corpus_zone], or wires
begin_zone/add/skip by hand as faithful_walker_drift_corpus does.

3. "wired and correct" → "wired and accounted". Agreed, and the distinction is the point of this
whole gate: #830 and #831 are open agent-honesty issues against that corpus. Neither alleges an
unaccounted drop path, so accounted is precisely the property I measured and can claim;
correct was a broader claim I had no basis for and would have quietly contradicted two open issues
in the same repo.

No intra-doc link target changed (edit 2 keeps the existing [open_corpus_zone] link and adds
faithful_walker_drift_corpus in plain backticks, not as a link), so I skipped the
-p eqoxide-nav run. Saying that plainly rather than implying it was run: the last measured figures
for these two targets remain round 2's 243 passed; 0 failed; 16 ignored for the lib and
0 passed; 0 failed; 0 ignored for Doc-tests eqoxide_nav.

The #839 correction, reconciled

Re-read #839 including the correction. Nothing in tracked text depended on the old numbers
the rustdoc defers the tally to the issue by design and states no count, which is exactly the
property that made the correction free. Verified by grep over all three changed files: the only
#839 mentions are the two pointers, with no site count beside them.

The PR body did depend on them, in two places, and I corrected it (this is the fourth edit,
flagged to the orchestrator rather than made silently):

  • "Closes #807 … cited for the seven that remain" → ten.
  • "Five more zone loops in collision.rs (7 sites) … Four of those seven print no line at
    all" → "(10 sites) … Four of those ten".

I also added a short note in the PR body recording why the number moved — the tally had been taken
with grep 'cols == 0 { continue', a pattern that by construction can only match the bare form, and
was then used to answer a question it could not answer ("how many drop sites are there" vs "which
drops are silent"). That is the #778 / #836-B3 shape again, and it is worth having in the merged
record next to the fix rather than only in an issue comment. The "four silent" figure is unchanged
and still correct.

One phrasing note, since I used your wording verbatim as instructed: "bare continue" now sits next
to "some printing nothing at all", which could be read as though the continues that print are not
"bare". I read "bare" as unaccounted — no skip/add before it — which is the sense that matters
and is true of all ten. Flagging it rather than editing your text; say the word if you want it
tightened.

…cted the clause after it

"drop zones on a bare `continue`" was meant as UNACCOUNTED (no skip/add before
it), which is true of all ten remaining sites. But the conventional reading of
"bare" is "a `continue` with nothing else", and under that reading the clause
contradicts the next one: "some printing nothing at all" would be redundant and
would imply the rest are not bare.

`without opening them here` already carries the whole meaning, so the word adds
nothing and costs an ambiguity. Without it the sentence is true under both
readings.

Refs #839.

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 — one word

Commit 41b7696. Deleted bare from water_grid.rs:425, which now reads:

… that still drop zones on a continue without opening them here — some accumulating into a local
Vec, some printing nothing at all …

I had meant "bare" as unaccounted, which is true of all ten remaining sites; the conventional
reading is "a continue with nothing else", under which the clause contradicted the one after it.
without opening them here already carries the whole meaning, so the word only bought an ambiguity.
The sentence is now true under both readings.

Doc-only, one line, no link target touched — no test run, no re-review. PR body unchanged.

@djhenry
djhenry merged commit 56b851f into main Aug 1, 2026
2 checks passed
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.

11 of 13 test-corpus drop paths still publish an unmeasured zone as a measured zero (#802 covers 2)

1 participant