-
Notifications
You must be signed in to change notification settings - Fork 957
fix(codex): restore GPT-5.6 for entitled accounts by raising the gated client-version floor #3035
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9e79dfe
6036881
52922f4
1d600a2
83cac9e
1b6b36b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 <file>` 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. | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -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`): | ||||||||||||||||||||||||||||||
|
Comment on lines
+12
to
+17
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
git show 870a2adb6:src/codex/model-entitlements.ts \
| nl -ba \
| rg 'resolveCodexEntitlementClientVersion|parseAccountModels|fetchAccountModels|cachedAvailableAccountGatedNativeModels'Repository: lidge-jun/opencodex Length of output: 193 🏁 Script executed: set -euo pipefail
doc='devlog/_plan/260831_prio70_entitlement_and_spill_train/001_research_3022_entitlement_floor.md'
src='src/codex/model-entitlements.ts'
commit='870a2adb6'
printf '%s\n' '--- document references ---'
grep -n -C 3 'model-entitlements.ts\|870a2adb6\|resolveCodexEntitlementClientVersion' "$doc" || true
printf '%s\n' '--- pinned source symbols ---'
git show "$commit:$src" | awk '
/resolveCodexEntitlementClientVersion|parseAccountModels|fetchAccountModels|cachedAvailableAccountGatedNativeModels/ {
start = NR - 2
if (start < 1) start = 1
print start ":" NR ":" $0
}
'
printf '%s\n' '--- pinned source ranges around matching symbols ---'
git show "$commit:$src" | grep -n -E 'resolveCodexEntitlementClientVersion|parseAccountModels|fetchAccountModels|cachedAvailableAccountGatedNativeModels|GATED_MODEL_CLIENT_VERSION_FLOOR|selectedVersion' || trueRepository: lidge-jun/opencodex Length of output: 3794 🏁 Script executed: set -euo pipefail
commit='870a2adb6'
src='src/codex/model-entitlements.ts'
doc='devlog/_plan/260831_prio70_entitlement_and_spill_train/001_research_3022_entitlement_floor.md'
printf '%s\n' '--- pinned source: floor and resolver ---'
git show "$commit:$src" | awk '
NR >= 50 && NR <= 90 || NR >= 120 && NR <= 140 { print NR ":" $0 }
'
printf '%s\n' '--- pinned source: roster parser and fetch cache ---'
git show "$commit:$src" | awk '
NR >= 365 && NR <= 425 { print NR ":" $0 }
'
printf '%s\n' '--- all source references in document ---'
grep -n -o -E 'src/codex/model-entitlements\.ts(:[0-9]+)?' "$doc" || trueRepository: lidge-jun/opencodex Length of output: 6311 Correct the roster confirmation references. At commit 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||||||
| 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=<v>` 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. | ||||||||||||||||||||||||||||||
|
Comment on lines
+102
to
+108
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Limit the minimum-version rule to the background tier. Lines 12-14 preserve inbound and runtime versions. Lines 110-117 also state that older-client behavior must remain unchanged. The requirement in Lines 102-108 is broader and can be read as invalidating any roster fetched below State that the measured minimum applies only when the background fallback tier is selected. Proposed clarification-3. A roster fetched under a version below the trustworthy minimum must not make
- omission authoritative for gated slugs.
+3. A roster fetched by the background fallback tier below the trustworthy minimum
+ must not make omission authoritative for gated slugs.📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ## 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. | ||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
Comment on lines
+58
to
+60
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- repository guidance ---'
head -5 /tmp/coderabbit-repo-knowledge/lidge-jun-opencodex-7afea732/*/*.md 2>/dev/null || true
printf '%s\n' '--- target file ---'
cat -n devlog/_plan/260831_prio70_entitlement_and_spill_train/002_research_3023_roster_ttl.md | sed -n '1,115p'
printf '%s\n' '--- related architecture and learnings filenames ---'
find /tmp/coderabbit-repo-knowledge/lidge-jun-opencodex-7afea732/{architecture,learnings} -maxdepth 1 -type f -print 2>/dev/null | sortRepository: lidge-jun/opencodex Length of output: 13058 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- entitlement state and refresh implementation ---'
rg -n -C 5 'confirmed-empty|confirmed: false|confirmed: true|expiresAt|resolveCodexModelEntitlements|inFlight|TTL|ttl' src/codex/model-entitlements.ts
printf '%s\n' '--- synchronous projection ---'
sed -n '395,430p' src/codex/catalog/metadata.ts
sed -n '580,610p' src/codex/model-entitlements.ts
printf '%s\n' '--- focused tests and plan references ---'
rg -n -C 4 'expired|unconfirmed|empty|TTL|sol|terra|luna|ensure|refresh' tests/codex-model-entitlements.test.ts tests/native-model-toggle.test.ts tests/management-client-config-route.test.ts tests/cli-export-command.test.ts devlog/_plan/260831_prio70_entitlement_and_spill_train/002_research_3023_roster_ttl.md 2>/dev/null || trueRepository: lidge-jun/opencodex Length of output: 50375 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- cache lookup and write path ---'
sed -n '250,290p' src/codex/model-entitlements.ts
sed -n '500,565p' src/codex/model-entitlements.ts
printf '%s\n' '--- empty and filtered roster tests ---'
sed -n '672,760p' tests/codex-model-entitlements.test.ts
printf '%s\n' '--- management regression test area ---'
sed -n '1,130p' tests/native-model-toggle.test.ts
rg -n -C 8 'expired|/api/models|gpt-5\.6|entitlement' tests/native-model-toggle.test.ts tests/management-client-config-route.test.ts tests/cli-export-command.test.tsRepository: lidge-jun/opencodex Length of output: 50375 Define The implementation has no At Lines 84-90, require the management tests to prove that an expired roster is exposed only after a successful non-empty refresh. Also assert that a failed, timed-out, or unavailable refresh omits 🤖 Prompt for AI Agents |
||
| (`: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. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<void> = 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a valid frozen-scope timestamp.
2026-08-31T01:1x KSTis not a parseable timestamp. The scope-freeze record cannot be reproduced from this value. Replace1xwith the exact minute and use an unambiguous timezone format.🤖 Prompt for AI Agents