fix(#803): make "no exits" and "could not read the exits" different types - #821
Conversation
…ypes `RegionMap::load` collapsed four distinct `.wtr` failures — missing, not region data, unsupported version, truncated — into one `None`. That `None` was installed on the collision grid, `Collision::zone_line_indices()` `.unwrap_or_default()`-ed it to an empty vec, and `/v1/observe/zone_exits` published the result as a bare `[]` with 200 OK. `[]` is also the true, common answer for a zone that genuinely has no zone-line regions, so the two were byte-identical. Exits are the only way out of a zone, so a failed FILE READ read to an agent as a fact about the world: sealed in, from a success response it had no way to doubt. Type-level fix rather than a caller-side guard, because the guard is what kept failing: * `Collision::zone_line_indices()` returns `Result<Vec<i32>, RegionDataAbsent>`. Every caller now fails to compile until it says out loud what it does with the failure. * `Collision`'s region slot is a `Result<Arc<RegionMap>, RegionDataAbsent>`, set through `set_region_data(Result<_, RegionLoadError>)` — the loader's own verdict, carried rather than discarded. * `RegionDataAbsent` adds the one case `RegionLoadError` cannot express: nothing was ever attached to this grid. * `RegionMap::load` is DELETED, not merely unused. While it existed, `load(..).map(Arc::new)` remained the shortest way to get a region map onto a grid, and it is lossy in the direction that lies. * `/v1/observe/zone_exits` refuses with 503 `zone_region_data_unavailable` plus a machine-readable `reason`, deliberately distinct from the #579 `zone_assets_not_ready` verdict (that one also gates the nav walker and the net thread's zone-cross drain; a missing `.wtr` does not invalidate the terrain those consume). `[]` with 200 now means exactly one thing: this zone's region map loaded and contains no zone-line regions. Closes #803. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
Independent acceptance review — CHANGES REQUESTEDI did not write this change; my job was to refute it. The code is right. I could not break the Reviewed on the merged tree: PR head BlockingB1 — /// `Ok(vec![])` is a real reading: this zone's region map loaded and contains no zone-line
/// regions (or is a v1 map, which carries no indices). `Err` means the question could not be
The parenthetical says a v1 map yields and This matters beyond pedantry in three ways: (a) it contradicts this PR's own contract in B2 — the new tracked comment at
There is a fourth production consumer that is not an The deviation itself is correct — the omission makes the author's outage argument understated, B3 —
B4 — the docstring at
I then tried to reach it in production and could not, so I am reporting this as an overstated
So Nothing enforces the coupling — not a type, not a test. The word "never" is doing work the code Required follow-up (coverage, not correctness) — two mutations I invented both survivedEvery mutation the author ran is at the library layer, and that layer is genuinely well covered. The
N2 is the significant one. With that one line gone, every zone's grid carries N1 is smaller but has a tracked claim on it: Minor
Verified, with the methodTier-1 lever — holds. Mutations — 3 independent spot-checks, all reproduce the author's table exactly. Mutated at the
Same counts and same test names the author reported. Two of their three restore md5s reproduce Carry-over: the mutations and the live run were executed on the previous merge base ( Cross-PR semantic conflict — checked, clean. The merge with #810 is textually clean across five #823 ( Figures — merged tree
All ten added tests ran and passed by name, and the one deleted test Delta reconciliation — now exact. Against the stated Live E2E — both directions, on a client I launched, pinned to this tree's binary.
NOT verified
Verdict: CHANGES REQUESTED. B1–B4 are doc/comment edits; nothing about the implementation needs |
…ing an unservable reason (B3) Addresses review round 2 findings **B1**, **B3** and minor **M2**. Comment/doc only; no behaviour change. **B1 — `collision.rs`, `zone_line_indices()`.** The doc said `Ok(vec![])` covers a map that "is a v1 map, which carries no indices". It does not. v1 records have no `zone_line_index` field, so a v1 zone line reads as index **0**, and index 0 is INCLUDED — the function returns `[0]`, not `[]`. `region_map.rs`'s own `zone_line_indices` doc says so, and `a_zero_index_zone_line_region_is_still_recognized_683` pins it green. The parenthetical restated the exact pre-#683 belief that locked a character in qrg forever, and it contradicted this PR's own `docs/http-api.md` contract ("`[]` means, and only means, …"). Replaced with the correction, named as such, so it cannot be re-derived. **B3 — `docs/http-api.md`.** `region_data_not_attached` was listed as a `reason` an agent may receive. It cannot occur in a release build: `NotAttached` comes only from the `Collision` constructors and from `set_water`, which is `#[cfg(any(test, feature = "test-fixtures"))]`, and the one production grid construction attaches region data in the same call. Removed from the outcome table and documented separately as what it actually is — an internal state that, if it ever reached an agent, means a CLIENT BUG rather than a missing asset. The claim is labelled as an argument from enumerating construction sites, not a type-level guarantee. **M2** — the refusal emits four keys (`error`, `reason`, `detail`, `message`); the doc showed three. `message` is now documented. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
… (B4), and recount usability()'s consumers (B2) Addresses review round 2 findings **B4**, **B2** and minor **M1**. **B4 — the "never" is now enforced by the code instead of asserted by a docstring.** `get_zone_exits` took permission from the zone-assets verdict and then read the grid from the SEPARATE `shared_collision` slot behind `if let Some(col)` with no `else`. A `None` there returned `200 []` — "this zone has no way out" — having consulted no region map at all. Nothing coupled the two slots: not a type, not a test. The reviewer's M5' run proved two currently-green tests reach `200 []` through exactly that path, and while it traced every production `SharedCollision` writer without finding a way in, it could not close the window where `zone_needs_reload` compares zones case-SENSITIVELY and `usability` case-INSENSITIVELY. Taken the reviewer's tier-1 option rather than softening the sentence. New `zone_assets::usable_collision(state, player_zone) -> Result<&Arc<Collision>, NotUsable>` returns the verdict AND the grid the `Ready` state OWNS, in one call, delegating to `usability` so the rule cannot fork. `get_zone_exits` uses it and the `if let Some(col)` is gone — there is no longer a branch that reaches the response builder without a grid, so every `[]` this endpoint emits has been through `Collision::zone_line_indices()`. `zone_assets_not_ready`'s body is split into `zone_assets_refusal` so both callers serve the byte-identical 503. Pinned three ways rather than described: * `zone_assets::…::usable_collision_agrees_with_usability_for_every_state` — over every state × zone combination the two agree, the `Ok` hands back the state's OWN `Arc` (`ptr_eq`), and the unreachable fallback never fires. * `observe::…::an_unset_shared_collision_slot_can_no_longer_produce_an_empty_exit_list` — the exact fixture shape the fall-through fired on (`Ready` for this zone, shared slot `None`), asserted in BOTH directions so it cannot pass by the endpoint having stopped reporting exits. * Two fixtures were updated rather than worked around: `ready_state()` and the #646 header loop now carry a region map that LOADED and has no zone lines. `test_ready()`'s grid has no region data attached at all, which is a state production never has — a `ready` zone has always had `set_region_data` called with an `Ok` or with the loader's `Err`. **B2 — `usability()` has FOUR non-test consumers, not three.** The missing one is `move_api.rs`'s `POST /v1/move/goto`, which is not an `/observe/*` route — which is how it went uncounted. The reviewer's judgement that the DEVIATION itself is correct is preserved and now rests on the true count: a `NotUsable` variant would also have taken down `/move/goto`, so the omission understated the outage. Corrected in both places that carried the wrong count: `observe.rs`'s justification comment and `zone_assets.rs`'s "verified by grep — #600" inventory. **M1** — `the_refusal_is_distinct_from_the_zone_assets_gate` asserted only `assert_ne!(j["error"], "zone_assets_not_ready")`, which is vacuously true on an ARRAY body, so it survived the endpoint being restored to `200 []` — the precise regression it is named after. Now asserts the 503 and the error string positively. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
…tations survived (N1, N2) Addresses review round 2's two SURVIVOR mutations. Both were tracked claims with no test behind them — the defect class this project ranks highest. **N2 — `src/app.rs`, the single production write.** Deleting `c.set_region_data(water)` in the zone-load worker left the whole root crate green, while a real client would then carry `Err(NotAttached)` on every zone's grid and answer `503` from `/v1/observe/zone_exits` in EVERY ZONE IN THE GAME. #803's type change forced all the LIBRARY call sites to speak up; the line that feeds them in the shipped client was pinned by nothing. The four lines lived inside a spawned thread closure and so could not be called from a test. Extracted verbatim into `app::build_zone_collision(za, maps_dir, zone_name)` — named, `pub(crate)`, documented as the client's ONE production construction of a collision grid — and pinned by `app::zone_load_wiring_803`, which calls the production function against a real `.wtr` written to a temp dir. The `.wtr` is built as BYTES rather than handed in as a parsed `RegionMap`, so the test covers the whole wiring: the `maps/water/<zone>.wtr` path convention, `try_load`'s `Ok` and `Err`, and the attach. Both directions: a zone with a `.wtr` enumerates its exit (`Ok([7])`), a zone without one refuses naming `region_data_missing` — NOT `region_data_not_attached`, which is precisely what severing the write produces. One deliberate behaviour change, disclosed: the `.wtr` load (and its `warn!`) now runs only when the terrain assets loaded. When they did not, no grid is built at all and the zone publishes `Failed`; warning about the region data of a world that does not exist was noise about a grid that is not there. **N1 — `water_grid.rs`, the `Unmeasured` arm.** `install()`'s comment claims "#803: BOTH arms now write the grid … makes the grid itself able to refuse honestly". Severing `col.set_region_data(Err(e.clone()))` changed no test outcome, so that stated behaviour change was unproven. `installing_an_unmeasured_zone_is_a_handled_failure_762` now asserts the grid's state BEFORE the install (`region_data_not_attached` — nobody asked) and AFTER (`region_data_missing` — we asked and the file was not there), plus that `zone_line_indices()` refuses with that cause rather than answering `[]`, plus that the `Measured` arm's write CLEARS the recorded failure instead of latching it. Its stale "the grid keeps `water: None`" comment is corrected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
Round 2 — all four blocking findings applied, both surviving mutations now killedHead is now
B1 — the v1-map falsehood (
|
|
APPROVED — independent adversarial review, round 2. All four round-1 blockers are fixed, and I Reviewed at head Round-1 findings: verified fixedB1 (was blocking) — fixed and the replacement is accurate. The v1 parenthetical is gone and what B2 (was blocking) — fixed in both inventories. Both now say FOUR and name B4 (was blocking) — closed at the type level, not relocated. On the case-sensitivity window you asked me to rule on: the author is right to call it out of B3 (was blocking) — accepting the doc disposition, and the doc's claims check out. I did not take
And the doc labels its own limit ("an argument from enumerating every construction site, not a Minor M1 from round 1 — fixed, and I measured that the fix bites. Both surviving mutations: reproduced RED, not acceptedControl (
Both match the reported figures exactly. N2's fix is the right shape and not a source-text pin: The one disclosed behaviour change in that extraction — the The "unreachable" universal: true, and pinned — measured both directionsYou were right that a passing run cannot discharge it, so I attacked the sentence with two mutations
On exhaustiveness (non-blocking, note 1). The test's state list is hand-written, so Fixture blast radius: measured, and confinedThis was the item most likely to hide something, so I measured it rather than reading the call sites.
All five mutated files restored by Figures —
|
1 participant
What this fixes
RegionMap::loadcollapsed four distinct.wtrfailures — missing, not region data, unsupportedversion, truncated — into
None. ThatNonewas installed onto the collision grid;zone_line_indices()did
.unwrap_or_default();get_zone_exitsiterated the empty vec and answered[]with200 OK.[]is also the true, common answer for "this zone has no zone lines". So a failed file read waspublished as a fact about the world, in the one endpoint that says how to leave a zone. An agent that
asks how to get out of a broken zone is told, confidently and with a 200, that there is no way out.
Closes #803.The fix: a type, not a guard
The lever is tier 1 of the verification hierarchy — make the bad state unrepresentable — and the
oracle is the compiler, not a source-text pin:
RegionMap::load(theOption-returning one) is deleted outright, not merely unused. Atombstone comment at its old site says why, so it does not grow back.
Collision::wateris nowResult<Arc<RegionMap>, RegionDataAbsent>, notOption<_>.Collision::zone_line_indices()changed return type fromVec<i32>toResult<Vec<i32>, RegionDataAbsent>. Every caller had to fail to compile, which is how eachone was found; there is no way to write the old lossy read and still build.
RegionDataAbsentcarries eitherNotAttachedorLoadFailed(RegionLoadError), andas_str()yields six distinct machine-readable reasons.
GET /v1/observe/zone_exitsreturns503 zone_region_data_unavailablewithreason+human
detailwhen the read failed.200 []now means exactly one thing: the region map loadedand holds no zone-line regions.
Design deviation from the issue, and why
The issue suggested routing the refusal through
zone_assets' usability verdict (aNotUsablevariant). I did not do that.
usability()has three consumers: every/observe/*endpoint, thenav walker's
drive_walkgate, andActionLoop::drain_zone_cross. A newNotUsablevariant wouldhave stopped routing and
/observe/framein every zone with a missing.wtr— a much largerbehaviour change than the honesty bug warrants, and one that trades a silent lie for a broad outage.
The refusal is local to
get_zone_exits, which is the endpoint that was lying.Files changed (10 files, +551 / -83)
Hunk inventory for the files this PR shares with #810
#810 also touches
crates/eqoxide-http/src/observe.rs,src/app.rs,src/movement.rs,docs/http-api.mdandcrates/eqoxide-net/src/action_loop.rs. Exact hunks here, so merge order canbe judged rather than guessed:
src/app.rs@@ -789,3 +789,9 @@,@@ -794 +800 @@RegionMap::load→try_load, awarn!on failure,set_water→set_region_data.src/movement.rs@@ -2810 +2810 @@#[cfg(test)] mod tests: same call-site move.crates/eqoxide-net/src/action_loop.rszone_line_indices()now returns aResult, so this call site had to compile. It keeps today's behaviour via.unwrap_or_default()deliberately, with a comment saying so; changingzone_cross's reporting is #815, not this PR.crates/eqoxide-http/src/observe.rs@@ -81,0 +82,36 @@(newregion_data_unavailablehelper),@@ -1887,2 +1923,10 @@(get_zone_entrances, forced by theResult),@@ -1955 +1999,9 @@(theget_zone_exitsrefusal, 9 lines),@@ -5172,0 +5225,114 @@(new test module)GUARD_REACH_SENTINEL_OBSERVE, which must stay last in the file.docs/http-api.md@@ -458,0 +459,3 @@(3-line cross-reference),@@ -475,0 +479,31 @@(new### Zone exits: [] means exactly one thing (#803)section)Mutation results — both directions, 7 mutations, 0 survivors
Green control M0 (unmutated):
eqoxide-core150 passed,eqoxide-http266 passed,eqoxide-nav232 passed / 16 ignored, allok, exit 0.Each mutation is applied to a
cp-restored pristine tree (single-occurrence anchor assert), thencargo test -p eqoxide-nav -p eqoxide-http -p eqoxide-core --lib --locked --no-fail-fast.collision.rs: restore the lossy read —Err(absent) => Err(absent.clone())becomesErr(_) => Ok(Vec::new())(the oldunwrap_or_default)collision::…::every_load_failure_is_an_error_not_an_empty_list,collision::…::a_grid_with_no_region_data_attached_is_an_error_too,observe::…::a_region_map_that_did_not_load_refuses_instead_of_reporting_no_exitsobserve.rs: endpoint guard removed — theErrarm returnsVec::new()instead of refusingobserve::…::a_region_map_that_did_not_load_refuses_instead_of_reporting_no_exits(collision stayed green — correct, the mutation is one layer up)observe.rs: refusal renamed"zone_region_data_unavailable"→"zone_assets_not_ready"observe::…::the_refusal_is_distinct_from_the_zone_assets_gate,observe::…::a_region_map_that_did_not_load_refuses_instead_of_reporting_no_exitsregion_map.rs: collapse all sixas_str()reasons to one stringregion_map::tests::region_data_absent_reasons_are_distinguishable,collision::…::the_reported_reason_names_the_actual_failure,observe::…::a_region_map_that_did_not_load_refuses_instead_of_reporting_no_exitsobserve.rs: the plausible wrong fix — refuse whenever the exit list is emptyobserve::…::a_zone_that_genuinely_has_no_zone_lines_still_answers_the_empty_list, plus two pre-existing tests:observe::zone_asset_gate_tests::zone_exits_answers_normally_once_ready,observe::tests::every_previously_age_less_bare_body_endpoint_carries_the_headercollision.rs:set_region_datadrops the cause, always storingNotAttachedcollision::…::the_reported_reason_names_the_actual_failure,collision::…::every_load_failure_is_an_error_not_an_empty_list,observe::…::a_region_map_that_did_not_load_refuses_instead_of_reporting_no_exitscollision.rs: blanket refusal —Ok(w) => Ok(…)becomesOk(_) => Err(NotAttached), i.e. never answer at allcollision::…::a_loaded_map_with_no_zone_lines_still_answers_the_empty_list,collision::…::a_loaded_map_with_a_zone_line_enumerates_it,observe::…::a_zone_with_a_zone_line_still_lists_it,observe::…::a_zone_that_genuinely_has_no_zone_lines_still_answers_the_empty_list, plus two pre-existingobserve::zone_cross_observables_713::*Survivors: none. M7 was added specifically because after M1–M6 three of the new tests
(
a_loaded_map_with_no_zone_lines_still_answers_the_empty_list,a_loaded_map_with_a_zone_line_enumerates_it,a_zone_with_a_zone_line_still_lists_it) had not beenkilled by anything and were therefore unproven controls rather than measured assertions. M7 kills all
three. Every one of the 9 added tests is now killed by at least one mutation, and the two directions
are both covered: M1/M2/M3/M4/M6 kill "stopped refusing", M5/M7 kill "started refusing when it
shouldn't".
Three files were mutated across the run. After the last mutation they were restored from pristine
copies and verified by
md5sum(collision.rs ea868f79…,observe.rs 2c5e4898…,region_map.rs ee594ce9…), withgit status --porcelainempty.Forced
.wtrfailure, observed live — not reasonedThree runs on one client, one port I own, against a private copy of the asset tree
(
XDG_DATA_HOMEpointed at a scratch dir). The real asset was never mutated: its md5(
6e941725de5301812187e6ca7dbbd738) was checked before and after and is unchanged.The corruption is 4 bytes patched in place in the copy — the
.wtrheader's node count, 14655 →999999, leaving the file length at 586218. That shape was chosen deliberately:
artifacts_intact()in
src/asset_sync.rsverifies only file size, so a genuinely shortened file would have beensilently repaired by the gamedata re-sync before the client ever read it. Same size, still
Truncated.Run 1 — this branch, healthy asset.
GET /v1/observe/zone_exits→200 OK[{"gated":false,"index":1,"location":[-853.66,199.42,-41.97],"zone_id":null}, {"gated":false,"index":2,"location":[-1860.26,790.68,-111.97],"zone_id":57}]Run 2 — this branch, corrupted asset, same zone.
GET /v1/observe/zone_exits→503 Service Unavailable{"error":"zone_region_data_unavailable", "reason":"region_data_truncated", "detail":".wtr is truncated: header declares 999999 BSP nodes, file is 586218 bytes"}A
WARNwas emitted at load.zone_assetsstill reportedreadywithcollision_loaded: trueandterrain_meshes: 82, and/zone_entrances,/doorsand/frameall still answered200— therefusal is scoped to the one endpoint that could not answer truthfully, exactly as intended.
Run 3 — the control that shows this is a real bug, not a hypothetical. The same corrupted file,
same zone, run against the release binary built from
main:…and zero log lines mentioning
region_map,.wtrortruncat. Total silence plus a confidentfalsehood: the agent is told this zone has no exits.
All three clients were shut down with
POST /v1/lifecycle/exit; afterwardspgrep -x eqoxidewasempty and the port was free.
Full workspace suite
running [0-9]+ tests?headerstest result:lines^test result: ok\. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out)^error/panicked/Killed: 0 hits in either stream.cargo check --workspace --all-targets:Finished dev profile in 1m 21s, 0 errors, 0 warnings.Test-count delta vs merge base
7159140:#[test]/#[tokio::test]attributes went 1805 → 1814,+9, which matches 10 added test functions minus 1 replaced
(
load_is_try_load_with_the_reason_thrown_awaybecameregion_data_absent_reasons_are_distinguishable, since the function it tested no longer exists).No source-text pins were added
Per the project's own history with
include_str!+containspins (they prove a line was written,not that it is reached), this PR adds none. The enforcement is the return type of
zone_line_indices(). I also checked the three existing pins that read files I edited, and none ofthem intersect my hunks:
action_loop.rs:4449reads only the| `idle` |row ofdocs/http-api.md;src/movement.rs:3715/3767are anchored onapp.rs's zone-change reload blockand camera-init step, not the zone-load worker I touched;
observe.rs:3701scans for net-healthstamp patterns, which my new module contains none of.
Follow-ups filed, not fixed here
zone_line_not_in_map(a map-data GAP) when the region map failed to LOAD #815 —POST /v1/move/zone_crossreportszone_line_not_in_map(documented as a map-datagap) when the region map failed to load. Same substitution, one caller over. Noted in the code
and in
docs/http-api.md.ZoneMap::loaddiscards its failure the same way, which silently drops fallback zonepoints from
/v1/observe/zone_entrances.NOT verified, and not claimed
7159140;origin/mainhas sincemoved to
9e41178(fix(#762): make "this zone's water data did not load" a distinct outcome from "this zone has no water" #802, fix(#796): pin the download-phase clock in tests instead of racing wall time #808, fix(#798): delete the five copies of JOINT_CAP rather than name one of six #809). I did not rebase and did not run anything against the mergedresult. Every figure above is from
ec42ee3on base7159140.+9test delta is derived from counting#[test]attributes in myown diff, not from a measured before/after suite.
mainrelease binary (mtime2026-07-31 15:42); I did not verify which commit produced it. It demonstrates the pre-fix behaviour
as shipped, not the behaviour of any specific SHA.
Truncatedonly. The other fivereasons (
missing,unreadable,not_region_data,unsupported_version,not_attached) arecovered by tests, not by a live run.
empty_state()in the HTTP testkit buildsshared_collision: Nonewhilezone_assetsisReady;/zone_exitsthen falls through theif let Some(col)and still answers[]/200. In productionfinish_zone_loadpublishes bothtogether, so
ReadyimpliesSome(collision)and the path is unreachable — but I have not provedthat invariant, only read it. Closing it would churn two tests owned by Staleness should be detectable generically: 13 of 16 /v1/observe routes carry no age at all #646/Agent can misread a mid-load zone void as 'missing geometry' — expose a zone-assets-loading state #579, so I left it and
am naming it rather than letting a reviewer find it.
endpoint no longer does, under the mutations listed. Two sibling call sites are still lossy by
design and are tracked as zone_cross reports
zone_line_not_in_map(a map-data GAP) when the region map failed to LOAD #815/ZoneMap::load discards its failure — silently missing fallback zone points on /observe/zone_entrances #816.Closes #803.