19 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status, wr_fixes_applied, wr_fixes_applied_date, open_findings
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | wr_fixes_applied | wr_fixes_applied_date | open_findings | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 08-alt-landmark-preprocessing-routing-backend-v2 | 2026-10-25T14:28:05Z | standard | 21 |
|
|
issues_found |
|
2026-10-25 |
|
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:
// 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:
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:
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