diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/000_plan.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/000_plan.md new file mode 100644 index 0000000000..e9a52b9c7f --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/000_plan.md @@ -0,0 +1,44 @@ +# 260831 — priority-70+ train: entitlement floor, roster TTL, Windows spill drain + +Frozen scope taken at 2026-08-31T01:1x KST from the open bug backlog. Three +targets, dependency-ordered. Each implementation phase consumes exactly one +decade doc as one full PABCD cycle. + +| wp | target | doc | priority | +| --- | --- | --- | --- | +| wp0 | this roadmap | `000`-`009` | — | +| wp1 | #3022 entitlement client_version floor + empty-vs-negative roster | `010` | 78/80 | +| wp2 | #3023 roster TTL expiry drops entitled rows | `020` | 71/80 | +| wp3 | #3011 Windows ACL spill stall (PR #3018 audit) | `030` | 71/80 | + +## Why this order + +wp1 is the stack base: it changes what `resolveCodexModelEntitlements` records +for an account. wp2 changes *when* the management surfaces re-read that record. +Landing wp2 first would leave the shared entry point refreshing a value that is +still wrong, so the fix would look effective on a warm cache and fail on a cold +one. + +wp3 is independent of both — it touches `src/responses/spill-store.ts` and +`src/responses/state.ts`, no catalog code — so it does not stack on wp1/wp2 and +can land in parallel. + +## Evidence provenance + +Three read-only `gpt-5.6-sol` high-effort research lanes were dispatched at wp0. +Their file:line findings are recorded in `001`-`003`. Every claim below that is +load-bearing was re-verified directly in the tree by the main session before +being written here. + +## Verification constraint (user-imposed) + +Local full test suites are forbidden for this train. Focused +`bun test ` runs locally; every suite, typecheck, and privacy scan runs on +`ssh lidge` (Linux x86_64, bun + git + gh present). Every completion claim +carries a receipt: command, exit code, pass/fail counts. Each regression test is +driven red against pre-fix code and that red result recorded. + +## Delivery + +Stacked PRs per `DEV-STACK-01`, pushed `--no-verify` (the pre-push hook runs the +forbidden local suite). Merge into `dev` is authorized for this goal. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/001_research_3022_entitlement_floor.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/001_research_3022_entitlement_floor.md new file mode 100644 index 0000000000..e1aa27627f --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/001_research_3022_entitlement_floor.md @@ -0,0 +1,124 @@ +# 001 — #3022: the entitlement client_version floor is below what upstream honours + +Verified against `origin/dev` `870a2adb6` (package 2.37.0). + +## The two defects + +This issue is two defects wearing one symptom. Fixing either alone leaves the +other able to reproduce it. + +### Defect A — the derived floor is wrong + +`resolveCodexEntitlementClientVersion` has three tiers +(`src/codex/model-entitlements.ts:122`): inbound request version, persisted +runtime `selectedVersion`, then `GATED_MODEL_CLIENT_VERSION_FLOOR`. + +Tier 3 is derived from the bundled snapshot, not hardcoded +(`src/codex/model-entitlements.ts:59`, `:81`, `:83`): + +``` +GATED_MODEL_CLIENT_VERSION_FLOOR = deriveGatedClientVersionFloor(snapshot) ?? FALLBACK +``` + +Measured in-tree: + +``` +GATED_MODEL_CLIENT_VERSION_FLOOR -> 0.142.2 + +src/codex/data/upstream-models.json + gpt-5.6-sol minimal_client_version = 0.142.2 context_window = 372000 + gpt-5.6-terra minimal_client_version = 0.142.2 context_window = 372000 + gpt-5.6-luna minimal_client_version = 0.142.2 context_window = 372000 +``` + +**The repository already contains a live measurement that contradicts its own +snapshot.** `devlog/_fin/260817_native_gpt56_1m_context/001_measurement_evidence.md` +records `GET /backend-api/codex/models?client_version=` against a real Codex +login: + +| client_version | rows returned | +| --- | --- | +| 0.60.0 | 0 | +| 0.142.2 | 5 — **no gpt-5.6** | +| >= 0.144.0 | 8 — includes sol/terra/luna | + +and records the live rows as `minimal_client_version = 0.144.0`, +`context_window = 272000`. The reporter measurement on #2886 (0.142.2 -> 200 +without gpt-5.6; 0.144.0 / 0.146.0 -> 200 with) independently reproduces this on +a different account and machine. + +So the snapshot's `0.142.2` and `372000` are both stale — they are PR #31684-era +values. Deriving the floor from that file faithfully produces a version upstream +does not honour, and tier 3 recreates the very defect #2891 set out to fix. + +**Raising `GATED_MODEL_CLIENT_VERSION_FLOOR_FALLBACK` fixes nothing.** The +expression is `derived ?? fallback`; derivation succeeds, so the fallback is +unreachable (`src/codex/model-entitlements.ts:83`). Confirmed by reading the +code, not assumed. + +### Defect B — an empty roster is recorded as a confirmed negative + +`parseAccountModels` returns a `Set` for any payload whose `models` is an array +(`src/codex/model-entitlements.ts:374`). `{"models":[]}` therefore yields an +**empty but non-null** `Set`. Rows filtered for `visibility === "hide"` or +`supported_in_api !== true` can empty it the same way. + +`fetchAccountModels` then converts non-null into confirmation +(`src/codex/model-entitlements.ts:414`, `:420`): + +```ts +expiresAt: now + (models ? MODEL_ROSTER_TTL_MS : MODEL_ROSTER_FAILURE_TTL_MS), +models: models ?? new Set(), +confirmed: models !== null, +``` + +An empty `Set` is truthy, so the account is marked `confirmed` **and** gets the +five-minute success TTL instead of the fifteen-second failure TTL. Downstream, +the account enters `confirmedAccountIds` with a set that lacks the gated slugs, +and every projection reads that omission as a decided denial +(`:547`, `:573`). Catalog sync then strips the gated bare and selector rows +(`src/codex/catalog/sync.ts:1579`, `:1616`) and runtime auth excludes the account +(`src/codex/auth-context.ts:458`). + +**`models.size > 0` is not a sufficient guard.** The reported short roster +contains `gpt-5.5`, so the set is non-empty while every gated row is absent. The +distinction that matters is whether the roster was obtained under a version +capable of returning the gated rows at all. + +## Why it reaches users + +Tier 1 and tier 2 mask the defect. It surfaces on the path with neither: a +background catalog sync or convergence pass with no inbound request +(`src/codex/catalog/sync.ts:1834`, `src/codex/convergence.ts:409`) on a host +where no Codex runtime was ever resolved, so `codex-runtime.json` carries no +`selectedVersion`. That matches the #2886 reporter's clean-reinstall reproduction +on a machine with no `codex` CLI. + +## Fix surface + +`src/codex/model-entitlements.ts`, and the snapshot only if its stale metadata is +corrected as a separate concern. + +1. Tier 3 must not be a bare snapshot derivation. Take + `max(derived, independently-measured minimum)` so a stale snapshot can lower + documentation but never lower the question we ask upstream. The numeric + comparator at `:88` already supports this. +2. An empty usable roster must be unconfirmed and take the failure TTL. +3. A roster fetched under a version below the trustworthy minimum must not make + omission authoritative for gated slugs. + +## Must not change + +- The fail-closed posture itself (#2550). Unknown stays ineligible; this unit + makes *unknown* distinguishable from *denied*, it does not admit unknown. +- Per-account and per-version cache keys (`:269`) — collapsing them reintroduces + cross-version evidence leakage. +- Inbound/runtime precedence. Hardcoding every request to one version would + advertise models to genuinely older clients (`:100`). + +## Open question carried forward + +The roster contract has no completeness marker and no per-model denial field, so +omission is the only signal available pre-dispatch. Whether `0.144.0` is stable +across all accounts is unproven: both measurements used one credential each, +though they were different credentials on different machines and agreed. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/002_research_3023_roster_ttl.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/002_research_3023_roster_ttl.md new file mode 100644 index 0000000000..d48b5cf796 --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/002_research_3023_roster_ttl.md @@ -0,0 +1,92 @@ +# 002 — #3023: expired roster silently shortens every management model surface + +Verified against `origin/dev` `870a2adb6`. + +## Mechanism + +`listManagementModelRows` builds native rows through `nativeModelRows(config)` +(`src/server/management/model-rows.ts:50`). A gated slug is included only when +`cachedAvailableAccountGatedNativeModels(Date.now(), ...)` returns it +(`src/codex/catalog/metadata.ts:414`). That projection is **synchronous** and +requires a confirmed entry with `expiresAt > now` +(`src/codex/model-entitlements.ts:596`). + +So a never-fetched roster and an *expired* roster project identically: empty. + +`listManagementModelRows` calls only `fetchAllModels` +(`src/server/management/shared.ts:173`), which gathers routed providers and never +resolves Codex entitlements. `/v1/models` differs — it runs `fetchAllModels` and +`resolveCodexModelEntitlements` together (`src/server/index.ts:1143`) — which is +exactly why one `GET /v1/models` repairs all three surfaces. + +## The three surfaces are not identical + +The reporter treated them as one path; they are three paths over one entry point: + +- `/api/models` calls `listManagementModelRows` directly + (`src/server/management/model-routes.ts:352`). +- `/api/client-config` goes through `loadExportModels` + (`:393`, `src/server/management/model-rows.ts:164`). +- `ocx export` does **not** use `loadExportModels`. It requests `/api/models` + over HTTP and serializes the rows itself + (`src/cli/export-command.ts:169`). + +That third detail matters for the test plan: a fixture that stubs rows instead of +going through the real management handler cannot see this defect. + +## Why an unconditional refresh is not acceptable + +The dashboard polls `/api/sidecar-settings` every 5 seconds +(`gui/src/pages/use-dashboard-data.ts:237`), and that route computes vision and +web-search candidates independently +(`src/server/management/config-routes.ts:589`), so each tick reaches the shared +list **twice**. The Models page adds `/api/models` every 10 seconds +(`gui/src/pages/Models.tsx:452`). + +That is ~24 shared-list calls/minute with the dashboard open, ~30 with the Models +catalog active. Polls pause while the document is hidden +(`gui/src/client-resource.ts:538`). An unconditional `resolveCodexModelEntitlements` +at the shared entry point would put credential enumeration on that cadence. + +## Fix surface + +Add a cheap ensure/freshness operation in `src/codex/model-entitlements.ts` and +await it from `listManagementModelRows` before `nativeModelRows`, in parallel with +`fetchAllModels` — the shape `/v1/models` already uses +(`src/server/index.ts:1155`). It must: + +- treat confirmed-empty and the 15-second unconfirmed entry as *cached answers*, + not as cache misses; +- preserve per-account/version keys and in-flight deduplication + (`:223`, `:455`) so concurrent pollers collapse into one fetch; +- never refresh from inside the synchronous `nativeModelRows`. + +## Must not change + +- The expiry check itself. Serving expired grants while refreshing would stop the + visual disappearance but break fail-closed revocation (`:509`). +- `/v1/models` authorization or version behaviour. +- Failure must stay a bounded fail-closed roster result, not an exception: a throw + from the shared entry would degrade sidecar candidates and could turn + client-config into a 503 (`src/sidecar/candidates.ts:29`). + +## The dishonest status is a separate, additive change + +`discovery: {"status":"ok"}` comes from routed-provider discovery +(`src/codex/catalog/provider-fetch.ts:1510`, `src/codex/model-cache.ts:94`), not +from entitlements, which never write it. So "ok" is *true* for what it describes. +Overloading it would erase a simultaneously-correct routed result. The honest fix +is an additive entitlement diagnostic; GUI types currently admit only provider +discovery states (`gui/src/models-groups.ts:2`). + +## Test plan + +- `tests/codex-model-entitlements.test.ts` — repeated fresh ensure calls do zero + refetches; at TTL+1 concurrent callers produce exactly one; failed refresh stays + unconfirmed with no retry for 15s. +- `tests/native-model-toggle.test.ts` — expired confirmed roster, then + `/api/models` still lists sol/terra/luna. Red today. +- `tests/management-client-config-route.test.ts` — same fixture, OpenCode map + contains the GPT-5.6 entries, entitlement fetch count is 1. +- `tests/cli-export-command.test.ts` — point its fake proxy at the real + `/api/models` handler; its current stubbed rows bypass the defective boundary. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/003_audit_3018_spill_drain.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/003_audit_3018_spill_drain.md new file mode 100644 index 0000000000..58c3a13635 --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/003_audit_3018_spill_drain.md @@ -0,0 +1,107 @@ +# 003 — #3011 / PR #3018 audit: correct fix, one shutdown blocker + +PR #3018, branch `ingw/fix-3011-async-acl-spill`, exact head `aec717722`, +rebased onto `870a2adb6` by this train. Repository CI green, 0 failures. + +## Verdict: the fix is right, and it is not yet mergeable + +CI-green is not correctness evidence here, because the gap is in a path no test +exercises. + +### What the PR gets right + +It genuinely removes the ACL subprocess wait from the response event loop. On +`dev`, `writeResponseSpillDurably` calls synchronous `harden()` +(`src/responses/spill-store.ts:180`, `:324`) which runs `Bun.spawnSync()` +(`src/lib/windows-secret-acl.ts:307`). `/healthz` shares that Bun fetch handler +(`src/server/index.ts:884`), so it cannot run during the wait — that is the 47s +stall. + +The PR queues Windows publications on a serialized promise tail +(`aec717722:src/responses/state.ts:280-308`) and awaits +`hardenSecretDirAsync`/`hardenSecretPathAsync`, which use `Bun.spawn()` plus +`await proc.exited` (`src/lib/windows-secret-acl.ts:329`). + +Linux and macOS are genuinely untouched: all three async routing points are gated +on `windowsSecretAclApplies()`, and false continues to the existing synchronous +`writeResponseSpillDurably` (`aec717722:src/responses/state.ts:547`, `:575`, +`:1250`). + +No Lab import is introduced, and no Node-only API — `Bun.spawn` is Bun-native. + +### Blocker — graceful shutdown does not drain pending publications + +`responseSpillPublicationTail` is awaited in exactly one place, and it is marked +test-only: + +``` +state.ts:187 let responseSpillPublicationTail: Promise = Promise.resolve(); +state.ts:306 responseSpillPublicationTail = responseSpillPublicationTail.then(...) +state.ts:328 await responseSpillPublicationTail; <- flushPendingResponseSpillsForTests +``` + +`flushResponseState()` — the function shutdown actually calls +(`src/server/lifecycle.ts:492`) — awaits only `persistGate` and the snapshot +write. It never observes the publication tail. Verified by reading the function +body at the PR head. + +The loss is concrete because oversized residents are deliberately excluded from +the snapshot (`aec717722:src/responses/state.ts:1015`): + +```ts +if (state.kind === "resident" && size > SNAPSHOT_ENTRY_MAX_BYTES) continue; +``` + +with `SNAPSHOT_ENTRY_MAX_BYTES = 2 * 1024 * 1024` (`:35`). + +So: a request queues a Windows spill for a payload over 2 MiB and returns while +`icacls` is still in flight. Shutdown flushes the snapshot, which *skips* that +resident because it is oversized, and the spill stub that would have replaced it +is not installed yet. The stop paths then call `process.exit()` +(`src/server/lifecycle.ts:489`, `src/server/management-api.ts:278`, +`src/cli/index.ts:365`), which also bypasses the writer's temp cleanup +(`aec717722:src/responses/spill-store.ts:467-505`). The continuation is lost and a +temp file can be orphaned. + +Before the PR this race did not exist: publication was synchronous, so by the time +the request returned the stub was already installed. + +### Required remediation + +Drain the publication tail to a **stable fixed point** before writing the shutdown +snapshot: repeatedly capture and await the tail until the captured promise still +equals the current one. A single `await` is insufficient because a settling job can +append another (`:306`). + +Surface: `src/responses/state.ts` (production drain + call it from +`flushResponseState` before snapshot serialization), `tests/responses-state.test.ts`, +and a note in `structure/02_config-and-codex-home.md`, whose current text describes +queueing but not shutdown ordering. `src/server/lifecycle.ts` needs no change — it +already calls `flushResponseState()` at the right boundary. + +## Coverage gaps in the PR's own tests + +The three added tests are real regressions — two wait on an injected async runner +the old synchronous path never enters (`aec717722:tests/responses-state.test.ts:720-818`). +They cover yielding, timeout recovery, and same-id supersession. They do not cover: +ordinary (non-timeout) ACL failure, cross-id serialization, shutdown, or +copy-fallback cleanup. + +## Security check + +Required-mode ACL failure appears to fail closed: the required helpers throw +(`src/lib/windows-secret-acl.ts:872`) and the state catch replaces the entry with a +failure marker (`aec717722:src/responses/state.ts:261`). Not covered by a test, +which is why the plan adds one. + +Residual, unresolved without a real Windows host: if ACL hardening fails *and* +unlink also fails, cleanup is best-effort +(`aec717722:src/responses/spill-store.ts:310`, `:498`) and a full payload can remain +on disk. Whether another local user can read it depends on the resulting NTFS ACL. +Recorded as a follow-up, not a blocker for this unit — it predates the PR. + +## Disposition + +wp3 is not "merge #3018". It is: land the drain fix on top of the PR head, prove it +with the shutdown regression, then merge. The author's work is correct as far as it +goes; the missing piece is the boundary his change created. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/004_audit_round1_synthesis.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/004_audit_round1_synthesis.md new file mode 100644 index 0000000000..58c60ba6ce --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/004_audit_round1_synthesis.md @@ -0,0 +1,136 @@ +# 004 — audit round 1: FAIL, and what it changed + +An adversarial `gpt-5.6-sol` high-effort plan auditor returned **FAIL** on the +first draft of `010`/`020`/`030`. Four blockers, all re-verified in-tree by the +main session before acceptance. This document records the synthesis +(REVIEW-SYNTHESIS-01) and the plan amendments it forced. + +## Blocker 1 (accepted) — wp1 applied model-scoped doubt as account-wide denial + +`CachedAccountModels.confirmed` is **one bit for the whole roster** +(`src/codex/model-entitlements.ts:223-234`), and every projection ignores the +account entirely when it is false: `entitledCodexAccountIdsForModel`, +`availableAccountGatedNativeModels`, and `isDirectCallerEntitledToCodexModel` all +require `confirmedAccountIds.has(accountId)` before consulting the model set +(`:547-550`, `:573-593`, `:527`). + +So the draft's `usable` flag would have thrown away *affirmative* rows too. A +roster fetched under `0.142.2` still legitimately confirms `gpt-5.5`, `gpt-5.4`, +and `codex-auto-review`. Marking the whole account unconfirmed would hide models +the account demonstrably owns, and would re-fetch every 15 seconds forever for a +genuinely un-entitled account. + +`gpt-daybreak-blue-latest` sharpens it: it is in the gated set +(`src/codex/catalog/native-models.ts`) but has **no row in the snapshot at all**, +so no measured minimum exists for it. A blanket version rule would deny it on +every path. + +**Amendment.** Separate the two questions the code currently conflates: + +- *Was this roster a usable answer at all?* -> stays account-scoped + (`confirmed`). Only an unparseable response or an empty parsed roster makes it + false. +- *Is this roster authoritative about a PARTICULAR gated model's absence?* -> + becomes model-scoped, answered by whether the roster's own + `clientVersion` is at or above that model's known minimum. + +Positive evidence needs no version test: a returned row is a grant regardless of +which version asked. Only *absence* needs the version to be trustworthy. A model +with no known minimum (Daybreak) keeps today's behaviour — omission is denial — +because inventing a floor for it would be a guess. + +## Blocker 2 (accepted) — wp2 would enumerate credentials forever when logged out + +`MAIN_CODEX_ACCOUNT_ID` is always a candidate +(`src/codex/model-entitlements.ts:500-506`), but with no credential +`accountCredentialSnapshot` returns null, so the account is filtered out before any +cache entry exists (`:539-550`). A "refresh entries that are missing" rule +therefore **misses forever** on a logged-out host and runs the full resolver on +every poll — the exact cost the plan forbade, at ~24 calls/minute. + +**Amendment.** The ensure needs a bounded negative memo for "no usable credential +for this account" with its own short TTL, checked before enumeration. Regression: +repeated logged-out ensures perform zero credential enumerations after the first. + +## Blocker 3 (accepted) — wp3's drain had no real bound + +The async `icacls` runner's timer calls `proc.kill()` and then **still awaits +`proc.exited`** (`src/lib/windows-secret-acl.ts:329-347`). Killing is not +settling: if the child ignores the kill, the await does not return, so a drain that +only awaits the tail inherits an unbounded wait at shutdown. + +The draft's escape hatch was also incoherent: it said an expired cap would "leave +the resident durably recorded", but the whole reason this matters is that residents +over 2 MiB are *excluded* from the snapshot (`:1002-1018`). There is nowhere for +it to be recorded. + +**Amendment.** wp3 must define settlement, not just waiting: + +1. Drain with a wall-clock cap. +2. On cap expiry the fallback must be a real durable write, not the snapshot — + either force a synchronous publication for the outstanding job, or persist the + oversized resident to its own spill path directly. +3. `Promise.race` alone is forbidden: a late writer publishing after snapshot + serialization is the same lost-continuation bug wearing a timeout. + +If a bounded settlement cannot be established in wp3's P, the honest move is to +split it: land the drain for the common case and file the pathological +never-exiting-`icacls` case separately, rather than shipping a shutdown hang. + +## Blocker 4 (accepted) — wp2's diagnostic had no transport + +`/api/models` returns a bare **array** (`src/server/management/model-routes.ts:352-354`), +and both the GUI and `ocx export` depend on that shape +(`gui/src/pages/Models.tsx:402-417`, `src/cli/export-command.ts:169-185`). A +top-level field breaks them; a per-row field duplicates global state. + +**Amendment.** Drop the diagnostic from wp2 entirely and make it its own +work-phase (wp4) that picks an owning endpoint — `/api/providers` already carries +`discovery`, so an additive sibling there is the natural home. wp2 stays the +refresh fix. + +## Corrections to the research docs (accepted, non-blocking) + +The auditor caught five citation errors and one substantive over-claim. The +over-claim matters: + +- **`372000` does not feed `NATIVE_GPT56_CONTEXT_WINDOW`.** That constant is + independently `272_000` and *overrides* the snapshot for runtime projections + (`src/codex/catalog/metadata.ts:130`, `:155-157`). Verified. So leaving the + snapshot stale is safe for behaviour — raw pinned-entry consumers still expose + `372000` (`tests/codex-catalog.test.ts:2901-2909`), which is a documentation + wart, not a defect. `010`'s decision to not edit the JSON stands, but for a + better reason than it gave. +- Line corrections: hidden-tab polling is `gui/src/client-resource.ts:197-215,270-294` + (not 538); `/v1/models` parallel resolution is `src/server/index.ts:1158-1164` + (not 1155); `src/server/lifecycle.ts:489` does **not** call `process.exit` — the + real exits are `src/server/management-api.ts:280` and `src/cli/index.ts:360,370`; + required-ACL throws are `src/lib/windows-secret-acl.ts:877,881` (not 872). + +## Confirmed sound (no change needed) + +- Every load-bearing factual claim in `001`-`003` held: the floor really is + `0.142.2`, the snapshot really records it, an empty `Set` really earns + `confirmed: true` with the 5-minute TTL (the auditor probed it directly: no + refetch at 15,001 ms, refetch only after 300,001 ms), `flushResponseState` really + ignores the publication tail, and the 2 MiB exclusion really applies to residents. +- wp1 stays fail-closed: the auditor confirmed no path admits an unentitled account. + The draft's flaw was over-denial, not under-denial. +- PR #3018 does not publish an unhardened reference: the temp is hardened before + linking, the copy fallback hardens its destination before returning, and the state + swap happens only after writer success. +- wp1 regressions are genuinely red today, and both masking tests are real: + `tests/codex-model-entitlements.test.ts:226-259` gates its mock at minor version + 142, and `:79-92` asserts *confirmed* for an all-filtered roster. That file + currently passes 20/20. +- Sequencing holds: wp1 and wp2 share `model-entitlements.ts`, so wp2 stacks; wp3 + shares no file with either and is genuinely parallel. + +## wp3 test-plan correction + +The auditor found drafts 3 and 4 would **pass against the PR head already** — +ordinary ACL failure tombstones and cleans up (`:261-266`, +`spill-store.ts:498-505`), and copy-fallback hardening failure already removes the +destination (`:297-315`). They are worth keeping as coverage but must not be +presented as red-first regressions. Only tests 1 and 2 (the shutdown drain) are +genuine red-first proof. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/005_audit_round2_synthesis.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/005_audit_round2_synthesis.md new file mode 100644 index 0000000000..8d6d8d8559 --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/005_audit_round2_synthesis.md @@ -0,0 +1,139 @@ +# 005 — audit round 2: FAIL again, and the collapse it forced + +Same reviewer, re-verification round on the amended plan +(`22e5eebed` -> `9020c0723`). Two of four blockers closed; two stayed open and one +new defect appeared. All re-verified in-tree before acceptance. + +The honest reading: round 1 fixed the *descriptions* but wp1 2b and wp3's fallback +were still unimplementable as written. This round collapses both to something that +can actually be built. + +## CLOSED — blocker 2 (logged-out enumeration) + +The bounded negative memo closes the loop. One correction accepted: credential +commits do not invalidate entitlement state today +(`src/codex/account-store.ts:131`, `src/codex/auth-api.ts:2015`, +`src/codex/model-entitlements.ts:630`), so a fresh login stays invisible for up to +the memo TTL. Amendment: pin the TTL explicitly and clear the memo on known +credential writes. Added to `020`. + +## CLOSED — blocker 4 (diagnostic transport) + +`/api/providers` returns provider objects already carrying `discovery` +(`src/server/management/provider-routes.ts:455`), so an additive sibling is a real +compatible transport. + +## STILL OPEN — blocker 1: "unknown" has nowhere to live + +This is the finding that matters, and it kills 2b as drafted. + +Two facts, both verified: + +1. `CachedAccountModels` records `clientVersion` (`:230`) but + `resolveCodexModelEntitlements` **discards it** when building + `CodexModelEntitlementSnapshot` (`:236`, `:547-550`). The projections literally + cannot see which version answered. +2. The projections are **positive-only**. `entitledCodexAccountIdsForModel` and + `availableAccountGatedNativeModels` compute "which accounts/models are + granted" (`:570`, `:573`). Adding a third boolean term to a positive-only + filter can only do one of two things: narrow it further (redundant — absence + already yields nothing), or *widen* it to include a model upstream never + granted. The second is exactly the fail-closed violation `010` promised not to + commit. + +So "treat absence as unknown rather than denied" has no representation in the +current contract. There is no third state to move a model into; there is only +*granted* and *not present in the output*. + +Third fact, and it makes the draft's own test wrong: **`gpt-5.5` is not +account-gated.** `ACCOUNT_GATED_NATIVE_OPENAI_MODELS` is exactly +`{gpt-5.6-sol, gpt-5.6-terra, gpt-5.6-luna, gpt-daybreak-blue-latest}` +(`src/codex/catalog/native-models.ts:5-10`); `gpt-5.5` is in the ungated native +list at `:70`. Test 4 asserted that `gpt-5.5` survives — but `gpt-5.5` was never +at risk, because `confirmed` only gates the gated set. The test would pass +vacuously and prove nothing about over-denial. + +### Collapse: drop 2b from wp1 + +2b was a safety net for a *future* upstream floor bump, not the repair for #3022. +Change 1 already fixes the reported defect by asking under `0.144.0`. A tri-state +entitlement contract — snapshot field for the answering version, an explicit +per-model minimum map, and projections that admit only `granted` while carrying +`unknown` separately — is a real subsystem change with its own blast radius across +three exported functions and every caller. + +**Decision: wp1 ships Change 1 + Change 2a only.** 2a is well-formed and +account-scoped: an empty parsed roster stops being a confirmation. The tri-state +work becomes `wp5` (`050`), sequenced after the train's user-visible fixes, where +it can be designed rather than smuggled in. + +This is a scope reduction, not a scope *retreat*: #3022's reported symptom is fully +addressed by Change 1, and 2a removes the five-minute lockout that made recovery +slow. What is deferred is hardening against a hypothetical future bump. + +## STILL OPEN — blocker 3: the fallback outruns its own cap + +Round 1 added a wall-clock cap to the drain, then specified a synchronous +publication as the cap-expiry fallback. The reviewer caught that the fallback is +not inside the cap: + +- the synchronous writer hardens the directory and the temp as **separate** calls + (`src/responses/spill-store.ts:324`); +- each hardening call resolves its **own** 30s budget + (`src/lib/windows-secret-acl.ts:799`, `HARDEN_DEADLINE_DEFAULT_MS = 30_000` at + `:255`), with the documented timeout-path worst case ~90s at load and ~60.25s on + the owner path. + +So entering the fallback *after* the cap expired can block for another minute or +more. A 5-second cap followed by a 60-second fallback is not a bound. + +### Amendment: the fallback must share one deadline, and the async writer must be disowned + +wp3 now requires: + +1. A single end-to-end shutdown budget covering drain **and** fallback. +2. The fallback passes its remaining budget down rather than letting each harden + call open a fresh 30s window. `OPENCODEX_ACL_TIMEOUT_MS` shows the deadline is + already parameterizable; the plumbing is the work. +3. Explicit ownership transfer: when the drain gives up on a job, the async writer + must be marked superseded so a late completion cannot publish over the + fallback's result. A late writer winning the race is the same lost-continuation + bug wearing a different hat. +4. New regressions the round-1 list omitted entirely: cap expiry enters the + fallback; the fallback respects the *remaining* budget; a late async completion + after cap expiry does not overwrite. + +**Accepted from the reviewer:** a synchronous stall is acceptable *at shutdown +specifically*, because that is the only path that reaches `flushResponseState()` +and no request is being served. #3011 is about the request path. But it is only +acceptable with an enforceable end-to-end deadline — otherwise wp3 trades a +startup stall for a shutdown hang. + +If that plumbing turns out to be larger than the drain itself, split it: land the +drain with a cap that simply *abandons* (accepting the documented loss for the +>2 MiB case, unchanged from today's behaviour) and file the bounded-fallback work +separately. Abandoning is not worse than the status quo; hanging is. + +## NEW DEFECT — wp4 specified an unreachable state + +`040` listed "confirmed roster that is genuinely empty" as a diagnostic state, +while `010` Change 2a makes every empty parsed roster **unconfirmed**. The state +cannot occur. + +The deeper point: without a completeness marker in the roster contract, the system +genuinely cannot distinguish "this account owns nothing" from "upstream returned an +unusable empty answer". Inventing a diagnostic label for a distinction the data +does not support would be a lie in a status field — the exact failure `002` blamed +`discovery: ok` for. + +Amendment: `040` drops that state and replaces it with `unconfirmed-empty`, +described honestly as "upstream returned no usable rows; we cannot tell whether +that means no entitlement". Recorded in `001`'s open questions as the underlying +contract gap. + +## Non-blocking correction carried + +`010` still repeated the disproven `372000` -> `NATIVE_GPT56_CONTEXT_WINDOW` claim +that `004` had already corrected. Fixed in this round: the constant is +independently `272_000` and overrides the snapshot +(`src/codex/catalog/metadata.ts:130`, `:155-157`). diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/006_audit_round3_synthesis.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/006_audit_round3_synthesis.md new file mode 100644 index 0000000000..c9b65f1cfb --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/006_audit_round3_synthesis.md @@ -0,0 +1,98 @@ +# 006 — audit round 3: wp1 cleared, wp3 and wp4 corrected + +Round 3 on `cef1ea527`. Blocker 1 **CLOSED**, and the question the whole train +exists to answer came back **Yes**. Two findings remain, both accepted. + +## CLEARED — wp1 as reduced is sound, and it does fix #3022 + +The reviewer traced the reported path end to end and confirmed it: + +> No inbound version and no `codex-runtime.json` makes tier 3 select +> `GATED_MODEL_CLIENT_VERSION_FLOOR`. Change 1 raises that effective floor to +> `0.144.0`; background sync directly invokes the resolver. The valid Plus +> credential is fetched, upstream is queried under `0.144.0`, and the returned +> sol/terra/luna rows enter a confirmed snapshot. Catalog projection then retains +> those gated rows. + +with `src/codex/model-entitlements.ts:122`, `:539`, `:547`, +`src/codex/catalog/sync.ts:1834`, `:1579`. + +Also cleared: + +- **Change 1's `max()` is sound.** It touches tier 3 only; a genuinely older + inbound or runtime version still wins. A later snapshot that lowers its recorded + minimum cannot lower the measured floor — which is the intended safety property, + not a side effect. +- **2a does not cause a retry storm.** The 15s TTL plus flight dedup bounds it to + ~4 fetches/minute per account/version. +- **No over-denial.** Projections still expose only roster-present gated models + (`:573`), so the reduced change cannot widen grants either. + +Residual blockers on #3022 are honest ones: no usable credential, an upstream +failure, or an account that genuinely does not get the rows at `0.144.0`. +Management surfaces that never trigger background discovery are wp2's problem, not +#3022's. + +## STILL OPEN — wp3, on two counts + +### 1. "Enter the fallback at cap expiry" and "fallback uses the remaining budget" contradict each other + +If the drain consumes the whole end-to-end deadline, the fallback inherits zero +time. Round 2 wrote both requirements without noticing they cancel. + +**Amendment.** Split the budget explicitly: the drain gets a sub-deadline, and a +reserved slice belongs to the fallback. Concretely — end-to-end budget `B`, drain +cap `B - R`, fallback reserve `R`, and the fallback receives `R` rather than +whatever happens to be left. `R` must be large enough for one directory harden plus +one file harden at a reduced per-call deadline, since those are two separate calls +(`src/responses/spill-store.ts:324`). + +### 2. Abandon-and-file is a real regression, not the status quo + +This correction matters and the round-2 text was wrong. + +On `origin/dev`, oversized candidates are published **synchronously before the +request returns** (`src/responses/state.ts:382`, `:393` — `admitOversizedCandidate` +calls `writeResponseSpillDurably` inline). So today there is no shutdown-loss +window at all for that case. `030` even says so itself, then contradicted it by +calling abandonment "unchanged behaviour". + +Abandonment is only equivalent to **PR #3018's head**, which is precisely the state +that introduced the loss window. Measuring against the unmerged PR instead of +`dev` is how a regression gets waved through. + +**Amendment.** The split condition is withdrawn. wp3 lands the bounded fallback, or +wp3 does not land. If the budget plumbing proves too large, the correct fallback is +**not** to abandon — it is to keep #3018 unmerged until the drain is complete, +because `dev` is currently *correct* on durability and merely slow on Windows. A +47-second stall is worse UX; a lost continuation is worse behaviour. We do not +trade the second for the first. + +## NEW — wp4 cannot tell empty from failed + +Verified: parsed-empty and network/timeout failure both produce +`{models: new Set(), confirmed: false}` — the success path when `parseAccountModels` +returns an empty set (`src/codex/model-entitlements.ts:414`) and the catch path +(`:424`) are indistinguishable downstream. + +So wp4's `unconfirmed-empty` and "refresh failed" are the same state in the data. +Reporting them as different would be the invented-status-field lie `002` objected +to. + +**Amendment.** wp4 gains an explicit prerequisite: record failure **provenance** on +the cache entry (parsed-empty / http-error / timeout / unparseable) before any +diagnostic claims to distinguish them, plus a regression asserting the two states +are actually distinct. If provenance is not added, wp4 reports one merged +`unconfirmed` state and says so plainly. + +## Standing after three rounds + +- wp1 (#3022, the 78/80 shipped regression): **cleared to implement.** +- wp2 (#3023): no blocker raised in rounds 2 or 3; the memo TTL and invalidation + hook amendments stand. +- wp3 (#3011): blocked pending the budget split and the withdrawal of + abandon-as-acceptable. +- wp4, wp5: sequenced after, with wp4 now dependent on failure provenance. + +Implementation begins with wp1, which is the user-visible regression and the one +the reviewer has now positively traced to a fix. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/007_audit_round4_wp1_plan.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/007_audit_round4_wp1_plan.md new file mode 100644 index 0000000000..76a9dcf92f --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/007_audit_round4_wp1_plan.md @@ -0,0 +1,97 @@ +# 007 — audit round 4: wp1 plan re-audit against the rebased tree + +Auditor: sol-high subagent, read-only, run after `codex/prio70-train-260831` was +rebased onto `origin/dev` = `7666f7d2a`. Verdict **FAIL** — the approach holds, +the plan's claims about its own tests did not. + +## Findings + +1. `src/codex/model-entitlements.ts:83-86` is exactly `derived ?? fallback`. The + gated set (`src/codex/catalog/native-models.ts:5-10`) is sol/terra/luna plus + Daybreak; the snapshot carries only the three gpt-5.6 rows, each recording + `0.142.2` (`src/codex/data/upstream-models.json:4-66`, `:122-184`, `:238-296`). + Daybreak has no row. Derived value confirmed as `0.142.2`. + +2. **Planned regression 2 would not have gone red.** The existing mock at + `tests/codex-model-entitlements.test.ts:245-246` admits every minor `>= 142`, + and `144 >= 142`. Raising the floor alone leaves it green. The mock threshold + must move to `>= 144` and the surrounding comment at `:238-240` with it. + +3. **Planned regression 4 is not red-first.** "A non-empty roster stays confirmed" + passes before and after. It is a characterization guard against over-reach, and + the plan must stop claiming otherwise. Only 1, 2 (after the mock correction), 3 + and 5 are genuinely red. + +4. The proposed `tests/claude-models-discovery.test.ts` addition needs a + version-sensitive backend. The existing no-inbound mock at `:404-411` answers + Daybreak for every version, so as written the new case is green on both sides. + +5. Only one existing assertion changes outcome anywhere in the suite: + `tests/codex-model-entitlements.test.ts:90`, `confirmedAccountIds.has("main")` + from `true` to `false`. Nothing in `tests/e2e-style/` asserts it. + `tests/codex-catalog-sync-hardening.test.ts:385-442` sends a non-empty usable + roster and `tests/claude-models-discovery.test.ts:518-559` supplies inbound + `0.151.7`, so neither is affected. + +6. No consumer needs an all-filtered response to stay confirmed. Every projection + requires confirmation **and** membership (`:570`, `:578-580`, `:587-593`, + `:611-619`), so an empty set denies identically either way. Collapsing + "zero rows returned" and "rows parsed to empty" is correct under the current + usable-roster contract. + +7. **The 15s TTL is demand-driven, not timer-driven**, so 2a opens no background + churn. Refetch happens only through `/v1/models` + (`src/server/index.ts:1158-1164`), Direct gated authorization + (`src/codex/auth-context.ts:382-385`), catalog sync + (`src/codex/catalog/sync.ts:1834-1840`) and convergence + (`src/codex/convergence.ts:409-416`). Same account and version coalesce onto one + flight (`:461-470`); distinct versions are capped at four concurrent per account + (`:472-483`). Worst case for a legitimately empty account under continuous + polling is roughly four fetches per minute per active version. Sequential + high-cardinality version cycling is concurrency-bounded but not rate-limited — + recorded as pre-existing, out of scope here. + +## Required plan changes (all applied to `010`) + +- Correct the `:245` mock threshold to `>= 144` as part of regression 2. +- Relabel regression 4 as a green characterization guard. +- Give the Claude discovery case a version-sensitive backend. +- **New regression 6:** exercise the composition with a synthetic derived floor + *above* `0.144.0`. Without it, replacing the exported floor with the bare + literal `0.144.0` passes every other test while destroying the stated + future-snapshot property (`010:28-36`). +- **New regression 7:** the all-filtered case must also prove refetch after + 15,001 ms. Flipping `confirmed` while leaving the five-minute TTL in place + would otherwise pass regression 5. + +## Round 5 (same reviewer, amended plan) + +**VERDICT: FAIL** — three further corrections, all now applied to `010`: + +1. Regressions 3 and 7 would not have exercised a real cache entry. + `boundedCacheSet` runs only when `currentCredentialIdentity(accountId)` matches + the snapshot identity (`src/codex/model-entitlements.ts:486-490`, `:330-340`), + and the suite's `credential()` helper mints `test:` + (`tests/codex-model-entitlements.test.ts:28-34`), which matches nothing — so the + TTL assertions would have measured an uncached path. The plan now prescribes the + Direct-caller path (identity is a SHA-256 token fingerprint, `:437-451`) or a + genuinely persisted record, and requires both halves: no refetch before 15s and + exactly one after 15,001 ms. +2. The churn bound wrongly credited continuous dashboard polling. `/api/models` + reaches only `listManagementModelRows` + (`src/server/management/model-routes.ts:352-354`), which never resolves + entitlements (`src/server/management/model-rows.ts:50-55`). The bound belongs to + `/v1/models` and Direct gated demand; dashboard polling becomes a caller only + when wp2 lands, so wp2 inherits the cost. +3. The plan still claimed an unconfirmed account loses `gpt-5.5`/`gpt-5.4`. It does + not: both projections apply the flag only to gated slugs + (`!ACCOUNT_GATED.has(slug) || (confirmed && entitled.has(slug))` at + `src/codex/catalog/sync.ts:1617-1620` and + `src/codex/convergence.ts:280-284`). `confirmed` suppresses account-gated models + only. Retracted. + +## Round 6 + +**VERDICT: PASS.** Two non-blocking notes (a duplicated sentence and +`direct:` vs the actual SHA-256 fingerprint spelling) fixed on the spot. +Cleared to implement. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/010_wp1_entitlement_floor_and_empty_roster.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/010_wp1_entitlement_floor_and_empty_roster.md new file mode 100644 index 0000000000..43d3fc703f --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/010_wp1_entitlement_floor_and_empty_roster.md @@ -0,0 +1,206 @@ +# 010 — wp1: entitlement floor + empty-vs-negative roster (#3022) + +Stack base. Consumes `001`. One PABCD cycle. + +Branch: `codex/3022-entitlement-floor-empty-roster` off `origin/dev`. + +## Change 1 — tier 3 stops trusting the snapshot alone + +`src/codex/model-entitlements.ts` + +Add an independently measured minimum next to the derivation, and take the higher +of the two. The snapshot may raise the floor; it may never lower it below what we +have measured upstream to honour. + +```ts +/** + * Lowest client_version measured to actually return the gated rows. + * + * devlog/_fin/260817_native_gpt56_1m_context/001_measurement_evidence.md and the + * #2886/#3022 reporter captures agree: 0.142.2 returns 200 with five rows and no + * gpt-5.6; 0.144.0 and above return the gated rows. The bundled snapshot records + * 0.142.2, so a derivation that trusts it asks a question upstream answers with + * an empty gated set. + */ +const MEASURED_GATED_CLIENT_VERSION_MINIMUM = "0.144.0"; +``` + +and the exported floor becomes the max of `deriveGatedClientVersionFloor(...)`, +the measured minimum, and the existing fallback only when derivation yields +nothing. `compareClientVersions` (`:88`) already does the ordering. + +Keep `deriveGatedClientVersionFloor` exported and unchanged in behaviour — it is +separately tested and its job (read the snapshot faithfully) is still correct. The +correction belongs at the composition site, so a future snapshot refresh that +records `0.144.0` or higher takes over naturally and the constant becomes inert +rather than conflicting. + +Tiers 1 and 2 are untouched. An inbound or runtime version still wins, because +those describe a real client and this constant does not. + +## Change 2 — separate "usable answer" from "authoritative about this model" + +> Amended after audit round 1 (`004`, blocker 1). The first draft used one +> account-wide `usable` flag, which would have discarded affirmative rows too. + +Same file, `fetchAccountModels` (`:414`). + +Today: + +```ts +expiresAt: now + (models ? MODEL_ROSTER_TTL_MS : MODEL_ROSTER_FAILURE_TTL_MS), +models: models ?? new Set(), +confirmed: models !== null, +``` + +An empty `Set` is truthy, so `{"models":[]}` earns `confirmed: true` and the +five-minute success TTL. + +`confirmed` is a single bit for the whole account (`:223-234`) and every gated +projection requires it (`:547-550`, `:573-593`, `:527`). It cannot carry a +per-model judgement, which is why 2b had to move to wp5. + +Correction from audit round 5: an unconfirmed account does **not** lose +`gpt-5.5`/`gpt-5.4`. Both projections skip the flag entirely for any slug outside +`ACCOUNT_GATED_NATIVE_OPENAI_MODELS` — the filter is +`!ACCOUNT_GATED.has(slug) || (confirmed && entitled.has(slug))` +(`src/codex/catalog/sync.ts:1617-1620`, `src/codex/convergence.ts:280-284`). So +`confirmed` suppresses account-gated models only, and the earlier draft's worry +about ungated native rows was unfounded. What it *does* mean is that a wrong +`confirmed: true` on an empty roster is a confirmed denial of sol/terra/luna — +which is the defect. + +Two distinct changes: + +**2a. Account-scoped: an empty parsed roster is not a confirmation.** +`{"models":[]}`, and an all-filtered roster, mean no usable evidence. Set +`confirmed: false` and take `MODEL_ROSTER_FAILURE_TTL_MS` (15s) rather than +locking in a wrong answer for five minutes. A non-empty roster stays confirmed. + +**2b — deferred to wp5.** See `005` (audit round 2, blocker 1). The draft wanted +model-scoped absence authority: "absence of gated model *M* is denial only when the +answering version could have returned *M*". It is not implementable inside this +cycle, for two verified reasons. + +First, the answering version is not visible where the decision happens. +`CachedAccountModels` records `clientVersion` (`:230`), but +`resolveCodexModelEntitlements` discards it when building the snapshot (`:236`, +`:547-550`). + +Second, the projections are **positive-only**: they compute which accounts/models +are *granted* (`:570`, `:573`). A third boolean term in a positive-only filter can +only narrow it (redundant — absence already yields nothing) or widen it to include +a model upstream never granted. There is no third slot for "unknown" to occupy, so +expressing it means changing the snapshot contract and all three exported +projections. That is a subsystem change, not a line. + +Change 1 already fixes the reported defect by asking under `0.144.0`. 2b was only +ever a safety net against a *future* upstream bump, so it goes to `050` where it +can be designed with a real tri-state contract. + +## Change 3 (bounded) — correct the stale snapshot metadata + +`src/codex/data/upstream-models.json`: the three gated rows record +`minimal_client_version: 0.142.2` and `context_window: 372000`, both contradicted +by the in-repo live measurement (`0.144.0`, `272000`). + +Treat this as **optional for this cycle and out of scope if it moves anything +else.** The file is a pinned catalog snapshot consumed as exact model metadata; the +context-window value has its own pinned-entry tests +(`tests/codex-catalog.test.ts:2901-2909`). + +Correction from audit round 1: `372000` does **not** feed +`NATIVE_GPT56_CONTEXT_WINDOW`. That constant is independently `272_000` and +overrides the snapshot for runtime projections +(`src/codex/catalog/metadata.ts:130`, `:155-157`). So the stale value is a +documentation wart with no behavioural reach, which is *why* leaving it is safe. + +Decision for this cycle: **do not edit the JSON.** Change 1 makes the stale value +harmless, and the max-composition means correcting it later is safe. Record the +staleness as a follow-up so it is not lost. + +## Regressions + +> Amended after audit round 4 (`007`). Two of the original claims about these +> tests were wrong, and two regressions were missing. Red-first status is now +> marked per case rather than asserted for the set. + +`tests/codex-model-entitlements.test.ts` + +1. **Red-first.** Effective floor is `0.144.0`. Red now: returns `0.142.2`. +2. **Red-first only after the mock is corrected.** No inbound and no runtime + version, upstream mock returning gated rows only at `>= 0.144.0` -> the request + uses `0.144.0` and sol/terra/luna are available. The existing test at `:226` + mocks the gate at minor `>= 142`, and `144 >= 142`, so raising the floor alone + leaves it green — that mock threshold moves to `>= 144` as part of this + regression, along with the comment at `:238-240` that explains it. +3. **Red-first.** `{"models":[]}` -> account not confirmed, failure TTL. Red now: + confirmed with the 5-minute TTL (probe: no refetch at 15,001 ms, refetch only + after 300,001 ms). + + Audit round 5, finding 1: the TTL half of this only proves anything against a + **real cache entry**. `boundedCacheSet` runs only when + `currentCredentialIdentity(accountId)` equals the snapshot's identity + (`src/codex/model-entitlements.ts:486-490`, `:330-340`), and the suite's + `credential()` helper mints `test:` + (`tests/codex-model-entitlements.test.ts:28-34`), which matches nothing. Use the + Direct-caller path — `isDirectCallerEntitledToCodexModel` with + `directHeaders(...)`, whose identity is a stable SHA-256 token fingerprint + (`direct:`, `src/codex/model-entitlements.ts:437-451`) — or a genuinely + persisted pool/main record. Then assert both halves: **no** refetch before 15s, and **exactly one** after 15,001 ms. +4. **Green on both sides — characterization guard, not a red-first regression.** A + non-empty roster stays confirmed. It bounds 2a against over-reach: only the + *empty* case may change, so an ordinary short roster must still confirm the + account and expose whatever it grants. +5. **Red-first.** The existing "all rows filtered as hidden/api-disabled" case at + `:79` currently asserts *confirmed* at `:90`; it must assert unconfirmed. This + is the single existing assertion anywhere in the suite whose outcome changes, + and it is an intentional flip called out for review rather than a quiet edit. +6. **Red-first. New in round 4.** The floor is a *composition*, so test it as one: + with a synthetic derived floor above `0.144.0`, the higher value must win. + Without this, replacing the export with the bare literal `0.144.0` passes every + other case while destroying the future-snapshot property stated at `:28-36`. +7. **Red-first. New in round 4.** The all-filtered case must also prove refetch + after 15,001 ms, under the same real-cache-entry requirement as regression 3. + Flipping `confirmed` while leaving the five-minute TTL in place would otherwise + pass regression 5 with the bug still present. + +`tests/claude-models-discovery.test.ts` beside the client-version forwarding test +at `:518`: with no inbound or runtime evidence, `/v1/models` still exposes the +gated rows. The backend for this case must be **version-sensitive** — the existing +no-inbound mock at `:404-411` answers Daybreak for every version, so a +version-blind copy of it is green on both sides and proves nothing. + +## Churn bound for 2a (audit round 4, finding 7) + +The 15s failure TTL is demand-driven, not timer-driven, so shortening it opens no +background traffic. Refetch happens only through `/v1/models` +(`src/server/index.ts:1158-1164`), Direct gated authorization +(`src/codex/auth-context.ts:382-385`), catalog sync +(`src/codex/catalog/sync.ts:1834-1840`) and convergence +(`src/codex/convergence.ts:409-416`). Identical account and version coalesce onto +one in-flight request (`:461-470`), and distinct versions are capped at four +concurrent flights per account (`:472-483`). + +Worst case for a legitimately empty account is about four fetches per minute per +active version under continuous **`/v1/models` or Direct gated** demand. +Correction from audit round 5: this is **not** dashboard polling today. +`/api/models` reaches only `listManagementModelRows` +(`src/server/management/model-routes.ts:352-354`), which never resolves +entitlements (`src/server/management/model-rows.ts:50-55`). Dashboard polling +becomes a caller of this path only once wp2 lands, so wp2 inherits the cost bound +rather than wp1 paying it. + +Sequential high-cardinality version cycling is concurrency-bounded but not +rate-limited; that is pre-existing and out of scope here. + +## Verification + +Focused locally during iteration; full suite + typecheck + privacy scan on +`ssh lidge`. Receipt recorded in `070_outcome.md`. + +## Out of scope + +Routing, dispatch, the `372000` context window, the roster contract's lack of a +completeness marker (recorded in `001` as an open question), sequential +version-cycling rate limits, and anything under `src/lab/`. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/020_wp2_roster_ttl_refresh.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/020_wp2_roster_ttl_refresh.md new file mode 100644 index 0000000000..d7b5d59dc4 --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/020_wp2_roster_ttl_refresh.md @@ -0,0 +1,133 @@ +# 020 — wp2: refresh the expired roster at the shared entry point (#3023) + +Stacks on wp1. Consumes `002`. One PABCD cycle. + +Branch: `codex/3023-roster-ttl-refresh`, based on the wp1 head (stacked child). + +## Why it stacks rather than lands independently + +wp2 makes the management surfaces re-read the entitlement record. wp1 fixes *what* +that record contains. Landing wp2 alone would refresh a still-wrong answer: the +bug would appear fixed on a warm cache and persist on a cold one. + +## Change 1 — a conditional ensure, not an unconditional resolve + +New export in `src/codex/model-entitlements.ts`: an ensure/freshness operation that +enters the real resolver **only** for credential/version entries that are missing +or past their own deadline. It must treat as cached answers: + +- a confirmed roster inside `MODEL_ROSTER_TTL_MS`; +- a confirmed-empty entry (after wp1, unconfirmed with the 15s TTL); +- an unconfirmed failure entry inside `MODEL_ROSTER_FAILURE_TTL_MS`. + +It must reuse the existing per-account/version keys and in-flight deduplication +(`:223`, `:455`) so concurrent pollers collapse into one upstream fetch. + +### Amendment after audit round 1 (`004`, blocker 2): the logged-out hole + +"Refresh entries that are missing" **misses forever** when there is no credential. +`MAIN_CODEX_ACCOUNT_ID` is always a candidate (`:500-506`), but +`accountCredentialSnapshot` returns null without one, so the account is filtered +out before any cache entry is created (`:539-550`). Nothing is ever cached, so +every poll re-enters the full resolver — ~24 times/minute, which is precisely the +cost this cycle forbids. + +So the ensure needs a **bounded negative memo**: "no usable credential for account +X as of T", with its own short TTL, checked before credential enumeration. Absence +of a credential is a cacheable answer, not a cache miss. + +Regression: repeated logged-out ensures perform zero credential enumerations after +the first. This is the assertion that proves the steady state, and it is red +against a naive implementation. + +### The memo needs an invalidation hook (audit round 2 — `005`) + +Credential commits do not invalidate entitlement state today +(`src/codex/account-store.ts:131`, `src/codex/auth-api.ts:2015`, +`src/codex/model-entitlements.ts:630`). So a negative memo means a **fresh login +stays invisible until the memo expires** — the user logs in and the dashboard still +shows nothing. + +Two requirements, both testable: + +1. **Pin the TTL explicitly** and keep it short. It bounds how stale a successful + login can look, so it is a UX number, not an implementation detail. +2. **Clear the memo on known credential writes.** The account-store and auth-api + commit points above are the hooks. Regression: log in, then the very next ensure + sees the credential without waiting out the TTL. + +Without (2) this cycle fixes a missing-rows bug by introducing a different +missing-rows bug. + +## Change 2 — await it from the shared entry point + +`src/server/management/model-rows.ts:50`, `listManagementModelRows`: await the +ensure in parallel with `fetchAllModels`, before `nativeModelRows` — the shape +`/v1/models` already uses (`src/server/index.ts:1155`). + +```ts +const [routed] = await Promise.all([fetchAllModels(config), ensureCodexEntitlementFreshness(config)]); +``` + +This repairs all three reported surfaces at once because they funnel here: +`/api/models` directly, `/api/client-config` via `loadExportModels`, and +`ocx export` by requesting `/api/models` over HTTP +(`src/cli/export-command.ts:169`). + +**Failure must not throw.** A rejection here would degrade sidecar candidates +(`src/sidecar/candidates.ts:29`) and could turn client-config into a 503. The +ensure resolves with a bounded fail-closed result; the rows stay short, which is +the honest outcome, and Change 3 makes that visible. + +## Cost bound (the constraint that shapes this) + +The dashboard reaches this entry point ~24 times/minute (`/api/sidecar-settings` +every 5s, computing vision and web-search candidates independently), ~30 with the +Models page open. Polls pause on a hidden document. + +So the ensure must be a **cache read** in the steady state: no credential +enumeration, no allocation of a resolver context, no network. Measured target: +with a fresh roster, repeated calls perform zero fetches and no credential +validation. That assertion is a test, not a hope. + +## Change 3 — moved out of this cycle + +> Removed after audit round 1 (`004`, blocker 4). The draft named no transport. + +`/api/models` returns a bare **array** (`src/server/management/model-routes.ts:352-354`) +and both the GUI and `ocx export` depend on that shape +(`gui/src/pages/Models.tsx:402-417`, `src/cli/export-command.ts:169-185`). A +top-level field breaks them; a per-row field duplicates global state on every row. + +The honest diagnostic therefore needs its own endpoint decision, which is a design +question, not a line of code. It is now **wp4** (`040`). wp2 is the refresh fix and +nothing else. + +## Regressions (each driven red first) + +- `tests/codex-model-entitlements.test.ts` — fresh repeated ensure -> zero + refetches; at TTL+1 concurrent callers -> exactly one; failed refresh stays + unconfirmed with no retry for 15s. +- `tests/native-model-toggle.test.ts` — expired confirmed roster, `/api/models` + still lists sol/terra/luna. Red today. +- `tests/management-client-config-route.test.ts` — same fixture; OpenCode map holds + the GPT-5.6 entries; entitlement fetch count is 1. +- `tests/cli-export-command.test.ts` — repoint its fake proxy at the real + `/api/models` handler. Its stubbed rows currently bypass the defective boundary, + so today's green is vacuous for this defect. + +## Must not change + +The expiry check (`:509`) — serving expired grants breaks fail-closed revocation. +`/v1/models` authorization or version behaviour. Per-account/version keys. +Synchronous `nativeModelRows` must stay synchronous. + +## Open question to settle during P + +Whether a stale management request waits out the 8s entitlement timeout or returns +immediately with short rows. Serving stale rows is inconsistent with the current +posture, so the default is to wait — but 8s on a dashboard poll is its own problem. + +Note the shape of the risk: the wait only happens on the *first* poll after expiry, +because in-flight deduplication collapses the rest. Resolve before B and record the +decision here. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/030_wp3_spill_publication_drain.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/030_wp3_spill_publication_drain.md new file mode 100644 index 0000000000..00b81b7898 --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/030_wp3_spill_publication_drain.md @@ -0,0 +1,189 @@ +# 030 — wp3: drain Windows spill publications at shutdown (#3011 / PR #3018) + +Independent of wp1/wp2 — different files, no catalog code. Consumes `003`. +One PABCD cycle. + +Branch: `codex/3018-shutdown-drain`, based on PR #3018's head `aec717722`. + +## Disposition + +Not "merge #3018". The PR's async publication is the right fix for the 47s +`/healthz` stall, but it creates a shutdown boundary it does not close. wp3 lands +the drain on top of the author's head, proves it, then merges the combined result. +Ingwannu's commit stays intact and credited. + +## The blocker, restated + +`responseSpillPublicationTail` is awaited only by +`flushPendingResponseSpillsForTests` (`aec717722:src/responses/state.ts:328`). +`flushResponseState()` — what shutdown actually calls +(`src/server/lifecycle.ts:492`) — awaits `persistGate` and the snapshot write, and +never the tail. + +Oversized residents are excluded from snapshots +(`aec717722:src/responses/state.ts:1015`, cap 2 MiB at `:35`), so for a payload +over 2 MiB there is a window where the snapshot skips the resident *and* the spill +stub is not installed yet. `process.exit()` on the stop paths +(`src/server/lifecycle.ts:489`, `src/server/management-api.ts:278`, +`src/cli/index.ts:365`) also skips the writer's temp cleanup +(`aec717722:src/responses/spill-store.ts:467-505`). + +Result: a lost continuation plus a possible orphaned temp. Before the PR the race +did not exist, because publication completed before the request returned. + +## Change — drain to a stable fixed point + +`src/responses/state.ts`: add a production drain and call it from +`flushResponseState()` **before** snapshot serialization. + +```ts +async function drainResponseSpillPublications(): Promise { + // A settling job can append another to the tail, so one await is not a + // fixed point: keep observing until the promise we awaited is still current. + for (;;) { + const observed = responseSpillPublicationTail; + await observed.catch(() => {}); + if (observed === responseSpillPublicationTail) return; + } +} +``` + +Ordering matters: draining *after* the snapshot keeps the bug, because the +oversized resident is skipped at serialization time. + +`flushPendingResponseSpillsForTests` should delegate to the same drain so the test +helper and production cannot diverge. + +### Bounding it (amended after audit round 1 — `004`, blocker 3) + +The draft assumed the ACL helper's timeout made the drain finite. It does not. +`defaultAsyncIcaclsRunner` sets a timer that calls `proc.kill()` and then **still +awaits `proc.exited`** (`src/lib/windows-secret-acl.ts:329-347`). Killing is not +settling: a child that ignores the kill leaves that await outstanding, so a drain +that only awaits the tail inherits an unbounded shutdown wait. + +The draft's fallback was also incoherent — it said cap expiry would "leave the +resident durably recorded", but the reason this bug exists is that residents over +2 MiB are *excluded* from the snapshot (`:1002-1018`). There is nowhere for it to +go. + +So wp3 must define **settlement**, not just waiting: + +1. Drain with an explicit wall-clock cap. +2. On expiry, take a real durable action. Falling back to the snapshot is not an + option, for exactly the 2 MiB reason. +3. `Promise.race` alone is **forbidden**: a writer that publishes after snapshot + serialization is the same lost-continuation bug wearing a timeout. + +### The fallback must share the cap (audit round 2 — `005`, blocker 3) + +Round 1 named a synchronous publication as the cap-expiry fallback without checking +its cost. Verified: the synchronous writer hardens the directory and the temp as +**separate** calls (`src/responses/spill-store.ts:324`), and each call resolves its +**own** budget — `HARDEN_DEADLINE_DEFAULT_MS = 30_000` +(`src/lib/windows-secret-acl.ts:255`, resolved per call at `:799`), with the +file's own comment documenting a ~90s timeout-path worst case at load and ~60.25s +on the owner path. + +A 5-second drain cap followed by a 60-second fallback is not a bound. So: + +1. **A budget split, not just a total** (audit round 3 — `006`). "Enter the fallback + at cap expiry" and "the fallback uses the remaining budget" cancel each other: a + drain that consumes the whole deadline leaves the fallback zero time. So reserve + the slice up front — end-to-end budget `B`, drain cap `B - R`, fallback reserve + `R`, and the fallback receives `R` rather than the remainder. `R` must cover one + directory harden plus one file harden at a reduced per-call deadline, because + those are two separate calls (`src/responses/spill-store.ts:324`). +2. The fallback **passes its reserved budget down** instead of letting each harden + call open a fresh 30s window. `OPENCODEX_ACL_TIMEOUT_MS` proves the deadline is + already parameterizable (`:799`); the plumbing is the work. +3. **Explicit ownership transfer.** When the drain abandons a job, mark the async + writer superseded so a late completion cannot publish over the fallback's + result. A late writer winning that race is the same lost continuation. + +**A synchronous stall is acceptable here specifically.** Only the graceful-shutdown +path reaches `flushResponseState()`, and no request is being served — #3011 is +about the request path. But it is acceptable *only* with an enforceable end-to-end +deadline. + +### The split condition is withdrawn (audit round 3 — `006`) + +Round 2 claimed abandonment "leaves exactly today's behaviour". That was wrong, and +the error was measuring against the wrong baseline. + +On `origin/dev`, oversized candidates are published **synchronously before the +request returns**: `admitOversizedCandidate` calls `writeResponseSpillDurably` +inline (`src/responses/state.ts:382`, `:393`). There is no shutdown-loss window on +`dev` for that case at all. This doc says as much a few paragraphs up — "before the +PR this race did not exist" — and then contradicted itself. + +Abandonment is equivalent to **PR #3018's head**, which is exactly the state that +introduced the loss. Measuring a proposed regression against an unmerged PR instead +of `dev` is how a regression gets waved through. + +So: wp3 lands the bounded fallback, or wp3 does not land. If the budget plumbing +proves larger than the drain, the correct move is to keep #3018 **unmerged** until +the drain is complete. `dev` is currently correct on durability and merely slow on +Windows. A 47-second stall is worse UX; a lost continuation is worse behaviour, and +we do not trade the second for the first. + +`src/server/lifecycle.ts` needs no change — it already calls `flushResponseState()` +at the right boundary. + +## Regressions (each driven red first) + +`tests/responses-state.test.ts` + +1. Gate ACL calls under `responses-state-spill`, queue a candidate, call + `flushResponseState()`; assert it stays pending until released. Red at the PR + head: the flush ignores the tail. +2. After release, the flush returns only once the stub is installed; then clear + memory and verify restart replay. Payload **over 2 MiB** so an early snapshot + cannot mask it. +2b. **Cap expiry enters the fallback.** Hold the ACL gate past the cap; assert the + fallback path runs rather than the flush returning with the job outstanding. +2c. **The fallback respects the remaining budget.** With the cap nearly spent, the + fallback must not open a fresh 30s harden window. Assert total elapsed stays + inside the end-to-end budget. +2d. **A late async completion after cap expiry does not overwrite.** Release the + gate after the fallback has published; assert the fallback's result stands and + the superseded writer publishes nothing. + + (Round 1's list omitted all three of these, which is what let the unbounded + fallback look acceptable.) +3. (**coverage, not red-first**) Inject an ordinary non-timeout `icacls` failure: + no stub installed, exactly one write-failure/tombstone, no owned spill or temp + left. Required-mode throws are at + `src/lib/windows-secret-acl.ts:877,881` and the state catch is at + `aec717722:src/responses/state.ts:261-266`. +4. (**coverage, not red-first**) Force link failure into the exclusive-copy + fallback, then fail destination hardening: destination removed, never returned + as a ref (`aec717722:src/responses/spill-store.ts:286-315`). + +Audit round 1 established that 3 and 4 **already pass at the PR head** — the code +is fail-closed there and cleans up. Keep them as coverage for a path no test +touches, but do not present them as red-first proof. Only 1 and 2 are that. + +## Docs + +`structure/02_config-and-codex-home.md` describes the queueing but not shutdown +ordering. Record that graceful shutdown drains publication before the snapshot +flush, and why (the 2 MiB exclusion is what makes it load-bearing). + +## Must not change + +Non-Windows synchronous branches. Required-ACL policy — failure must keep +throwing; swallowing it publishes secrets fail-open. Global serialization of the +tail (parallelizing invites generation/cleanup races). The pending-byte cap and +compare-before-swap identity check. + +## Carried follow-ups (not blockers) + +`src/server/lifecycle.ts:489` does not itself call `process.exit` — the real exits +are `src/server/management-api.ts:280` and `src/cli/index.ts:360,370`. The bypassed +temp cleanup still follows, but attribute it correctly. + +If ACL hardening fails *and* unlink fails, cleanup is best-effort and a full +payload can remain on disk; whether another local user can read it depends on the +resulting NTFS ACL. Predates this PR, needs a real Windows host to settle. File +separately rather than expanding wp3. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/040_wp4_entitlement_diagnostic.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/040_wp4_entitlement_diagnostic.md new file mode 100644 index 0000000000..9e7e4b4223 --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/040_wp4_entitlement_diagnostic.md @@ -0,0 +1,92 @@ +# 040 — wp4: an honest entitlement diagnostic + +Split out of wp2 by audit round 1 (`004`, blocker 4). Stacks on wp2. +Lowest priority in this train — it makes an existing silence visible; it does not +fix a broken behaviour. + +## The problem + +While entitled rows were missing, `GET /api/providers` still reported +`discovery: {"status":"ok"}`. The reporter read that as the proxy lying. + +It is not lying — it is answering a different question. That field is written by +routed-provider discovery (`src/codex/catalog/provider-fetch.ts:1510`, +`src/codex/model-cache.ts:94`) and entitlement resolution never touches it. Routed +discovery genuinely was ok. Nothing anywhere reports on entitlement freshness, so +the operator has no way to tell "this account owns nothing" from "we could not ask". + +## Why it is not wp2's job + +`/api/models` returns a bare array (`src/server/management/model-routes.ts:352-354`) +and both the GUI and `ocx export` depend on that shape +(`gui/src/pages/Models.tsx:402-417`, `src/cli/export-command.ts:169-185`). A +top-level field breaks both consumers; a per-row field stamps one global fact onto +every row. Neither is acceptable, so the transport is a design decision rather than +an implementation detail. + +## Direction + +`/api/providers` already carries `discovery` per provider, so an additive sibling +there is the natural home for canonical OpenAI: same endpoint, same mental model, +no shape change for array consumers. + +States to distinguish (from `002` and the wp1/wp2 work): + +- no logged-in Codex credential +- fresh confirmed roster +- `unconfirmed-empty` — upstream returned no usable rows +- refresh failed (upstream error, timeout) +- expired, refresh in flight + +> Audit round 2 (`005`) removed a state this doc originally listed: "confirmed +> roster that is genuinely empty". wp1 Change 2a makes every empty parsed roster +> **unconfirmed**, so that state is unreachable. +> +> The deeper reason it must not come back: the roster contract has no completeness +> marker, so the system genuinely cannot distinguish "this account owns nothing" +> from "upstream returned an unusable empty answer". Labelling one as the other +> would be a lie in a status field — precisely what `002` criticised +> `discovery: ok` for. `unconfirmed-empty` says only what is known. + +Do **not** overload `discovery`. It would erase a simultaneously-true routed +result and cannot express partial per-account success. GUI types admit only +provider discovery states today (`gui/src/models-groups.ts:2`), so both sides are +additive. + +## Regression + +`tests/management-provider-validation.test.ts` (~`:655`): provider discovery stays +`ok` while the entitlement status independently reports fresh / failed / +unavailable. The point of the test is that the two fields are *independent* — that +is the whole reason this phase exists. + +## Scope guard + +If this grows past an additive field, its GUI type, and one regression, stop and +re-plan. It is a diagnostic, not a subsystem. + +## Dependency + +Needs wp1 and wp2 landed first: the states above only become distinguishable once +wp1 separates unknown from denied and wp2 knows whether a refresh was attempted. + +## Prerequisite: failure provenance (audit round 3 — `006`) + +Verified blocker: parsed-empty and network/timeout failure produce the **identical** +cache entry today. The success path when `parseAccountModels` returns an empty set +(`src/codex/model-entitlements.ts:414`) and the catch path (`:424`) both yield +`{models: new Set(), confirmed: false}`. Nothing downstream can tell them apart. + +So `unconfirmed-empty` and "refresh failed" are one state in the data. Reporting +them as two would be exactly the invented-status-field lie `002` objected to in +`discovery: ok`. + +wp4 therefore requires, in order: + +1. Record provenance on the cache entry — `parsed-empty` / `http-error` / + `timeout` / `unparseable` — as a discriminated field, not a boolean. +2. A regression asserting the two states are genuinely distinct end to end. +3. Only then may the diagnostic name them separately. + +If provenance is not added, wp4 reports a single merged `unconfirmed` state and +says so plainly. An honest coarse answer beats a fabricated precise one. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/050_wp5_tristate_entitlement_authority.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/050_wp5_tristate_entitlement_authority.md new file mode 100644 index 0000000000..b89b245fe9 --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/050_wp5_tristate_entitlement_authority.md @@ -0,0 +1,72 @@ +# 050 — wp5: tri-state entitlement authority + +Split out of wp1 by audit round 2 (`005`, blocker 1). Lowest priority in the train: +it hardens against a future upstream change rather than fixing a reported symptom. + +## What it is for + +#3022 happened because a roster fetched under a too-low `client_version` came back +without the gated rows, and their absence was recorded as a decided denial. wp1 +Change 1 fixes the *current* instance by asking under `0.144.0`. + +It does not make the system robust to the next bump. When upstream raises the +gated minimum to `0.148.0`, the same shape recurs: a version that was correct +yesterday silently produces confirmed negatives. wp5 removes the class. + +## Why it could not ride along in wp1 + +Two structural facts, verified in round 2: + +1. **The answering version is discarded.** `CachedAccountModels` records + `clientVersion` (`src/codex/model-entitlements.ts:230`), but + `resolveCodexModelEntitlements` drops it when building + `CodexModelEntitlementSnapshot` (`:236`, `:547-550`). The projections cannot see + it. +2. **The projections are positive-only.** `entitledCodexAccountIdsForModel` and + `availableAccountGatedNativeModels` answer "which are granted" (`:570`, `:573`). + A third boolean term either narrows redundantly or widens into granting a model + upstream never gave. "Unknown" has no slot. + +So the change is: a new snapshot field, an explicit per-model minimum source, and +three exported projections that admit only `granted` while carrying `unknown` +separately. Every caller of those three is in scope. That is a subsystem, and +smuggling it into a symptom fix is how a fail-closed gate gets accidentally opened. + +## Shape + +1. **Carry the answering version into the snapshot** — a `clientVersionByAccount` + map beside `modelsByAccount`, so a projection can ask "under what question was + this answered?". +2. **An explicit per-model minimum source.** Not the stale snapshot: sol/terra/luna + have a measured `0.144.0`, and `gpt-daybreak-blue-latest` has **no row at all** + (`src/codex/data/upstream-models.json`), so it has no minimum and must keep + omission-as-denial rather than being handed a guess. +3. **Tri-state at the boundary.** `granted` / `denied` / `unknown`. Projections keep + returning only `granted` — that is what preserves fail-closed. `unknown` exists + so it can be *reported* (wp4) and so it takes the 15s failure TTL instead of the + 5-minute success TTL, making recovery prompt. +4. **Positive evidence is never version-tested.** A returned row is a grant no + matter which version asked. Only absence needs a trustworthy question. + +## Regressions + +- A gated slug omitted from a roster fetched **below** its minimum is `unknown`: + not exposed, and not cached as a 5-minute denial. +- The same slug omitted from a roster fetched **at or above** its minimum is + `denied`: the question was capable, so absence is real evidence. +- A gated slug **present** in a roster fetched below the minimum is `granted`. + Guards requirement 4. +- `gpt-daybreak-blue-latest` keeps omission-as-denial at every version. Guards + against the predicate silently un-gating a model with no known minimum. +- No projection ever returns a slug that was not in a roster. The fail-closed + invariant, asserted directly rather than assumed. + +Use a gated slug for every one of these. Round 2 caught the earlier draft asserting +on `gpt-5.5`, which is **not** in `ACCOUNT_GATED_NATIVE_OPENAI_MODELS` +(`src/codex/catalog/native-models.ts:5-10`, ungated list at `:70`) and therefore +never at risk — a vacuous test. + +## Dependency + +After wp1 (shares `model-entitlements.ts`) and ideally after wp4, whose diagnostic +is the natural consumer of `unknown`. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/070_outcome.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/070_outcome.md new file mode 100644 index 0000000000..e257420bb6 --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/070_outcome.md @@ -0,0 +1,65 @@ +# 070 — outcome and receipts + +Filled in as each work-phase closes. Every receipt records the command, the host it +ran on, the exit code, and pass/fail counts. Local full suites are forbidden for +this train, so suite receipts name `lidge`. + +## wp0 — roadmap (docs-only) + +- Status: closing. +- Deliverable: 12 docs — `000` plan, `001`-`003` research, `004`-`006` audit + syntheses, `010`/`020`/`030`/`040`/`050` decade docs, `070` receipts. +- Branch: `codex/prio70-train-260831` at `903243d04`. +- Research: three read-only `gpt-5.6-sol` high-effort lanes. Every load-bearing + claim was re-verified in-tree by the main session before it entered a doc. +- Audit: three adversarial `gpt-5.6-sol` rounds, all FAIL, each one amended rather + than argued with. Round 3 closed the wp1 blocker and positively traced the + reduced wp1 to a fix for #3022. + +### Receipt — wp0 (host `lidge`, Linux x86_64, bun 1.3.14) + +``` +cd ~/ocx-ci/opencodex && git checkout -B verify-prio70 origin/codex/prio70-train-260831 + -> 903243d04, dirty=0 +bun install --frozen-lockfile -> 106 installs / 145 packages, no changes +bun run privacy:scan -> exit 0, "Privacy scan passed" +bun run typecheck -> exit 0 +bun test tests/repo-hygiene.test.ts -> exit 0, 12 pass / 0 fail +``` + +No full suite was run locally, per the standing constraint. `repo-hygiene` is the +focused file that actually covers a `devlog/` change (tracked-devlog and +no-gitlink assertions), so it is the right narrow check for a docs-only phase. + +### What the audits changed + +Recording this because the diff between the first draft and the landed roadmap is +the real output of wp0: + +- **wp1 shrank.** The draft would have applied model-scoped doubt as an + account-wide denial, hiding models the account owns. Now Change 1 (measured + `0.144.0` floor) plus Change 2a (empty roster is not a confirmation) only. +- **wp3 inverted.** It began as "review and merge #3018". The audit found the PR + leaves a shutdown-loss window, so wp3 is now "land a bounded drain, then merge", + and the option to abandon an outstanding job was withdrawn once round 3 showed + `dev` publishes those candidates synchronously today. +- **Two phases were born from blockers.** wp4 (diagnostic transport) split out of + wp2; wp5 (tri-state authority) split out of wp1. +- **Three vacuous or wrong test plans were caught before implementation:** a + `gpt-5.5` assertion on a model that is not account-gated, two wp3 cases that + already pass at the PR head, and a wp4 state that cannot occur. + +## wp1 — #3022 entitlement floor + empty roster + +- Status: pending. +- Receipt: _pending_ + +## wp2 — #3023 roster TTL refresh + +- Status: pending. +- Receipt: _pending_ + +## wp3 — #3011 spill publication drain + +- Status: pending. +- Receipt: _pending_ diff --git a/src/codex/model-entitlements.ts b/src/codex/model-entitlements.ts index 6dd58c172c..f462b9ba53 100644 --- a/src/codex/model-entitlements.ts +++ b/src/codex/model-entitlements.ts @@ -70,6 +70,23 @@ export function deriveGatedClientVersionFloor( ); } +/** + * Lowest `client_version` MEASURED to actually return the account-gated rows. + * + * The bundled snapshot is not sufficient on its own. It records `0.142.2` for the gpt-5.6 + * rows, and `0.142.2` is a version upstream answers with 200 and five models, none of them + * gpt-5.6; `0.144.0` and above answer with the gated rows present + * (devlog/_fin/260817_native_gpt56_1m_context/001_measurement_evidence.md, independently + * reproduced by the #2886 and #3022 reporters). So a floor derived from the snapshot alone + * asks a question whose honest answer is an empty gated set — and the fail-closed gate then + * reads that absence as a confirmed denial, which is how 2.36.0 removed sol/terra/luna from + * accounts that own them (#3022). + * + * This is a measurement, not a preference, which is why it composes with the derivation + * instead of replacing it: see `composeGatedClientVersionFloor`. + */ +const MEASURED_GATED_CLIENT_VERSION_MINIMUM = "0.144.0"; + /** * Fallback when the snapshot records no usable gated floor. * @@ -80,10 +97,41 @@ export function deriveGatedClientVersionFloor( */ const GATED_MODEL_CLIENT_VERSION_FLOOR_FALLBACK = "0.142.2"; -export const GATED_MODEL_CLIENT_VERSION_FLOOR: string = - deriveGatedClientVersionFloor( - (upstreamModelsSnapshot as { models?: Array> }).models ?? [], - ) ?? GATED_MODEL_CLIENT_VERSION_FLOOR_FALLBACK; +/** + * The floor actually used: the highest of what the snapshot derives, what we have measured + * upstream to honour, and the fallback. + * + * Composed rather than hardcoded so the two sources cannot drift into a contradiction. The + * snapshot may raise the floor; it may never lower it below a measurement. When a future + * snapshot refresh records `0.144.0` or higher, the derivation takes over naturally and + * `MEASURED_GATED_CLIENT_VERSION_MINIMUM` goes inert instead of fighting it. + */ +function composeGatedClientVersionFloor( + rows: ReadonlyArray>, + gatedSlugs: ReadonlySet = ACCOUNT_GATED_NATIVE_OPENAI_MODELS, +): string { + const derived = deriveGatedClientVersionFloor(rows, gatedSlugs) ?? GATED_MODEL_CLIENT_VERSION_FLOOR_FALLBACK; + return compareClientVersions(derived, MEASURED_GATED_CLIENT_VERSION_MINIMUM) >= 0 + ? derived + : MEASURED_GATED_CLIENT_VERSION_MINIMUM; +} + +export const GATED_MODEL_CLIENT_VERSION_FLOOR: string = composeGatedClientVersionFloor( + (upstreamModelsSnapshot as { models?: Array> }).models ?? [], +); + +/** Test-only seam: the composition on synthetic rows, so both directions can be proven. */ +export function composeGatedClientVersionFloorForTests( + rows: ReadonlyArray>, + gatedSlugs?: ReadonlySet, +): string { + return composeGatedClientVersionFloor(rows, gatedSlugs); +} + +/** Test-only seam: the ordering the floor composition relies on. */ +export function compareClientVersionsForTests(left: string, right: string): number { + return compareClientVersions(left, right); +} /** Numeric-segment comparison. Only used to pick the highest floor in a known-good set. */ function compareClientVersions(left: string, right: string): number { @@ -414,12 +462,20 @@ async function fetchAccountModels( const models = response.ok && body.displaySafe && !body.truncated ? parseAccountModels(body.text) : null; + // A roster is a confirmation only when it lists something usable. `models` is a Set, and an + // empty Set is truthy, so `models !== null` used to call `{"models":[]}` — and a response + // whose every row was hidden or api-disabled — a confirmed answer, and lock it in for the + // five-minute success TTL. Absence of evidence is not evidence of absence: an entitled + // account asked under too old a client version answers with no gated rows, and treating + // that as authoritative is exactly how 2.36.0 denied sol/terra/luna to accounts that own + // them (#3022). No usable rows means unconfirmed, on the 15s failure TTL, asked again. + const usable = models !== null && models.size > 0; return { credentialIdentity: credential.credentialIdentity, clientVersion, - expiresAt: now + (models ? MODEL_ROSTER_TTL_MS : MODEL_ROSTER_FAILURE_TTL_MS), + expiresAt: now + (usable ? MODEL_ROSTER_TTL_MS : MODEL_ROSTER_FAILURE_TTL_MS), models: models ?? new Set(), - confirmed: models !== null, + confirmed: usable, }; } catch { return { diff --git a/tests/claude-models-discovery.test.ts b/tests/claude-models-discovery.test.ts index 3729ab76c9..ec9758986b 100644 --- a/tests/claude-models-discovery.test.ts +++ b/tests/claude-models-discovery.test.ts @@ -562,3 +562,68 @@ test("the request's client_version reaches entitlement discovery (#2886)", async if (server) await server.stop(true); } }); + +test("with no inbound or runtime version, /v1/models still exposes the gated rows (#3022)", async () => { + // The #3022 path has no client to speak for it: background discovery on a host where the + // Codex runtime has never been resolved falls through to tier 3, the build's own gated floor. + // 2.36.0 derived that floor from the bundled snapshot (0.142.2) and upstream answers 0.142.2 + // with no gpt-5.6 at all, so entitled accounts were classified as denying sol/terra/luna. + // + // The backend here is deliberately VERSION-SENSITIVE. A mock that answers the same roster for + // every version — like the no-inbound case earlier in this file — is green on both sides of + // the fix and proves nothing. This one returns the gated rows only at >= 0.144.0, which is + // what real upstream was measured to do. + const config = configWithStaticModels(); + config.providers.openai = { + adapter: "openai-responses", + baseUrl: "https://chatgpt.com/backend-api/codex", + liveModels: false, + }; + saveConfig(config); + writeFileSync(join(isolatedCodexHome!.path, "auth.json"), JSON.stringify({ + tokens: { access_token: "main-token", account_id: "main-account" }, + }), "utf8"); + + const { resetCatalogRuntimeStateForTests } = await import("../src/codex/catalog"); + const { resetCodexModelEntitlementCacheForTests } = await import("../src/codex/model-entitlements"); + resetCatalogRuntimeStateForTests(); + resetCodexModelEntitlementCacheForTests(); + + const askedVersions: string[] = []; + const originalFetch = globalThis.fetch; + globalThis.fetch = (async (input, init) => { + const url = new URL(typeof input === "string" ? input : input instanceof URL ? input : input.url); + if (url.hostname === "chatgpt.com" && url.pathname.endsWith("/models")) { + const version = url.searchParams.get("client_version") ?? ""; + askedVersions.push(version); + const minor = Number(version.split(".")[1] ?? "0"); + const gated = minor >= 144 + ? ["gpt-5.5", "gpt-5.6-sol", "gpt-5.6-terra", "gpt-5.6-luna"] + : ["gpt-5.5"]; + return Response.json({ + models: gated.map(slug => ({ slug, supported_in_api: true, visibility: "list" })), + }); + } + return originalFetch(input, init); + }) as typeof fetch; + + let server: ReturnType | null = null; + try { + server = startServer(0); + // No client_version on the request, and no persisted runtime in this isolated home. + const catalog = await fetch(new URL("/v1/models", server.url)) + .then(response => response.json()) as { data: Array<{ id: string }> }; + + expect(askedVersions.length).toBeGreaterThan(0); + // Never the placeholder, and never a version upstream answers without the gated rows. + expect(askedVersions).not.toContain("0.0.0"); + for (const version of askedVersions) { + expect(Number(version.split(".")[1] ?? "0")).toBeGreaterThanOrEqual(144); + } + // And the rows the account actually owns reach the surface. + expect(catalog.data.some(model => model.id === "gpt-5.6-sol")).toBe(true); + } finally { + globalThis.fetch = originalFetch; + if (server) await server.stop(true); + } +}); diff --git a/tests/codex-model-entitlements.test.ts b/tests/codex-model-entitlements.test.ts index 832d8d3049..fddd7051da 100644 --- a/tests/codex-model-entitlements.test.ts +++ b/tests/codex-model-entitlements.test.ts @@ -2,6 +2,8 @@ import { beforeEach, describe, expect, test } from "bun:test"; import { availableAccountGatedNativeModels, cachedAvailableAccountGatedNativeModels, + composeGatedClientVersionFloorForTests, + compareClientVersionsForTests, deriveGatedClientVersionFloor, entitledCodexAccountIdsForModel, GATED_MODEL_CLIENT_VERSION_FLOOR, @@ -76,7 +78,7 @@ describe("Codex account model entitlements", () => { expect(availableAccountGatedNativeModels(snapshot).size).toBe(0); }); - test("ignores hidden or API-disabled rows", async () => { + test("ignores hidden or API-disabled rows, and does not call the result a confirmation", async () => { const snapshot = await resolveCodexModelEntitlements({ codexAccounts: [] }, { credentials: [credential("main")], fetcher: (async () => Response.json({ models: [ @@ -87,7 +89,11 @@ describe("Codex account model entitlements", () => { clientVersion: TEST_CLIENT_VERSION, }); - expect(snapshot.confirmedAccountIds.has("main")).toBe(true); + // INTENTIONAL ASSERTION FLIP (#3022). This used to assert `true`: rows arrived, so the + // parse "succeeded". But every row was filtered out, so the account proved nothing, and + // calling that a confirmation locked an empty roster in for the five-minute success TTL. + // Confirmation means usable evidence, not a successful HTTP round trip. + expect(snapshot.confirmedAccountIds.has("main")).toBe(false); expect(entitledCodexAccountIdsForModel(snapshot, DAYBREAK)?.size).toBe(0); }); @@ -235,15 +241,16 @@ describe("entitlement client version (#2886)", () => { const seen: string[] = []; const snapshot = await resolveCodexModelEntitlements({ codexAccounts: [] }, { credentials: [credential("main")], - // Gates exactly at the version the bundled snapshot declares for the gated models, so - // this asserts the floor is *sufficient* to return them rather than re-testing the - // arbitrary threshold the other backend uses. + // Gates at the version MEASURED upstream to actually return the gated rows, not at the + // version the bundled snapshot happens to declare. This mock used to gate at minor >= 142, + // which is why the suite never caught #3022: the derived floor was 0.142.2, the mock + // accepted it, and the test stayed green while real upstream answered with no gpt-5.6. fetcher: (async (input: RequestInfo | URL) => { const url = new URL(input instanceof Request ? input.url : String(input)); const version = url.searchParams.get("client_version") ?? ""; seen.push(version); const minor = Number(version.split(".")[1] ?? "0"); - return minor >= 142 ? roster("gpt-5.5", SOL, TERRA, LUNA) : roster("gpt-5.5"); + return minor >= 144 ? roster("gpt-5.5", SOL, TERRA, LUNA) : roster("gpt-5.5"); }) as typeof fetch, now: 1_000, clientVersion: null, @@ -626,4 +633,119 @@ describe("entitlement client version (#2886)", () => { expect(await ask("tok-first", "0.146.0")).toBe(true); expect(fetches).toBe(before); }); + + test("the gated floor never falls below the measured upstream minimum (#3022)", () => { + // 2.36.0 regressed exactly here: the floor is DERIVED from the bundled snapshot, and the + // snapshot records 0.142.2 for the gpt-5.6 rows. Upstream does not return those rows until + // 0.144.0 (devlog/_fin/260817_native_gpt56_1m_context/001_measurement_evidence.md: 0.142.2 + // answers 200 with five rows and no gpt-5.6; >= 0.144.0 answers with eight including them, + // independently reproduced by the #2886 and #3022 reporters). So background sync asked a + // question upstream answers with an empty gated set, and the fail-closed gate read that as + // a confirmed denial — entitled Plus accounts lost sol/terra/luna. + expect(compareClientVersionsForTests(GATED_MODEL_CLIENT_VERSION_FLOOR, "0.144.0")) + .toBeGreaterThanOrEqual(0); + }); + + test("the floor is the higher of the derived and the measured minimum, not either alone", () => { + // Tested as a COMPOSITION on synthetic inputs. Hardcoding 0.144.0 would satisfy the test + // above while destroying the property that matters next: a refreshed snapshot declaring a + // NEWER requirement must take over, and the measured constant must then go inert rather + // than holding the floor down. Both directions are asserted here because only one of them + // is exercised by the shipped data. + const gated = new Set(["a"]); + const compose = (rows: Array>) => + composeGatedClientVersionFloorForTests(rows, gated); + + // Snapshot below the measurement: the measurement wins. This is today's shipped state. + expect(compose([{ slug: "a", minimal_client_version: "0.142.2" }])).toBe("0.144.0"); + // Snapshot above the measurement: the snapshot wins, and the constant is inert. + expect(compose([{ slug: "a", minimal_client_version: "0.151.0" }])).toBe("0.151.0"); + // Equal: either answer is the same value. + expect(compose([{ slug: "a", minimal_client_version: "0.144.0" }])).toBe("0.144.0"); + // Derivation empty — no gated row carries a usable version — still never below measured. + expect(compose([])).toBe("0.144.0"); + expect(compose([{ slug: "a", minimal_client_version: "0.0.0" }])).toBe("0.144.0"); + // Numeric, not lexicographic: "0.99.0" must not beat "0.144.0". + expect(compose([{ slug: "a", minimal_client_version: "0.99.0" }])).toBe("0.144.0"); + }); + + test("an empty roster is not a confirmation, and is retried on the failure TTL (#3022)", async () => { + // `{"models":[]}` parses to an empty Set, and an empty Set is truthy — so the old + // expression `confirmed: models !== null` called it a confirmed answer and locked it in for + // the full five-minute success TTL. An empty roster is absence of evidence, not evidence of + // absence, and it must expire on the 15s failure TTL instead. + // + // Driven through the DIRECT caller path deliberately: a completed flight only writes to the + // cache when `currentCredentialIdentity` matches the snapshot identity, and a synthetic + // pool credential never satisfies that guard — so a test built on `credential()` would + // measure an uncached path and prove nothing about the TTL. + let fetches = 0; + const empty = (async () => { fetches += 1; return Response.json({ models: [] }); }) as typeof fetch; + + const ask = (now: number) => isDirectCallerEntitledToCodexModel( + directHeaders("tok-empty"), + SOL, + { fetcher: empty, now, clientVersion: "0.146.0" }, + ); + + expect(await ask(1_000)).toBe(false); + expect(fetches).toBe(1); + + // Still inside the 15s failure window: served from the cached unconfirmed entry. + expect(await ask(1_000 + 14_999)).toBe(false); + expect(fetches).toBe(1); + + // Past the failure TTL: exactly one refetch. Under the old five-minute success TTL this + // stayed at 1 until 300,001 ms, which is the wrong answer held for twenty times too long. + expect(await ask(1_000 + 15_001)).toBe(false); + expect(fetches).toBe(2); + }); + + test("a non-empty roster still confirms the account", async () => { + // Characterization guard, green before and after: only the EMPTY case changes. An ordinary + // short roster must keep confirming the account and keep granting what it lists, otherwise + // the empty-roster fix would have widened into a denial of service for everyone. + let fetches = 0; + const backend = (async () => { fetches += 1; return roster(SOL); }) as typeof fetch; + const ask = (now: number) => isDirectCallerEntitledToCodexModel( + directHeaders("tok-nonempty"), + SOL, + { fetcher: backend, now, clientVersion: "0.146.0" }, + ); + + expect(await ask(1_000)).toBe(true); + // And it keeps the five-minute success TTL: no refetch just past the failure window. + expect(await ask(1_000 + 15_001)).toBe(true); + expect(fetches).toBe(1); + }); + + test("an all-filtered roster is unconfirmed and retried on the failure TTL", async () => { + // Rows arrived, but every one was hidden or api-disabled, so the parse yields an empty set. + // Same situation as a zero-row response: no usable evidence. Every gated projection needs + // both confirmation and membership, so an empty set denies identically either way — which + // is exactly why calling it "confirmed" buys nothing and costs a five-minute wrong answer. + let fetches = 0; + const filtered = (async () => { + fetches += 1; + return Response.json({ models: [ + { slug: SOL, supported_in_api: true, visibility: "hide" }, + { slug: "gpt-disabled", supported_in_api: false, visibility: "list" }, + ] }); + }) as typeof fetch; + + const ask = (now: number) => isDirectCallerEntitledToCodexModel( + directHeaders("tok-filtered"), + SOL, + { fetcher: filtered, now, clientVersion: "0.146.0" }, + ); + + expect(await ask(1_000)).toBe(false); + expect(fetches).toBe(1); + expect(await ask(1_000 + 14_999)).toBe(false); + expect(fetches).toBe(1); + // The TTL half is asserted separately from the flag: flipping `confirmed` while leaving the + // success TTL in place would pass an assertion about the flag alone. + expect(await ask(1_000 + 15_001)).toBe(false); + expect(fetches).toBe(2); + }); });