docs(08): WR fixes addendum — WR-01/02/04 applied (11e55c1, 95399ae, e77bbe9); WR-03 + infos stay open for the verifier

2026-10-25 22:36:43 +08:00
parent e77bbe9f72
commit 4942c770f3
1 changed files with 187 additions and 0 deletions
@@ -0,0 +1,187 @@
 ---
 phase: 08-alt-landmark-preprocessing-routing-backend-v2
 reviewed: 2026-10-25T14:28:05Z
 depth: standard
 files_reviewed: 21
 files_reviewed_list:
  - routing/src/astar.ts
  - routing/src/graph.ts
  - routing/src/landmark.ts
  - routing/scripts/build-road-graph.mjs
  - routing/scripts/measure-optimality-baseline.mjs
  - routing/scripts/measure-optimality-gate.mjs
  - routing/scripts/measure-astar-oracle-sweep.mjs
  - routing/scripts/measure-route-cpu.mjs
  - scripts/lib/budget-gates.mjs
  - routing/test/graph.test.mjs
  - routing/test/landmark.test.mjs
  - routing/test/astar.test.mjs
  - routing/test/rollout.test.mjs
  - routing/test/snap.test.mjs
  - routing/test/fixtures/build-mini-graph.mjs
  - bot/test/webhook.test.mjs
  - bot/test/guidance-cron.test.mjs
  - scripts/test/budget-audit.test.mjs
  - docs/BUDGETS.md
  - docs/KNOWN-LIMITATIONS.md
  - docs/ROAD-GRAPH.md
 findings:
  critical: 0
  warning: 4
  info: 7
  total: 11
 status: issues_found
 wr_fixes_applied: [WR-01, WR-02, WR-04]
 wr_fixes_applied_date: 2026-10-25
 open_findings: [WR-03, IN-01, IN-02, IN-03, IN-04, IN-05, IN-06, IN-07]
 ---
 # Phase 8: Code Review Report
 **Reviewed:** 2026-10-25T14:28:05Z
 **Depth:** standard
 **Files Reviewed:** 21
 **Status:** issues_found
 ## Summary
 Reviewed the Phase-8 ALT implementation at standard depth: the ARG2 codec (`graph.ts`), the landmark preprocessing core (`landmark.ts`), the ALT-steered bidirectional A\* (`astar.ts`), the five maintainer measurement/build scripts, `scripts/lib/budget-gates.mjs`, the six touched test suites, and the three phase docs.
 **The shipped search path is sound.** I traced the ALT machinery end-to-end looking for the class of bug the phase itself documented (per-node sentinel masks, fromTable one-directional Lipschitz, meet-rule breaks) and found the defenses correctly in place: sentinels are compared only at setup scope against one named constant (`astar.ts:62,242,368-370`), the active landmark list is search-scoped off the clean-column memo (`astar.ts:358-362`), the fromTable family is absent from the potential (`astar.ts` never reads `landmarkFrom` — pinned by A11), the quantum floor-slack absorbs the f32 quantum-readback rounding (worst-case ~1.5e-8 relative overestimate, well inside the 0.1-quantum per-edge floor headroom, so reduced weights stay non-negative), the heap/pop/meet/reconstruction logic is correct under lazy deletion, `landmarkNodes` is always 4-byte aligned (offset = `toTable + 4·L·N`), and the decode size-check runs before any view allocation. Verified against data: the frozen baseline record's pair set is bit-identical to what `--extended` regenerates today (100/100 pairs match — the "same pair set" D-04 claim holds on the current binary), and the record/verdict numbers quoted across `BUDGETS.md`, `KNOWN-LIMITATIONS.md`, `ROAD-GRAPH.md`, and `budget-gates.mjs` are mutually consistent (53 routed / 152.788 / 185.692 / 182.725 / 204.499 ms / 400 ms drift guard).
 The findings below are concentrated where this phase's real product lives — the **verification gates** — plus two library-contract holes and doc drift. The strongest: the oracle sweep (the gate built specifically to catch what fixtures couldn't) is **one-sided** and would pass a regression that makes the search *faster than the oracle* (e.g., a traversal-law break), and the frozen D-03/D-04 evidence base is **53 measured pairs, not the ~100 D-04 committed to**, because the baseline recorder lacks the oversampling its sibling tool has.
 ## Warnings
 ### WR-01: Oracle sweep's optimality check is one-sided — a "faster than oracle" law break passes the gate
 **File:** `routing/scripts/measure-astar-oracle-sweep.mjs:231`
 **Issue:** `const suboptimalHere = result.seconds > optimal + ORACLE_EPSILON_S;` only flags routes SLOWER than the brute-force oracle. If a future regression makes `routeAstar` traverse an edge the oracle never would (e.g., relaxing `edgeFlags` bit0, or a broken reduced-weight sign letting it cut a oneway), the search returns seconds BELOW the oracle — `suboptimalHere` is false, the row is stamped `pass`, `worstDeltaPct` (positive-only, line 233) never moves, and the gate prints `GATE PASS: 0/200 suboptimal`. The header claims the gate proves "routeAstar seconds must equal the Dijkstra oracle"; equality is not what is implemented. This is the exact failure class this sweep was built to catch on the real graph (its own header: it exists because the D-03 corpus "structurally cannot catch a stochastic per-edge inconsistency"), and the class-break check (lines 218-227) does not cover it — both sides still route.
 **Fix:**
 ```js
 // symmetric: the oracle is the ground truth in BOTH directions
 const deltaS = result.seconds - optimal;
 const suboptimalHere = Math.abs(deltaS) > ORACLE_EPSILON_S;
 ...
 if (Math.abs(deltaPct) > Math.abs(worstDeltaPct)) worstDeltaPct = deltaPct; // track magnitude, not sign
 ```
 (Optionally label a `seconds < optimal − ε` row as `ORACLE-MISMATCH` instead of `SUBOPTIMAL` — same violation count, clearer escalation message.)
 ### WR-02: CPU3 hard-fails on any clone without the gitignored binary and contradicts CPU4's notRun contract
 **File:** `scripts/test/budget-audit.test.mjs:104-118` (esp. line 107)
 **Issue:** CPU3 asserts `assert.equal(r.notRun, undefined, "the graph binary is committed — the ladder must run, not skip")` and then runs the full 25-route real-graph ladder. The binary is **not committed**: `routing/data/.gitignore` line 3 ignores `*.bin` (verified — `git check-ignore` matches `kl-roadgraph.v1.bin`, a 45 MB local artifact), and `docs/ROAD-GRAPH.md` states forks build their own. On any fresh clone or fork — the suite's documented audience ("forks rerun it before their first driver", `docs/TESTING.md`) — CPU3 FAILs on the identical environment condition that CPU4 (lines 120-126) explicitly pins as the legal `notRun` class. One suite, two contradictory contracts for "binary absent", plus a factually false claim in the assertion message.
 **Fix:** Make CPU3's precondition explicit and consistent with CPU4:
 ```js
 const graphPresent = existsSync(path.join(ROOT, "routing", "data", "kl-roadgraph.v1.bin"));
 if (!graphPresent) {
  console.log("SKIP — CPU3 needs the gitignored local binary (build it first); not a failure");
 } else {
  // existing body, minus the false "committed" claim
 }
 ```
 Or fold CPU3's post-ALT wording pins (the `EXECUTED in Phase 8` / no-`next-milestone` asserts, which are the part worth keeping hermetic) into a source-inspection test that doesn't need the binary.
 ### WR-03: Baseline recorder has no oversampling — 41/100 frozen pairs are no-ops; D-04's "~100 pairs" delivered n=53 measured
 **File:** `routing/scripts/measure-optimality-baseline.mjs:95-118` (with the committed record `routing/test/fixtures/alt-baseline.json`)
 **Issue:** `generateCrossMetroPairs` draws uniformly from the full KL bbox and records unsnappable endpoints as-is. The frozen record's verdict histogram is `{unsnappable: 41, reachable: 53, unreachable: 6}` — 41% of the D-03/D-04 evidence corpus never exercised the search at all, so the optimality gate's green verdict rests on **53 measured pairs**, not the "~100 cross-metro worst-case pairs" D-04 specifies. The sibling tool solved this exact problem and documented why ("random bbox coordinates often sit off-road — the KL bbox is wide", `measure-astar-oracle-sweep.mjs:26-34`) via `SWEEP_OVERSAMPLE_FACTOR = 3`; the baseline recorder predates and lacks it. The numbers are honestly printed everywhere (n=53), so this is an evidence-strength defect, not a dishonesty defect — but the phase's headline optimality claim is thinner than its own decision record promises, and the record is now immutable.
 **Fix:** For any future re-record (explicit git revert + re-run): add an oversample-and-consume loop mirroring the sweep's — draw `count × OVERSAMPLE_FACTOR` raw pairs with one pinned seed, consume in order until `count` pairs have both endpoints snapped, and print the rejection count. Do not edit or re-record the existing frozen record to apply this (Pitfall 1 holds); document the 53-pair basis where the record is cited if it isn't already.
 ### WR-04: Quantized weight can wrap negative in `Int32Array`, silently bypassing the range law
 **File:** `routing/src/landmark.ts:114`
 **Issue:** `qWeight[e] = Math.floor(edge.seconds / input.quantumS);` — for `seconds / quantumS ≥ 2³¹` (~214,748,364.8 s at the 0.1 s quantum ≈ 6.8 years on one edge), the `Int32Array` store wraps modulo 2³² and can produce a **negative** weight (e.g. 3e10 → −64,771,072). The `nd > LANDMARK_MAX_QUANTA` throw in `dijkstraPass` (line 233) never fires on a negative contribution; Dijkstra then runs with a negative edge, `finish()` stores the wrapped label into a `Uint16Array` (a label of −1 becomes 65535 — **the sentinel** — reachable nodes silently marked unreachable, others silently rescaled), and the resulting tables can yield ALT triangle bounds above the true distance — inadmissible steering, with nothing thrown. This contradicts the module's own header law: "any label beyond LANDMARK_MAX_QUANTA (65 000) throws at build time… Nothing is clamped, ever." Not reachable from the in-repo build script (haversine-bounded segments in a ~0.7° bbox max out around 1e5 quanta, which throws loud), but `landmark.ts` is a pure library consumed by four call sites and any future tool; the law has a silent hole at the storage boundary.
 **Fix:** Validate at the store, where the law lives:
 ```js
 const q = Math.floor(edge.seconds / input.quantumS);
 if (q > Number.MAX_SAFE_INTEGER || q >= 0x7fffffff) {
  fail(context, `quantized weight ${q} overflows the i32 weight domain (raise landmarkQuantumS)`);
 }
 qWeight[e] = q;
 ```
 ## Info
 ### IN-01: ROAD-GRAPH.md claims an encode round-trip gate the build script does not perform
 **File:** `docs/ROAD-GRAPH.md:34-36`
 **Issue:** "writes `routing/data/kl-roadgraph.v1.bin` — **only** after the encode round-trip and the size gate pass." `build-road-graph.mjs` performs no decode round-trip — `decodeGraph` is imported only inside `--check` (line 195); the write path is encode → stats print → size gates → write (lines 474-530). The script's own header describes it correctly ("after the encode and the stats gate both pass", lines 31-33); the doc overstates the enforcement. Round-trip correctness is proven by the G1 suite on fixtures, not by the build.
 **Fix:** Reword to "only after the encode and the size gates both pass (round-trip equality is pinned by the G1 codec suite, not re-proven per build)" — or actually add the decode-and-compare if the stronger claim is wanted.
 ### IN-02: Date and stale-comment drift (2025 vs 2026; GC1 header contradicts its own assertions)
 **File:** `docs/BUDGETS.md:261`; `bot/test/guidance-cron.test.mjs:14` and `:11-12`
 **Issue:** (a) `docs/BUDGETS.md` line 261: "RESOLUTION (launch-gate decision, **2025**-10-25)" and `guidance-cron.test.mjs` line 14: "launch-gate retune, **2025**-10-25" — the surrounding record dates the same decision and its measurements 2026-10-24/25 (audit date, commit SHAs, this phase). A year-early stamp on the decision that resolved the KV-list finding. (b) `guidance-cron.test.mjs` GC1's header comment (lines 11-12) still describes the pre-retune vector ("T+300 … '667 M', ETA 13:15 · 6.7 KM LEFT") while the test body asserts the retuned values (T+240, "2.0 KM", ETA 13:16 · 8.0 KM LEFT) — the comment documents behavior the test explicitly does not assert.
 **Fix:** Correct both dates to 2026-10-25; rewrite GC1's header to the retuned vector the body pins.
 ### IN-03: `routeAstar` silently returns wrong seconds on a corrupt chain
 **File:** `routing/src/astar.ts:536`
 **Issue:** `seconds += edge >= 0 ? graph.edgeSeconds[edge]! : 0; // -1 only on a corrupt chain — loud in deriveSteps`. The loudness only materializes if the caller later derives steps; `routeAstar`'s own return value in that case is a path whose seconds quietly omit edges — a fabricated (too-low) duration, which the fail-honest law treats as the one disallowed outcome. Reachable only via a code bug or content-corrupt binary (reconstruction bridges are checked traversable at relaxation time), so likelihood is low — but the repo's own law ("never a fabricated route") argues the corrupt case should not launder into a confident number.
 **Fix:** Throw on `edge < 0` here (mirroring `deriveSteps`' contract: "a caller bug is loud"), or return `seconds: NaN` — anything but a plausible wrong number.
 ### IN-04: `decodeGraph` accepts non-finite coordinates/bbox/maxSpeed; the bad maxSpeed then silently changes the heuristic's units
 **File:** `routing/src/graph.ts:208-214,230-231` (decode reads with no finiteness checks); consumed at `routing/src/astar.ts:319-320`
 **Issue:** Encode validates coordinate and `maxSpeedKmh` finiteness loudly; decode — the fail-honest path — validates structure only (magic/version/counts/quantum/size). A crafted or corrupted binary with NaN/Inf coordinates or `maxSpeedKmh ≤ 0/NaN` decodes non-null. `routeAstar` then hits `heuristicScale = maxSpeedMps > 0 ? 1 / maxSpeedMps : 1` — for maxSpeed ≤ 0 or NaN the heuristic silently becomes meters compared against seconds (no crash, nonsense steering); NaN coordinates poison `metersTo` and get cached as NaN in `hfCache`/`hbCache` (the NaN-as-unset sentinel doubles as a cache miss, so it recomputes forever). Owner-hosted R2 makes this defense-in-depth rather than an exposure, and the documented decode law scopes validation to structure — but the asymmetry with `encodeGraph` is a gap, not a decision.
 **Fix:** In `decodeGraph`, reject non-finite `maxSpeedKmh` (one guard, mirrors the quantum check at line 223) and consider an `isFinite` spot-check on the four bbox corners; coordinate-level checks can stay out if the trust model is documented where the decode law is.
 ### IN-05: Every `budget-audit` run prints a known-false "1,440/day OVER" KV-list row
 **File:** `docs/BUDGETS.md:156-160` (documenting `scripts/budget-audit.mjs`, outside this file list)
 **Issue:** The record itself states the audit CLI's comparison table "still hard-codes the pre-retune `derived 1,440/day … OVER` row" while the authoritative state is 720/day under the ceiling. So the one command the doc tells maintainers and forks to run (`npm run budget-audit`) prints a stale over-ceiling line on every execution, in tension with the print-the-measurement law the same record enforces elsewhere. Acknowledged, but acknowledgment doesn't stop the false line from training the next reader's skepticism in the wrong direction.
 **Fix:** Derive the row from `bot/wrangler.jsonc`'s `crons` + `CARD_CADENCE_S` at print time (the derivation is 6 lines), or update the hard-coded row to 720/day with the derivation comment.
 ### IN-06: p95 sample excludes unreachable probes — the costliest requests are ungated
 **File:** `routing/scripts/measure-route-cpu.mjs:229-244` (default mode), `:130-139` (extended)
 **Issue:** Unreachable pairs are excluded from the gated sample in both modes ("reported separately"). An unreachable pair is the search that runs until both heaps empty — typically the maximum-work request — and in production a Worker request that returns null burns the same 10 ms CPU budget as one that returns a route; Error 1102 does not care about the response body. Excluding them means the gated p95 systematically understates the per-request CPU class that actually risks the free-plan kill. The decision is documented and consistent, and with the current verdict failing by ~20× it changes no outcome today — noted because if the escalation ever lands and the gate goes green, the green verdict will not have covered the null-returning request class.
 **Fix:** When the gate is next re-anchored, add a second gated statistic: `p95(all probes incl. null)` alongside the routed-only p95, so the honest-degradation path has its own number against the budget.
 ### IN-07: D03A delta silently passes on a zero `pre` (NaN comparison)
 **File:** `routing/scripts/measure-optimality-gate.mjs:148-149`
 **Issue:** `deltaPct = ((post - pre) / pre) * 100` — with `pre === 0` (a `reachable` record row carrying `seconds: 0`), `deltaPct` is NaN and `Math.abs(NaN) > OPTIMALITY_EPSILON_PCT` is false, so the pair stamps `pass` with no seconds comparison at all. Unreachable for the frozen record (verified: no zero-second rows; 12 km minimum pair distance makes origin===goal snap impossible), so this is a defensive gap for future records only — but a gate whose failure mode on bad input is "pass" is the wrong default.
 **Fix:** Guard explicitly: `if (pre > 0) { …delta check… } else { cls = "violation"; reason: "record seconds 0 is outside the domain of the epsilon check" }` — fail loud on domain violations, consistent with the gate's own constant-mirror posture.
 ## WR Fixes Addendum (2026-10-25)
 Three of the four warnings were fixed and verified the same day; **WR-03 and all seven infos remain OPEN** for the verifier record.
 | Finding | Status | Commit | Fix summary |
 | ------- | ------ | ------ | ----------- |
 | WR-01 | **APPLIED** | `11e55c1` | `measure-astar-oracle-sweep.mjs` compare is symmetric: `Math.abs(deltaS) > ORACLE_EPSILON_S` is the violation test in BOTH directions; a faster-than-oracle row stamps `ORACLE-MISMATCH` (traversal-law-break signature) instead of `pass`; `worstDeltaPct` tracks magnitude; gate description/exit-codes/GATE-FAIL/GATE-PASS text updated to the \|Δ\|-both-directions law. Re-run on the current binary post-fix: **GATE PASS, 0/200 \|Δ\| violations (182 routed / 18 unreachable, worst \|Δ\| +0.000%), exit 0.** |
 | WR-02 | **APPLIED** | `95399ae` | CPU3 no longer claims the binary is committed (it is gitignored — `routing/data/.gitignore` ignores `*.bin`; `git check-ignore` verified). It now mirrors CPU4's law: absent binary → honest SKIP (never a failure); the ladder assertions fire only when the binary is present. Reason text says "gitignored fork-rebuildable artifact". Suite: **19/19 vectors ok** with the binary present. |
 | WR-03 | open | — | Deferred by maintainer decision (baseline-recorder oversampling; frozen record immutable — see the finding's own fix note). |
 | WR-04 | **APPLIED** | `e77bbe9` | `landmark.ts` closes the negative-wrap hole: new named constant `LANDMARK_I32_WEIGHT_CEILING` (0x7fffffff); `prepare()` validates the raw seconds/quantumS quotient BEFORE the `Int32Array` store and throws the quantum-ceiling error family (edge, ceiling, `raise landmarkQuantumS`) — negative-wrap / absurd weights can no longer silently corrupt the tables. Validation is per-edge at table-build entry; the search hot path stays allocation-free. New regression vector **L5b** in `routing/test/landmark.test.mjs`. |
 **Post-fix battery:** `npm run routing:test` 82/82 (was 81 + L5b), `npm run bot:test` 126/126, routing + bot typechecks clean, oracle sweep GATE PASS (above). `web:test` not touched, not re-run.
 ---
 _Reviewed: 2026-10-25T14:28:05Z_
 _Reviewer: OpenCode (gsd-code-reviewer)_
 _Depth: standard_