fix(routing,codex): latency scoring, workspace-denial classification, and a sync regression

devlog/_plan/260816_wave34_closeout/000_research.md

@@ -0,0 +1,81 @@
+# 000 — Wave 3/4 research: what the roadmap got right and wrong
+
+Source: the external Wave 3-4 roadmap (same audit conversation, second answer). Baseline `origin/dev` = `7c348a032`.
+
+The roadmap was written against a GitHub snapshot taken while the Wave 0/1/2 loop was still running, so it is stale in BOTH directions: it asks for work already done, and it invents symbols that do not exist while missing machinery that does.
+
+## W3-00 — already satisfied
+
+The roadmap opens with a public-dev convergence gate because it saw Wave 2 PRs still open. Verified now: PRs `#1805 #1806 #1741 #1825 #1817 #1819 #1788 #1780 #1792 #1703` are all MERGED and issues `#1786 #1824 #1785 #1700 #1767 #1668 #1697` are all CLOSED. The gate passes; no action.
+
+## Roadmap symbols that do not exist
+
+| Roadmap name | Reality |
+|---|---|
+| `materializeCodexUpstreamAuth()` | absent. The real shared builder is `headersForCodexAuthContext(headers, ctx)` at `src/codex/auth-context.ts:454`. |
+| `CatalogConvergenceError` | absent. Failures are DATA, not throws: `CatalogDisposition` with `reason: provider-auth \| provider-network \| disk` at `src/codex/convergence-types.ts:160`. |
+| `provider_family` / `route_account_key` / `conversation_root_key` / `model_key` | absent. A stronger contract already exists: `OcxReasoningReplayIdentity` at `src/types.ts:3-21`. |
+| `clean/routed/recoverable/ambiguous/invalid` classifier | absent as one enum. Three separate classifiers exist: residue `clean\|residue\|indeterminate` (`src/codex/native-residue.ts:46`), integration record `missing\|ready\|invalid` (`src/codex/integration-record.ts:98`), coordinator `ready\|legacy-ambiguous\|unavailable` (`src/codex/convergence-types.ts:298`). |
+| `adoption-pending` row | exists only in the ARCHIVED design doc `devlog/_fin/260804_codex_write_substrate/005_contract.md:709`, not in runtime. |
+| `context_window_too_small` / `modality_unknown` / `compatible_fallback` | absent. |
+
+## Roadmap machinery that already exists
+
+- `DataPlaneAdmission` DOES exist (`src/server/auth-cors.ts:314`) as `configured\|environment\|loopback`. It is credential-identity-aware but NOT presentation-source-aware — it does not record whether the token arrived as dedicated header, bearer, or `x-api-key`.
+- A fresh-disk, field-scoped mutation primitive already exists: `mutatePersistedConfig<T>()` at `src/config.ts:2884`, with rebase-up-to-3-times and exact-raw-string comparison.
+- A baseline/live/persisted three-way merge already exists: `saveConfigPreservingClaudeCode` at `src/config.ts:3264`, though its baseline is armed only for long-lived server instances (`src/server/index.ts:556`), so unarmed CLI calls get lock + atomic write but no rebase.
+- Real latency IS measured — but only inside `healthScore()` at `src/routing/health.ts:386`, scaled by `optimize.health`, never by `optimize.latency`.
+
+## The security claim is wrong, and that matters
+
+The roadmap asserts a direct main path forwards the caller's admission secret upstream. Traced at this SHA, that is **not** reachable:
+
+- `/v1/responses` admission reads ONLY `x-opencodex-api-key`; bearer is rejected (`src/server/auth-cors.ts:441`, pinned by `tests/data-plane-admission-identity.test.ts:116`).
+- Direct then runs `validateForwardAdmissionCredential` BEFORE upstream auth resolution and throws 401 on a recognized proxy secret (`src/server/responses/core.ts:970`; the validator executes at `src/server/auth-cors.ts:407`).
+- Pool/main-pool overwrite Authorization with the stored account token (`src/codex/auth-context.ts:460`), proven by `tests/server-auth.test.ts:1543`.
+
+A caller bearer IS forwarded in Direct, but only under canonical `authMode: "forward"` — intentional passthrough, not a leak.
+
+So #1686 is real for the OPPOSITE reason the roadmap gives: the proxy **refuses** the bearer-admission flow instead of admitting it and substituting stored main auth. Implementing it means widening admission while GUARANTEEING overwrite — and relaxing `validateForwardAdmissionCredential` alone would create exactly the leak that guard currently prevents.
+
+## Issue-by-issue disposition
+
+| Issue | Roadmap wave | Verified disposition |
+|---|---|---|
+| `#1686` | W3-01/02 | Real. Needs source-aware admission + guaranteed substitution. Large, security-boundary. |
+| `#1049` | W3-03/04 | Real. Legacy homes bypass the lock (`src/codex/inject.ts:901`); a test currently PINS that (`tests/codex-inject-write-lock.test.ts:144`). High migration risk. |
+| `#1802` | W3-06 | **Already satisfied on dev.** `/api/sync` calls `loadConfig()` at the route boundary (`src/server/management/config-routes.ts:383`) and `syncModelsToCodex` writes Codex artifacts, not `config.json`. Needs a regression test, then close. |
+| `#1835`/`#1838` | W3-06 | Both CLOSED as duplicates, but the technical report is still accurate: `config set/unset` reads outside the lock (`src/cli/config-command.ts:133`) then whole-snapshot saves (`:145`). Small, landable via existing `mutatePersistedConfig`. |
+| `#1798` | W3-08 | Real. Restore is exact-byte restore-or-strip; an app-rewritten unmarked `openai_base_url` survives because ownership requires the marker (`src/codex/injected-marker.ts:53`, `src/codex/inject.ts:1193`). |
+| `#1789` | W3-09 | Real. 403 → `credential` → `markAccountNeedsReauth` unconditionally (`src/codex/routing.ts:330`, `:1678`); a test PINS it (`tests/codex-routing.test.ts:377`). |
+| `#1791` | W3-10 | Real. Storage is fixed `weeklyPercent`/`monthlyPercent`, not window-generic (`src/codex/quota.ts:7`); a 5-hour primary + 7-day secondary loses the true weekly window (`:439`). |
+| `#1784` | W3-12 | Real. Cause discarded twice: `src/codex/management-convergence.ts:107` and `src/server/management-api.ts:177` both manufacture `reason: "disk"`. |
+| `#1834` | W3-13 | CLOSED not_planned (template), refiled as **`#1837`**. Real: evaluator reads only health/quota/cost (`src/routing/evaluator.ts:397`); `components.latency` is never populated. |
+| `#1830` | W4-03 | Real, and PR #1832 is CI-green but its evidence is thin — no test builds the real advertised catalog and asserts the 120KB budget with `exec`/`wait` present. |
+| `#1524` | W4-08 | Real. Fallback reuses frozen eligibility (`src/server/responses/policy-fallback.ts:153`); `payloadEligible` checks only encrypted-task decryptability (`src/server/responses/core.ts:1463`). |
+| `#1795` | W4-04 | Stays open. No live SenseNova/Kimi reproduction has been run. |
+
+## W4 items already landed (verify only, do NOT reimplement)
+
+`#1741` linear name recovery (`src/chat/inbound.ts:102`), `#1825` malformed→502 (`src/adapters/openai-chat.ts:292`), `#1817`+`#1844` Cursor nested-helper guidance (`src/adapters/cursor/tool-definitions.ts:194`). All present at `7c348a032`.
+
+## PR #1840 correction
+
+The roadmap warns it is a pending 331-file archive PR that will wreck rebases. It is already MERGED (`d07ec0a7d`), GitHub reports 369 changed files, and no open-branch conflict traces to it. No action.
+
+## Scope decision for this unit
+
+The roadmap's dependency chain ("migrate every writer before anything else") is overstated: `mutatePersistedConfig` already exists, so `config set/unset` needs no new primitive. The genuinely large primitives — legacy-home adoption (#1049) and drifted restore (#1798) — are independent of it.
+
+Ordered by (real user impact) / (implementation risk), landing smallest-first:
+
+1. `#1802` — evidence only, then close.
+2. `#1837` — latency term or honest removal.
+3. `#1789` — workspace outcome split.
+4. `#1784` — typed cause propagation.
+5. `#1791` — quota window generalization.
+6. `#1835` — CLI set/unset through the existing primitive.
+7. `#1823` — scoped signature replay (PR redesign).
+8. `#1830` — real catalog serialization evidence.
+9. `#1524` — capability preflight in fallback.
+10. `#1686`, `#1049`, `#1798` — the three large ones, each its own cycle.

