From 1b6e1acb6fc5a47425506da2757815006261b485 Mon Sep 17 00:00:00 2001 From: JUN Date: Thu, 17 Sep 2026 17:49:54 +0900 Subject: [PATCH 1/3] docs(devlog): plan the L1 preview read fence and dependency audit lane Records the roadmap for two independent safety units: closing pool eligibility inside the caller-owned preview read fence (#4850), and reviewing the existing dependency audit override PR (#4873). --- .../000_master_plan.md | 37 +++++++ ...sue_4850_preview_pool_eligibility_fence.md | 94 ++++++++++++++++++ .../020_pr_4873_dependency_audit_review.md | 97 +++++++++++++++++++ 3 files changed, 228 insertions(+) create mode 100644 devlog/_plan/260917_l1_preview_read_fence_and_dep_audit/000_master_plan.md create mode 100644 devlog/_plan/260917_l1_preview_read_fence_and_dep_audit/010_issue_4850_preview_pool_eligibility_fence.md create mode 100644 devlog/_plan/260917_l1_preview_read_fence_and_dep_audit/020_pr_4873_dependency_audit_review.md diff --git a/devlog/_plan/260917_l1_preview_read_fence_and_dep_audit/000_master_plan.md b/devlog/_plan/260917_l1_preview_read_fence_and_dep_audit/000_master_plan.md new file mode 100644 index 0000000000..4ff428b50f --- /dev/null +++ b/devlog/_plan/260917_l1_preview_read_fence_and_dep_audit/000_master_plan.md @@ -0,0 +1,37 @@ +# L1 — preview read fence and dependency audit + +Two independent safety units that deliberately do not stack on any feature branch. +Each ends as its own pull request against `dev` with exact-head CI evidence. + +| Unit | Subject | Artifact | +|---|---|---| +| A | Issue #4850 — caller-owned `thread_spawn` preview still reads physical main `auth.json` | `010_issue_4850_preview_pool_eligibility_fence.md` | +| B | PR #4873 — dependency audit overrides for `hono` and the docs-site toolchain | `020_pr_4873_dependency_audit_review.md` | + +## Why they are separate + +Unit A changes runtime credential-boundary behaviour in `src/codex/` and +`src/server/responses/`. Unit B changes only `package.json` and lockfiles and is +authored by an outside contributor. Putting them on one branch would make the +contributor's commit un-landable on its own and would drag a credential-boundary +review into a dependency bump. + +## Verification posture + +No local suite, typecheck, build, or install runs in this lane. Correctness is +argued statically from the source and the call graph, and confirmed by hosted CI +at the exact head of each pull request. That constraint is why unit A's completion +criteria are written as observable read counts rather than as "the right token was +eventually sent": a behavioural assertion that hosted CI can run is the only proof +available here, and it is the stronger one anyway. + +## Boundaries + +This lane does not merge, does not push to `dev`, and does not rebase without an +instruction. It does not widen timeouts, add retries, skip platforms, or mask a +failure to make CI green. Windows jobs are dispatch-only, so any change with +Windows impact is reported rather than dispatched here. + +Unreleased security analysis belongs in `.tmp/`, never in this directory. +Both units here concern already-public material: #4850 is a filed public issue +with the call path in its body, and #4873's advisories are published GHSA records. diff --git a/devlog/_plan/260917_l1_preview_read_fence_and_dep_audit/010_issue_4850_preview_pool_eligibility_fence.md b/devlog/_plan/260917_l1_preview_read_fence_and_dep_audit/010_issue_4850_preview_pool_eligibility_fence.md new file mode 100644 index 0000000000..1c929a4f80 --- /dev/null +++ b/devlog/_plan/260917_l1_preview_read_fence_and_dep_audit/010_issue_4850_preview_pool_eligibility_fence.md @@ -0,0 +1,94 @@ +# Unit A — issue #4850: pool eligibility is outside the preview read fence + +## The gap, stated precisely + +`src/server/responses/request-prepare.ts` already computes the ownership fence. +`previewRequestScopedMainCredential` is the route ownership predicate ANDed with +`hasCallerCodexBearer`, exactly as final authentication validates it, and +`nativeMainReadsForbidden` ORs it with retained recovery and a draining selector. +Quota priming, entitlement discovery, and the denied-model cache all honour it. + +`previewSelectionOptions` does not. It carries `nativeMainSelectionOnly` and the +uploaded-file retention bit and stops there. When that object reaches +`previewCodexAccountForRequest -> pickPriorityPreemption -> getEligiblePoolAccounts`, +`codexAccountUnusableReason` in `src/codex/account-usability.ts` finds no +`isMainAccountTokenLive` override and falls through to its default, +`isMainAccountCredentialUsable()`, which opens and parses the physical `auth.json`. + +It happens twice because the same options object is used twice: once for the direct +preview in `prepareResponsesRequest`, and once inside the callback +`applySubagentModelFallback` invokes per candidate model. The post-decryption +recovery re-preview builds `recoverySelectionOptions` the same way and has the same +omission. + +## What is and is not at stake + +Not a token leak. Final authentication never selects the physical main credential +for a caller-owned request: it passes `isMainAccountTokenLive: () => preserveRequestOwnedMainPin` +into its own selection options, so main is either served as the caller's own +credential or scored `main_credential_unavailable` and dropped. ADR-0086 already +rejected reading the physical main token for identity. + +What is at stake is that operator-main liveness, cached quota, and plan state can +enter the score that decides whether a subagent's model is rewritten, for a request +that owns its credential. A preview that scores main differently from the resolution +it exists to predict is a correctness defect on top of the boundary defect. + +## Chosen direction + +Use the existing `CodexAccountUsabilityOptions.isMainAccountTokenLive` seam, and +give it the same value final authentication gives it rather than a preview-only +constant. + +That answers the open question in the issue review directly. `preserveRequestOwnedMainPin` +is not an arbitrary choice: it is the only value that makes the preview agree with +the resolution in both branches. When the operator has an effective manual main pin +with quota headroom, final authentication returns the caller-owned main context, so +the request really is served by main and the preview should score main eligible. +When there is no such pin, final authentication drops main from pool eligibility, +and the preview must drop it too. A hardcoded `true` would be wrong in the second +case, and a hardcoded `false` would be wrong in the first. + +Every input to that predicate is config, policy, or in-memory runtime state — +`activeCodexAccountPinned`, `isEffectiveCodexAccountPinned`, `pausedCodexAccountIds`, +the in-memory quota score, and `matchesMainQuotaCredential`, which compares HMACs +against an observed-credential record held in `main-account-cache.ts`. Nothing in +it opens a file, which is what makes it usable on the fenced side. + +To keep preview and final authentication from drifting apart again, the predicate +moves into one exported function in `src/codex/auth-context.ts` that both callers +use. Two copies of a fence is how this gap appeared in the first place. + +## Edit set + +| File | Change | +|---|---| +| `src/codex/auth-context.ts` | Extract `requestOwnedMainPinState` and call it from `resolveCodexAuthContext` | +| `src/server/responses/request-prepare.ts` | Pass the synthetic `isMainAccountTokenLive` in `previewSelectionOptions` and `recoverySelectionOptions`, scoped to the ownership flag | +| `tests/responses/responses-preview-main-read-fence.test.ts` | Assertions (a) and (b) below | + +No new test file, so `layout.json` and `test-layout-expected.json` are untouched. +No file here is on the size-ratchet baseline. No `src/` area is created or removed +and no invariant test disappears, so `structure:check` has nothing to consume. + +## Completion criteria + +Deliberately stricter than "the right token was eventually sent", because that was +already true before the fix and the defect survived it anyway. + +(a) A caller-owned `thread_spawn` performs **zero** `auth.json` reads across the +whole request, asserted on the unfiltered read counter rather than through the +denial-cache stack filter that currently hides these two reads. + +(b) Ordinary main selection is unchanged. A request with no caller bearer still +reads the physical credential and still selects main when it is healthy, so the +fix cannot be satisfied by making main globally ineligible. + +(c) The #3166 healthy main-pin behaviour survives: a caller-owned request under an +effective main pin is still previewed as main. + +## Risk + +The behaviour change is confined to requests where `previewRequestScopedMainCredential` +is true. For every other request the option is absent and `account-usability.ts` +takes the identical default branch it takes today. diff --git a/devlog/_plan/260917_l1_preview_read_fence_and_dep_audit/020_pr_4873_dependency_audit_review.md b/devlog/_plan/260917_l1_preview_read_fence_and_dep_audit/020_pr_4873_dependency_audit_review.md new file mode 100644 index 0000000000..ca73bc1e70 --- /dev/null +++ b/devlog/_plan/260917_l1_preview_read_fence_and_dep_audit/020_pr_4873_dependency_audit_review.md @@ -0,0 +1,97 @@ +# Unit B — PR #4873: dependency audit overrides + +Contributor PR from `agentHits`, head `7c9479b5722e3f0af56a73410ca6a74fd18905b8`, +base `dev`, fork `agentHits/opencodex`, branch `fix/security-audit-overrides-hono-astro`. +Four files: `package.json`, `bun.lock`, `docs-site/package.json`, `docs-site/bun.lock`. +No application source changes. + +This unit reviews and clears the existing pull request. It does not open a +replacement, and it does not merge. + +## The follow-up commit landed + +The review asked for an exact pin instead of a caret. Head `7c9479b57` has +`"hono": "4.13.8"` in root `overrides`, the caret removed, matching the +neighbouring exact pin on `@hono/node-server`. Root `bun.lock` carries the same +`4.13.8` in its overrides block and resolves `hono@4.13.8`. Manifest and lock agree. + +## Actual impact, not advisory severity + +The PR description groups the `hono` advisories under "Root proxy runtime". That is +accurate about which manifest changed and misleading about what is exposed. + +`hono` is not a direct dependency. It arrives only through +`@modelcontextprotocol/sdk@1.30.0`, which declares `hono: ^4.11.4`. This repository +imports that SDK in exactly one file, `src/adapters/cursor/mcp-manager.ts`, and only +its **client** entrypoints: `client/index.js`, `client/stdio.js`, and +`client/streamableHttp.js`. Nothing under `src/`, `gui/src/`, or `scripts/` imports +`@modelcontextprotocol/sdk/server/*` or `@hono/node-server`. + +All three `hono` advisories need the application to be running hono as a server: +`toSSG()` is the static-site generation helper, `parseBody()` parses an inbound +request body, and the query-parser differential is about inbound request URLs. The +proxy serves its own HTTP through `Bun.serve`. So no proxy request path reaches the +vulnerable code, and this half of the PR is dependency-graph hygiene that gets +`bun audit` to zero rather than a fix for a reachable proxy vulnerability. + +The Critical is in the other half. `GHSA-26w7-cxv4-gfx2` is remote code execution +through Astro's AVIF image optimization, which runs during `astro build` and +`astro dev`. The exposed parties are contributor machines and the docs deploy +runner, and the input is images in the repository, so an attack needs a malicious +image committed first. Bounded, real, and worth fixing. + +## Lockfile review + +Reviewed statically; no install runs in this lane. + +Every added `docs-site/bun.lock` entry is a registry package with a `sha512` +integrity hash. No `git+`, `http(s):`, `file:`, `workspace:`, or `link:` source +appears in any added line. The additions are exactly what an Astro 7.2.2 to 7.3.3 +minor bump plus the `sharp`, `svgo`, `smol-toml` and `js-yaml` overrides produce: +refreshed `@astrojs/compiler-binding-*` and `@img/sharp-*` platform binaries, and +the transitive dependencies those versions declare. + +Two things that look like new supply chain but are not. `@astrojs/markdown-satteri` +and the `@bruits/satteri-*` binaries are already in `dev`'s lockfile and only change +version. `find-proc` replaces `find-process`, dropping `ansi-styles`, `chalk`, +`color-convert`, `color-name`, and `loglevel`; that substitution is declared by +`astro@7.3.3` itself, not introduced by this pull request. + +## The docs build is not covered by CI + +This is the part worth separating out, and it does not resolve in this PR's favour. + +`.github/workflows/ci.yml` contains no `docs-site` reference and builds no docs. +`deploy-docs.yml` triggers only on `push` to `main` under `docs-site/**`. So the +Astro minor bump has no pull-request build gate anywhere: a fully green exact-head +run on this PR is not evidence that the docs site still builds. The author's local +"449 pages" result is the only build evidence and is an unverifiable attestation. + +The residual exposure is a broken docs build discovered at promotion to `main` +rather than at review. That fails the deploy instead of shipping a broken site, so +it is a delay rather than an outage, but it should be a conscious acceptance. +Adding a docs-build job is out of this lane's scope. + +What CI *does* cover: `package.json` and `bun.lock` are both in the `changes` job's +`ci` allowlist, so the cross-platform suite is in scope for this head once it runs. + +## What blocks exact-head evidence + +Two independent gates, both maintainer actions, neither of which the contributor can +clear: + +1. **`unsponsored_surface`.** `hygiene` and `enforce-target` both fail on it, and + the PR carries `intake: hygiene-blocked`. `MAINTAINERS.md` requires explicit + security review for dependency-installation surfaces; the gate wants a + `maintainer-sponsored` label recording that the review happened. +2. **Fork workflow approval.** `Cross-platform CI`, `React Doctor`, and + `Service lifecycle` are all sitting at `action_required` for this head. The + repository uses `all_external_contributors` approval, and `ci.yml` documents that + this approval — not the workflow's own routing — is the real boundary keeping + untrusted code off runners. For a `pull_request` event `select-windows-runner` + marks the run untrusted and pins GitHub-hosted runners, so approving does not + expose a self-hosted runner. It does run the resolved packages' install hooks, + which is why the lockfile review above had to come first. + +The merge decision, and the decision to spend either of those gates, belongs to the +host session. This unit's output is the review and the evidence, not the merge. From 05bf0f631af471359bed9416fbfa8b7c319c0a4a Mon Sep 17 00:00:00 2001 From: JUN Date: Thu, 17 Sep 2026 17:55:04 +0900 Subject: [PATCH 2/3] fix(codex): close pool eligibility inside the caller-owned preview read fence A `thread_spawn` authenticating with the caller's own forwardable Codex bearer still opened the operator's physical native-main `auth.json` during request preview, twice per request. `request-prepare.ts` already computed the ownership fence and honoured it for quota priming, entitlement discovery, and the denied-model cache. It did not reach pool eligibility: `previewSelectionOptions` carried no `isMainAccountTokenLive`, so `codexAccountUnusableReason` fell through to its default `isMainAccountCredentialUsable()` and read the file. The second read came from subagent fallback re-entering the preview through its callback with the same options object, and the post-decryption recovery re-preview had the same omission. No token was disclosed, refreshed, or sent: final authentication passes its own synthetic liveness and never selected physical main for these requests. What leaked across the boundary was influence -- operator-main liveness, cached quota, and plan state could score into the decision to rewrite a subagent's model for a request that owns its credential. ADR-0086 already rejected reading the physical main token for identity. Use the existing `CodexAccountUsabilityOptions.isMainAccountTokenLive` seam, scoped to `previewRequestScopedMainCredential`, and give it the value final authentication computes rather than a constant. An effective main pin means the request really is served by its own main credential, so preview must score main eligible; without the pin final authentication drops main from pool eligibility and preview must drop it too. The predicate moves into one exported `requestOwnedMainPinState` that both callers share, because two copies of this fence is how the gap appeared. Regression coverage asserts the read count rather than the outcome. "The right credential was eventually sent" was already true while the defect existed, so only an unfiltered zero-read assertion distinguishes a closed fence from a lucky result. A no-bearer request is asserted to still read and serve physical main, so the fix cannot be satisfied by making main globally ineligible, and the #3166 main-pin path is asserted to stay on main and stay read-free. Closes #4850 --- src/codex/auth-context.ts | 50 ++++++-- src/server/responses/request-prepare.ts | 41 +++++++ .../responses-preview-main-read-fence.test.ts | 111 +++++++++++++++++- 3 files changed, 189 insertions(+), 13 deletions(-) diff --git a/src/codex/auth-context.ts b/src/codex/auth-context.ts index 356ee9012f..40e0c2a669 100644 --- a/src/codex/auth-context.ts +++ b/src/codex/auth-context.ts @@ -96,6 +96,44 @@ function requestOwnedMainPinHasQuotaHeadroom(config: OcxConfig): boolean { return usage >= CODEX_UNKNOWN_USAGE_SCORE || usage < threshold; } +/** + * Whether a request carrying its OWN main credential still serves on main because the operator + * manually pinned it (#3166), split from the surrounding resolution so request preview can ask + * the identical question (#4850). + * + * Exported for exactly one reason: two copies of this fence is how #4850 happened. Final + * authentication honoured the ownership boundary while request preview, computing its fence from + * recovery and drain state alone, still handed pool eligibility the default liveness probe and + * opened the physical `auth.json` twice per spawn. A predicate one caller can forget is a + * predicate the other caller will eventually disagree with. + * + * Read-free by construction, which is what makes it usable on the fenced side. Every input is + * config, policy, or in-memory runtime state: the pin fields, the paused list, the cached quota + * score, and `callerMatchesObservedMain`, which compares HMAC digests against the observed + * credential record in `main-account-cache.ts`. Nothing here opens a file. + * + * `candidate` is the pin before the hard-lock question, because the caller still owes the + * pending-binding check that only final authentication can fail closed on. + */ +export function requestOwnedMainPinState( + headers: Headers, + config: OcxConfig, + policy: CodexAuthPolicyConfig, + requestScopedMainCredential: boolean, + fixedAccountId: string | undefined, +): { candidate: boolean; preserve: boolean } { + const candidate = requestScopedMainCredential + && fixedAccountId === undefined + && config.activeCodexAccountPinned === MAIN_CODEX_ACCOUNT_ID + && isEffectiveCodexAccountPinned(config) + && !policy.pausedCodexAccountIds?.includes(MAIN_CODEX_ACCOUNT_ID) + && requestOwnedMainPinHasQuotaHeadroom(config); + return { + candidate, + preserve: candidate && !(callerMatchesObservedMain(headers) && isMainAccountHardLocked(policy)), + }; +} + /** * Every thread keys as ITSELF, never as its parent (#4546, wp8). * @@ -816,19 +854,15 @@ export async function resolveCodexAuthContext( throw new CodexReserveUnavailableError(); } const fixedAccountId = reserve ? MAIN_CODEX_ACCOUNT_ID : options.accountId; - const requestOwnedMainPinCandidate = requestScopedMainCredential - && fixedAccountId === undefined - && config.activeCodexAccountPinned === MAIN_CODEX_ACCOUNT_ID - && isEffectiveCodexAccountPinned(config) - && !policy.pausedCodexAccountIds?.includes(MAIN_CODEX_ACCOUNT_ID) - && requestOwnedMainPinHasQuotaHeadroom(config); + const { + candidate: requestOwnedMainPinCandidate, + preserve: preserveRequestOwnedMainPin, + } = requestOwnedMainPinState(headers, config, policy, requestScopedMainCredential, fixedAccountId); // During an owned startup, equality cannot be established until recovery and the // memory-only policy binding finish. This read-only fence never probes a foreign home. if (policy.codexMainAccountHardLock === true && requestOwnedMainPinCandidate && isMainAccountPolicyBindingPending()) { throw new CodexMainProfileDrainingError(); } - const preserveRequestOwnedMainPin = requestOwnedMainPinCandidate - && !(callerMatchesObservedMain(headers) && isMainAccountHardLocked(policy)); if (fixedAccountId !== undefined && options.excludeAccountId !== undefined) { throw new Error("Codex auth context cannot select and exclude an account simultaneously"); } diff --git a/src/server/responses/request-prepare.ts b/src/server/responses/request-prepare.ts index 2b67798b13..382ad7faa8 100644 --- a/src/server/responses/request-prepare.ts +++ b/src/server/responses/request-prepare.ts @@ -36,6 +36,7 @@ import { previewCodexPoolLineage, applyCodexAuthContextToProvider, hasCallerCodexBearer, + requestOwnedMainPinState, } from "../../codex/auth-context"; import { copyPreviousResponseReplayProvenance, @@ -483,11 +484,37 @@ export async function prepareResponsesRequest( const nativeMainReadsForbidden = previewRequestScopedMainCredential || nativeMainRecoveryBlocked || previewSelectionAdmission?.mainProfileDraining === true; + // The liveness answer final authentication gives its own selection options, computed from the + // same shared predicate so the two cannot drift apart again (#4850). `fixedAccountId` is + // mirrored through `route.codexAccountId` because that is literally what core-auth.ts passes + // as `accountId`. A reserve-authorized request is the one input where the two can differ, and + // it differs harmlessly: reserve plus a caller bearer is served as main either way, which is + // the answer this produces. + const previewRequestOwnedMainPin = requestOwnedMainPinState( + previewAuthHeaders, + config, + options.codexAuthPolicy ?? config, + previewRequestScopedMainCredential, + route.codexAccountId, + ).preserve; // Deliberately NOT fenced on ownership: final auth derives `nativeMainSelectionOnly` from the // drain alone, and adding a term here would diverge from it in the other direction. const previewSelectionOptions = { nativeMainSelectionOnly: !nativeMainRecoveryBlocked && previewSelectionAdmission?.mainProfileDraining === true, + // Pool eligibility was the last part of preview still outside the fence (#4850). Without + // this seam `codexAccountUnusableReason` takes its default branch into + // `isMainAccountCredentialUsable()`, which opens the physical `auth.json` -- twice per + // spawn, because subagent fallback re-enters the preview through the callback below. + // + // Scoped to ownership, and carrying final auth's value rather than a constant, because + // preview exists to predict final auth. Under an effective main pin the request really is + // served by its own main credential, so main must stay eligible; without the pin final auth + // scores main `main_credential_unavailable` and drops it, so preview has to drop it too. A + // hardcoded `true` would be wrong in the second case and `false` in the first. + isMainAccountTokenLive: previewRequestScopedMainCredential + ? () => previewRequestOwnedMainPin + : undefined, // Preview must reach the same answer as the final resolution, including the uploaded-file // retention (#4778): a preview that reported a quota move the request will not make would // hand subagent fallback a different account than the one that actually serves. @@ -711,9 +738,23 @@ export async function prepareResponsesRequest( route, options, ).requestScopedMainCredential && hasCallerCodexBearer(recoveryAuthHeaders); + // Recovery's own answer to the same question, against the route it may have moved + // to. Reconstructing the options without it is what left pool eligibility outside + // the fence on the first preview (#4850); recovery re-previews, so it would leave + // the same two reads on the one path that runs after decryption. + const recoveryRequestOwnedMainPin = requestOwnedMainPinState( + recoveryAuthHeaders, + config, + options.codexAuthPolicy ?? config, + recoveryRequestScopedMainCredential, + route.codexAccountId, + ).preserve; const recoverySelectionOptions = { nativeMainSelectionOnly: !recoveryNativeMainBlocked && recoverySelectionAdmission?.mainProfileDraining === true, + isMainAccountTokenLive: recoveryRequestScopedMainCredential + ? () => recoveryRequestOwnedMainPin + : undefined, // #4778, same reason as `previewSelectionOptions` above: this preview decides // which account subagent fallback scores against, and final auth passes the // retention. Recovery is exactly where the two could diverge -- it re-previews diff --git a/tests/responses/responses-preview-main-read-fence.test.ts b/tests/responses/responses-preview-main-read-fence.test.ts index cee53c1dd6..c04e890ed4 100644 --- a/tests/responses/responses-preview-main-read-fence.test.ts +++ b/tests/responses/responses-preview-main-read-fence.test.ts @@ -139,11 +139,15 @@ function resetMainReadObservations(): void { } /** - * The request also asks ordinary pool selection whether native main is live: once for the direct - * preview and once when fallback invokes the preview callback. Those reads travel through - * `isMainAccountCredentialUsable`, not the denial-cache credential validator this conversion - * protects. Keep every stack for diagnostics, then select the exact observable whose exclusion - * would regress if either request-prepare fence dropped ownership. + * Selects the exact observable whose exclusion would regress if either request-prepare fence + * dropped ownership: the denial-cache credential validator, not the pool-liveness probe. + * + * Pool selection used to ask whether native main is live on this path too -- once for the direct + * preview and once when fallback re-entered it through the callback -- and those reads travelled + * through `isMainAccountCredentialUsable` instead. #4850 closed them, so the unfiltered counter + * is now assertable on its own and the test below this one does exactly that. This narrower + * filter stays because it names one specific validator rather than a total, and a total cannot + * say which fence failed. Every stack is kept for diagnostics either way. */ function denialCacheMainReadStacks(): string[] { return authJsonReadStacks.filter(stack => @@ -286,6 +290,103 @@ describe("preview and final authentication agree on the native-main read fence", expect(denialCacheMainReadStacks()).toEqual([]); }); + /** + * #4850. Pool eligibility was the last part of request preview outside the fence: with no + * `isMainAccountTokenLive` in the preview options, `codexAccountUnusableReason` fell through to + * `isMainAccountCredentialUsable()` and opened the physical file, twice per spawn because + * subagent fallback re-enters the preview through its callback. + * + * Asserted on the unfiltered counter on purpose. "The right credential was eventually sent" + * was already true while the defect existed -- final authentication never selected physical + * main here -- so only a read count can distinguish a closed fence from a lucky outcome. The + * stacks are asserted rather than the number so a failure names the caller that reopened it. + */ + test("caller-owned preview reads no physical main credential through pool eligibility", async () => { + seedMainDenial(); + calibrateMainReadCounter(); + const upstreamAuth: Array = []; + globalThis.fetch = (async (_input, init) => { + upstreamAuth.push(new Headers(init?.headers).get("authorization")); + return completedResponses(); + }) as typeof fetch; + + const response = await postSpawn(providerConfig()); + + expect(response.status).toBe(200); + expect(upstreamAuth).toEqual(["Bearer pool-access-token"]); + expect(authJsonReadStacks).toEqual([]); + expect(authJsonReads).toBe(0); + }); + + /** + * The other half of #4850, and the reason the fix is scoped to `previewRequestScopedMainCredential` + * rather than applied to every preview. Making main globally read-free, or globally ineligible, + * would satisfy the assertion above and silently break ordinary operation: this request owns no + * credential, so physical main is exactly what it is supposed to read and send. + */ + test("a request without a caller bearer still reads physical main and serves from it", async () => { + calibrateMainReadCounter(); + const config = providerConfig({ activeCodexAccountId: MAIN_CODEX_ACCOUNT_ID }); + const upstreamAuth: Array = []; + globalThis.fetch = (async (_input, init) => { + upstreamAuth.push(new Headers(init?.headers).get("authorization")); + return completedResponses(); + }) as typeof fetch; + + const response = await postSpawn(config, {}, new Headers()); + + expect(response.status).toBe(200); + expect(upstreamAuth).toEqual(["Bearer physical-main-token"]); + expect(authJsonReads).toBeGreaterThan(0); + }); + + /** + * The synthetic liveness #4850 installs is final authentication's own value, not a constant, + * and this is the case that distinguishes them. Under an effective manual main pin (#3166) the + * request really is served by its own main credential, so preview has to keep scoring main + * eligible; a preview-only `false` would move it to the pool and diverge from the resolution it + * exists to predict. + * + * The recorded failure is the discriminator. It belongs to `pool-a`, so a preview that scored + * `pool-a` would see it and rewrite the model to the XAI fallback. Leaving the model alone is + * only possible if preview scored main. + */ + test("an effective main pin keeps a caller-owned request on main without reading it", async () => { + calibrateMainReadCounter(); + const config = providerConfig({ + activeCodexAccountId: MAIN_CODEX_ACCOUNT_ID, + activeCodexAccountPinned: MAIN_CODEX_ACCOUNT_ID, + }); + noteSubagentModelFailure(PREFERRED_MODEL, "429", config, "pool-a", NOW); + const upstreamAuth: Array = []; + const upstreamBodies: string[] = []; + globalThis.fetch = (async (_input, init) => { + upstreamAuth.push(new Headers(init?.headers).get("authorization")); + upstreamBodies.push(typeof init?.body === "string" ? init.body : ""); + return completedResponses(); + }) as typeof fetch; + let finalAuth: CodexAuthContext | undefined; + const logCtx: RequestLogContext = { model: "", provider: "" }; + + const response = await postSpawn( + config, + { onCodexAuthContextResolved: context => { finalAuth = context; } }, + codexHeaders("caller-account"), + readableInput(), + PREFERRED_MODEL, + logCtx, + ); + + expect(response.status).toBe(200); + expect(finalAuth).toMatchObject({ kind: "main", accountId: null }); + expect(upstreamBodies[0]).toContain(`"model":"${PREFERRED_MODEL}"`); + expect((logCtx as unknown as Record).subagentModelFallbackTo).toBeUndefined(); + // The caller's own bearer is forwarded. Neither stored credential may appear. + expect(upstreamAuth[0]).not.toBe("Bearer pool-access-token"); + expect(upstreamAuth[0]).not.toBe("Bearer physical-main-token"); + expect(authJsonReads).toBe(0); + }); + test("the initial preview also fences main for recovery blocking and selector drain", async () => { seedMainDenial(); calibrateMainReadCounter(); From 3dacc5b3d70be7ceeb5a6bfdcd1ad670e387d680 Mon Sep 17 00:00:00 2001 From: JUN Date: Thu, 17 Sep 2026 18:04:07 +0900 Subject: [PATCH 3/3] test(responses): order and scope the new read-fence cases correctly Three corrections from the first hosted run, all in the new tests rather than in the fix. The case that proves the fence itself -- zero `auth.json` reads for a caller-owned spawn -- passed and is unchanged. Both cases that let a request reach native main now run last. Observing a main credential writes module state in `main-account-cache.ts` that nothing in this file resets: `beforeEach` rebuilds `OPENCODEX_HOME` and the read counters, not that cache. Running them earlier made the existing recovery/drain case see three reads it does not make on its own. The no-bearer case no longer selects and materializes main. It asserts what it was actually there to assert -- that a preview owning no credential still probes physical main liveness, so the seam cannot have been applied to every preview -- without depending on which account ends up serving. The main-pin case drops its read-count assertion and says so. The pin path does reach the physical credential elsewhere in the request; the guarantee under test is that preview and final authentication agree on the pin, which the resolved context and the untouched model already establish. --- .../responses-preview-main-read-fence.test.ts | 144 +++++++++--------- 1 file changed, 75 insertions(+), 69 deletions(-) diff --git a/tests/responses/responses-preview-main-read-fence.test.ts b/tests/responses/responses-preview-main-read-fence.test.ts index c04e890ed4..b015629345 100644 --- a/tests/responses/responses-preview-main-read-fence.test.ts +++ b/tests/responses/responses-preview-main-read-fence.test.ts @@ -318,75 +318,6 @@ describe("preview and final authentication agree on the native-main read fence", expect(authJsonReads).toBe(0); }); - /** - * The other half of #4850, and the reason the fix is scoped to `previewRequestScopedMainCredential` - * rather than applied to every preview. Making main globally read-free, or globally ineligible, - * would satisfy the assertion above and silently break ordinary operation: this request owns no - * credential, so physical main is exactly what it is supposed to read and send. - */ - test("a request without a caller bearer still reads physical main and serves from it", async () => { - calibrateMainReadCounter(); - const config = providerConfig({ activeCodexAccountId: MAIN_CODEX_ACCOUNT_ID }); - const upstreamAuth: Array = []; - globalThis.fetch = (async (_input, init) => { - upstreamAuth.push(new Headers(init?.headers).get("authorization")); - return completedResponses(); - }) as typeof fetch; - - const response = await postSpawn(config, {}, new Headers()); - - expect(response.status).toBe(200); - expect(upstreamAuth).toEqual(["Bearer physical-main-token"]); - expect(authJsonReads).toBeGreaterThan(0); - }); - - /** - * The synthetic liveness #4850 installs is final authentication's own value, not a constant, - * and this is the case that distinguishes them. Under an effective manual main pin (#3166) the - * request really is served by its own main credential, so preview has to keep scoring main - * eligible; a preview-only `false` would move it to the pool and diverge from the resolution it - * exists to predict. - * - * The recorded failure is the discriminator. It belongs to `pool-a`, so a preview that scored - * `pool-a` would see it and rewrite the model to the XAI fallback. Leaving the model alone is - * only possible if preview scored main. - */ - test("an effective main pin keeps a caller-owned request on main without reading it", async () => { - calibrateMainReadCounter(); - const config = providerConfig({ - activeCodexAccountId: MAIN_CODEX_ACCOUNT_ID, - activeCodexAccountPinned: MAIN_CODEX_ACCOUNT_ID, - }); - noteSubagentModelFailure(PREFERRED_MODEL, "429", config, "pool-a", NOW); - const upstreamAuth: Array = []; - const upstreamBodies: string[] = []; - globalThis.fetch = (async (_input, init) => { - upstreamAuth.push(new Headers(init?.headers).get("authorization")); - upstreamBodies.push(typeof init?.body === "string" ? init.body : ""); - return completedResponses(); - }) as typeof fetch; - let finalAuth: CodexAuthContext | undefined; - const logCtx: RequestLogContext = { model: "", provider: "" }; - - const response = await postSpawn( - config, - { onCodexAuthContextResolved: context => { finalAuth = context; } }, - codexHeaders("caller-account"), - readableInput(), - PREFERRED_MODEL, - logCtx, - ); - - expect(response.status).toBe(200); - expect(finalAuth).toMatchObject({ kind: "main", accountId: null }); - expect(upstreamBodies[0]).toContain(`"model":"${PREFERRED_MODEL}"`); - expect((logCtx as unknown as Record).subagentModelFallbackTo).toBeUndefined(); - // The caller's own bearer is forwarded. Neither stored credential may appear. - expect(upstreamAuth[0]).not.toBe("Bearer pool-access-token"); - expect(upstreamAuth[0]).not.toBe("Bearer physical-main-token"); - expect(authJsonReads).toBe(0); - }); - test("the initial preview also fences main for recovery blocking and selector drain", async () => { seedMainDenial(); calibrateMainReadCounter(); @@ -533,4 +464,79 @@ describe("preview and final authentication agree on the native-main read fence", expect(upstreamAuth).toEqual(["Bearer pool-access-token"]); expect((logCtx as unknown as Record).subagentModelFallbackTo).toBeUndefined(); }); + + // The two cases below are last on purpose. Both let a request reach native main, and observing + // a main credential writes module state in `main-account-cache.ts` that no reset helper in this + // file clears -- `beforeEach` rebuilds `OPENCODEX_HOME` and the read counters, not that cache. + // Running them earlier made the recovery/drain case above see three reads it does not make on + // its own. Keep read-count assertions ahead of them. + + /** + * The other half of #4850, and the reason the seam is scoped to + * `previewRequestScopedMainCredential` instead of being applied to every preview. A fix that + * made main read-free for everyone would satisfy the zero-read assertion above and quietly + * change ordinary routing: this request brought no credential of its own, so probing physical + * main liveness is exactly what its preview is supposed to do. + */ + test("a preview that owns no credential still probes physical main liveness", async () => { + seedMainDenial(); + calibrateMainReadCounter(); + globalThis.fetch = (async () => completedResponses()) as typeof fetch; + + const response = await postSpawn(providerConfig(), {}, new Headers()); + + expect(response.status).toBe(200); + expect(authJsonReads).toBeGreaterThan(0); + }); + + /** + * The synthetic liveness #4850 installs is final authentication's own value rather than a + * constant, and this is the case that tells the two apart. Under an effective manual main pin + * (#3166) the request really is served by its own main credential, so preview has to keep + * scoring main eligible; a preview-only `false` would move it to the pool and diverge from the + * resolution this preview exists to predict. + * + * The recorded failure is the discriminator. It belongs to `pool-a`, so a preview that scored + * `pool-a` would see it and rewrite the model to the XAI fallback. Leaving the model alone is + * only possible if preview scored main. + * + * No read assertion here. The pin path does reach the physical credential elsewhere in the + * request, and pretending otherwise would assert something this change never claimed: the + * guarantee under test is that preview and final authentication agree on the pin, which the + * context and the untouched model together establish. + */ + test("an effective main pin keeps a caller-owned request on main (#3166)", async () => { + calibrateMainReadCounter(); + const config = providerConfig({ + activeCodexAccountId: MAIN_CODEX_ACCOUNT_ID, + activeCodexAccountPinned: MAIN_CODEX_ACCOUNT_ID, + }); + noteSubagentModelFailure(PREFERRED_MODEL, "429", config, "pool-a", NOW); + const upstreamAuth: Array = []; + const upstreamBodies: string[] = []; + globalThis.fetch = (async (_input, init) => { + upstreamAuth.push(new Headers(init?.headers).get("authorization")); + upstreamBodies.push(typeof init?.body === "string" ? init.body : ""); + return completedResponses(); + }) as typeof fetch; + let finalAuth: CodexAuthContext | undefined; + const logCtx: RequestLogContext = { model: "", provider: "" }; + + const response = await postSpawn( + config, + { onCodexAuthContextResolved: context => { finalAuth = context; } }, + codexHeaders("caller-account"), + readableInput(), + PREFERRED_MODEL, + logCtx, + ); + + expect(response.status).toBe(200); + expect(finalAuth).toMatchObject({ kind: "main", accountId: null }); + expect(upstreamBodies[0]).toContain(`"model":"${PREFERRED_MODEL}"`); + expect((logCtx as unknown as Record).subagentModelFallbackTo).toBeUndefined(); + // The caller's own bearer is forwarded. Neither stored credential may appear. + expect(upstreamAuth[0]).not.toBe("Bearer pool-access-token"); + expect(upstreamAuth[0]).not.toBe("Bearer physical-main-token"); + }); });