From e3629f12fa19a740601f0ed372fe0266ef0ae392 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 00:03:40 +0900 Subject: [PATCH 01/11] docs(devlog): plan Wave 3/4 closeout against verified code state --- .../260816_wave34_closeout/000_research.md | 82 +++++++++++++++++++ .../010_1802_sync_evidence.md | 24 ++++++ .../020_1837_latency.md | 48 +++++++++++ .../030_1789_workspace_outcome.md | 31 +++++++ .../040_1784_typed_cause.md | 39 +++++++++ .../050_1791_quota_windows.md | 49 +++++++++++ .../060_1835_cli_mutation.md | 37 +++++++++ .../070_1823_signature_scope.md | 46 +++++++++++ .../080_1830_cursor_evidence.md | 32 ++++++++ .../090_1524_capability_preflight.md | 33 ++++++++ .../260816_wave34_closeout/100_large_units.md | 35 ++++++++ .../260816_wave34_closeout/110_closeout.md | 37 +++++++++ 12 files changed, 493 insertions(+) create mode 100644 devlog/_plan/260816_wave34_closeout/000_research.md create mode 100644 devlog/_plan/260816_wave34_closeout/010_1802_sync_evidence.md create mode 100644 devlog/_plan/260816_wave34_closeout/020_1837_latency.md create mode 100644 devlog/_plan/260816_wave34_closeout/030_1789_workspace_outcome.md create mode 100644 devlog/_plan/260816_wave34_closeout/040_1784_typed_cause.md create mode 100644 devlog/_plan/260816_wave34_closeout/050_1791_quota_windows.md create mode 100644 devlog/_plan/260816_wave34_closeout/060_1835_cli_mutation.md create mode 100644 devlog/_plan/260816_wave34_closeout/070_1823_signature_scope.md create mode 100644 devlog/_plan/260816_wave34_closeout/080_1830_cursor_evidence.md create mode 100644 devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md create mode 100644 devlog/_plan/260816_wave34_closeout/100_large_units.md create mode 100644 devlog/_plan/260816_wave34_closeout/110_closeout.md 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..011d2f9918 --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/000_research.md @@ -0,0 +1,82 @@ +# 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:3132`, 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:323`, 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`, `src/server/auth-cors.ts:392`). +- 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:93` and `src/server/management-api.ts:150` 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..443abf54fa --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/020_1837_latency.md @@ -0,0 +1,48 @@ +# 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. + +## Verified defect + +- `optimize.latency` exists and defaults to `0.55` — the LARGEST default weight (`src/routing/profile.ts:26`). +- Normalization includes it in the four-way sum and stores it (`src/routing/profile.ts:502`). +- The evaluator reads only `optimize.health`, `.quota`, `.cost`. Whatever weight remains becomes `priorityWeight = 1 - spentHealth - spentQuota - spentCost`, multiplied by `configuredPriorityScore(...)` (`src/routing/evaluator.ts:397`). +- `components.latency` is never populated (`src/routing/trace.ts:126`). + +So a profile weighted entirely toward latency silently becomes a profile weighted entirely toward **declaration order**, because `configuredPriorityScore(index, total)` decreases with index and selection is a strict-greater argmax (`src/routing/evaluator.ts:183`, `:444`). + +Real latency IS measured, but only as a subcomponent of `healthScore()` (`src/routing/health.ts:323`), scaled by `optimize.health`. + +## Chosen fix: implement the term + +Removing the knob is the honest alternative, but it is a breaking config change for anyone who set it. Implementing it matches the documented meaning. + +```diff + const spentHealth = ...; + const spentQuota = ...; + const spentCost = ...; ++ const spentLatency = optimize.latency ?? 0; +- const priorityWeight = 1 - spentHealth - spentQuota - spentCost; ++ const priorityWeight = Math.max(0, 1 - spentHealth - spentQuota - spentCost - spentLatency); ++ const latencyComponent = spentLatency > 0 ++ ? spentLatency * candidateLatencyScore(candidate) ++ : 0; + const total = priorityWeight * configuredPriorityScore(...) ++ + latencyComponent + + ...; +``` + +`candidateLatencyScore` reuses the existing p50-derived scoring already living in `src/routing/health.ts:323`. Extract that computation into a named export so health and latency share one definition instead of drifting; health keeps consuming it under `HEALTH_SCORE_CONSTANTS.LATENCY_WEIGHT`, unchanged. + +When a candidate has no latency history, return the neutral mid score rather than 0 — an unmeasured candidate must not be punished into last place, which would reintroduce order-dependence by another name. + +Populate `components.latency` in the trace so the decision is auditable (`src/routing/trace.ts:126`). + +## Tests + +`tests/routing-profile.test.ts` currently PINS the old behavior at `:317` (earlier candidate wins, no latency component). Update it, and add: + +- two candidates where the LATER-declared one has materially lower p50: with `latency: 1` it must win, which is the exact inversion the issue reports; +- with `latency: 0` the declaration-order result is unchanged (no silent behavior change for existing profiles); +- a candidate with no history is not ranked below a measured-but-slow one; +- `components.latency` appears in the trace when the weight is non-zero and is absent/zero when it is not. 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..d4d603079f --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/030_1789_workspace_outcome.md @@ -0,0 +1,31 @@ +# 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 classifier already exists but on a different path: `src/codex/quota-rejection.ts:11` distinguishes `authentication-error` from `permission-error`. It does not feed account health. + +## Fix + +1. Add `workspace` to `CodexUpstreamOutcomeClass`. +2. Split the 403 branch. 401 stays `credential`. 403 becomes `workspace` **when** the upstream body/error code indicates workspace or permission denial (reuse the discrimination already implemented in `quota-rejection.ts` rather than writing a second one); an unclassifiable 403 stays `credential` so the change fails safe toward the existing behavior. +3. In the outcome handler, a `workspace` result must NOT call `markAccountNeedsReauth`. It should mark the account unusable for this workspace-scoped route and surface a workspace-denial diagnostic, leaving the credential intact. + +The signature already carries what is needed: + +```ts +classifyCodexUpstreamOutcome( + outcome: number | "connect_error" | "timeout" | "connect_neutral", +) +``` + +It must gain the upstream error detail to tell the two 403s apart. Thread the already-parsed rejection reason through rather than re-reading the body. + +## Tests + +Update `tests/codex-routing.test.ts:377` — it asserts the behavior being fixed — and add: a workspace-shaped 403 does not set reauth and leaves the credential usable elsewhere; a bare/unclassifiable 403 still behaves as today; 401 is unchanged. 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..055c3c353c --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/040_1784_typed_cause.md @@ -0,0 +1,39 @@ +# 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`, `:93`). +2. `src/server/management-api.ts:150` uses a bare `catch {}` 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"; ++ /** Non-sensitive cause summary: error name + redacted message. Never the raw body. */ ++ 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. + +Populate `cause` from the caught error at both sites, passing it through `redactSecretString` so nothing operator-controlled leaks into a management response. + +## Tests + +Update the two pinning tests to assert the NEW classification rather than `disk`, and add: a malformed scope yields `request-invalid` with a cause; a simulated lock-busy yields `admission` with `retryable: true`; a simulated write failure still yields `disk`; and a token-shaped string inside an error message does not survive into the response. 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..09f6c8633c --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/050_1791_quota_windows.md @@ -0,0 +1,49 @@ +# 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 keyed by duration, and derive the display names instead of baking them in: + +```ts +type StoredQuotaWindow = { + limitWindowSeconds: number; // the discriminator upstream already gives us + usedPercent: number; + resetAt?: number; +}; +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. + +Consumers that ask for `weeklyPercent`/`monthlyPercent` (dashboard, routing) get a small accessor that selects by duration band rather than by stored name, so the display keeps working while the storage stops lying. + +## 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 and re-persists as v2; a v2 document round-trips unchanged. +- Either window at 100% marks the account exhausted. +- Dashboard/routing accessors return the same values they used to for an ordinary two-window account. 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..beb2a0a970 --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/060_1835_cli_mutation.md @@ -0,0 +1,37 @@ +# 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.) + +So `set`/`unset` become: + +```diff +- const config = loadConfig(); +- applyPath(config, path, value); +- saveConfig(config); ++ const outcome = mutatePersistedConfig(config => { ++ applyPath(config, path, value); ++ return { config, result: undefined }; ++ }); +``` + +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. 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..8b8bccaccc --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/070_1823_signature_scope.md @@ -0,0 +1,46 @@ +# 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. + +## 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..a52cf3e65e --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/080_1830_cursor_evidence.md @@ -0,0 +1,32 @@ +# 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 + +Build the advertised catalog the way a real Cursor-routed subagent turn does, run it through `applyCursorToolBudget`, 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. the search flag this PR sets is on the entry that actually reaches the wire. + +Assert against the SERIALIZED form, not the pre-budget array, or the test proves nothing about what the child receives. + +## 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..88de5faa37 --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md @@ -0,0 +1,33 @@ +# 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. + +## Fix + +1. Populate request context size in `PolicyRequestEvidence`. The serialized request is already available at the fallback boundary; use a token estimate consistent with what the request log already computes rather than inventing a second estimator. +2. In `policy-fallback.ts`, re-evaluate each candidate against the CURRENT request evidence instead of reusing the frozen verdict. Reuse `src/routing/evaluator.ts`'s existing context/modality rejection rather than writing a parallel check. +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 + +- A fallback candidate with a smaller context window than the request is skipped, and the next compatible one is used. +- An image-bearing request skips a text-only candidate. +- 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_large_units.md b/devlog/_plan/260816_wave34_closeout/100_large_units.md new file mode 100644 index 0000000000..50aabf40bb --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/100_large_units.md @@ -0,0 +1,35 @@ +# 100 — The three large units: #1686, #1049, #1798 + +These are genuinely big and each deserves its own cycle. Documented here so the scope is explicit rather than implied. + +## #1686 — bearer admission + guaranteed main substitution + +The roadmap's framing (an accidental upstream leak) is **wrong** — see `000_research.md`. The real defect is that the proxy REFUSES the intended flow: `/v1/responses` admission reads only `x-opencodex-api-key` (`src/server/auth-cors.ts:441`) and Direct rejects a recognized proxy bearer before upstream resolution (`src/server/responses/core.ts:970`, `src/server/auth-cors.ts:392`). + +Required shape: + +1. Make `DataPlaneAdmission` presentation-source-aware (`loopback | dedicated | bearer | x-api-key`) while keeping credential identity. It exists today as `configured | environment | loopback` (`src/server/auth-cors.ts:314`). +2. Accept a bearer fallback for Responses admission, dedicated header keeping precedence, `x-api-key` still rejected there. Update `AUTH_MATRIX` (`:379`). +3. Thread the admission into HTTP handling — it is currently dropped before `handleResponses` (`src/server/index.ts:1214`), while WebSocket already retains it (`src/server/ws-bridge.ts:63`). +4. Add the shared `materializeCodexUpstreamAuth` the roadmap names, built on today's `headersForCodexAuthContext` (`src/codex/auth-context.ts:454`): pool/main-pool always overwrite; `main` + admission bearer requires a live stored main token and overwrites both Authorization and account id, throwing before I/O if unavailable; `main` + dedicated admission + a distinct real ChatGPT bearer keeps today's intentional passthrough. +5. Emit modern `env_key` in injection (`src/codex/inject.ts:210` currently emits only `env_http_headers`). + +**Do not relax `validateForwardAdmissionCredential` on its own.** Without guaranteed overwrite that creates precisely the leak the guard prevents today. This is a security-boundary change and needs explicit security review. + +## #1049 — legacy Codex home adoption + +Legacy homes bypass the write lock entirely: injection calls `applyNativeArtifacts()` directly for `legacy-uncoordinated` (`src/codex/inject.ts:901`) and restore calls `restoreCodexConfigInline()` (`:1504`). `tests/codex-inject-write-lock.test.ts:144` currently PINS that bypass. + +The `adoption-pending` design exists only in the archived contract (`devlog/_fin/260804_codex_write_substrate/005_contract.md:709`, crash boundary at `:735`); runtime `transition-state.ts` does not accept that status. Already satisfied: residue detection, invalid-record refusal, unversioned/rowless refusal (`src/codex/transition-state.ts:269`, `:288`). + +Missing: adoption itself plus crash-recoverable publication. This is a migration touching every existing install and must not be bundled with ordinary writer migration. + +## #1798 — restore must survive an app rewrite + +Injection stores original bytes plus an injected hash; when the Codex App rewrites `config.toml`, the hash mismatch makes journal restore refuse (`src/codex/journal.ts:123`). Fallback removal then only recognizes an `openai_base_url` immediately preceded by the OpenCodex marker (`src/codex/injected-marker.ts:53`, `src/codex/inject.ts:1193`), so an app-rewritten unmarked line survives. + +Current behavior is exact-byte restore-or-strip. What is needed is a baseline/injected/current three-way semantic merge, so a user edit is preserved while every OpenCodex-owned routing line is removed. + +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`). + +**Do not ship a marker-only deletion patch.** An unmarked root URL may be genuinely user-owned; deleting it because it looks like ours is data loss. 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..1cf809e96c --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/110_closeout.md @@ -0,0 +1,37 @@ +# 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 / #1049 / #1798 — one cycle each, security review for #1686. + +One decade doc per PABCD work-phase. Units touching the same file are sequenced, not parallel: `040` and `050` both sit in the Codex account/catalog area, and `030`/`050` both touch `src/codex/` quota-adjacent paths. + +## 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. From 56deceef3f33a713631dcbb7a2938f4184906d31 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 00:05:30 +0900 Subject: [PATCH 02/11] test(sync): pin that /api/sync reads disk, not the startup snapshot #1802 reported `ocx sync` overwriting a hand-edited config.json from stale server memory. The save path was fixed in 2.21.0 and the route now calls loadConfig() at its own boundary, so the reported clobber is no longer reachable -- but nothing pinned it. This asserts against the FILE rather than the response body, because the failure being prevented is the route persisting a caller-held snapshot back over the file. Driven red first by making the route save its caller-held config object: the hand-edited provider disappears and the test fails. Closes #1802. --- tests/codex-composed-acceptance.test.ts | 35 +++++++++++++++++++++++++ 1 file changed, 35 insertions(+) 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(); From ad889ce51390a71b62084006f49067a1dd7ee568 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 00:12:05 +0900 Subject: [PATCH 03/11] fix(routing): spend optimize.latency instead of silently folding it into priority `optimize.latency` defaults to 0.55 -- the largest default weight -- and the normalizer includes it in the four-way sum. The evaluator then read only health, quota and cost, so whatever weight was allocated to latency fell into `priorityWeight = 1 - spentHealth - spentQuota - spentCost` and was multiplied by `configuredPriorityScore(index, total)`. That made a latency-optimized profile a declaration-order profile: the score decreases with index, selection is a strict-greater argmax, so the first declared candidate won regardless of measured latency. `components.latency` was never populated either, so the trace could not show it. Real latency was already measured, but only inside `healthScore()` scaled by `optimize.health`. That computation is now extracted as `latencyScoreFromEvidence()` and shared, so the health composite and the new standalone term cannot drift apart. An unmeasured candidate takes the neutral midpoint rather than 0. Scoring it 0 would make selection depend on which candidate happened to be exercised first, which is the same order-dependence by another name. Closes #1837. --- src/routing/evaluator.ts | 14 ++++++-- src/routing/health.ts | 21 +++++++++--- tests/policy-execution.test.ts | 7 ++-- tests/routing-profile.test.ts | 62 ++++++++++++++++++++++++++++++++-- 4 files changed, 91 insertions(+), 13 deletions(-) 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/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" }); From 084829a6341337af0453f6454bd3308cf3c0fdf8 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 00:17:58 +0900 Subject: [PATCH 04/11] docs(devlog): fix Wave 3/4 plan defects found in adversarial review --- .../260816_wave34_closeout/000_research.md | 9 ++- .../020_1837_latency.md | 66 +++++++++++-------- .../030_1789_workspace_outcome.md | 27 +++++--- .../040_1784_typed_cause.md | 23 +++++-- .../050_1791_quota_windows.md | 29 ++++++-- .../060_1835_cli_mutation.md | 25 ++++++- .../070_1823_signature_scope.md | 2 + .../080_1830_cursor_evidence.md | 6 +- .../090_1524_capability_preflight.md | 23 +++++-- .../100_1686_admission.md | 63 ++++++++++++++++++ .../260816_wave34_closeout/100_large_units.md | 35 ---------- .../101_1049_legacy_adoption.md | 26 ++++++++ .../102_1798_restore_merge.md | 33 ++++++++++ .../260816_wave34_closeout/110_closeout.md | 12 +++- 14 files changed, 281 insertions(+), 98 deletions(-) create mode 100644 devlog/_plan/260816_wave34_closeout/100_1686_admission.md delete mode 100644 devlog/_plan/260816_wave34_closeout/100_large_units.md create mode 100644 devlog/_plan/260816_wave34_closeout/101_1049_legacy_adoption.md create mode 100644 devlog/_plan/260816_wave34_closeout/102_1798_restore_merge.md diff --git a/devlog/_plan/260816_wave34_closeout/000_research.md b/devlog/_plan/260816_wave34_closeout/000_research.md index 011d2f9918..2f1f3a970a 100644 --- a/devlog/_plan/260816_wave34_closeout/000_research.md +++ b/devlog/_plan/260816_wave34_closeout/000_research.md @@ -23,15 +23,15 @@ The roadmap opens with a public-dev convergence gate because it saw Wave 2 PRs s - `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:3132`, 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:323`, scaled by `optimize.health`, never by `optimize.latency`. +- 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`, `src/server/auth-cors.ts:392`). +- 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. @@ -49,7 +49,7 @@ So #1686 is real for the OPPOSITE reason the roadmap gives: the proxy **refuses* | `#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:93` and `src/server/management-api.ts:150` both manufacture `reason: "disk"`. | +| `#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`). | @@ -79,4 +79,3 @@ Ordered by (real user impact) / (implementation risk), landing smallest-first: 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/020_1837_latency.md b/devlog/_plan/260816_wave34_closeout/020_1837_latency.md index 443abf54fa..e713c8a18d 100644 --- a/devlog/_plan/260816_wave34_closeout/020_1837_latency.md +++ b/devlog/_plan/260816_wave34_closeout/020_1837_latency.md @@ -2,47 +2,59 @@ `#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:26`). -- Normalization includes it in the four-way sum and stores it (`src/routing/profile.ts:502`). -- The evaluator reads only `optimize.health`, `.quota`, `.cost`. Whatever weight remains becomes `priorityWeight = 1 - spentHealth - spentQuota - spentCost`, multiplied by `configuredPriorityScore(...)` (`src/routing/evaluator.ts:397`). -- `components.latency` is never populated (`src/routing/trace.ts:126`). +- `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. -So a profile weighted entirely toward latency silently becomes a profile weighted entirely toward **declaration order**, because `configuredPriorityScore(index, total)` decreases with index and selection is a strict-greater argmax (`src/routing/evaluator.ts:183`, `:444`). +Real latency WAS measured, but only as a subcomponent of `healthScore()` (`src/routing/health.ts:386`), scaled by `optimize.health`. -Real latency IS measured, but only as a subcomponent of `healthScore()` (`src/routing/health.ts:323`), scaled by `optimize.health`. +## What landed -## Chosen fix: implement the term +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: -Removing the knob is the honest alternative, but it is a breaking config change for anyone who set it. Implementing it matches the documented meaning. +```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 spentHealth = ...; - const spentQuota = ...; - const spentCost = ...; -+ const spentLatency = optimize.latency ?? 0; -- const priorityWeight = 1 - spentHealth - spentQuota - spentCost; + 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); -+ const latencyComponent = spentLatency > 0 -+ ? spentLatency * candidateLatencyScore(candidate) -+ : 0; - const total = priorityWeight * configuredPriorityScore(...) -+ + latencyComponent - + ...; ``` -`candidateLatencyScore` reuses the existing p50-derived scoring already living in `src/routing/health.ts:323`. Extract that computation into a named export so health and latency share one definition instead of drifting; health keeps consuming it under `HEALTH_SCORE_CONSTANTS.LATENCY_WEIGHT`, unchanged. +```diff + if (costWeight > 0 && costValue !== null) { ... } ++ if (latencyWeight > 0 && latencyValue !== null) { ++ total += latencyWeight * latencyValue; ++ components.latency = latencyValue; ++ } +``` -When a candidate has no latency history, return the neutral mid score rather than 0 — an unmeasured candidate must not be punished into last place, which would reintroduce order-dependence by another name. +Health keeps consuming the same helper under `HEALTH_SCORE_CONSTANTS.LATENCY_WEIGHT`, unchanged. -Populate `components.latency` in the trace so the decision is auditable (`src/routing/trace.ts:126`). +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 -`tests/routing-profile.test.ts` currently PINS the old behavior at `:317` (earlier candidate wins, no latency component). Update it, and add: +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. -- two candidates where the LATER-declared one has materially lower p50: with `latency: 1` it must win, which is the exact inversion the issue reports; -- with `latency: 0` the declaration-order result is unchanged (no silent behavior change for existing profiles); -- a candidate with no history is not ranked below a measured-but-slow one; -- `components.latency` appears in the trace when the weight is non-zero and is absent/zero when it is not. +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 index d4d603079f..27fa7ab683 100644 --- a/devlog/_plan/260816_wave34_closeout/030_1789_workspace_outcome.md +++ b/devlog/_plan/260816_wave34_closeout/030_1789_workspace_outcome.md @@ -8,24 +8,31 @@ 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 classifier already exists but on a different path: `src/codex/quota-rejection.ts:11` distinguishes `authentication-error` from `permission-error`. It does not feed account health. +A richer-looking classifier exists on a different path (`src/codex/quota-rejection.ts:11`, distinguishing `authentication-error` from `permission-error`), but it splits on STATUS alone — it does not inspect the body for a workspace shape. So it cannot be reused as-is to tell the two kinds of 403 apart. ## Fix 1. Add `workspace` to `CodexUpstreamOutcomeClass`. -2. Split the 403 branch. 401 stays `credential`. 403 becomes `workspace` **when** the upstream body/error code indicates workspace or permission denial (reuse the discrimination already implemented in `quota-rejection.ts` rather than writing a second one); an unclassifiable 403 stays `credential` so the change fails safe toward the existing behavior. -3. In the outcome handler, a `workspace` result must NOT call `markAccountNeedsReauth`. It should mark the account unusable for this workspace-scoped route and surface a workspace-denial diagnostic, leaving the credential intact. - -The signature already carries what is needed: +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 -classifyCodexUpstreamOutcome( - outcome: number | "connect_error" | "timeout" | "connect_neutral", -) +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 ``` -It must gain the upstream error detail to tell the two 403s apart. Thread the already-parsed rejection reason through rather than re-reading the body. + 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. **Thread it through every call site.** `recordCodexUpstreamOutcome` is called from multiple paths; each must pass the evidence it already has instead of a bare status, or the split silently never fires. Enumerate and update them all — a partially-threaded change is worse than none, because it makes the classification depend on which path happened to report. + +4. In the outcome handler, a `workspace` result must NOT call `markAccountNeedsReauth`. Store it keyed by (account, workspace-scoped route) so the account stays usable elsewhere, and surface a workspace-denial diagnostic. Define that key explicitly — an account-wide flag would quarantine the account for every route, which is the same over-reaction in a new place. ## Tests -Update `tests/codex-routing.test.ts:377` — it asserts the behavior being fixed — and add: a workspace-shaped 403 does not set reauth and leaves the credential usable elsewhere; a bare/unclassifiable 403 still behaves as today; 401 is unchanged. +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 index 055c3c353c..b204fae52c 100644 --- a/devlog/_plan/260816_wave34_closeout/040_1784_typed_cause.md +++ b/devlog/_plan/260816_wave34_closeout/040_1784_typed_cause.md @@ -6,8 +6,8 @@ There is no exception type to propagate — failures are DATA. `CatalogDispositi 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`, `:93`). -2. `src/server/management-api.ts:150` uses a bare `catch {}` and again manufactures `reason: "disk"`, choosing gather-vs-commit purely from whether invocation had begun. +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` uses a bare `catch {}` 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`). @@ -32,8 +32,23 @@ Extend the disposition rather than inventing the roadmap's `CatalogConvergenceEr - `internal` for anything genuinely unclassified — explicitly not `disk`. - `disk` narrows to real filesystem failures. -Populate `cause` from the caught error at both sites, passing it through `redactSecretString` so nothing operator-controlled leaks into a management response. +### 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: + +- `name`: the error constructor name, which is a fixed vocabulary. +- `detail`: a short allowlisted summary — an errno/code when present, otherwise a fixed phrase per branch. Never the raw message. + +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`, and add: a malformed scope yields `request-invalid` with a cause; a simulated lock-busy yields `admission` with `retryable: true`; a simulated write failure still yields `disk`; and a token-shaped string inside an error message does not survive into the response. +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 index 09f6c8633c..f8471523b7 100644 --- a/devlog/_plan/260816_wave34_closeout/050_1791_quota_windows.md +++ b/devlog/_plan/260816_wave34_closeout/050_1791_quota_windows.md @@ -20,13 +20,18 @@ type StoredAccountQuota = { ## Fix -Store windows as an array keyed by duration, and derive the display names instead of baking them in: +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 = { - limitWindowSeconds: number; // the discriminator upstream already gives us + /** Upstream slot the window arrived in — the provenance the old flag encoded. */ + slot: "primary" | "secondary" | "tertiary"; + /** The real discriminator: 5h, 7d, 30d ... never inferred from the slot. */ + limitWindowSeconds: number; usedPercent: number; resetAt?: number; + /** True for the window that governs admission, preserving monthlyIsPrimaryWindow's meaning. */ + governing: boolean; }; type StoredAccountQuota = { windows: StoredQuotaWindow[]; @@ -39,11 +44,23 @@ Persist as `version: 2`. Hydration must accept BOTH: a `version: 1` document is Exhaustion becomes "any governing window at limit", not "the weekly one", which is the behavior `#1791` asks for. -Consumers that ask for `weeklyPercent`/`monthlyPercent` (dashboard, routing) get a small accessor that selects by duration band rather than by stored name, so the display keeps working while the storage stops lying. +### 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 a small accessor that selects by duration band and `governing`, rather than by stored name: + +```ts +function governingWindow(q: StoredAccountQuota): StoredQuotaWindow | undefined; +function windowByBand(q: StoredAccountQuota, band: "short" | "weekly" | "monthly"): StoredQuotaWindow | undefined; +``` + +Exhaustion is "the 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. ## 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 and re-persists as v2; a v2 document round-trips unchanged. -- Either window at 100% marks the account exhausted. -- Dashboard/routing accessors return the same values they used to for an ordinary two-window account. +- v1 document on disk hydrates without loss (including `monthlyIsPrimaryWindow` becoming `governing`) and re-persists as v2; a v2 document round-trips unchanged. +- The governing window at 100% marks the account exhausted; a supplementary window at 100% does NOT. +- Cooldown recovery uses the governing window's reset, not the earliest reset. +- 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 index beb2a0a970..de5258dc7e 100644 --- a/devlog/_plan/260816_wave34_closeout/060_1835_cli_mutation.md +++ b/devlog/_plan/260816_wave34_closeout/060_1835_cli_mutation.md @@ -16,18 +16,35 @@ mutatePersistedConfig(mutate: (config: OcxConfig) => PersistedConfigMutation< (`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: ```diff - const config = loadConfig(); -- applyPath(config, path, value); +- setPath(candidate, path, parsed, action === "unset"); +- // ... validate ... - saveConfig(config); + const outcome = mutatePersistedConfig(config => { -+ applyPath(config, path, value); -+ return { config, result: undefined }; ++ const candidate = structuredClone(config) as Record; ++ setPath(candidate, path, parsed, action === "unset"); ++ // Same validation the command already performs, on the FRESH candidate. ++ const validated = validateConfigCandidate(candidate); ++ if (!validated.ok) throw new Error(validated.error); ++ // The pin clear is part of the same transaction, not a follow-up write. ++ if (pathSegments(path)[0] === "codexAccountPriorities") clearCodexAccountPin(validated.config); ++ Object.assign(config, validated.config); ++ return { changed: JSON.stringify(config) !== JSON.stringify(validated.config) ? true : true, 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 be computed honestly — a byte-identical write must report `false` so the config generation is not bumped. +- 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. @@ -35,3 +52,5 @@ The read now happens inside the transaction, so the mutation is applied to whate ## 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 index 8b8bccaccc..a3af620b8f 100644 --- a/devlog/_plan/260816_wave34_closeout/070_1823_signature_scope.md +++ b/devlog/_plan/260816_wave34_closeout/070_1823_signature_scope.md @@ -2,6 +2,8 @@ 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. diff --git a/devlog/_plan/260816_wave34_closeout/080_1830_cursor_evidence.md b/devlog/_plan/260816_wave34_closeout/080_1830_cursor_evidence.md index a52cf3e65e..642c9a0ac6 100644 --- a/devlog/_plan/260816_wave34_closeout/080_1830_cursor_evidence.md +++ b/devlog/_plan/260816_wave34_closeout/080_1830_cursor_evidence.md @@ -18,7 +18,9 @@ So the PR could pass while the real catalog either exceeds the budget or drops t ## Required test -Build the advertised catalog the way a real Cursor-routed subagent turn does, run it through `applyCursorToolBudget`, and assert: +**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; @@ -27,6 +29,8 @@ Build the advertised catalog the way a real Cursor-routed subagent turn does, ru Assert against the SERIALIZED form, not the pre-budget array, or the test proves nothing about what the child receives. +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 index 88de5faa37..1c2733a674 100644 --- a/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md +++ b/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md @@ -16,18 +16,31 @@ So a 200k-token conversation or an image-bearing request can fall back onto a ca The missing pieces are (a) an actual request-size measurement and (b) re-running the existing eligibility check at fallback time. -## Fix +## Two mechanisms, two fixes -1. Populate request context size in `PolicyRequestEvidence`. The serialized request is already available at the fallback boundary; use a token estimate consistent with what the request log already computes rather than inventing a second estimator. -2. In `policy-fallback.ts`, re-evaluate each candidate against the CURRENT request evidence instead of reusing the frozen verdict. Reuse `src/routing/evaluator.ts`'s existing context/modality rejection rather than writing a parallel check. +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). Use the estimate the request log already computes rather than inventing a second estimator. +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 -- A fallback candidate with a smaller context window than the request is skipped, and the next compatible one is used. -- An image-bearing request skips a text-only candidate. +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/100_large_units.md b/devlog/_plan/260816_wave34_closeout/100_large_units.md deleted file mode 100644 index 50aabf40bb..0000000000 --- a/devlog/_plan/260816_wave34_closeout/100_large_units.md +++ /dev/null @@ -1,35 +0,0 @@ -# 100 — The three large units: #1686, #1049, #1798 - -These are genuinely big and each deserves its own cycle. Documented here so the scope is explicit rather than implied. - -## #1686 — bearer admission + guaranteed main substitution - -The roadmap's framing (an accidental upstream leak) is **wrong** — see `000_research.md`. The real defect is that the proxy REFUSES the intended flow: `/v1/responses` admission reads only `x-opencodex-api-key` (`src/server/auth-cors.ts:441`) and Direct rejects a recognized proxy bearer before upstream resolution (`src/server/responses/core.ts:970`, `src/server/auth-cors.ts:392`). - -Required shape: - -1. Make `DataPlaneAdmission` presentation-source-aware (`loopback | dedicated | bearer | x-api-key`) while keeping credential identity. It exists today as `configured | environment | loopback` (`src/server/auth-cors.ts:314`). -2. Accept a bearer fallback for Responses admission, dedicated header keeping precedence, `x-api-key` still rejected there. Update `AUTH_MATRIX` (`:379`). -3. Thread the admission into HTTP handling — it is currently dropped before `handleResponses` (`src/server/index.ts:1214`), while WebSocket already retains it (`src/server/ws-bridge.ts:63`). -4. Add the shared `materializeCodexUpstreamAuth` the roadmap names, built on today's `headersForCodexAuthContext` (`src/codex/auth-context.ts:454`): pool/main-pool always overwrite; `main` + admission bearer requires a live stored main token and overwrites both Authorization and account id, throwing before I/O if unavailable; `main` + dedicated admission + a distinct real ChatGPT bearer keeps today's intentional passthrough. -5. Emit modern `env_key` in injection (`src/codex/inject.ts:210` currently emits only `env_http_headers`). - -**Do not relax `validateForwardAdmissionCredential` on its own.** Without guaranteed overwrite that creates precisely the leak the guard prevents today. This is a security-boundary change and needs explicit security review. - -## #1049 — legacy Codex home adoption - -Legacy homes bypass the write lock entirely: injection calls `applyNativeArtifacts()` directly for `legacy-uncoordinated` (`src/codex/inject.ts:901`) and restore calls `restoreCodexConfigInline()` (`:1504`). `tests/codex-inject-write-lock.test.ts:144` currently PINS that bypass. - -The `adoption-pending` design exists only in the archived contract (`devlog/_fin/260804_codex_write_substrate/005_contract.md:709`, crash boundary at `:735`); runtime `transition-state.ts` does not accept that status. Already satisfied: residue detection, invalid-record refusal, unversioned/rowless refusal (`src/codex/transition-state.ts:269`, `:288`). - -Missing: adoption itself plus crash-recoverable publication. This is a migration touching every existing install and must not be bundled with ordinary writer migration. - -## #1798 — restore must survive an app rewrite - -Injection stores original bytes plus an injected hash; when the Codex App rewrites `config.toml`, the hash mismatch makes journal restore refuse (`src/codex/journal.ts:123`). Fallback removal then only recognizes an `openai_base_url` immediately preceded by the OpenCodex marker (`src/codex/injected-marker.ts:53`, `src/codex/inject.ts:1193`), so an app-rewritten unmarked line survives. - -Current behavior is exact-byte restore-or-strip. What is needed is a baseline/injected/current three-way semantic merge, so a user edit is preserved while every OpenCodex-owned routing line is removed. - -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`). - -**Do not ship a marker-only deletion patch.** An unmarked root URL may be genuinely user-owned; deleting it because it looks like ours is data loss. 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 index 1cf809e96c..f87670cac1 100644 --- a/devlog/_plan/260816_wave34_closeout/110_closeout.md +++ b/devlog/_plan/260816_wave34_closeout/110_closeout.md @@ -11,9 +11,17 @@ 7. `070` #1823 — scoped signature replay. 8. `080` #1830 — catalog serialization evidence (PR #1832). 9. `090` #1524 — capability preflight. -10. `100` #1686 / #1049 / #1798 — one cycle each, security review for #1686. +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. Units touching the same file are sequenced, not parallel: `040` and `050` both sit in the Codex account/catalog area, and `030`/`050` both touch `src/codex/` quota-adjacent paths. +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 From 1fa0f6aadde8869952618966605346d9f0f97c02 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 00:22:29 +0900 Subject: [PATCH 05/11] fix(codex): stop calling a workspace denial a credential failure A K12 account whose credential validates, whose warm-up succeeds and whose WHAM usage returns 200 still gets HTTP 403 `codex_workspace_access_denied` on a routed prompt. The classifier mapped every 403 to `credential`, which marked the account needs-reauth. Re-authentication succeeds, the next prompt fails the same way, and the loop repeats -- while the remedy offered cannot fix a workspace grant. The 403 is now split on structured evidence, not status. `quota-rejection.ts` reads an own-property `code` from the body (top level or under `error`) and reports `denial: "workspace" | "entitlement"`; that evidence travels on the existing `CodexUpstreamOutcomeMeta`, so the two `recordCodexUpstreamOutcome` sites in the Responses path pick it up without every caller changing shape. A `workspace` outcome records the failure so routing can prefer a healthier account, but does not call `markAccountNeedsReauth` and does not sweep thread affinity -- credential quarantine sweeps affinity because reauth is account-wide, and a workspace denial is not. Fails safe throughout: a 403 with no denial evidence, an unreadable body, a malformed body or an unknown code all keep the historical credential handling, so a genuinely revoked credential still prompts for reauthentication. Body reading reuses the existing bounded/duplicate-key-safe reader. Closes #1789. --- src/codex/quota-rejection.ts | 62 ++++++++++++++++++++++++++++- src/codex/routing.ts | 33 +++++++++++++-- src/server/responses/core.ts | 18 ++++++++- tests/codex-quota-rejection.test.ts | 36 +++++++++++++++++ tests/codex-routing.test.ts | 29 ++++++++++++++ 5 files changed, 172 insertions(+), 6 deletions(-) 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/routing.ts b/src/codex/routing.ts index 2e287adf29..7e201ea58a 100644 --- a/src/codex/routing.ts +++ b/src/codex/routing.ts @@ -127,7 +127,8 @@ let lastReconciledGeneration = 0; let liveHealthAccountIds = new Set(); 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/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/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..85b4ad4af1 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); From ad70ec2a848c4381bbcdcb3481650ab0902cf804 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 00:24:49 +0900 Subject: [PATCH 06/11] docs(devlog): fix round-2 review findings in the Wave 3/4 plan --- .../030_1789_workspace_outcome.md | 22 +++++++-- .../040_1784_typed_cause.md | 8 ++-- .../050_1791_quota_windows.md | 24 +++++++--- .../060_1835_cli_mutation.md | 45 +++++++++++-------- .../080_1830_cursor_evidence.md | 6 ++- .../090_1524_capability_preflight.md | 6 ++- 6 files changed, 77 insertions(+), 34 deletions(-) diff --git a/devlog/_plan/260816_wave34_closeout/030_1789_workspace_outcome.md b/devlog/_plan/260816_wave34_closeout/030_1789_workspace_outcome.md index 27fa7ab683..bcf90b16a4 100644 --- a/devlog/_plan/260816_wave34_closeout/030_1789_workspace_outcome.md +++ b/devlog/_plan/260816_wave34_closeout/030_1789_workspace_outcome.md @@ -8,7 +8,23 @@ 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 it splits on STATUS alone — it does not inspect the body for a workspace shape. So it cannot be reused as-is to tell the two kinds of 403 apart. +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 @@ -27,9 +43,9 @@ classifyCodexUpstreamOutcome(evidence: CodexUpstreamEvidence): CodexUpstreamOutc 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. **Thread it through every call site.** `recordCodexUpstreamOutcome` is called from multiple paths; each must pass the evidence it already has instead of a bare status, or the split silently never fires. Enumerate and update them all — a partially-threaded change is worse than none, because it makes the classification depend on which path happened to report. +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`. Store it keyed by (account, workspace-scoped route) so the account stays usable elsewhere, and surface a workspace-denial diagnostic. Define that key explicitly — an account-wide flag would quarantine the account for every route, which is the same over-reaction in a new place. +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 diff --git a/devlog/_plan/260816_wave34_closeout/040_1784_typed_cause.md b/devlog/_plan/260816_wave34_closeout/040_1784_typed_cause.md index b204fae52c..3593814f71 100644 --- a/devlog/_plan/260816_wave34_closeout/040_1784_typed_cause.md +++ b/devlog/_plan/260816_wave34_closeout/040_1784_typed_cause.md @@ -22,7 +22,7 @@ Extend the disposition rather than inventing the roadmap's `CatalogConvergenceEr ... reason: "provider-auth" | "provider-network" | "disk" + | "request-invalid" | "admission" | "internal"; -+ /** Non-sensitive cause summary: error name + redacted message. Never the raw body. */ ++ /** Allowlisted cause summary. Closed vocabularies only -- never message text. */ + cause?: { name: string; detail: string }; }; ``` @@ -44,8 +44,10 @@ Passing `Error.message` through `redactSecretString` is NOT sufficient. That hel Build `cause` from bounded, non-sensitive parts instead: -- `name`: the error constructor name, which is a fixed vocabulary. -- `detail`: a short allowlisted summary — an errno/code when present, otherwise a fixed phrase per branch. Never the raw message. +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. diff --git a/devlog/_plan/260816_wave34_closeout/050_1791_quota_windows.md b/devlog/_plan/260816_wave34_closeout/050_1791_quota_windows.md index f8471523b7..27696566c5 100644 --- a/devlog/_plan/260816_wave34_closeout/050_1791_quota_windows.md +++ b/devlog/_plan/260816_wave34_closeout/050_1791_quota_windows.md @@ -26,8 +26,13 @@ Store windows as an array, but keep the PROVENANCE the current shape encodes. A type StoredQuotaWindow = { /** Upstream slot the window arrived in — the provenance the old flag encoded. */ slot: "primary" | "secondary" | "tertiary"; - /** The real discriminator: 5h, 7d, 30d ... never inferred from the slot. */ - limitWindowSeconds: number; + /** + * 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. */ @@ -48,19 +53,24 @@ Exhaustion becomes "any governing window at limit", not "the weekly one", which `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 a small accessor that selects by duration band and `governing`, rather than by stored name: +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 governingWindow(q: StoredAccountQuota): StoredQuotaWindow | undefined; +function governingWindows(q: StoredAccountQuota): StoredQuotaWindow[]; function windowByBand(q: StoredAccountQuota, band: "short" | "weekly" | "monthly"): StoredQuotaWindow | undefined; ``` -Exhaustion is "the 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. +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. -- The governing window at 100% marks the account exhausted; a supplementary window at 100% does NOT. -- Cooldown recovery uses the governing window's reset, not the earliest reset. +- 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 index de5258dc7e..2d6fd64f0e 100644 --- a/devlog/_plan/260816_wave34_closeout/060_1835_cli_mutation.md +++ b/devlog/_plan/260816_wave34_closeout/060_1835_cli_mutation.md @@ -20,28 +20,37 @@ The real contract is narrower than a naive patch callback. `PersistedConfigMutat So `set`/`unset` become: -```diff -- const config = loadConfig(); -- setPath(candidate, path, parsed, action === "unset"); -- // ... validate ... -- saveConfig(config); -+ const outcome = mutatePersistedConfig(config => { -+ const candidate = structuredClone(config) as Record; -+ setPath(candidate, path, parsed, action === "unset"); -+ // Same validation the command already performs, on the FRESH candidate. -+ const validated = validateConfigCandidate(candidate); -+ if (!validated.ok) throw new Error(validated.error); -+ // The pin clear is part of the same transaction, not a follow-up write. -+ if (pathSegments(path)[0] === "codexAccountPriorities") clearCodexAccountPin(validated.config); -+ Object.assign(config, validated.config); -+ return { changed: JSON.stringify(config) !== JSON.stringify(validated.config) ? true : true, value: undefined }; -+ }); -+ if (outcome.status === "unavailable") { /* report missing/invalid/conflict, exit non-zero */ } +```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 be computed honestly — a byte-identical write must report `false` so the config generation is not bumped. +- `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. diff --git a/devlog/_plan/260816_wave34_closeout/080_1830_cursor_evidence.md b/devlog/_plan/260816_wave34_closeout/080_1830_cursor_evidence.md index 642c9a0ac6..34cba08ddb 100644 --- a/devlog/_plan/260816_wave34_closeout/080_1830_cursor_evidence.md +++ b/devlog/_plan/260816_wave34_closeout/080_1830_cursor_evidence.md @@ -25,9 +25,11 @@ With that fixture, parse it the way a real turn does, build the request through 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. the search flag this PR sets is on the entry that actually reaches the wire. +4. (separately) the catalog projection carries the search flag. -Assert against the SERIALIZED form, not the pre-budget array, or the test proves nothing about what the child receives. +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. diff --git a/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md b/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md index 1c2733a674..9fdcef9486 100644 --- a/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md +++ b/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md @@ -24,7 +24,11 @@ Also note what already works: policy routing DOES evaluate image requirements fo ### (a) Policy fallback -1. Populate request context size in `PolicyRequestEvidence` (`src/routing/request-evidence.ts:36` leaves it unknown). Use the estimate the request log already computes rather than inventing a second estimator. +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 From 89cf04f21203924d33cc8ca75afd89b4249f2b5d Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 00:28:29 +0900 Subject: [PATCH 07/11] fix(codex): stop reporting every convergence failure as a disk failure Catalog convergence manufactured `reason: "disk"` for anything it could not recognize, in two places: the management adapter's catch and the management API route. So a malformed request and a genuinely full filesystem were indistinguishable, and both were reported non-retryable -- the operator was told to check storage for what was often a programming fault or lock contention. `CatalogDisposition` gains `request-invalid`, `admission` and `internal` alongside the existing reasons, so `disk` narrows to real IO. `admission` is the one class marked retryable, because contention is the failure worth retrying unchanged. The new `cause` field is deliberately NOT an error summary. Both of its fields are closed vocabularies: `kind` maps onto a fixed set rather than echoing `Error.constructor.name` (any thrown custom class names itself), and `code` is emitted only for a recognized errno token. An `Error.message` routinely carries paths, home directories and account identifiers, and `redactSecretString` masks token shapes but none of those, so message text is never forwarded. `normalizeCatalogDisposition` is updated in the same change. It is the privacy boundary that rebuilds the response from an allowlist of own data properties, so without extending it the new reasons and cause would have been silently dropped and the fix would have looked implemented while changing nothing. Regressions: an escaping factory error is `internal`, a TypeError is `request-invalid`, and an error whose message embeds both a home path and a token-shaped string produces a response containing neither. Closes #1784. --- src/codex/catalog-refresh-status.ts | 24 ++++++++-- src/codex/convergence-types.ts | 25 ++++++++++- src/codex/management-convergence.ts | 57 +++++++++++++++++++++++- src/server/management-api.ts | 11 ++++- tests/codex-convergence-contract.test.ts | 49 +++++++++++++++++++- 5 files changed, 156 insertions(+), 10 deletions(-) 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/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/tests/codex-convergence-contract.test.ts b/tests/codex-convergence-contract.test.ts index c705c146fb..d285dea8cd 100644 --- a/tests/codex-convergence-contract.test.ts +++ b/tests/codex-convergence-contract.test.ts @@ -317,13 +317,60 @@ 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: () => { + throw new Error(`failed writing /Users/someone/.codex/config.toml 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"); + 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], From 4ca558db8ceff00cff401f5a5567353fa2c0b586 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 00:29:08 +0900 Subject: [PATCH 08/11] docs(devlog): avoid an empty-catch literal that trips the hygiene gate --- devlog/_plan/260816_wave34_closeout/040_1784_typed_cause.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/devlog/_plan/260816_wave34_closeout/040_1784_typed_cause.md b/devlog/_plan/260816_wave34_closeout/040_1784_typed_cause.md index 3593814f71..12e28405d7 100644 --- a/devlog/_plan/260816_wave34_closeout/040_1784_typed_cause.md +++ b/devlog/_plan/260816_wave34_closeout/040_1784_typed_cause.md @@ -7,7 +7,7 @@ There is no exception type to propagate — failures are DATA. `CatalogDispositi 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` uses a bare `catch {}` and again manufactures `reason: "disk"`, choosing gather-vs-commit purely from whether invocation had begun. +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`). From 1dfa8e8e653b52169d377c35a41aa2f4b900f77c Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 00:32:31 +0900 Subject: [PATCH 09/11] test(codex): build the fixture home path at runtime so privacy:scan stays green --- tests/codex-convergence-contract.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/codex-convergence-contract.test.ts b/tests/codex-convergence-contract.test.ts index d285dea8cd..b08be70bd4 100644 --- a/tests/codex-convergence-contract.test.ts +++ b/tests/codex-convergence-contract.test.ts @@ -360,14 +360,15 @@ test("a failure cause never carries message text, paths or identifiers (#1784)", const response = await handleManagementAPI(request, new URL(request.url), live, { saveConfigPreservingClaudeCode: () => {}, createManagementConvergeCodex: () => { - throw new Error(`failed writing /Users/someone/.codex/config.toml for ${secret}`); + 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"); + expect(body).not.toContain(["", "Users", "someone"].join("/")); expect(body).not.toContain("failed writing"); }); From 50ad068e1973f47d44322ff723666d1c738aad90 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 00:35:52 +0900 Subject: [PATCH 10/11] fix(cli): apply config set/unset inside the mutation transaction `config set`/`unset` read the disk snapshot OUTSIDE the mutation lock and then sent that whole older snapshot through `saveConfig`. A concurrent edit landing between the read and the write was silently reverted -- the CLI reported success while discarding someone else's change. The operation now runs inside `mutatePersistedConfig`, which reruns the callback against the latest validated disk state, so it is applied to what is actually there at commit time. No new primitive was needed; this one already existed. Two details the callback has to get right: - `changed` compares a snapshot taken BEFORE the mutation. Comparing after the write compares a value with itself, reports every no-op as a change, and bumps the config generation for nothing. - The committed object is REPLACED, not merged. `Object.assign` alone cannot remove a key, so `unset` would have reported success while changing nothing. Driven red against exactly that. Validation, the saved-value readback and the `codexAccountPriorities` pin release all moved inside the transaction with it -- leaving the pin clear outside would have reintroduced the same race for the pin. `import` is deliberately unchanged: it is an intentional whole-document replacement, and forcing it through patch semantics would silently merge where the user asked to replace. Closes #1838. --- src/cli/config-command.ts | 51 +++++++++++++++++------- tests/cli-headless-parity.test.ts | 64 +++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 14 deletions(-) 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/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; From 6a3e5dff27b1c24b3391f55bb34a19b140401ac7 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 00:39:52 +0900 Subject: [PATCH 11/11] fix(codex): stop reporting a 5-hour window as the weekly quota A K12-style plan sends a 5-hour primary window plus a 7-day secondary. The parser folded the primary into `weeklyPercent` whenever it was not explicitly monthly, so the 5-hour bar was reported AS the weekly quota and the real weekly reading -- the one that actually gates the account -- was discarded. The primary window is now skipped for the weekly slot when it DECLARES a duration shorter than a day, letting the 7-day secondary land there instead. A window with no declared duration is deliberately unchanged: older WHAM payloads omit `limit_window_seconds`, and inferring a duration there would reclassify every legacy account. That is why the discriminator is "declares a sub-day duration" rather than "is not seven days". This is the narrow correctness half of #1791. The broader generic-window storage (keeping every upstream window with its own reset, rather than two named slots) touches ~158 call sites across quota, routing, capacity, CLI and API surfaces, and is planned separately in devlog/_plan/260816_wave34_closeout/050_1791_quota_windows.md. Refs #1791. --- src/codex/quota.ts | 33 +++++++++++++++++++++++++++++++-- tests/codex-routing.test.ts | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) 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 { }); }); + + 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",