devlog/_plan/260816_wave34_closeout/010_1802_sync_evidence.md

@@ -0,0 +1,24 @@
+# 010 — #1802: prove `/api/sync` cannot clobber a hand edit, then close
+
+Roadmap put this under "migrate every writer". The audit found the acceptance condition is **already met** on `dev`; what is missing is proof.
+
+## Why it is already met
+
+- `POST /api/sync` calls `loadConfig()` at the route boundary and passes that fresh object onward (`src/server/management/config-routes.ts:383`, `:390`). It does not use a stale in-memory snapshot.
+- `syncModelsToCodex` writes Codex artifacts, not OpenCodex `config.json` (`src/codex/sync.ts:60`, `:153`). The reported clobber path does not exist here anymore.
+
+## Required regression
+
+In `tests/management-config-routes.test.ts` (or the nearest sync-owning test file):
+
+1. Start the server so a live config is held in memory.
+2. Hand-edit `config.json` on disk out of band — add a provider and change a `modelCosts` row — so the on-disk state is strictly newer than the server's snapshot.
+3. `POST /api/sync`.
+4. Assert the on-disk `config.json` still contains the hand-edited provider and cost row byte-for-byte.
+5. Assert `loadConfig()` after the call returns those same values.
+
+The test must fail if someone later reintroduces a cached-config read at that route, so assert against the DISK, not the response body.
+
+## Close-out
+
+Merge the test, then close `#1802` explaining that the 2.21.0 save-path fix plus the route-boundary `loadConfig()` already close the sync path, and that this regression now pins it.

