From 4942c770f352a8acdc40a0619951a66c6a9774c6 Mon Sep 17 00:00:00 2001 From: araharah-executor Date: Sun, 25 Oct 2026 22:36:43 +0800 Subject: [PATCH] =?UTF-8?q?docs(08):=20WR=20fixes=20addendum=20=E2=80=94?= =?UTF-8?q?=20WR-01/02/04=20applied=20(11e55c1,=2095399ae,=20e77bbe9);=20W?= =?UTF-8?q?R-03=20+=20infos=20stay=20open=20for=20the=20verifier?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../08-REVIEW.md | 187 ++++++++++++++++++ 1 file changed, 187 insertions(+) create mode 100644 .planning/phases/08-alt-landmark-preprocessing-routing-backend-v2/08-REVIEW.md diff --git a/.planning/phases/08-alt-landmark-preprocessing-routing-backend-v2/08-REVIEW.md b/.planning/phases/08-alt-landmark-preprocessing-routing-backend-v2/08-REVIEW.md new file mode 100644 index 0000000..e1028ec --- /dev/null +++ b/.planning/phases/08-alt-landmark-preprocessing-routing-backend-v2/08-REVIEW.md @@ -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_