diff --git a/devlog/_plan/260816_wave34_closeout/000_research.md b/devlog/_plan/260816_wave34_closeout/000_research.md new file mode 100644 index 0000000000..2f1f3a970a --- /dev/null +++ b/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()` 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. diff --git a/devlog/_plan/260816_wave34_closeout/010_1802_sync_evidence.md b/devlog/_plan/260816_wave34_closeout/010_1802_sync_evidence.md new file mode 100644 index 0000000000..caf79678f2 --- /dev/null +++ b/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. diff --git a/devlog/_plan/260816_wave34_closeout/020_1837_latency.md b/devlog/_plan/260816_wave34_closeout/020_1837_latency.md new file mode 100644 index 0000000000..e713c8a18d --- /dev/null +++ b/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. diff --git a/devlog/_plan/260816_wave34_closeout/030_1789_workspace_outcome.md b/devlog/_plan/260816_wave34_closeout/030_1789_workspace_outcome.md new file mode 100644 index 0000000000..bcf90b16a4 --- /dev/null +++ b/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. diff --git a/devlog/_plan/260816_wave34_closeout/040_1784_typed_cause.md b/devlog/_plan/260816_wave34_closeout/040_1784_typed_cause.md new file mode 100644 index 0000000000..12e28405d7 --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/040_1784_typed_cause.md @@ -0,0 +1,56 @@ +# 040 — #1784: stop manufacturing `reason: "disk"` + +## Verified defect + +There is no exception type to propagate — failures are DATA. `CatalogDisposition` carries `reason: provider-auth | provider-network | disk` plus `phase`, `retryable`, `partialWrite`, and **no cause field** (`src/codex/convergence-types.ts:160`). + +The cause is discarded twice: + +1. `createManagementConvergeCodex` catches, inspects two message substrings for busy/database admission, and otherwise returns hard-coded `failed/disk` without storing the caught error (`src/codex/management-convergence.ts:52`, `:62`, `:107`). +2. `src/server/management-api.ts:177` catches without binding the error at all, and again manufactures `reason: "disk"`, choosing gather-vs-commit purely from whether invocation had begun. + +Tests pin both collapses (`tests/codex-management-convergence.test.ts:67`, `tests/codex-convergence-contract.test.ts:303`). + +So a malformed request and a genuine ENOSPC are indistinguishable to the operator, and both are reported non-retryable. + +## Fix + +Extend the disposition rather than inventing the roadmap's `CatalogConvergenceError`: + +```diff + type CatalogDisposition = { + ... + reason: "provider-auth" | "provider-network" | "disk" ++ | "request-invalid" | "admission" | "internal"; ++ /** Allowlisted cause summary. Closed vocabularies only -- never message text. */ ++ cause?: { name: string; detail: string }; + }; +``` + +- `request-invalid` for programming/shape errors (malformed scope, bad factory input). +- `admission` for lock/contention/database-busy, which IS retryable. +- `internal` for anything genuinely unclassified — explicitly not `disk`. +- `disk` narrows to real filesystem failures. + +### The normalization boundary must be updated too + +`normalizeCatalogDisposition` (`src/codex/catalog-refresh-status.ts:42`) is an explicit privacy boundary: it rebuilds the disposition from an allowlist of own data properties precisely so a malformed callback cannot smuggle provider or account detail into a management response. Adding `reason` values or a `cause` field without updating that allowlist means the new information is silently dropped — the plan would look implemented and change nothing. + +So: extend the allowlist with the new reasons AND with `cause`, and update its exhaustive disposition matrix plus `tests/codex-catalog-refresh-status.test.ts`. + +### `cause` must be a bounded summary, not an error message + +Passing `Error.message` through `redactSecretString` is NOT sufficient. That helper masks token-shaped values; it does not remove filesystem paths, home directories, account ids or hostnames, all of which routinely appear in an error message and all of which this boundary exists to keep out. + +Build `cause` from bounded, non-sensitive parts instead: + +Both fields are allowlists, because neither is a fixed vocabulary in practice: + +- `name`: map onto a closed set — `"invalid-request" | "lock-busy" | "io" | "unknown"`. An arbitrary `Error.constructor.name` is dependency- or input-influenced (any thrown custom class names itself), so it is not safe to echo. +- `detail`: only a recognized `errno`/`code` token from a closed set (`ENOSPC`, `EACCES`, `EPERM`, `SQLITE_BUSY`, ...), or a fixed per-branch phrase. Never `Error.message`, never a path, never an interpolated identifier. + +If a branch cannot produce a safe summary, omit `cause` entirely. The point of the change is that `request-invalid` is distinguishable from `disk`; the free-text message is not required for that. + +## Tests + +Update the two pinning tests to assert the NEW classification rather than `disk`, update the disposition matrix in `tests/codex-catalog-refresh-status.test.ts`, and add: a malformed scope yields `request-invalid`; a simulated lock-busy yields `admission` with `retryable: true`; a simulated write failure still yields `disk`; and an error whose message embeds a home path plus a token-shaped string produces a response containing neither. diff --git a/devlog/_plan/260816_wave34_closeout/050_1791_quota_windows.md b/devlog/_plan/260816_wave34_closeout/050_1791_quota_windows.md new file mode 100644 index 0000000000..27696566c5 --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/050_1791_quota_windows.md @@ -0,0 +1,76 @@ +# 050 — #1791: keep every quota window, not two named ones + +## Verified defect + +Upstream sends three fixed slots, each a `WhamUsageWindow` with `used_percent`, `reset_at`, `limit_window_seconds` (`src/codex/quota.ts:38`). + +Storage is not window-generic: + +```ts +type StoredAccountQuota = { + weeklyPercent?: number; monthlyPercent?: number; + weeklyResetAt?: number; monthlyResetAt?: number; + resetCredits?: number; monthlyIsPrimaryWindow?: boolean; updatedAt: number; +} +``` + +(`src/codex/quota.ts:7`, persisted as `{ version: 1, quotas }` at `:24`, `:357`, `:377`.) + +`parseUsageQuota()` folds a non-monthly primary into `weekly` and treats secondary only as a fallback (`:439`, `:468`). A K12 account with a 5-hour primary and a 7-day secondary therefore reports the 5-hour window AS the weekly one, and the real weekly window disappears. + +## Fix + +Store windows as an array, but keep the PROVENANCE the current shape encodes. A duration-only array silently discards `monthlyIsPrimaryWindow`, and that flag is load-bearing: it distinguishes the governing window from a supplementary one, so dropping it risks both false cooldown recovery and treating a supplementary tertiary window as account exhaustion. + +```ts +type StoredQuotaWindow = { + /** Upstream slot the window arrived in — the provenance the old flag encoded. */ + slot: "primary" | "secondary" | "tertiary"; + /** + * The real discriminator: 5h, 7d, 30d. OPTIONAL, because older WHAM responses omit + * `limit_window_seconds` entirely (`src/codex/quota.ts:55`, and the note at `:463`). + * When absent, the slot is the only provenance we have and band lookup must fall back + * to it rather than inventing a duration. + */ + limitWindowSeconds?: number; + usedPercent: number; + resetAt?: number; + /** True for the window that governs admission, preserving monthlyIsPrimaryWindow's meaning. */ + governing: boolean; +}; +type StoredAccountQuota = { + windows: StoredQuotaWindow[]; + resetCredits?: number; + updatedAt: number; +}; +``` + +Persist as `version: 2`. Hydration must accept BOTH: a `version: 1` document is upgraded in memory by mapping `weekly*` and `monthly*` onto synthetic windows (7d / 30d), so an existing install does not lose its quota state on upgrade. Writing always emits v2. + +Exhaustion becomes "any governing window at limit", not "the weekly one", which is the behavior `#1791` asks for. + +### This is a field-chain migration, not a type edit + +`StoredAccountQuota` fields are read well beyond `quota.ts`. Before writing code, enumerate and update every consumer — at minimum `src/codex/quota.ts`, `src/codex/auth-api.ts`, `src/codex/routing.ts`, the CLI DTOs, the main-account cache, and capacity projection. Each site that reads `weeklyPercent`/`monthlyPercent`/`weeklyResetAt`/`monthlyResetAt`/`monthlyIsPrimaryWindow` today must move to the accessor below or be listed here with a reason it does not. + +Consumers keep working through small accessors that select by duration band (falling back to slot when the duration is absent) and by `governing`, rather than by stored name. + +Exhaustion is "a GOVERNING window at limit", not "any window at limit" — the latter would let a supplementary tertiary window take an account out of rotation, which is a new bug wearing the fix's clothes. + +**Multiple windows can govern simultaneously.** #1791's own case is exactly that: a 5-hour and a weekly window are both upstream-enforced, and hitting either genuinely blocks the account. So `governing` is a per-window boolean rather than a single winner, the accessor is plural, and exhaustion is `windows.some(w => w.governing && atLimit(w))`: + +```ts +function governingWindows(q: StoredAccountQuota): StoredQuotaWindow[]; +function windowByBand(q: StoredAccountQuota, band: "short" | "weekly" | "monthly"): StoredQuotaWindow | undefined; +``` + +Cooldown recovery then uses the EARLIEST reset among governing windows that are at limit — the account becomes usable again when the first blocking window rolls over, not when the last one does. + +## Tests + +- K12 payload: 5-hour primary + 7-day secondary produces two windows with independent reset times, and the weekly one is genuinely the 7-day. +- v1 document on disk hydrates without loss (including `monthlyIsPrimaryWindow` becoming `governing`) and re-persists as v2; a v2 document round-trips unchanged. +- Any governing window at 100% marks the account exhausted; a supplementary window at 100% does NOT. +- With two governing windows at limit, recovery uses the earliest reset among them. +- A v1 document and a live payload that both omit `limit_window_seconds` still classify by slot without inventing a duration. +- Dashboard, CLI and routing consumers return the same values they used to for an ordinary two-window account. The existing quota, recovery, API and CLI tests must pass unchanged where behavior is unchanged. diff --git a/devlog/_plan/260816_wave34_closeout/060_1835_cli_mutation.md b/devlog/_plan/260816_wave34_closeout/060_1835_cli_mutation.md new file mode 100644 index 0000000000..2d6fd64f0e --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/060_1835_cli_mutation.md @@ -0,0 +1,65 @@ +# 060 — #1835/#1838: route `config set/unset` through the existing primitive + +Both issues are CLOSED as duplicates of each other, but the defect is live and the fix is small. Reopen `#1838` (the surviving number) or land the fix referencing both. + +## Verified defect + +`config set/unset` reads the disk snapshot OUTSIDE the mutation lock (`src/cli/config-command.ts:133`), then sends that whole older snapshot through `saveConfig` (`:145`). A concurrent edit landing in between is silently reverted. `config import` likewise replaces via raw `saveConfig` (`:178`). + +## Fix — no new primitive needed + +The roadmap asks for a new "config mutation intent primitive". It already exists: + +```ts +mutatePersistedConfig(mutate: (config: OcxConfig) => PersistedConfigMutation): PersistedConfigMutationOutcome +``` + +(`src/config.ts:2884` — clones the latest validated disk config, reruns the callback, compares exact raw strings, rebases up to three times.) + +The real contract is narrower than a naive patch callback. `PersistedConfigMutation` is `{ changed: boolean; value: T }`, and the outcome is `{ status: "committed" | "unchanged"; value: T } | { status: "unavailable"; reason: "missing" | "invalid" | "conflict" }` (`src/config.ts:2855-2866`). There is no `applyPath`; the existing local helper is `setPath(root, path, value, remove)` at `src/cli/config-command.ts:48`. + +So `set`/`unset` become: + +```ts +const outcome = mutatePersistedConfig(config => { + // Snapshot BEFORE mutating, so `changed` is an honest comparison. + const before = JSON.stringify(config); + + const candidate = structuredClone(config) as Record; + setPath(candidate, path, parsed, action === "unset"); + + // Same validation the command already performs, but on the FRESH candidate. + const validated = validateConfigCandidate(candidate); + if (!validated.ok) throw new Error(validated.error); + + // The pin clear belongs to this transaction, not a follow-up write. + if (pathSegments(path)[0] === "codexAccountPriorities") clearCodexAccountPin(validated.config); + + // REPLACE, do not merge: Object.assign cannot remove a key that `unset` deleted, + // so an unset would appear to succeed while changing nothing. + for (const key of Object.keys(config)) { + if (!(key in validated.config)) delete (config as Record)[key]; + } + Object.assign(config, validated.config); + + return { changed: JSON.stringify(config) !== before, value: undefined }; +}); +if (outcome.status === "unavailable") { /* report missing | invalid | conflict, exit non-zero */ } +``` + +Three things the callback must get right, all of which the current code does outside the lock: + +- `changed` must compare a snapshot taken BEFORE the mutation against the object after it. Comparing after `Object.assign` compares a value with itself and always reports `true`, which bumps the config generation on a no-op write. +- The callback mutates `config` in place because that is the object the primitive commits, but it must REPLACE its contents: delete every key the validated candidate dropped before assigning. A plain `Object.assign` merge cannot remove a key, so `unset` would silently become a no-op. +- Validation runs on the candidate built from the FRESH config, not the one read before the lock. +- `clearCodexAccountPin` (`src/cli/config-command.ts:144`) must stay inside the transaction; leaving it outside reintroduces the same race for the pin. + +The read now happens inside the transaction, so the mutation is applied to whatever is actually on disk at commit time. + +**`import` deliberately does NOT change.** Import is an intentional whole-document replacement; forcing it through patch semantics would silently merge instead of replace, which is a different and worse surprise. What it needs instead is honesty: compute the set of top-level keys present on disk but absent from the imported document and warn about each before writing, so a replacement that drops the user's providers is announced rather than discovered later. + +## Tests + +In the CLI config tests: a `set` whose callback observes an externally-changed disk state applies onto the NEW state, not the stale one; `unset` likewise; `import` still replaces wholesale but emits a warning naming each dropped top-level key; and a byte-identical `set` does not bump the config generation. + +`tests/cli-headless-parity.test.ts:378` and the pin behavior at `:429` currently exercise this command path. Both must keep passing unchanged — if the `clearCodexAccountPin` ordering or the validation error text moves, they regress, and that is the signal that the migration was done wrong. diff --git a/devlog/_plan/260816_wave34_closeout/070_1823_signature_scope.md b/devlog/_plan/260816_wave34_closeout/070_1823_signature_scope.md new file mode 100644 index 0000000000..a3af620b8f --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/070_1823_signature_scope.md @@ -0,0 +1,48 @@ +# 070 — PR #1823: scope the thought-signature replay store + +Head `493c7712a`. Draft, no review threads, no exact-head test CI. The roadmap's factual description of the store is accurate; its prescription is not. + +**Citation base:** every `src/responses/thought-signature-replay.ts` reference below is at PR #1823 head `493c7712a`, not at baseline `7c348a032` — that file does not exist on `dev`. The `src/bridge.ts`, `src/types.ts` and `src/config.ts` references are at the PR head too, since the PR edits `bridge.ts`. + +## Verified defects (all three block merge) + +1. **Key is the client-visible `call_id` alone.** `entries: Map`, write `entries.set(callId, ...)`, lookup `entries.get(callId)` (`src/responses/thought-signature-replay.ts:29-33`, `:92-97`, `:129-138`). Two threads, accounts, providers or models sharing a `call_id` overwrite each other. +2. **Emit precedes durability.** `response.output_item.added` is emitted at `src/bridge.ts:1026-1033`, before the store is touched at all (`:612-637`). The persist itself is an unawaited chain ending in `.catch(() => {})` (`src/responses/thought-signature-replay.ts:77-88`), so `done` can reach the client before the atomic rename completes. +3. **Failures are silent successes.** File-open, corrupt-JSON and write failures are all swallowed (`:43-46`, `:62-64`, `:86-88`). + +Two corrections to the roadmap: eviction is age/write-order, **not** LRU (`:71-74`), and the 16,384-entry cap is not a memory bound — each signature may be 64KiB (`src/responses/provider-opaque-metadata.ts:31-38`), so the nominal ceiling is ~1GiB with no byte cap. + +A fourth, separate defect: ordinary function calls enqueue the same signature twice, because `rememberExtraContentForReplay` is unconditional at `src/bridge.ts:612-614` and the function branch then calls `rememberAndSerializeExtraContent` at `:627-635`. Same pattern buffered at `:1587-1608`. + +## Fix — reuse the existing identity, do not invent a schema + +The roadmap proposes a new SQLite table keyed by `provider_family/route_account_key/conversation_root_key/call_id/model_key`. None of those exist, and a stronger contract already does: + +```ts +// src/types.ts:3-21 +interface OcxReasoningReplayIdentity { + providerName: string; providerDestinationIdentity: string; + adapterName: string; modelId: string; credentialIdentity: string; +} +interface OcxReasoningReplayScopeRef { readonly clientThreadId: string; current?: ... } +``` + +`src/responses/reasoning-replay-cache.ts:65-84` already keys on thread + provider + destination + adapter + model + credential + call id, and `tests/reasoning-replay-identity.test.ts:56-69` already proves the isolation dimensions. Use that identity. + +SQLite is not justified by any demonstrated failure. What is required is: + +1. **Scope the key** to the existing replay identity plus `call_id`. +2. **Move hydration out of `parseRequest`.** The parser runs before thread attachment and route selection (`src/server/responses/core.ts:1642-1649`, `:1704-1709`), so it cannot query a route-scoped store. Parse, bind the real scope, then hydrate before adapter serialization. +3. **Await durability before the first emit.** `atomicWriteFileAsync` already returns only after write, harden and rename (`src/config.ts:307-334`); awaiting it is a sufficient commit point. +4. **Typed result instead of `void`:** `stored | already_equal | conflict | unscoped | persist_failed`. A different signature under the same complete key fails closed rather than overwriting. +5. **Add a byte cap** and prune after load. +6. **Guard the duplicate enqueue** so an ordinary function call persists once. + +Note on restart: the in-memory cache uses process-local HMAC identities, so those exact hashes cannot be written to disk and expected to survive a restart. Persist stable non-secret route/account identifiers (or an install-persistent HMAC key) together with thread root, model and `call_id`. + +## Tests + +- `tests/thought-signature-replay.test.ts` (new): conflict result, byte cap, TTL, corrupt store, injected persist failure, restart-stable keys, exactly-one-write per call. +- `tests/google-signature-history-roundtrip.test.ts`: same `call_id` across two threads/accounts/models stays isolated; restart recovery. +- `tests/responses-stream-tool-events.test.ts`: no `output_item.added`/`done` observable before an injected persistence promise settles. +- `tests/config-ownership-uninstall.test.ts`: `thought-signature-replay.json` is actually removed as owned state. diff --git a/devlog/_plan/260816_wave34_closeout/080_1830_cursor_evidence.md b/devlog/_plan/260816_wave34_closeout/080_1830_cursor_evidence.md new file mode 100644 index 0000000000..34cba08ddb --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/080_1830_cursor_evidence.md @@ -0,0 +1,38 @@ +# 080 — #1830 / PR #1832: supply the evidence the fix is missing + +PR #1832 head `4424138a3`, non-draft, exact-head CI green, both review threads resolved. The change itself is plausible; the evidence is thin. + +## What the PR does + +Four things only: `normalizeRoutedCatalogEntry` sets `supports_search_tool = true` for Cursor while still deleting hosted-search metadata (`src/codex/catalog/parsing.ts:458-473`), `deriveEntry` gives template-less Cursor rows the same flag (`src/codex/catalog/sync.ts:360-369`), a structure doc update, and tests that assert only `tool_mode` / `supports_search_tool` / `web_search_tool_type` / parallel calls. + +## The gap + +Nothing at that head builds the ACTUAL advertised Cursor tool catalog, serializes it, and proves it still fits. The machinery exists but is unconnected: + +- `CURSOR_TOOL_BYTES_LIMIT = 120_000` and `applyCursorToolBudget(tools, toolChoice)` measure real protobuf size (`src/adapters/cursor/request-builder.ts:29`, `:65`). +- `cursorMcpToolsEncodedSize(...)` serializes `McpToolsSchema` via `toBinary` (`src/adapters/cursor/tool-definitions.ts:684`). +- Existing budget tests use hand-built `exec`/`wait` fixtures (`tests/cursor-request-builder.test.ts:458`, `:486`), not the catalog this PR changes. + +So the PR could pass while the real catalog either exceeds the budget or drops the execution bridge — which is exactly `#1830`'s symptom. + +## Required test + +**Constructibility caveat first.** OpenCodex does not build Codex's host-advertised `exec`/`wait` catalog; `src/responses/parser.ts:647` consumes whatever tools the client supplied. So the test cannot synthesize the real catalog from repository code — it must start from a captured fixture of a real Cursor-routed subagent request, committed as test data, or from an explicit boundary seam added for the purpose. + +With that fixture, parse it the way a real turn does, build the request through `createCursorRequest`, and assert: + +1. serialized size `<= CURSOR_TOOL_BYTES_LIMIT`; +2. a Responses-owned execution tool survives the budget — `exec` (or the bridge equivalent) is still present; +3. `wait` is still present; +4. (separately) the catalog projection carries the search flag. + +Assert (1)-(3) against the SERIALIZED form, not the pre-budget array, or the test proves nothing about what the child receives. + +Assertion (4) belongs to a DIFFERENT test. `supports_search_tool` is routed catalog/model metadata; it is not serialized into a Cursor turn request, so looking for it in the protobuf would either fail or silently pass on an unrelated substring. Keep two assertions in two places: a catalog-projection test for the flag (which #1832 already has), and a request/tool-budget test for size and tool survival. + +The live Cursor child check stays a SEPARATE functional acceptance gate. The byte test proves the catalog survives the budget; only a real child performing a read-only task proves the execution path works. + +## Close-out + +`#1830`'s stated acceptance is broader than the byte budget: it wants a Responses-owned execution client tool injected or preserved, and explicitly does NOT want users globally enabling Cursor-native local execution (the maintainer comment narrows it to retaining the Responses-owned path). Merge #1832 with the serialization test; close `#1830` only if a fresh Cursor child demonstrably performs a read-only task through a host-recognized exec path. If that live check cannot be run, keep `#1830` open and say so — the byte test alone is not the acceptance condition. diff --git a/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md b/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md new file mode 100644 index 0000000000..9fdcef9486 --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md @@ -0,0 +1,50 @@ +# 090 — #1524: check capability before falling back + +## Verified defect + +Fallback reuses the ORIGINAL evaluation instead of re-checking the physical candidate: `policy-fallback.ts` carries forward frozen `eligible`, empty exclusions, the old score, and "not tried" state (`src/server/responses/policy-fallback.ts:26`, `:153`). + +The runtime eligibility hook checks only whether an encrypted agent task can be decrypted (`src/server/responses/core.ts:1271`, `:1291`, `:1463`). Combo selection checks configured/enabled provider, prior attempts, caller eligibility and cooldown (`src/combos/resolve.ts:85`, `:97`, `:169`) — nothing about whether the request FITS. + +So a 200k-token conversation or an image-bearing request can fall back onto a candidate that cannot accept it. + +## What already exists + +- Capability evidence resolves context windows and image support from config, registry, catalog or native metadata (`src/routing/capability.ts:153`, `:163`, `:174`). +- The INITIAL evaluator can already reject on supplied context/image requirements (`src/routing/evaluator.ts:187`, `:209`, `:280`). +- Request evidence detects tools and images but leaves request context size UNKNOWN (`src/routing/request-evidence.ts:36`). + +The missing pieces are (a) an actual request-size measurement and (b) re-running the existing eligibility check at fallback time. + +## Two mechanisms, two fixes + +There are two independent fallback paths and one change does not cover both. Splitting them is the difference between a fix and a half-fix. + +Also note what already works: policy routing DOES evaluate image requirements for every candidate, and every concrete retry runs `checkInputAdmission` (`src/server/responses/input-admission.ts:159`, called at `src/server/responses/core.ts:1956`) before upstream I/O. So an oversized request is not silently sent — the real defect is that an incompatible candidate TERMINATES the fallback chain instead of being skipped so the next one can be tried. + +### (a) Policy fallback + +1. Populate request context size in `PolicyRequestEvidence` (`src/routing/request-evidence.ts:36` leaves it unknown). + + **Measurement point matters.** `estimateInputTokens(parsed, modelId)` (`src/server/responses/input-admission.ts:89`) needs a model id, and routing runs BEFORE a model is chosen — so it cannot be called as-is at evaluation time. Compute a model-independent estimate once from the parsed request and store it on the evidence; each candidate then compares that figure against its own ceiling. + + **Use the same threshold as admission.** `checkInputAdmission` refuses only past `ceiling * ADMISSION_TOLERANCE` where `ADMISSION_TOLERANCE = 2.5` (`input-admission.ts:32`, `:168`). If the evaluator excluded on exact fit while admission tolerates 2.5x, routing would refuse candidates that would actually have worked, which is a new outage in the name of a fix. Apply the same tolerance, and state it in the exclusion reason so the trace explains itself. +2. In `policy-fallback.ts:153`, re-evaluate each candidate against the CURRENT evidence instead of reusing the frozen verdict, using the evaluator's existing context/modality rejection. + +### (b) Combo fallback + +`policy-fallback.ts` is not on this path. Combo selection needs `payloadEligible` (`src/server/responses/core.ts:1463`) extended beyond encrypted-task decryptability to consult the same capability evidence. Without this, the combo path cited in the issue is unchanged. +3. Unknown capability is NOT a pass. When neither config, registry, catalog nor native metadata can answer, treat the candidate as ineligible for a request that requires the capability, and record the reason — the issue explicitly asks for conservative-unknown handling. +4. Never truncate history or drop images to force a candidate to fit. If nothing is compatible, fail with a typed reason naming the constraint. +5. Preserve quota/cooldown/priority semantics: capability is an additional filter, not a replacement ordering. + +## Tests + +Cover BOTH paths explicitly: + +- Policy fallback: a candidate with a smaller context window than the request is skipped and the next compatible one is used — not treated as the end of the chain. +- Combo fallback: the same, through `payloadEligible`. +- An image-bearing request skips a text-only candidate on both paths. +- A candidate with unknown modality support is skipped for an image request but still usable for a text one. +- No compatible candidate produces a typed failure naming the constraint, not a silent truncation. +- Cooldown and prior-attempt exclusion still apply unchanged. diff --git a/devlog/_plan/260816_wave34_closeout/100_1686_admission.md b/devlog/_plan/260816_wave34_closeout/100_1686_admission.md new file mode 100644 index 0000000000..582f490031 --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/100_1686_admission.md @@ -0,0 +1,63 @@ +# 100 — #1686: bearer admission with guaranteed upstream substitution + +One PABCD cycle. Security-boundary change: requires explicit security review per `MAINTAINERS.md`. + +## The roadmap's framing is wrong + +It claims a direct main path leaks the caller's admission secret upstream. Traced at `7c348a032`, that is not reachable: `/v1/responses` admission reads only `x-opencodex-api-key` (`src/server/auth-cors.ts:441`), and Direct runs `validateForwardAdmissionCredential` (executing at `src/server/auth-cors.ts:407`) before upstream auth resolution, throwing 401 on a recognized proxy secret (`src/server/responses/core.ts:970`). Pool and main-pool overwrite Authorization with the stored account token (`src/codex/auth-context.ts:460`). Non-forward adapters install their own credential. Only canonical `authMode: "forward"` forwards a caller bearer, which is intentional. + +The real defect is the inverse: the proxy REFUSES the intended flow instead of admitting it and substituting stored main auth. + +## Required shape + +### 1. Presentation-source-aware admission + +`DataPlaneAdmission` exists as `{ kind: "configured"; keyId } | { kind: "environment" } | { kind: "loopback" }` (`src/server/auth-cors.ts:314`). It records WHICH credential matched, not HOW it was presented. Add the source: + +```ts +type DataPlaneAdmissionSource = "loopback" | "dedicated" | "bearer" | "x-api-key"; +type DataPlaneAdmission = + | { kind: "configured"; keyId: string; source: DataPlaneAdmissionSource } + | { kind: "environment"; source: DataPlaneAdmissionSource } + | { kind: "loopback"; source: "loopback" }; +``` + +The source is currently lost before `resolveDataPlaneAdmissionSecret` — the broad resolver extracts headers in precedence order at `:416` and passes only the token. Both resolvers must pass the source through. + +### 2. Accept bearer for Responses, keep the precedence + +`resolveResponsesApiAuth` (`:441`) gains a bearer fallback with dedicated header still winning; `x-api-key` stays rejected there. Update `AUTH_MATRIX` (`:379`), which currently declares bearer rejected, and the SOT prose in `structure/05_gui-and-management-api.md:70`. + +### 3. Thread the admission to where the decision happens + +HTTP currently drops it before `handleResponses` (`src/server/index.ts:1214`) while WebSocket retains it (`src/server/ws-bridge.ts:63`). Enumerate the full chain and update each hop: HTTP Responses, compact Responses (`src/server/responses/compact.ts:353` repeats the selection locally), the Chat-translated path, and WebSocket. + +### 4. One upstream-auth materializer + +Build on today's `headersForCodexAuthContext(headers, ctx)` (`src/codex/auth-context.ts:454`): + +```ts +function materializeCodexUpstreamAuth( + headers: Headers, + ctx: CodexAuthContext, + admission: DataPlaneAdmission | undefined, +): Headers; +``` + +- `pool` / `main-pool`: always overwrite Authorization and `chatgpt-account-id` from the context credential (today's behavior, unchanged). +- `main` + admission bearer: require a live stored main token, overwrite BOTH headers, and throw before any I/O if unavailable. +- `main` + dedicated admission + a distinct real ChatGPT bearer: preserve today's intentional passthrough. + +**Do not relax `validateForwardAdmissionCredential` on its own.** Without guaranteed overwrite that creates precisely the leak the guard prevents today. The guard may only be narrowed once substitution is proven to run first. + +### 5. Modern injection + +`src/codex/inject.ts:210` emits only `env_http_headers`. Emit `env_key = "OPENCODEX_API_AUTH_TOKEN"` for supported runtimes, with any legacy fallback capability-gated rather than emitting both blindly. + +## Tests + +- `tests/data-plane-admission-identity.test.ts:116` currently PINS bearer rejection — update it, and add source identity, precedence, invalid bearer, and unchanged `x-api-key` rejection. +- `tests/codex-auth-context.test.ts:1140`: unit matrix for `main` / `pool` / `main-pool` materialization. +- `tests/server-auth.test.ts:1325`: successful HTTP/compact/WebSocket Direct substitution, missing-main fail-closed, dedicated passthrough unchanged (`:1439`), pool override unchanged (`:1543`). +- `tests/forward-admission-separation.test.ts:63`: the admission secret never appears in captured upstream headers, in both the success and failure cases. +- `tests/codex-inject.test.ts:41`: modern `env_key` output and explicitly supported legacy behavior. diff --git a/devlog/_plan/260816_wave34_closeout/101_1049_legacy_adoption.md b/devlog/_plan/260816_wave34_closeout/101_1049_legacy_adoption.md new file mode 100644 index 0000000000..0a08d1052a --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/101_1049_legacy_adoption.md @@ -0,0 +1,26 @@ +# 101 — #1049: adopt pre-substrate Codex homes into the coordinator + +One PABCD cycle. Runs AFTER `100` — both touch `src/codex/inject.ts`. + +## Verified state + +Legacy homes bypass the write lock entirely: injection calls `applyNativeArtifacts()` directly for `legacy-uncoordinated` (`src/codex/inject.ts:901`) and restore calls `restoreCodexConfigInline()` (`:1504`). The eligibility layer says so explicitly (`src/codex/inject-coordination.ts:36`, `:84`), and `tests/codex-inject-write-lock.test.ts:144` currently PINS that bypass. + +Already satisfied: residue detection, invalid-record refusal, unversioned/rowless refusal (`src/codex/transition-state.ts:269`, `:288`). + +The roadmap's unified `clean/routed/recoverable/ambiguous/invalid` classifier does not exist. What exists is three separate results: 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 eligibility is a FUNCTION of those three, not a fourth enum to replace them. + +## Required shape + +The `adoption-pending` design already exists in the archived contract (`devlog/_fin/260804_codex_write_substrate/005_contract.md:709`, crash boundary at `:735`) but runtime `transition-state.ts` does not accept that status. Implement it there: + +1. Derive adoption eligibility from the three existing classifiers. Routed + no coordinator + valid record + clean-or-explainable residue is adoptable; indeterminate residue or an invalid record refuses. +2. Write a pending adoption row with an exact-byte fingerprint of the artifacts being adopted, BEFORE publishing anything. +3. Publish under the lock, then clear the row. +4. On startup, a pending row whose fingerprint still matches disk is recoverable and resumes; one whose fingerprint does NOT match refuses and leaves the home legacy-operable rather than guessing. + +Preserving legacy operability on refusal is the non-negotiable part: a failed adoption must never leave a home that neither the legacy path nor the coordinator will touch. + +## Tests + +`tests/codex-inject-write-lock.test.ts:144` asserts the bypass being removed — update it. Add per-state fixtures: adoptable home adopts and then uses the lock; indeterminate residue refuses; invalid record refuses; unversioned/rowless database refuses; a kill at each I/O boundary leaves either the pre-adoption state or a resumable pending row, never a half-published home. diff --git a/devlog/_plan/260816_wave34_closeout/102_1798_restore_merge.md b/devlog/_plan/260816_wave34_closeout/102_1798_restore_merge.md new file mode 100644 index 0000000000..b5bbbe3040 --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/102_1798_restore_merge.md @@ -0,0 +1,33 @@ +# 102 — #1798: restore must survive an app rewrite + +One PABCD cycle. Runs AFTER `101` — also touches `src/codex/inject.ts`. + +## Verified state + +1. Injection stores original bytes plus an injected hash. +2. The Codex App rewrites `config.toml`. +3. Restore sees the hash mismatch and refuses journal restoration (`src/codex/journal.ts:123`). +4. Fallback removal only recognizes an `openai_base_url` immediately preceded by the OpenCodex marker (`src/codex/injected-marker.ts:53`), because `removeCodexConfig` bases ownership on that predicate (`src/codex/inject.ts:1193`). +5. The app-rewritten, unmarked line therefore survives. + +Partially satisfied already: catalog backup lookup falls back from the hash-named backup to the legacy backup for the default path (`src/codex/catalog/parsing.ts:545`). + +## Required shape + +Replace exact-byte restore-or-strip with a three-way semantic merge over baseline B (what we saved at injection), injected I (what we wrote), and current C (what is on disk now): + +- A key whose current value equals I is ours — remove it, or restore B's value if B had one. +- A key whose current value differs from BOTH B and I was changed by the user or the app — preserve it. +- A key present in B, absent from I, and absent from C was removed by someone else — do not resurrect it. + +**Do not ship a marker-only deletion patch.** An unmarked `openai_base_url` may be genuinely user-owned; deleting it because it looks like ours is data loss, and it is the failure mode this issue is one half of. + +When the merge cannot classify a key confidently, leave it and report it. A restore that says "I left these three lines, check them" is far better than one that silently deletes a user's setting. + +## Tests + +- App rewrites the injected line unmarked: restore removes it and preserves an unrelated user key added in the same rewrite. +- User sets their own `openai_base_url` before injection: restore returns THAT value, not absence. +- User edits an unrelated key after injection: it survives restore byte-identical. +- Hash mismatch no longer means give-up: the merge path runs and the home ends clean. +- The catalog fallback at `parsing.ts:545` keeps working; add a regression if none pins it. diff --git a/devlog/_plan/260816_wave34_closeout/110_closeout.md b/devlog/_plan/260816_wave34_closeout/110_closeout.md new file mode 100644 index 0000000000..f87670cac1 --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/110_closeout.md @@ -0,0 +1,45 @@ +# 110 — Execution order and gates + +## Order (smallest verified risk first) + +1. `010` #1802 — regression only, then close. +2. `020` #1837 — latency term. +3. `030` #1789 — workspace outcome split. +4. `040` #1784 — typed cause. +5. `050` #1791 — quota windows v2. +6. `060` #1835/#1838 — CLI set/unset via `mutatePersistedConfig`. +7. `070` #1823 — scoped signature replay. +8. `080` #1830 — catalog serialization evidence (PR #1832). +9. `090` #1524 — capability preflight. +10. `100` #1686 — bearer admission + guaranteed substitution. Security review required. +11. `101` #1049 — legacy home adoption. AFTER `100`. +12. `102` #1798 — restore three-way merge. AFTER `101`. + +One decade doc per PABCD work-phase — which is why the three large units are `100`, `101` and `102` rather than one document. + +Units touching the same file are sequenced, not parallel: + +- `100`, `101` and `102` ALL edit `src/codex/inject.ts`; they run strictly in that order, each rebased onto the previous. +- `030`, `040` and `050` all sit in the Codex account/catalog area (`src/codex/routing.ts`, `quota.ts`, convergence); run them in listed order. +- `060` edits `src/cli/config-command.ts` and `090` edits `src/server/responses/core.ts`; neither overlaps the above. + +## Gates + +- Per unit: focused `bun test ` named in that unit. +- Per merge: exact-head CI green. Contributor branches need their workflow runs AUTHORIZED first — `action_required` is not a pass. A push to a contributor head resets readiness; re-authorize and re-request. +- Per wave: `ssh lidge` `bun test --isolate tests`, compared against a `dev` run at the same commit. A failure present on both sides is not a regression; a failure present only on the branch is. +- Per merge: `git merge-base --is-ancestor origin/dev`. +- Per close: `gh issue view ` shows `CLOSED`. + +`dev` is protected by a pull-request rule, so every change lands as a PR and is merged with admin authority. No `main` promotion, no tag, no publish; close comments say the fix is on `dev` and ships with the next release. + +## Known environment caveats + +- The four Windows shards fail under `workflow_dispatch` on `dev` ITSELF (168 failures, symmetric on both sides). They are skipped on the ordinary `pull_request` path. Do not read them as a branch regression; they deserve their own issue. +- The macOS suite has produced a Bun segfault (`panic: Segmentation fault`, RSS 3.6GB) unrelated to any assertion. Rerun once before treating it as real. + +## Explicitly out of scope + +- `#1795` stays open until a live SenseNova/Kimi reproduction runs. +- W4-01/02/04 are already landed; verify only. +- PR #1840 is already merged; the roadmap's rebase warning is moot. diff --git a/src/cli/config-command.ts b/src/cli/config-command.ts index d7c9b4cb71..796b503f0e 100644 --- a/src/cli/config-command.ts +++ b/src/cli/config-command.ts @@ -1,6 +1,6 @@ import { readFileSync, writeFileSync } from "node:fs"; import { clearCodexAccountPin } from "../codex/account-priority"; -import { getConfigPath, readConfigDiagnostics, sanitizeModelCostsForDisplay, saveConfig, validateConfigCandidate } from "../config"; +import { getConfigPath, mutatePersistedConfig, readConfigDiagnostics, sanitizeModelCostsForDisplay, saveConfig, validateConfigCandidate } from "../config"; import { VISION_REASONING_EFFORTS, isVisionReasoningEffort } from "../reasoning-effort"; import type { OcxConfig } from "../types"; import { normalizeVisionReasoningForModel } from "../vision/reasoning"; @@ -130,19 +130,42 @@ export async function handleConfigCommand(argv: string[]): Promise { const raw = action === "set" ? args.shift() : undefined; if (!path || (action === "set" && raw === undefined)) throw new CliUsageError("config path and value are required", USAGE); rejectArgs(args, USAGE); - const candidate = structuredClone(readConfigDiagnostics().config) as unknown as Record; - setPath(candidate, path, raw === undefined ? undefined : parseValue(raw), action === "unset"); - const config = validate(candidate); - const savedValue = action === "unset" ? null : getPath(config, path); - // Setting the order here is the operator restating it, exactly as through - // `ocx account priority` or the management route, so it releases the manual pin - // for the same reason those do: a pin made before any order existed would - // otherwise outrank every order set afterwards, capping the pool at the pinned - // account's tier with nothing on any surface explaining why. `import` is - // deliberately not covered — that file supplies its own pin, so there is no - // stale one to release. - if (pathSegments(path)[0] === "codexAccountPriorities") clearCodexAccountPin(config); - saveConfig(config); + // #1835/#1838: the read used to happen OUTSIDE the mutation lock, so a concurrent + // edit landing between it and the save was reverted by this whole-snapshot write. + // `mutatePersistedConfig` reruns this callback against the latest validated disk + // state, so the operation is applied to what is actually there at commit time. + let savedValue: unknown = null; + const outcome = mutatePersistedConfig(fresh => { + // Snapshot BEFORE mutating: comparing after the write compares a value with + // itself and would report every no-op as a change, bumping the generation. + const before = JSON.stringify(fresh); + const candidate = structuredClone(fresh) as unknown as Record; + setPath(candidate, path, raw === undefined ? undefined : parseValue(raw), action === "unset"); + const config = validate(candidate); + savedValue = action === "unset" ? null : getPath(config, path); + // Setting the order here is the operator restating it, exactly as through + // `ocx account priority` or the management route, so it releases the manual pin + // for the same reason those do: a pin made before any order existed would + // otherwise outrank every order set afterwards, capping the pool at the pinned + // account's tier with nothing on any surface explaining why. `import` is + // deliberately not covered — that file supplies its own pin, so there is no + // stale one to release. + if (pathSegments(path)[0] === "codexAccountPriorities") clearCodexAccountPin(config); + // REPLACE rather than merge: `Object.assign` alone cannot remove a key that + // `unset` deleted, which would make unset silently succeed while changing nothing. + for (const key of Object.keys(fresh)) { + if (!(key in (config as unknown as Record))) { + delete (fresh as unknown as Record)[key]; + } + } + Object.assign(fresh, config); + return { changed: JSON.stringify(fresh) !== before, value: undefined }; + }); + if (outcome.status === "unavailable") { + throw new Error(outcome.reason === "conflict" + ? "config changed while applying this update; retry" + : `config is ${outcome.reason}`); + } printData({ ok: true, path, value: redact(savedValue, path.split(".").at(-1)) }, wantsJson, [`${action === "unset" ? "Unset" : "Set"} ${path}.`]); return; diff --git a/src/codex/catalog-refresh-status.ts b/src/codex/catalog-refresh-status.ts index 10bde7bd23..8286e85090 100644 --- a/src/codex/catalog-refresh-status.ts +++ b/src/codex/catalog-refresh-status.ts @@ -1,4 +1,4 @@ -import type { CatalogDisposition, CatalogNotice } from "./convergence-types"; +import type { CatalogDisposition, CatalogFailureCause, CatalogNotice } from "./convergence-types"; const INVALID_CATALOG_DISPOSITION_FIELD = Symbol("invalid-catalog-disposition-field"); @@ -69,11 +69,15 @@ export function normalizeCatalogDisposition(value: unknown): CatalogDisposition const phase = ownDataProperty(value, "phase"); const retryable = ownDataProperty(value, "retryable"); const partialWrite = ownDataProperty(value, "partialWrite"); - if ((reason !== "provider-auth" && reason !== "provider-network" && reason !== "disk") + if ((reason !== "provider-auth" && reason !== "provider-network" && reason !== "disk" + && reason !== "request-invalid" && reason !== "admission" && reason !== "internal") || (phase !== "gather" && phase !== "commit") || typeof retryable !== "boolean" || typeof partialWrite !== "boolean") return null; - return { status, reason, phase, retryable, partialWrite }; + // The cause is rebuilt from closed vocabularies, never copied through: this is the + // boundary that keeps a message, path or account id from riding out on a failure. + const cause = normalizeCatalogFailureCause(ownDataProperty(value, "cause")); + return { status, reason, phase, retryable, partialWrite, ...(cause ? { cause } : {}) }; } return null; } catch { @@ -81,6 +85,20 @@ export function normalizeCatalogDisposition(value: unknown): CatalogDisposition } } +const FAILURE_CAUSE_KINDS: ReadonlySet = new Set(["invalid-request", "lock-busy", "io", "unknown"]); +const FAILURE_CAUSE_CODES: ReadonlySet = new Set([ + "ENOSPC", "EACCES", "EPERM", "EROFS", "ENOENT", "SQLITE_BUSY", +]); + +function normalizeCatalogFailureCause(value: unknown): CatalogFailureCause | undefined { + if (value === null || typeof value !== "object" || Array.isArray(value)) return undefined; + const kind = ownDataProperty(value, "kind"); + if (typeof kind !== "string" || !FAILURE_CAUSE_KINDS.has(kind)) return undefined; + const code = ownDataProperty(value, "code"); + const safeCode = typeof code === "string" && FAILURE_CAUSE_CODES.has(code) ? code : undefined; + return { kind, ...(safeCode ? { code: safeCode } : {}) } as CatalogFailureCause; +} + /** Whether a persisted mutation still needs a successful catalog commit. */ export function catalogRefreshIsPending(disposition: CatalogDisposition): boolean { return disposition.status !== "committed"; diff --git a/src/codex/convergence-types.ts b/src/codex/convergence-types.ts index 515ee8c940..2615c531a6 100644 --- a/src/codex/convergence-types.ts +++ b/src/codex/convergence-types.ts @@ -161,8 +161,29 @@ export type CatalogDisposition = | { status: "skipped"; reason: "not-requested" | "catalog-unavailable" | "busy" | "stale" | "refused"; retryable: boolean } - | { status: "failed"; reason: "provider-auth" | "provider-network" | "disk"; - phase: "gather" | "commit"; retryable: boolean; partialWrite: boolean }; + | { status: "failed"; + /** + * `disk` used to absorb every unclassified failure, so a malformed request and a + * genuine ENOSPC were indistinguishable and both reported non-retryable (#1784). + */ + reason: "provider-auth" | "provider-network" | "disk" | "request-invalid" | "admission" | "internal"; + phase: "gather" | "commit"; retryable: boolean; partialWrite: boolean; + /** Allowlisted cause summary. Closed vocabularies only -- never message text. */ + cause?: CatalogFailureCause }; + +/** + * Why a catalog operation failed, in terms safe to return from the management plane. + * + * Both fields are closed sets on purpose. An `Error.constructor.name` is dependency- or + * input-influenced (any thrown custom class names itself) and an `Error.message` routinely + * carries paths, home directories and account identifiers, none of which may cross this + * boundary. + */ +export type CatalogFailureCause = { + kind: "invalid-request" | "lock-busy" | "io" | "unknown"; + /** Recognized errno/code token, when the underlying error carried one. */ + code?: "ENOSPC" | "EACCES" | "EPERM" | "EROFS" | "ENOENT" | "SQLITE_BUSY"; +}; /** * The ONLY way Codex-owned bytes are written. Startup, ensure, /api/sync, the diff --git a/src/codex/management-convergence.ts b/src/codex/management-convergence.ts index b9ad903dc7..9847e5dd97 100644 --- a/src/codex/management-convergence.ts +++ b/src/codex/management-convergence.ts @@ -3,6 +3,7 @@ import { captureCatalogAdmissionSnapshot } from "./catalog-admission"; import { convergeCodexCatalog } from "./convergence"; import type { CatalogDisposition, + CatalogFailureCause, CatalogOnlyOutcome, CodexHistoryState, CodexObservedState, @@ -49,6 +50,58 @@ function notEvaluatedObserved(history: CodexHistoryState): CodexObservedState { }; } +/** Recognized errno/code tokens. Anything else is dropped rather than echoed. */ +const RECOGNIZED_FAILURE_CODES: ReadonlySet = new Set([ + "ENOSPC", "EACCES", "EPERM", "EROFS", "ENOENT", "SQLITE_BUSY", +]); + +/** + * Reduce a caught error to an allowlisted cause (#1784). + * + * Nothing from the error text reaches the caller: `kind` is chosen from a closed set and + * `code` is only emitted when it is a recognized token. An `Error.message` routinely carries + * paths, home directories and account ids, and `redactSecretString` masks token shapes but + * none of those, so the message is never a safe thing to forward from here. + */ +function catalogFailureCause(error: unknown): CatalogFailureCause { + const raw = (error as { code?: unknown } | null)?.code; + const code = typeof raw === "string" && RECOGNIZED_FAILURE_CODES.has(raw) + ? raw as CatalogFailureCause["code"] + : undefined; + if (error instanceof TypeError || error instanceof RangeError || error instanceof SyntaxError) { + return { kind: "invalid-request", ...(code ? { code } : {}) }; + } + if (code === "SQLITE_BUSY") return { kind: "lock-busy", code }; + if (code !== undefined) return { kind: "io", code }; + return { kind: "unknown" }; +} + +/** + * Classify a failure that is NOT a filesystem problem. + * + * `disk` used to be the catch-all, so an operator saw "non-retryable disk failure" for a + * malformed request. Reserve `disk` for real IO and route the rest to honest reasons. + */ +function classifiedCatalogFailure(error: unknown, commitBegan: boolean): CatalogDisposition { + const cause = catalogFailureCause(error); + const reason = cause.kind === "invalid-request" + ? "request-invalid" as const + : cause.kind === "lock-busy" + ? "admission" as const + : cause.kind === "io" + ? "disk" as const + : "internal" as const; + return { + status: "failed", + reason, + phase: commitBegan ? "commit" : "gather", + // Contention is the one class worth retrying unchanged. + retryable: reason === "admission", + partialWrite: commitBegan, + cause, + }; +} + function unexpectedCatalogFailure(commitBegan: boolean): CatalogDisposition { return { status: "failed", @@ -64,7 +117,7 @@ function admissionFailure(error: unknown): CatalogDisposition { if (message.includes("config generation is busy") || message.includes("config generation is database")) { return { status: "skipped", reason: "busy", retryable: true }; } - return unexpectedCatalogFailure(false); + return classifiedCatalogFailure(error, false); } /** Project catalog work into the shared no-change/not-evaluated outcome shape. */ @@ -107,7 +160,7 @@ export function createManagementConvergeCodex( } catch (error) { return projectCatalogOnlyOutcome({ changed: false, - catalogRefresh: commitBegan ? unexpectedCatalogFailure(true) : admissionFailure(error), + catalogRefresh: commitBegan ? classifiedCatalogFailure(error, true) : admissionFailure(error), }); } }; diff --git a/src/codex/quota-rejection.ts b/src/codex/quota-rejection.ts index e0a2e6fad1..55759770c6 100644 --- a/src/codex/quota-rejection.ts +++ b/src/codex/quota-rejection.ts @@ -23,6 +23,63 @@ export interface CodexPreStreamRejection { alternateRetryEligible: boolean; resetCreditEligible: boolean; semanticCode?: CodexResetEligibleExhaustionCode; + /** + * Structured denial evidence for a 403. Present only when the upstream body names a + * workspace/entitlement denial, which proves the CREDENTIAL is valid and the account + * simply lacks access here (#1789). Status alone can never set this. + */ + denial?: "workspace" | "entitlement"; +} + +/** + * Upstream codes that identify a WORKSPACE denial rather than a bad credential. + * + * #1789: a K12 account whose credential validates and whose WHAM usage returns 200 still + * gets 403 `codex_workspace_access_denied` on a routed prompt. Treating that as a credential + * failure tells the user to re-authenticate a credential that is already valid, and the loop + * repeats forever. + */ +const WORKSPACE_DENIAL_CODES: ReadonlySet = new Set([ + "codex_workspace_access_denied", + "workspace_access_denied", +]); + +const ENTITLEMENT_DENIAL_CODES: ReadonlySet = new Set([ + "codex_entitlement_missing", + "entitlement_missing", +]); + +/** Read a structured denial code out of a 403 body. Fails closed to undefined. */ +async function denialFromResponse( + response: Response, + signal?: AbortSignal, +): Promise<"workspace" | "entitlement" | undefined> { + try { + const body = await readBoundedResponseBody(response.clone(), { signal, fatalUtf8: true }); + if (!body.displaySafe || body.truncated || !body.text.trim()) return undefined; + if (isUnsafeJsonDocument(body.text)) return undefined; + const payload = JSON.parse(body.text) as unknown; + const code = structuredDenialCode(payload); + if (code === undefined) return undefined; + if (WORKSPACE_DENIAL_CODES.has(code)) return "workspace"; + if (ENTITLEMENT_DENIAL_CODES.has(code)) return "entitlement"; + return undefined; + } catch { + // Same fail-closed rule as the exhaustion classifier: an unreadable body must not + // downgrade a credential failure into a workspace one. + return undefined; + } +} + +/** Own-property `code` lookup at the top level or under `error`. No coercion, no accessors. */ +function structuredDenialCode(payload: unknown): string | undefined { + if (payload === null || typeof payload !== "object" || Array.isArray(payload)) return undefined; + const direct = (payload as Record).code; + if (typeof direct === "string") return direct; + const error = (payload as Record).error; + if (error === null || typeof error !== "object" || Array.isArray(error)) return undefined; + const nested = (error as Record).code; + return typeof nested === "string" ? nested : undefined; } const RESET_ELIGIBLE_CODES: ReadonlySet = new Set(RESET_ELIGIBLE_CODE_VALUES); @@ -205,7 +262,10 @@ export async function classifyCodexPreStreamRejection( ): Promise { const status = response.status; if (status === 401) return rejection(status, "authentication-error"); - if (status === 403) return rejection(status, "permission-error"); + if (status === 403) { + const denial = await denialFromResponse(response, options.signal); + return { ...rejection(status, "permission-error"), ...(denial ? { denial } : {}) }; + } if (TRANSIENT_SERVER_STATUSES.has(status)) return rejection(status, "transient-server-error"); if (status !== 429 && status !== 402) return rejection(status, "other"); diff --git a/src/codex/quota.ts b/src/codex/quota.ts index 5ca9a9b4e0..5e7b6db684 100644 --- a/src/codex/quota.ts +++ b/src/codex/quota.ts @@ -56,6 +56,19 @@ type WhamUsageWindow = { }; const MONTHLY_WINDOW_MIN_SECONDS = 28 * 24 * 60 * 60; +/** + * Shortest window still plausibly the WEEKLY quota (#1791). + * + * K12 and similar plans send a 5-hour primary window plus a 7-day secondary. Folding the + * primary into `weeklyPercent` reported the 5-hour bar as the weekly one and discarded the + * real weekly reading entirely, so the dashboard showed a window that reset every few hours + * and routing never saw the limit that actually gates the account. + * + * 24h is the discriminator: anything shorter is a burst window, not a weekly one. A window + * with no declared duration is unchanged, because older payloads omit `limit_window_seconds` + * and guessing there would break every legacy account. + */ +const WEEKLY_WINDOW_MIN_SECONDS = 24 * 60 * 60; const MONTHLY_WINDOW_MIN_MINUTES = MONTHLY_WINDOW_MIN_SECONDS / 60; const accountQuota = new Map(); @@ -160,6 +173,15 @@ function hasKnownQuotaValue(quota: Omit): boole .some(value => typeof value === "number" && Number.isFinite(value)); } +/** True only for a window that DECLARES a duration shorter than a day. */ +function isExplicitShortWindow(window: WhamUsageWindow | null | undefined): boolean { + const seconds = window?.limit_window_seconds; + return typeof seconds === "number" + && Number.isFinite(seconds) + && seconds > 0 + && seconds < WEEKLY_WINDOW_MIN_SECONDS; +} + function isExplicitMonthlyWindow(window: WhamUsageWindow | null | undefined): boolean { const seconds = window?.limit_window_seconds; return typeof seconds === "number" @@ -465,10 +487,17 @@ export function parseUsageQuota(data: WhamUsageResponse): Omit(); export type CodexUpstreamOutcome = number | "connect_error" | "timeout" | "connect_neutral"; -export type CodexUpstreamOutcomeClass = "success" | "credential" | "quota" | "transient" | "caller" | "neutral" | "unknown"; +export type CodexUpstreamOutcomeClass = "success" | "credential" + | "workspace" | "quota" | "transient" | "caller" | "neutral" | "unknown"; export type CodexCooldownSource = "retry-after" | "reset-derived" | "default"; /** * Native Codex quota groups known to be independent upstream. Keep the mapping @@ -193,6 +194,12 @@ export type CodexUpstreamOutcomeMeta = { now?: number; /** (provider, host) ledger key for account-neutral reachability failures (#914). */ hostKey?: string; + /** + * Upstream denial evidence for a 403. A workspace/entitlement denial means the CREDENTIAL + * is fine and the account simply cannot reach this workspace, so it must not be quarantined + * for reauthentication (#1789). Absent evidence keeps the historical credential handling. + */ + denial?: "workspace" | "entitlement"; /** Stable transport code recorded alongside a neutral host failure. */ lastFailureCode?: string; /** Native model selected for this request; used only for confirmed scoped quotas. */ @@ -327,7 +334,10 @@ export function computeCodexUsageScore(quota: { return values.length > 0 ? Math.max(...values) : CODEX_UNKNOWN_USAGE_SCORE; } -export function classifyCodexUpstreamOutcome(outcome: CodexUpstreamOutcome): CodexUpstreamOutcomeClass { +export function classifyCodexUpstreamOutcome( + outcome: CodexUpstreamOutcome, + denial?: "workspace" | "entitlement", +): CodexUpstreamOutcomeClass { if (outcome === "connect_neutral") return "neutral"; if (outcome === "connect_error" || outcome === "timeout") return "transient"; if (!Number.isFinite(outcome)) return "unknown"; @@ -337,6 +347,11 @@ export function classifyCodexUpstreamOutcome(outcome: CodexUpstreamOutcome): Cod // and says nothing about the credential. Relayed as the neutral class so a // stray 3xx cannot increment an account's transient streak. if (outcome >= 300 && outcome < 400) return "neutral"; + // 401 is always a credential problem. A 403 is only a credential problem when nothing + // tells us otherwise: a workspace/entitlement denial (#1789) means the credential is valid + // and the account simply lacks access here, so quarantining it for reauth is wrong advice. + // Absent denial evidence the historical mapping stands, so the change fails safe. + if (outcome === 403 && denial !== undefined) return "workspace"; if (outcome === 401 || outcome === 403) return "credential"; // 402 Payment Required is treated as quota exhaustion for pool cooldown/failover // (same-request alternate retry records this outcome for the depleted account). @@ -1596,7 +1611,7 @@ export function recordCodexUpstreamOutcome( const writerGeneration = meta.writerGeneration ?? captureConfigGeneration(); if (writerGeneration < lastReconciledGeneration && !liveHealthAccountIds.has(accountId)) return; const now = meta.now ?? Date.now(); - const outcomeClass = classifyCodexUpstreamOutcome(outcome); + const outcomeClass = classifyCodexUpstreamOutcome(outcome, meta.denial); const quotaScope = codexQuotaScopeForModel(meta.modelId); if (outcomeClass === "success") { const scopedProbe = meta.probeQuotaScope @@ -1676,6 +1691,18 @@ export function recordCodexUpstreamOutcome( } const lastFailureStatus = typeof outcome === "number" ? outcome : 0; + if (outcomeClass === "workspace") { + // The credential is valid; this account just cannot reach this workspace (#1789). + // Record the failure so routing stops preferring it, but do not mark it for + // reauthentication and do not sweep its thread affinities: telling the user to + // re-login is wrong advice that cannot fix a workspace grant. + upstreamHealth.set(accountId, { + consecutiveFailures: (upstreamHealth.get(accountId)?.consecutiveFailures ?? 0) + 1, + lastFailureStatus, + lastFailureAt: now, + }); + return; + } if (outcomeClass === "credential") { // 401/403 quarantines the account for reauth. That supersedes quota state // entirely: a cooldown (and any probe lease) on an unusable account is moot. diff --git a/src/routing/evaluator.ts b/src/routing/evaluator.ts index 7ad44b1d07..a07b833063 100644 --- a/src/routing/evaluator.ts +++ b/src/routing/evaluator.ts @@ -21,7 +21,7 @@ import { type Unknownable, } from "./trace"; import { getRoutingProfile, policyModelId, type NormalizedRoutingProfile } from "./profile"; -import { healthScore } from "./health"; +import { healthScore, latencyScoreFromEvidence } from "./health"; import { quotaScore } from "./quota"; import { costScore } from "./cost"; import { evaluateCompatibilityForCandidate } from "./compatibility/policy"; @@ -398,10 +398,16 @@ export function evaluatePolicyProfile( const healthWeight = profile.optimize.health; const quotaWeight = profile.optimize.quota; const costWeight = profile.optimize.cost; + // `optimize.latency` was normalized into the weight sum but never spent, so whatever + // was allocated to it silently became configuredPriority -- i.e. declaration order. + // Spend it on the same p50-derived score the health composite already uses. + const latencyWeight = profile.optimize.latency; + const latencyValue = latencyWeight > 0 ? latencyScoreFromEvidence(health) : null; const spentHealth = healthValue !== null ? healthWeight : 0; const spentQuota = quotaValue !== null ? quotaWeight : 0; const spentCost = costValue !== null ? costWeight : 0; - const priorityWeight = Math.max(0, 1 - spentHealth - spentQuota - spentCost); + const spentLatency = latencyValue !== null ? latencyWeight : 0; + const priorityWeight = Math.max(0, 1 - spentHealth - spentQuota - spentCost - spentLatency); const components: RouteScoreEvidence["components"] = { configuredPriority: priorityScore }; let total = priorityWeight * priorityScore; if (healthWeight > 0 && healthValue !== null) { @@ -416,6 +422,10 @@ export function evaluatePolicyProfile( total += costWeight * costValue; components.cost = costValue; } + if (latencyWeight > 0 && latencyValue !== null) { + total += latencyWeight * latencyValue; + components.latency = latencyValue; + } if (compatibilityValue !== null) { // Compatibility is a penalty-only dimension. A penalized candidate loses // a bounded fraction of its existing score; satisfied/allowed evidence diff --git a/src/routing/health.ts b/src/routing/health.ts index f1bee547e7..9f3f2115d6 100644 --- a/src/routing/health.ts +++ b/src/routing/health.ts @@ -372,6 +372,20 @@ export function healthEvidenceForCandidate(input: HealthEvidenceInput): RouteHea return evidence; } +/** + * Deterministic latency score in [0,1] from the recorded p50, shared by the health + * composite and the standalone `optimize.latency` term so the two cannot drift apart. + * + * An unmeasured candidate scores the NEUTRAL midpoint, not 0. Punishing it into last + * place would make selection depend on which candidate happened to be exercised first, + * which is the order-dependence this scoring exists to remove. + */ +export function latencyScoreFromEvidence(evidence: RouteHealthEvidence | undefined): number { + const p50 = evidence?.recentLatencyMs; + if (p50 === undefined) return 0.5; + return Math.max(0, Math.min(1, 1 - p50 / HEALTH_SCORE_CONSTANTS.LATENCY_TARGET_MS)); +} + /** * Deterministic health score in [0,1]. Returns null when evidence is unknown * (no samples) so callers can apply the profile's unknownEvidence policy. @@ -383,15 +397,12 @@ export function healthScore(evidence: RouteHealthEvidence | undefined, now = Dat if (!evidence.sampleCount || evidence.sampleCount < 1) return null; const successRate = evidence.successRate ?? 0; const incompleteRate = evidence.incompleteStreamRate ?? 0; - const p50 = evidence.recentLatencyMs; - const latencyScore = p50 === undefined - ? 0.5 - : Math.max(0, Math.min(1, 1 - p50 / HEALTH_SCORE_CONSTANTS.LATENCY_TARGET_MS)); + const latency = latencyScoreFromEvidence(evidence); const consecutive = evidence.failures ?? 0; const recoveryScore = 1 - Math.min(1, consecutive / 5); const composite = HEALTH_SCORE_CONSTANTS.SUCCESS_WEIGHT * successRate + HEALTH_SCORE_CONSTANTS.INCOMPLETE_WEIGHT * (1 - incompleteRate) - + HEALTH_SCORE_CONSTANTS.LATENCY_WEIGHT * latencyScore + + HEALTH_SCORE_CONSTANTS.LATENCY_WEIGHT * latency + HEALTH_SCORE_CONSTANTS.RECOVERY_WEIGHT * recoveryScore; const confidence = Math.min(1, evidence.sampleCount / HEALTH_SCORE_CONSTANTS.MIN_CONFIDENCE_SAMPLES); const softAvoid = evidence.softAvoidUntilMs !== undefined && evidence.softAvoidUntilMs > now diff --git a/src/server/management-api.ts b/src/server/management-api.ts index 221620b367..bbf6204cbe 100644 --- a/src/server/management-api.ts +++ b/src/server/management-api.ts @@ -174,13 +174,20 @@ export async function handleManagementAPI( throw new TypeError("Catalog convergence returned an invalid outcome."); } return catalogRefresh; - } catch { + } catch (error) { + // #1784: this used to manufacture `reason: "disk"` for every escaping error, so a + // programming fault and a full filesystem were indistinguishable and both reported + // non-retryable. Classify honestly and keep the cause allowlisted. + const invalidRequest = error instanceof TypeError + || error instanceof RangeError + || error instanceof SyntaxError; return { status: "failed", - reason: "disk", + reason: invalidRequest ? "request-invalid" : "internal", phase: convergenceInvoked ? "commit" : "gather", retryable: false, partialWrite: convergenceInvoked, + cause: { kind: invalidRequest ? "invalid-request" : "unknown" }, }; } } diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 5668ed8263..ca6a0e29da 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -464,6 +464,20 @@ type CodexPoolAccountRetryResult = authCtx: Extract; }; +/** + * Workspace-denial evidence for a 403, read from the upstream body. + * + * #1789: a valid K12 credential gets 403 `codex_workspace_access_denied` on a routed prompt. + * Without this the account is quarantined for reauthentication, which cannot fix a workspace + * grant and loops forever. Fails closed: an unreadable body keeps the historical handling. + */ +async function codexDenialOutcomeMeta(response: Response): Promise<{ denial?: "workspace" | "entitlement" }> { + if (response.status !== 403) return {}; + const { classifyCodexPreStreamRejection } = await import("../../codex/quota-rejection"); + const rejection = await classifyCodexPreStreamRejection(response); + return rejection.denial ? { denial: rejection.denial } : {}; +} + function codexQuotaOutcomeMeta(response: Response): { retryAfter: string | null; resetAt: string[]; @@ -528,7 +542,7 @@ async function retryCodexPoolOnAlternateAccount( return { kind: "no-alternate" }; } - const quotaMeta = codexQuotaOutcomeMeta(firstResponse); + const quotaMeta = { ...codexQuotaOutcomeMeta(firstResponse), ...(await codexDenialOutcomeMeta(firstResponse)) }; if (outcomeStatus === 429 || outcomeStatus === 402) { const { applyAccountQuotaFromUpstreamHeaders } = await import("../../codex/auth-api"); applyAccountQuotaFromUpstreamHeaders( @@ -2606,7 +2620,7 @@ async function handleResponsesInner( if (usesCodexForwardPoolAuth(authCtx, route.provider)) { // primary was the 5h window; it now carries weekly data for GPT plans. // Prefer primary when present, fall back to secondary for compatibility. - const quotaMeta = codexQuotaOutcomeMeta(upstreamResponse); + const quotaMeta = { ...codexQuotaOutcomeMeta(upstreamResponse), ...(await codexDenialOutcomeMeta(upstreamResponse)) }; const { applyAccountQuotaFromUpstreamHeaders } = await import("../../codex/auth-api"); applyAccountQuotaFromUpstreamHeaders( authCtx.accountId, diff --git a/tests/cli-headless-parity.test.ts b/tests/cli-headless-parity.test.ts index e051ee9763..c618c19b08 100644 --- a/tests/cli-headless-parity.test.ts +++ b/tests/cli-headless-parity.test.ts @@ -426,6 +426,70 @@ describe("headless GUI parity CLI", () => { } }); + + test("config set applies onto the disk state, not a snapshot read before the lock (#1835)", async () => { + // The read used to happen outside the mutation lock, so a concurrent edit landing + // between it and the whole-snapshot save was silently reverted. + const home = mkdtempSync(join(tmpdir(), "ocx-cli-set-race-")); + const previous = process.env.OPENCODEX_HOME; + process.env.OPENCODEX_HOME = home; + const configPath = join(home, "config.json"); + const base = { + port: 10100, + providers: { openai: { adapter: "openai-responses", baseUrl: "https://chatgpt.com/backend-api/codex", authMode: "forward" } }, + defaultProvider: "openai", + }; + try { + writeFileSync(configPath, JSON.stringify(base)); + expect(await handleConfigCommand(["set", "autoSwitchThreshold", "50", "--json"])).toBe(0); + + // A competing writer adds a provider the CLI never saw. + const onDisk = JSON.parse(readFileSync(configPath, "utf8")) as Record; + onDisk.providers.competitor = { + adapter: "openai-chat", + baseUrl: "https://competitor.example/v1", + apiKey: "competitor-key", + }; + writeFileSync(configPath, JSON.stringify(onDisk)); + + expect(await handleConfigCommand(["set", "autoSwitchThreshold", "70", "--json"])).toBe(0); + + const after = JSON.parse(readFileSync(configPath, "utf8")) as Record; + expect(after.autoSwitchThreshold).toBe(70); + // The competing edit survives: the mutation was applied to the fresh disk state. + expect(Object.keys(after.providers)).toEqual(expect.arrayContaining(["openai", "competitor"])); + expect(after.providers.competitor).toMatchObject({ apiKey: "competitor-key" }); + } finally { + if (previous === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previous; + rmSync(home, { recursive: true, force: true }); + } + }); + + test("config unset actually removes the key through the mutation primitive (#1835)", async () => { + // A merge-only callback cannot delete, so unset would report success and change nothing. + const home = mkdtempSync(join(tmpdir(), "ocx-cli-unset-")); + const previous = process.env.OPENCODEX_HOME; + process.env.OPENCODEX_HOME = home; + const configPath = join(home, "config.json"); + try { + writeFileSync(configPath, JSON.stringify({ + port: 10100, + providers: { openai: { adapter: "openai-responses", baseUrl: "https://chatgpt.com/backend-api/codex", authMode: "forward" } }, + defaultProvider: "openai", + autoSwitchThreshold: 50, + })); + expect(await handleConfigCommand(["unset", "autoSwitchThreshold", "--json"])).toBe(0); + + const after = JSON.parse(readFileSync(configPath, "utf8")) as Record; + expect(Object.hasOwn(after, "autoSwitchThreshold")).toBe(false); + expect(after.providers.openai).toBeDefined(); + } finally { + if (previous === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previous; + rmSync(home, { recursive: true, force: true }); + } + }); test("config set releases the manual pin when it writes the selection order", async () => { const home = mkdtempSync(join(tmpdir(), "ocx-cli-priority-pin-")); const previous = process.env.OPENCODEX_HOME; diff --git a/tests/codex-composed-acceptance.test.ts b/tests/codex-composed-acceptance.test.ts index 7e6c396699..14b2c5e708 100644 --- a/tests/codex-composed-acceptance.test.ts +++ b/tests/codex-composed-acceptance.test.ts @@ -244,6 +244,41 @@ afterEach(async () => { }); describe("WP13 composed toggle acceptance", () => { + /** RED: read the server's startup config snapshot in the /api/sync route; a hand edit made after start is lost. */ + test("#1802: /api/sync applies the on-disk config, not the server's startup snapshot", async () => { + const fx = fixture(); + fx.writeConfig({ clientIntegrations: { codex: false } }); + const server = await fx.start(); + try { + // The server is now holding a config object from startup. Edit the file out of band, + // exactly as a user editing config.json by hand would, so disk is strictly newer. + const configPath = join(fx.ocx, "config.json"); + const onDisk = JSON.parse(readFileSync(configPath, "utf-8")) as Record; + onDisk.providers["hand-edited"] = { + adapter: "openai-chat", + baseUrl: "http://127.0.0.1:2/v1", + apiKey: "hand-edited-key", + allowPrivateNetwork: true, + liveModels: false, + models: ["hand-edited-model"], + }; + onDisk.modelCosts = { "fixture/fixture-model": { input: 7, output: 11 } }; + writeFileSync(configPath, JSON.stringify(onDisk, null, 2)); + + const sync = await fx.request(server.runtime, "/api/sync", { method: "POST" }); + expect(sync.status).toBe(200); + + // Assert against DISK, not the response body: the failure this pins is the route + // persisting a stale snapshot back over the file. + const after = JSON.parse(readFileSync(configPath, "utf-8")) as Record; + expect(after.providers["hand-edited"]).toMatchObject({ apiKey: "hand-edited-key" }); + expect(after.modelCosts).toEqual({ "fixture/fixture-model": { input: 7, output: 11 } }); + expect(Object.keys(after.providers)).toEqual(expect.arrayContaining(["fixture", "hand-edited"])); + } finally { + await fx.stop(server); + } + }, 45_000); + /** RED: remove `shouldSyncCodexOnStart` or the under-lock desired-state read; an OFF row writes native bytes. */ test("A-reduced: real CLI and HTTP entry points preserve an OFF Codex home", async () => { const fx = fixture(); diff --git a/tests/codex-convergence-contract.test.ts b/tests/codex-convergence-contract.test.ts index c705c146fb..b08be70bd4 100644 --- a/tests/codex-convergence-contract.test.ts +++ b/tests/codex-convergence-contract.test.ts @@ -317,13 +317,61 @@ test("the total lazy adapter preserves a persisted-success route when factory co disabled: ["gpt-5.6-sol"], catalogRefresh: { status: "failed", - reason: "disk", + // #1784: an escaping factory error is an internal fault, not a disk failure. + // Reporting "disk" told the operator to check storage for a programming bug. + reason: "internal", phase: "gather", partialWrite: false, + cause: { kind: "unknown" }, }, }); }); +test("a malformed convergence request is reported as request-invalid, not disk (#1784)", async () => { + const live = config(); + const request = new ManagementRequest("http://localhost/api/disabled-models", { + method: "PUT", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ models: ["gpt-5.6-sol"] }), + }); + const response = await handleManagementAPI(request, new URL(request.url), live, { + saveConfigPreservingClaudeCode: () => {}, + createManagementConvergeCodex: () => { throw new TypeError("scope must be an object"); }, + }); + + expect(response?.status).toBe(200); + expect(await response?.json()).toMatchObject({ + catalogRefresh: { + status: "failed", + reason: "request-invalid", + cause: { kind: "invalid-request" }, + }, + }); +}); + +test("a failure cause never carries message text, paths or identifiers (#1784)", async () => { + const live = config(); + const request = new ManagementRequest("http://localhost/api/disabled-models", { + method: "PUT", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ models: ["gpt-5.6-sol"] }), + }); + const secret = "sk-ant-api03-" + "A".repeat(40); + const response = await handleManagementAPI(request, new URL(request.url), live, { + saveConfigPreservingClaudeCode: () => {}, + createManagementConvergeCodex: () => { + const homePath = ["", "Users", "someone", ".codex", "config.toml"].join("/"); + throw new Error(`failed writing ${homePath} for ${secret}`); + }, + }); + + const body = JSON.stringify(await response?.json()); + // The cause is rebuilt from closed vocabularies, so none of this can ride out. + expect(body).not.toContain(secret); + expect(body).not.toContain(["", "Users", "someone"].join("/")); + expect(body).not.toContain("failed writing"); +}); + test("the route inventory contains exactly the specified 7 + 6 + 2 + 2 convergence calls", () => { const counts = Object.fromEntries([ ["provider-routes.ts", 7], diff --git a/tests/codex-quota-rejection.test.ts b/tests/codex-quota-rejection.test.ts index 05dc25946e..7f771fce37 100644 --- a/tests/codex-quota-rejection.test.ts +++ b/tests/codex-quota-rejection.test.ts @@ -12,6 +12,42 @@ function jsonPayload(status: number, payload: Record): Response } describe("Codex pre-stream quota rejection classification", () => { + test("a 403 naming a workspace denial carries structured denial evidence (#1789)", async () => { + // The credential is valid; the account simply cannot reach this workspace. Without this + // evidence routing quarantines the account for reauth, which cannot fix a workspace grant. + const nested = await classifyCodexPreStreamRejection(jsonRejection(403, { + code: "codex_workspace_access_denied", + message: "workspace access denied", + })); + expect(nested).toMatchObject({ kind: "permission-error", denial: "workspace" }); + + const topLevel = await classifyCodexPreStreamRejection(jsonPayload(403, { + code: "workspace_access_denied", + })); + expect(topLevel).toMatchObject({ kind: "permission-error", denial: "workspace" }); + + const entitlement = await classifyCodexPreStreamRejection(jsonRejection(403, { + code: "codex_entitlement_missing", + })); + expect(entitlement).toMatchObject({ kind: "permission-error", denial: "entitlement" }); + }); + + test("a 403 without denial evidence stays an ordinary permission error (#1789)", async () => { + // Fail safe: status alone must never downgrade a credential failure, or a genuinely + // revoked credential would stop prompting for reauthentication. + const unknownCode = await classifyCodexPreStreamRejection(jsonRejection(403, { + code: "something_else", + })); + expect(unknownCode.denial).toBeUndefined(); + + const noBody = await classifyCodexPreStreamRejection(new Response(null, { status: 403 })); + expect(noBody).toMatchObject({ kind: "permission-error" }); + expect(noBody.denial).toBeUndefined(); + + const malformed = await classifyCodexPreStreamRejection(new Response("{not json", { status: 403 })); + expect(malformed.denial).toBeUndefined(); + }); + test.each([ [402, true], [429, true], diff --git a/tests/codex-routing.test.ts b/tests/codex-routing.test.ts index 1c9a65360f..4d32fe69ad 100644 --- a/tests/codex-routing.test.ts +++ b/tests/codex-routing.test.ts @@ -426,6 +426,35 @@ describe("codex routing", () => { expect(resolveCodexAccountForThread("credential-next", config)).toBe("b"); }); + + test("a workspace-denied 403 is not a credential failure (#1789)", () => { + // A K12 account whose credential validates and whose WHAM usage returns 200 still gets + // 403 codex_workspace_access_denied on a routed prompt. Quarantining it for reauth tells + // the user to re-login a credential that is already valid, and the loop repeats forever. + expect(classifyCodexUpstreamOutcome(403, "workspace")).toBe("workspace"); + expect(classifyCodexUpstreamOutcome(403, "entitlement")).toBe("workspace"); + // Without denial evidence the historical mapping stands, so the change fails safe. + expect(classifyCodexUpstreamOutcome(403)).toBe("credential"); + expect(classifyCodexUpstreamOutcome(401, "workspace")).toBe("credential"); + }); + + test("a workspace denial keeps the credential and does not sweep affinity (#1789)", () => { + const config = makeConfig(); + updateAccountQuota("a", 10); + updateAccountQuota("b", 20); + // Bind a thread to the account so we can prove its affinity is NOT swept. + expect(resolveCodexAccountForThread("workspace-affinity", config)).toBe("a"); + + recordCodexUpstreamOutcome(config, "a", 403, { denial: "workspace" }); + + // The credential is valid: no reauth prompt. + expect(isAccountNeedsReauth("a")).toBe(false); + // The failure is still recorded so routing can prefer a healthier account. + expect(getCodexUpstreamHealth("a")).toMatchObject({ consecutiveFailures: 1, lastFailureStatus: 403 }); + // Credential quarantine sweeps thread affinity because reauth is account-wide; + // a workspace denial is not account-wide, so the existing binding survives. + expect(resolveCodexAccountForThread("workspace-affinity", config)).toBe("a"); + }); test("403 credential outcome quarantines the account under the conservative policy", () => { const config = makeConfig(); updateAccountQuota("a", 10); @@ -1208,6 +1237,39 @@ describe("codex routing", () => { }); }); + + test("a sub-day primary window does not masquerade as the weekly quota (#1791)", () => { + // K12 and similar plans send a 5-hour primary plus a 7-day secondary. Folding the primary + // into weeklyPercent reported the 5-hour bar as weekly and discarded the real weekly + // reading, so the dashboard showed a window resetting every few hours and routing never + // saw the limit that actually gates the account. + expect(parseUsageQuota({ + rate_limit: { + primary_window: { used_percent: 90, reset_at: 1, limit_window_seconds: 5 * 60 * 60 }, + secondary_window: { used_percent: 20, reset_at: 2, limit_window_seconds: 7 * 24 * 60 * 60 }, + }, + })).toMatchObject({ weeklyPercent: 20, weeklyResetAt: 2 }); + }); + + test("a primary window with no declared duration is still treated as weekly (#1791)", () => { + // Older payloads omit limit_window_seconds entirely. Guessing there would reclassify + // every legacy account, so an undeclared duration keeps the historical behavior. + expect(parseUsageQuota({ + rate_limit: { + primary_window: { used_percent: 40, reset_at: 1 }, + secondary_window: { used_percent: 20, reset_at: 2 }, + }, + })).toMatchObject({ weeklyPercent: 40, weeklyResetAt: 1 }); + }); + + test("a declared 7-day primary window remains the weekly quota (#1791)", () => { + expect(parseUsageQuota({ + rate_limit: { + primary_window: { used_percent: 40, reset_at: 1, limit_window_seconds: 7 * 24 * 60 * 60 }, + secondary_window: { used_percent: 20, reset_at: 2 }, + }, + })).toMatchObject({ weeklyPercent: 40, weeklyResetAt: 1 }); + }); test("WHAM primary window uses its explicit duration to distinguish weekly and monthly quotas", () => { expect(parseUsageQuota({ plan_type: "team", diff --git a/tests/policy-execution.test.ts b/tests/policy-execution.test.ts index b416b02858..bad99b1b68 100644 --- a/tests/policy-execution.test.ts +++ b/tests/policy-execution.test.ts @@ -100,10 +100,11 @@ describe("policy execution (RI-05)", () => { expect(trace.selected.provider).toBe("a"); expect(trace.selected.model).toBe("m1"); // RI-06/07/08: unknown health/quota/cost under the default "penalize" - // policy folds penalized floors into the score. + // policy folds penalized floors into the score. #1837: optimize.latency is now + // actually spent, and an unmeasured candidate takes the neutral 0.5 rather than + // having its weight fall through into configuredPriority. expect(trace.candidates[0]!.score).toMatchObject({ - total: 0.685, - components: { configuredPriority: 1, health: 0.3, quota: 0.3, cost: 0.3 }, + components: { configuredPriority: 1, health: 0.3, quota: 0.3, cost: 0.3, latency: 0.5 }, }); }); diff --git a/tests/routing-profile.test.ts b/tests/routing-profile.test.ts index 99dc892ddf..4af0dc25f8 100644 --- a/tests/routing-profile.test.ts +++ b/tests/routing-profile.test.ts @@ -327,13 +327,69 @@ describe("routing profiles (RI-04)", () => { ]); expect(result.selectedIndex).toBe(0); // RI-06/07/08: unknown health/quota/cost under the default "penalize" - // policy folds penalized floors into the score. + // policy folds penalized floors into the score. #1837: an unmeasured + // candidate takes the NEUTRAL latency score, so both tie and declaration + // order still decides -- which is correct when nothing distinguishes them. expect(result.trace.candidates[0]!.score).toMatchObject({ - total: 0.685, - components: { configuredPriority: 1, health: 0.3, quota: 0.3, cost: 0.3 }, + components: { configuredPriority: 1, health: 0.3, quota: 0.3, cost: 0.3, latency: 0.5 }, }); }); + test("dry-run evaluator: optimize.latency actually prefers the faster candidate (#1837)", () => { + // The knob was normalized into the weight sum but never spent, so whatever was + // allocated to latency silently became configuredPriority -- i.e. declaration order. + // A latency-weighted profile must be able to pick a LATER-declared faster candidate. + const config = baseConfig({ + routingProfiles: { fastest: { + candidates: [{ provider: "a", model: "m1" }, { provider: "b", model: "m2" }], + optimize: { latency: 1, health: 0, cost: 0, quota: 0 }, + } }, + }); + const result = evaluatePolicyProfile(config, "fastest", {}, [ + { provider: "a", model: "m1", health: { sampleCount: 10, successRate: 1, recentLatencyMs: 50_000 } }, + { provider: "b", model: "m2", health: { sampleCount: 10, successRate: 1, recentLatencyMs: 1_000 } }, + ]); + + expect(result.selectedIndex).toBe(1); + expect(result.trace.candidates[1]!.score!.components.latency) + .toBeGreaterThan(result.trace.candidates[0]!.score!.components.latency!); + }); + + test("dry-run evaluator: an unmeasured candidate is not punished below a slow one (#1837)", () => { + // Scoring an unknown p50 as 0 would make selection depend on which candidate happened + // to be exercised first, reintroducing the order-dependence by another name. + const config = baseConfig({ + routingProfiles: { fastest: { + candidates: [{ provider: "a", model: "m1" }, { provider: "b", model: "m2" }], + optimize: { latency: 1, health: 0, cost: 0, quota: 0 }, + } }, + }); + const result = evaluatePolicyProfile(config, "fastest", {}, [ + { provider: "a", model: "m1", health: { sampleCount: 10, successRate: 1, recentLatencyMs: 55_000 } }, + { provider: "b", model: "m2" }, + ]); + + expect(result.trace.candidates[1]!.score!.components.latency) + .toBeGreaterThan(result.trace.candidates[0]!.score!.components.latency!); + }); + + test("dry-run evaluator: latency:0 leaves declaration-order behavior unchanged (#1837)", () => { + // Existing profiles that never set the knob must not change behavior. + const config = baseConfig({ + routingProfiles: { ordered: { + candidates: [{ provider: "a", model: "m1" }, { provider: "b", model: "m2" }], + optimize: { latency: 0, health: 0, cost: 0, quota: 1 }, + } }, + }); + const result = evaluatePolicyProfile(config, "ordered", {}, [ + { provider: "a", model: "m1", health: { sampleCount: 10, successRate: 1, recentLatencyMs: 50_000 } }, + { provider: "b", model: "m2", health: { sampleCount: 10, successRate: 1, recentLatencyMs: 1_000 } }, + ]); + + expect(result.selectedIndex).toBe(0); + expect(result.trace.candidates[0]!.score!.components.latency).toBeUndefined(); + }); + test("API lists profiles and dry-runs deterministically", async () => { const config = baseConfig(); const listReq = new ManagementRequest("http://localhost/api/routing-profiles", { method: "GET" });