devlog/_plan/260816_wave34_closeout/020_1837_latency.md

@@ -0,0 +1,60 @@
+# 020 — #1837: make `optimize.latency` real, or stop claiming it
+
+`#1834` was closed `not_planned` purely for bypassing the issue template; `#1837` is the refiled copy and is the live one.
+
+**Status: IMPLEMENTED** at commit `ad889ce51`. This document records the verified defect and the shape that landed.
+
+## Verified defect
+
+- `optimize.latency` exists and defaults to `0.55` — the LARGEST default weight (`src/routing/profile.ts:27`).
+- Normalization includes it in the four-way sum and stores it (`src/routing/profile.ts:505-517`).
+- The evaluator read only `optimize.health`, `.quota`, `.cost`. Whatever weight remained became `priorityWeight = 1 - spentHealth - spentQuota - spentCost`, multiplied by `configuredPriorityScore(...)` (`src/routing/evaluator.ts:397`).
+- `components.latency` was declared in the trace type (`src/routing/trace.ts:132`) and in the parse allowlist (`:631`) but never populated.
+
+So a profile weighted entirely toward latency silently became a profile weighted entirely toward **declaration order**: `configuredPriorityScore(index, total)` decreases with index and selection is a strict-greater argmax (`src/routing/evaluator.ts:183`, `:444`), so the first declared candidate won regardless of measured latency.
+
+Real latency WAS measured, but only as a subcomponent of `healthScore()` (`src/routing/health.ts:386`), scaled by `optimize.health`.
+
+## What landed
+
+There is no `candidate` variable at the evaluator's scoring site; the available object is `evidence`, and the p50 lives at `evidence.health?.recentLatencyMs`. The existing computation was extracted into a shared export with an explicit unknown contract:
+
+```ts
+// src/routing/health.ts — shared so the health composite and the standalone
+// term cannot drift apart. Unknown p50 returns the NEUTRAL midpoint, not 0.
+export function latencyScoreFromEvidence(evidence: RouteHealthEvidence | undefined): number;
+```
+
+```diff
+   const costWeight = profile.optimize.cost;
++  const latencyWeight = profile.optimize.latency;
++  const latencyValue = latencyWeight > 0 ? latencyScoreFromEvidence(health) : null;
+   const spentCost = costValue !== null ? costWeight : 0;
++  const spentLatency = latencyValue !== null ? latencyWeight : 0;
+-  const priorityWeight = Math.max(0, 1 - spentHealth - spentQuota - spentCost);
++  const priorityWeight = Math.max(0, 1 - spentHealth - spentQuota - spentCost - spentLatency);
+```
+
+```diff
+   if (costWeight > 0 && costValue !== null) { ... }
++  if (latencyWeight > 0 && latencyValue !== null) {
++    total += latencyWeight * latencyValue;
++    components.latency = latencyValue;
++  }
+```
+
+Health keeps consuming the same helper under `HEALTH_SCORE_CONSTANTS.LATENCY_WEIGHT`, unchanged.
+
+Scoring an unknown p50 as 0 would punish an unmeasured candidate into last place, which reintroduces order-dependence by another name — so the neutral midpoint is the contract, not a convenience.
+
+## Tests
+
+Two existing tests asserted the exact composite score and BOTH broke: `tests/routing-profile.test.ts` ("deterministic priority picks the earlier candidate") and `tests/policy-execution.test.ts:105`, each expecting `total: 0.685` with no latency component. Both now assert components rather than a brittle total.
+
+Added regressions:
+
+- two candidates where the LATER-declared one has materially lower p50: with `latency: 1` it wins, which is the exact inversion the issue reports;
+- an unmeasured candidate is not ranked below a measured-but-slow one;
+- with `latency: 0` the declaration-order result is unchanged and `components.latency` is absent, so existing profiles see no behavior change.
+
+Verified green: `routing-profile`, `policy-execution`, `routing-compatibility`, `combos`, `health-scoring`, `codex-routing` (total 172 pass / 0 fail), plus `bun x tsc --noEmit` clean.

devlog/_plan/260816_wave34_closeout/030_1789_workspace_outcome.md

@@ -0,0 +1,54 @@
+# 030 — #1789: stop calling a workspace denial a credential failure
+
+## Verified defect
+
+`CodexUpstreamOutcomeClass` is `success | credential | quota | transient | caller | neutral | unknown` — no workspace or entitlement member (`src/codex/routing.ts:129`).
+
+`classifyCodexUpstreamOutcome` maps **both** 401 and 403 to `credential` (`src/codex/routing.ts:330`), and a `credential` outcome unconditionally calls `markAccountNeedsReauth`, clears quota health, and drops thread affinity (`src/codex/routing.ts:1678`).
+
+So a K12 workspace 403 tells the user to re-authenticate a credential that is perfectly valid. `tests/codex-routing.test.ts:377` currently PINS that mapping.
+
+A richer-looking classifier exists on a different path (`src/codex/quota-rejection.ts:11`, distinguishing `authentication-error` from `permission-error`), but at `7c348a032` its 403 branch returns immediately WITHOUT reading the body. Nothing anywhere parses the denial today, so the evidence has to be produced, not merely threaded.
+
+### The denial parser
+
+Add it beside the existing exhaustion-code reader, reusing the same bounded, duplicate-key-safe machinery:
+
+```ts
+const WORKSPACE_DENIAL_CODES = new Set(["codex_workspace_access_denied", "workspace_access_denied"]);
+const ENTITLEMENT_DENIAL_CODES = new Set(["codex_entitlement_missing", "entitlement_missing"]);
+
+async function denialFromResponse(r: Response, signal?: AbortSignal):
+  Promise<"workspace" | "entitlement" | undefined>;
+```
+
+It reads through `readBoundedResponseBody`, refuses a truncated / non-display-safe / duplicate-key document exactly as `resetEligibleCodeFromResponse` does, then looks for an OWN-property `code` at the top level or under `error` — no coercion, no accessors. An allowlisted code maps to a denial; anything else returns `undefined`.
+
+Fail-closed is the point: an unreadable or unknown body keeps the historical credential handling, or a genuinely revoked credential would stop prompting for reauthentication.
+
+## Fix
+
+1. Add `workspace` to `CodexUpstreamOutcomeClass`.
+2. Introduce an explicit discriminator rather than sniffing strings at the classifier. The upstream rejection is already parsed once; carry its structured outcome forward:
+
+```ts
+type CodexUpstreamEvidence = {
+  status: number | "connect_error" | "timeout" | "connect_neutral";
+  /** Set when the upstream body identified a workspace/entitlement denial rather than a bad credential. */
+  denial?: "workspace" | "entitlement";
+};
+
+classifyCodexUpstreamOutcome(evidence: CodexUpstreamEvidence): CodexUpstreamOutcomeClass
+```
+
+   401 stays `credential`. A 403 with `denial: "workspace"` becomes `workspace`; a 403 with no denial evidence stays `credential`, so the change fails safe toward today's behavior.
+
+3. **Carry it on the existing meta rather than changing every signature.** `CodexUpstreamOutcomeMeta` already reaches `recordCodexUpstreamOutcome` from every call site, so adding `denial?: "workspace" | "entitlement"` there means only the sites that can actually observe a 403 body need to populate it — in the Responses path, the two `quotaMeta` construction points.
+
+4. In the outcome handler, a `workspace` result must NOT call `markAccountNeedsReauth`. Record the failure in `upstreamHealth` so routing can prefer a healthier account, then return — deliberately NOT clearing thread affinity. Credential quarantine sweeps affinity because reauthentication is account-wide; a workspace denial is not, so existing bindings stay valid. No new per-route store is introduced: the health entry plus the preserved affinity IS the behavior change, which keeps the blast radius to the one wrong remedy.
+
+## Tests
+
+Two existing tests encode the current policy and both must be revisited, not just the first: the classifier assertion at `tests/codex-routing.test.ts:377`, and the 403 quarantine behavior at `:429`. Update them to the new contract.
+
+Add: a workspace-denial 403 does not set reauth and leaves the credential usable on a non-workspace route; a bare/unclassifiable 403 still behaves exactly as today; 401 is unchanged; and a workspace denial recorded through one call site is classified the same as one recorded through another.