diff --git a/devlog/_plan/260819_unclaimed_bug_selection/100_post_split_rebase.md b/devlog/_plan/260819_unclaimed_bug_selection/100_post_split_rebase.md new file mode 100644 index 0000000000..359c5c4cd8 --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/100_post_split_rebase.md @@ -0,0 +1,61 @@ +# 100 — Post-split rebase of the stack + +## What changed under us + +The mega-file split landed while this work was open: + +| PR | Merge | +|---|---| +| #2019 WP1 types | `da86a830a` | +| #2023 WP1b clusters | `2235f456d` | +| #2036 WP2a config leaf | `eca18d0c8` | + +`src/types/tools.ts` now exists on `dev` and `OcxTool` lives there. **#1934 also +merged** (`a5289aad5`), which `070` had named as the real collision hazard for +`#1587` — it touched all five of that fix's files. + +So both of `#1587`'s blockers cleared at once. It moves from "last, most +contested files" to implementable. + +## Rebase + +Bottom-up, three branches, **zero conflicts**: + +``` +fix/service-proxy-env eb910776a -> d7caaa9bf +fix/tray-registry-encoding e3b063750 -> f1f400fea +fix/cursor-abort-teardown 346eaa80d -> 2c4e00ede +``` + +Zero conflicts is the expected result and not the interesting part: none of the +three fixes touches `types.ts`, `config.ts`, or anything the split moved. That +was the collision analysis in `070`, and it held. + +## The verification that actually mattered + +A green result from *before* the rebase proves nothing here — the split replaced +`types.ts` with a barrel and moved every type cluster, so the whole tree these +branches compile against is different. + +Re-run on the rebased top of stack: + +``` +bun test service + tray + windows-text + cursor-cancel + cursor-eof + 163 pass / 0 fail +bun x tsc --noEmit + exit 0 +``` + +## Deferred items: what is still blocked + +Re-checked live rather than carried from the previous turn: + +| PR | State | Consequence | +|---|---|---| +| #2029 | OPEN, `CHANGES_REQUESTED` | `#2114` still must not get a competing PR | +| #2101 | OPEN | `#2108` phase 1 still collides on `server/index.ts` + `auth-context.ts` | +| #2054 | OPEN, `CHANGES_REQUESTED` | the kimi-k3 and 429 halves of `#1527` still cannot start | + +Only `#1587` actually became unblocked. The other three deferrals stand for the +same reasons they were recorded, which is worth stating explicitly — "the split +landed" is not a general unblock. diff --git a/devlog/_plan/260819_unclaimed_bug_selection/110_2114_disposition.md b/devlog/_plan/260819_unclaimed_bug_selection/110_2114_disposition.md new file mode 100644 index 0000000000..202372f679 --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/110_2114_disposition.md @@ -0,0 +1,64 @@ +# 110 — #2114 disposition: unblock, do not compete + +Outcome: **no code PR.** Two comments — the containment on #2029 +(`5342888183`) and the ownership record on #2114 (`5342892253`). + +## What the reviewer actually objected to + +Read rather than summarized. @Ingwannu on #2029: + +> A missing user bus does not prove that `~/.config/systemd/user/opencodex-proxy.service` +> is absent. The definition can remain on disk while the user manager is +> unavailable, and it can name another `CODEX_HOME` or `OPENCODEX_HOME`. +> Returning absent before inspecting that artifact lets the ownership preflight +> treat the machine as unclaimed and overwrite a foreign or interrupted +> installation. + +That is the same hazard `020` arrived at independently and wrote down as +"test 3". Two people reaching it separately is the strongest signal in this +whole unit that it is the real constraint. + +## The finding that made this a comment instead of a PR + +The reviewer asked for: inspect the unit file, absent → `absent`, exists → +present claim with registration absent, unreadable → `unknown`. + +**`inspectSystemd` already does exactly that**, at lines 289-313 — check +`artifactPresence(definitionPath)`, `readFileSync`, parse +`unitEnvValue(body, "CODEX_HOME")` and `OPENCODEX_HOME`, return a `present` +claim. The no-bus branch simply returns before reaching it. + +So the containment needs **no new machinery** — only a different control path +through code that is already there. A separate PR would have re-implemented +something sitting twenty lines below. + +## Why not just open our own PR anyway + +#2114 and #1939 are one classifier with two symptoms: a refused sync there, a +process-lifetime native-main 503 here. A second PR on the same function means +two people making the same fail-closed security-adjacent call, and whichever +lands second silently overwrites the first. + +The stderr shapes make the overlap concrete. #2029 matches two messages and +pins everything else as `unknown`; #2114's shape is a third +(`Failed to connect to user scope bus via local transport…`) and is currently +on the pinned side. + +## What was offered + +- The concrete code shape, reusing the existing helpers. +- The locale-fragility tradeoff **stated rather than hidden**: string matching + will miss a non-English systemd. Worth noting that with the disk check, a + mismatch degrades to a missed recovery rather than a wrong admission — which + is a much safer thing to be locale-sensitive about. Alternative named + (`LC_ALL=C`) without pretending it is free. +- Six regressions, including the #2114 stderr and the existing + non-bus-non-zero assertion kept as the over-widening guard. +- A rebase note: the review's "125 commits behind" is stale, but `dev` has since + taken the split, so a rebase is needed regardless. + +## What was deliberately not done + +No push to a contributor branch, no competing PR, and #2114 left **open and +linked** rather than closed as a duplicate — the 503 symptom is distinct enough +that someone hitting it should find that thread rather than a redirect. diff --git a/devlog/_plan/260819_unclaimed_bug_selection/120_2108_phase1.md b/devlog/_plan/260819_unclaimed_bug_selection/120_2108_phase1.md new file mode 100644 index 0000000000..8da480d530 --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/120_2108_phase1.md @@ -0,0 +1,174 @@ +# 120 — #2108 phase 1: log the concrete native-main gate reason + +Work-phase `wp2108`. One PABCD cycle. Phase 2 (retryable boot-time `unknown`) +is explicitly NOT in this cycle. + +**Revision note.** The first draft of this doc proposed a response header +`x-ocx-native-main-gate-reason` alongside the log line. An adversarial audit +returned `fail` and it was correct; the header is removed. What follows is the +design that survived, with the refutation recorded because it is the reason the +scope is what it is. + +## The decision that gated this cycle + +`040_2108_windows_reboot_gate.md` said: check PR #2101 before starting, because +it edits both files phase 1 touches. + +**#2101 is still OPEN** (`REVIEW_REQUIRED`, head `ingw/fix-daybreak-account-entitlement`, +1397 lines, last touched 2026-08-19T12:00:56Z). The collision is real, so the +only question is whether it is avoidable. + +Its hunks in `src/codex/auth-context.ts` cover, in old-file lines: 23-28, +237-242, 260-267, 273-278, 302-308, 312-318, 325-330. Its hunks in +`src/server/index.ts` cover 58, 894, 906, 940, 957, 971, 1038. + +**`auth-context.ts:115-131` is clear** — nearest hunk edges are 28 and 237. +This cycle changes nothing else, in any file. + +That is what makes this an implement rather than a defer. All three throw sites +sit in or beside #2101's hunks: `:313` (inside 312-318), `:319` (one line past +that hunk, still inside its rebase blast radius), and `:326` (inside 325-330). +A design that had to edit any of them would have been a defer. + +## What ships + +One file: `src/codex/auth-context.ts`, lines 115-131 only. No other production +file, and no signature any other file calls. + +### The reason is captured and logged in the constructor + +`CodexMainProfileDrainingError` has a zero-arg constructor today. It gains: + +- a `readonly reason?: NativeMainStartupBlockReason`, read from + `nativeMainStartupGateSnapshot()` **in the constructor**; +- one `console.warn` naming that reason, deduped per distinct reason. + +Capturing in the constructor is the whole design. It is the only moment where +the reason is both in scope and true — every catch site has already lost it, and +re-reading the gate later can observe a different value. + +### Why there is no header + +The first draft wanted a header so the reason would be machine-readable. Three +findings killed it, and each one independently: + +**It cannot reach the caller without editing five files.** +`codexMainProfileDrainingResponse()` is called with zero arguments at +`core.ts:1121`, `compact.ts:390`, `search.ts:126`, `live.ts:542`, +`images.ts:441`. Threading a reason means editing all five — and `core.ts` and +`compact.ts` are both edited by #2101. The collision we set out to avoid comes +straight back. + +**Having the response re-read the gate is worse, not cheaper.** It looks like a +one-file fix, but `nativeMainStartupGateSnapshot()` returns fresh state each +call and `completeNativeMainRecovery()` can flip it to `ready` between the throw +and the catch. The header would then be blank or wrong exactly when recovery is +racing — which is the scenario #2108 is about. + +**It would not reach the Claude surface anyway.** `claude-messages.ts:823-829` +builds a new `Response` with a hand-written header object carrying only +`Content-Type` and `Retry-After`; every other header is dropped. The WebSocket +fence path exposes no response headers at all. + +So the log line is not a consolation prize. It is the only surface that reaches +every path this fence can fire on. + +### Why not the message, and why not `/api/logs` + +Both are closed off, which is worth stating because they are the obvious first +guesses. + +**The message must not change.** `claude-messages.ts:818-820` identifies this +response by `message === CODEX_MAIN_PROFILE_MAINTENANCE_MESSAGE`. Break that +match and `isTransientUpstreamStatus(503)` takes over, remapping the fence to +Anthropic 529 — a local maintenance state would start telling Claude Code to +back off and retry an upstream that was never involved. + +**`/api/logs` cannot carry it either.** `upstreamError` is populated by reading +the response body back and taking `json.error.message` and nothing else +(`request-log.ts:755-759`, via `relay.ts:434-442`). Headers are already gone. +`error.code` is likewise filtered: `requestLogErrorCode` pins 503 to +`server_is_overloaded` and refuses to forward arbitrary codes by design +(`request-log.ts:508-540`, comment at :514). + +That leaves stdout — which is where the reporter was reading anyway. Their +excerpt interleaves `[17:25:23] opencodex service wrapper start` with the 503 +lines, so a `console.warn` lands in the stream they already quoted. + +### Dedup, and the site that has no reason + +Deduped per distinct reason: the reporter's log shows three 503s in eleven +seconds and a real client retries harder than that. A per-request line buries +the signal it exists to produce. Dedup state is module-level, so the module +exports a reset for tests — otherwise the second test in a Bun process observes +nothing and the assertion passes vacuously. + +The third throw site, `:326`, fires when `claimMainProfile()` refuses. That is +the **turn-drain** fence (`lifecycle.ts:180`), not the startup gate — line 318 +has already established `nativeMainTrafficBlocked` is false there, so the +snapshot reads `ready` and carries no reason. + +That site must stay silent. Constructor capture gets this right by +construction: a reason is recorded only when the snapshot is `blocked`. A 503 +with no reason therefore means "not the startup fence", which is itself a +distinction #2108 needs — the reporter could not make it. + +Only `reason` is emitted. The snapshot also carries `homeId`, which is derived +from a profile directory path and has no business in a log line. + +## Why logging and not the mechanism + +The trigger is still unidentified. `040` names two candidates that settle to +different reasons: ACL fail-closed gives `owner-unavailable` +(`native-profile-startup.ts:138-139`), probe fail-closed gives +`ownership-unknown` (`:311`). One log line separates them. + +**No test anywhere asserts `owner-unavailable`.** It is produced at +`native-profile-startup.ts:139` and asserted nowhere in the suite — the +candidate branch we most need named in the field is the one with no coverage. + +## Test plan + +Drive the real gate, never a module mock — the existing tests already do this +(`tests/codex-auth-context.test.ts:250-254` via +`initializeNativeMainStartupGate({ probeRecoveryState: () => "manual" })`, +`tests/native-profile-startup.test.ts:291-292` via +`blockNativeMainStartupForUnownedServiceHome`). Capture the log with a local +`spyOn(console, "warn")`, the repo idiom (`app-owned-memory.test.ts:237`). + +1. `ownership-unknown` fence → `err.reason` is `ownership-unknown` **and the + warn fires carrying it**. The log assertion is the deliverable; without it + this cycle ships something unobserved. +2. `foreign-ownership` fence → same, different value. Proves the value is read + rather than hardcoded. +3. Second construction under the same reason → no second warn. Pins the dedup. +4. No fence active → no reason, no warn. Pins the `:326` turn-drain case. +5. Message, status, `Retry-After` unchanged. This is the claude-messages guard. + Worth knowing it is weaker than it sounds: grepping + `tests/claude-messages-endpoint.test.ts` for `nativeMainFence`, `529`, or the + message constant returns nothing, so the 503→529 remap has no direct + regression test of its own. + +**Red-drive is mandatory.** Lesson 1 of this session: the first #1527 attempt +patched a helper that was not on the failure path and all ten tests still +passed. Each assertion must be seen failing before the fix. + +## Verification + +``` +bun test tests/codex-auth-context.test.ts tests/native-profile-startup.test.ts tests/native-profile-drain-server.test.ts +bun x tsc --noEmit +bun run privacy:scan +``` + +`privacy:scan` is not ceremony here — this cycle adds a log statement, which is +the exact thing that scan exists to police. + +CI is ignored this cycle by standing instruction while the merge train churns; +local green is the judgment surface. + +## Stack position + +Stacks on `fix/cursor-abort-teardown` (#2118), targeting it. Retarget to `dev` +after the parents land. + diff --git a/devlog/_plan/260819_unclaimed_bug_selection/121_2108_implementation.md b/devlog/_plan/260819_unclaimed_bug_selection/121_2108_implementation.md new file mode 100644 index 0000000000..864a3843ec --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/121_2108_implementation.md @@ -0,0 +1,95 @@ +# 121 — #2108 phase 1 implementation record + +Shipped: PR [#2121](https://github.com/lidge-jun/opencodex/pull/2121), commit +`18a383e8e`, branch `fix/native-main-gate-reason` stacked on +`fix/cursor-abort-teardown` (#2118). + +Issue disposition posted: +[#2108 comment 5343287759](https://github.com/lidge-jun/opencodex/issues/2108#issuecomment-5343287759). +`Refs`, not `Closes` — this makes the next occurrence diagnosable, it does not +stop the fence sticking. + +## What the plan audit changed + +Draft 1 of `120` proposed a response header `x-ocx-native-main-gate-reason` +alongside the log line. The A-gate audit returned **fail**, and the header was +dropped entirely. The refutation is worth keeping because it is not obvious: + +- The header cannot reach the caller without editing the five zero-arg + `codexMainProfileDrainingResponse()` call sites — and `core.ts`/`compact.ts` + are both edited by #2101, so the collision the whole design existed to avoid + came straight back. +- Having the response builder re-read the gate instead *looks* like a one-file + fix, but `completeNativeMainRecovery()` can flip the snapshot to `ready` + between throw and catch. The header would go blank or wrong exactly when + recovery is racing, which is the scenario #2108 is about. +- It would not reach the Claude surface anyway: `claude-messages.ts:823-829` + rebuilds its response with a hand-written header object. + +The audit also caught a third throw site the plan had missed (`:326`, +turn-drain), that the `console.warn` assertion — phase 1's entire purpose — was +absent from the test plan, and that `privacy:scan` was missing from a cycle +whose only product is a log statement. + +Round 2 returned **pass**, and the reviewer went further than asked: it copied +`src`/`tests` to a scratch tree and mutated them there. Removing the +`console.warn` body goes red (2 fail); removing the dedup check goes red. That +is stronger evidence than my own red-drive, because it tests the assertions +rather than the fix. + +## The shipped shape + +`src/codex/auth-context.ts:115-166` only, plus a derived type export in +`native-profile-startup.ts` and the tests. + +`CodexMainProfileDrainingError` reads `nativeMainStartupGateSnapshot()` in its +constructor, records `reason` when the gate is `blocked`, and warns once per +distinct reason. Every call site — 3 constructions, 5 response builders — is +byte-identical. + +The `:326` turn-drain site stays silent by construction (early return on +`status !== "blocked"`), which makes a reasonless 503 mean "not the startup +fence". That distinction did not exist before and the reporter could not make it. + +## Collision outcome + +`git diff -U0` puts this change at old-file lines 15 and 118-124. #2101's +`auth-context.ts` hunks are 23-28, 237-242, 260-267, 273-278, 302-308, 312-318, +325-330. Zero overlap, no adjacency. `native-profile-startup.ts` is not among +#2101's 20 files. In the test file both PRs add imports around lines 59-63, +which merges cleanly. + +## Verification + +204 pass / 0 fail / 903 expect() across the five native-main gate suites plus +`chat-completions-endpoint` and `claude-messages-endpoint`. `tsc --noEmit` +exit 0. `privacy:scan` exit 0. + +Red-drive: with the fix staged as a no-op export, 2 fail on +`reason === undefined` and the silence case passed — the assertions were doing +the work, not a module-load error. + +**Coverage caveat, stated in the PR too:** macOS only. `owner-unavailable` is a +Windows icacls path, and nothing in the suite asserts it — it is produced at +`native-profile-startup.ts:139` and asserted nowhere. The branch the reporter +most likely hit is the one with no coverage, which is the argument for shipping +the diagnostic before the mechanism. + +## Stack correction worth recording + +The commit first landed on `fix/cursor-abort-teardown` itself rather than a new +branch. Corrected by `git switch -c fix/native-main-gate-reason` followed by +`git branch -f fix/cursor-abort-teardown origin/fix/cursor-abort-teardown` — +resetting the local branch to its already-pushed head, no force-push, no +contributor branch touched. Caught by reading `git log` after the commit rather +than assuming the branch was where I left it. + +## What phase 2 needs + +A field report that names a reason. Phase 2 makes a boot-time `unknown` +retryable while `OCX_SERVICE=1` instead of a process-lifetime fence, keeping +genuine `foreign` fail-closed with a retry cap. Two narrower fixes stand on +their own: a timed-out `sc.exe query` with WinSW xml and exe both absent must +not mark the machine `unknown`, and a second ACL `ETIMEDOUT` should back off +and retry so a warm icacls reopens the gate without `ocx restart`. + diff --git a/devlog/_plan/260819_unclaimed_bug_selection/130_ci_proxy_env_leak.md b/devlog/_plan/260819_unclaimed_bug_selection/130_ci_proxy_env_leak.md new file mode 100644 index 0000000000..a25c547f70 --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/130_ci_proxy_env_leak.md @@ -0,0 +1,110 @@ +# 130 — CI red: the proxy env leak that made 73 Lab tests fail + +Found because the user asked why PR #2116 was red. It was not flake, and it was +not inherited from `dev`. + +## What the checks actually said + +| PR | `macos` | failures | +|---|---|---| +| #2116 / #2117 / #2118 | fail | **73** | +| `dev` @ `0fc8d136e` | fail | **1** (`provider request pacing queue`) | + +So `dev` was red too, which is what made this easy to wave off. But 73 ≠ 1, and +the 73 were all Lab/fabric tests that #2116 never touches. + +## The failure + +``` +LabSandboxError: proxy environment variable HTTP_PROXY is forbidden + code: "harness_failure" + at rejectProxyEnvironment (src/lab/live/sandbox.ts:14) + at runFabricSyntheticPatchTaskInternal (src/lab/fabric/executor.ts:157) +``` + +The Lab sandbox refuses to run if any proxy variable is set on the live +`process.env` — it must not dial out through a proxy. Correct, and it was doing +its job. + +## The cause + +The #2107 tests set the real environment and restored it in a `finally`: + +```ts +const saved = { ...process.env }; +try { + process.env.HTTP_PROXY = "http://127.0.0.1:7890"; + ... +} finally { /* restore */ } +``` + +That reads as airtight. It is not, because **`bun test a.test.ts b.test.ts` +runs every file in one process, and `--isolate` does not change that.** The +variable outlived the file, and every Lab file loaded afterwards died on an +environment it never touched. + +## Isolating it + +The bisect that settled it, all on our branch: + +| Run | Result | +|---|---| +| Lab suites alone | 42 pass / 0 fail | +| `service.test.ts` + Lab suites | 39 fail | +| Same pair on `origin/dev` (our commits absent) | 144 pass / **0 fail** | + +The third row is the one that mattered: same files, same machine, our commits +removed, green. That converts "CI is flaky" into "we broke it". + +A probe file printing `process.env` at module-evaluation time then showed +`HTTP_PROXY` already set **before** `service.test.ts`'s own tests ran, which is +what proved the leak was cross-file rather than a bad `finally`. + +## A wrong turn worth recording + +The first fix assumed Bun's `{ ...process.env }` yields `null` rather than +`undefined` for absent keys, so the restore's `=== undefined` check took the +wrong branch. A probe did print `null` — but that was the *restore loop's own +output*, not the snapshot. Ablation killed the theory: with and without the +"fix", 50 fail / 50 fail, byte-identical. A change that does not move the number +is not a fix, however good the story is. + +## The actual fix + +Stop mutating global state to test a pure function. + +`buildUnit()` and `buildPlist()` now take the resolved proxy entries as a +parameter defaulting to `resolvedProxyEnv()`. Production behavior is unchanged +— the default is the old call — and the tests hand in a literal environment +instead of assigning onto `process.env`. `resolvedProxyEnv()` already accepted +an `env` argument; it is now exported so a test can use it the way the runtime +does. + +A third assertion was added while the seam was open: a lower-case `http_proxy` +must be baked under the canonical upper-case name. That behavior was implemented +and documented in #2107 but never asserted. + +## Verification + +The five suites that carried the failure — `service`, `lab-live-probe`, +`lab-fabric-task`, `lab-automation`, `api-key-attribution` — go **50 fail → 0 +fail, 236 pass**. `tsc --noEmit` exit 0. + +Full suite runs on `ssh lidge` per standing instruction, not the workstation. + +## Stack consequence + +The fix belongs to #2116, the bottom of the stack, so it was committed there +(`2d7b945b6`) and the other three branches were rebased onto it. All four +force-pushed with `--force-with-lease`. + +`dev`'s own single failure (`provider request pacing queue`) is a separate +matter and is not ours to fix inside this stack. + +## What this changes about the working rule + +The standing instruction was to ignore CI while the merge train churns and judge +from local green. That was right for a churning `dev` — but "ignore CI" cannot +mean "do not look at CI". Local green missed this entirely, because the local +runs were per-suite and the defect only exists across suites in one process. + diff --git a/src/codex/auth-context.ts b/src/codex/auth-context.ts index b74ef0bfa4..c67844dd50 100644 --- a/src/codex/auth-context.ts +++ b/src/codex/auth-context.ts @@ -12,7 +12,8 @@ import { ConfigMutationLockError } from "../config"; import { isCodexAccountUsable } from "./account-usability"; import { reconcileMainCodexAccountRuntimeState } from "./account-lifecycle"; import { MAIN_CODEX_ACCOUNT_ID, getMainAccountToken, isMainAccountTokenLive } from "./main-account"; -import { isNativeMainTrafficBlocked } from "./native-profile-startup"; +import { isNativeMainTrafficBlocked, nativeMainStartupGateSnapshot } from "./native-profile-startup"; +import type { NativeMainStartupBlockReason } from "./native-profile-startup"; import { codexQuotaScopeForModel, getCodexQuotaHealthSnapshot, @@ -116,12 +117,55 @@ export const CODEX_MAIN_PROFILE_MAINTENANCE_MESSAGE = "OpenCodex local native-main profile maintenance is active; retry this request"; export class CodexMainProfileDrainingError extends Error { + /** + * Which startup-gate state fenced this request, when one did. Undefined means the + * fence came from somewhere other than the startup gate — the turn-drain claim race + * throws this same error while the gate reads `ready`, and inventing a reason there + * would point the next report at a gate that never closed. + * + * Captured here rather than at the throw sites because this is the last moment it is + * both in scope and still true: every catch site has already lost it, and re-reading + * the gate later can observe a recovery that completed in between (#2108). + */ + readonly reason?: NativeMainStartupBlockReason; + constructor() { super(CODEX_MAIN_PROFILE_MAINTENANCE_MESSAGE); this.name = "CodexMainProfileDrainingError"; + const gate = nativeMainStartupGateSnapshot(); + if (gate.status !== "blocked") return; + this.reason = gate.reason; + reportNativeMainFenceReason(gate.reason); } } +/** + * #2108: a reboot could leave this fence closed until `ocx restart`, and the report was + * unactionable because the settled reason was never written anywhere. It cannot ride the + * message (claude-messages.ts matches that string exactly to keep the fence a 503 rather + * than an Anthropic 529) and it cannot ride a header (/api/logs reads only error.message + * from the body, and the Claude surface rebuilds its response headers from scratch), so + * stdout is the one surface that covers every path this fence fires on. + * + * Deduped per distinct reason: the original report shows three 503s in eleven seconds and + * a real client retries harder than that, so a per-request line would bury the signal. + * Only the reason is emitted; the snapshot's homeId is derived from a profile directory. + */ +const reportedFenceReasons = new Set(); + +function reportNativeMainFenceReason(reason: NativeMainStartupBlockReason): void { + if (reportedFenceReasons.has(reason)) return; + reportedFenceReasons.add(reason); + console.warn( + `native-main admission is fenced (reason: ${reason}); native model requests return 503 until it clears`, + ); +} + +/** Test-only: the dedup above is module state, so a second test would otherwise observe nothing. */ +export function __resetNativeMainFenceReasonLog(): void { + reportedFenceReasons.clear(); +} + export function codexMainProfileDrainingResponse(): Response { const response = formatErrorResponse(503, "server_busy", CODEX_MAIN_PROFILE_MAINTENANCE_MESSAGE); const headers = new Headers(response.headers); diff --git a/src/codex/native-profile-startup.ts b/src/codex/native-profile-startup.ts index 0722a077dd..bf1349aafa 100644 --- a/src/codex/native-profile-startup.ts +++ b/src/codex/native-profile-startup.ts @@ -31,6 +31,10 @@ export type NativeMainStartupGateSnapshot = | "stage-cleanup-required"; }; +/** The settled reason a blocked gate carries, named so consumers can hold one without the union. */ +export type NativeMainStartupBlockReason = + Extract["reason"]; + export interface NativeMainStartupGateDeps { manager?: NativeProfileManager; /** Test-only barrier used to prove admission stays closed while startup recovery is pending. */ diff --git a/tests/codex-auth-context.test.ts b/tests/codex-auth-context.test.ts index 6ff8b481fa..e57bff1766 100644 --- a/tests/codex-auth-context.test.ts +++ b/tests/codex-auth-context.test.ts @@ -13,6 +13,7 @@ import { CodexPoolAuthenticationError, CodexThreadAffinityExpiredError, codexMainProfileDrainingResponse, + __resetNativeMainFenceReasonLog, cooldownErrorMessage, cooldownErrorResponse, headersForCodexAuthContext, @@ -54,6 +55,7 @@ import { import type { OcxConfig, OcxProviderConfig } from "../src/types"; import { setIcaclsRunnerForTests } from "../src/lib/windows-secret-acl"; import { + blockNativeMainStartupForUnownedServiceHome, completeNativeMainRecovery, initializeNativeMainStartupGate, } from "../src/codex/native-profile-startup"; @@ -1346,3 +1348,62 @@ describe("cooldown error surface", () => { expect(cooldownErrorResponse(err, now).headers.get("Retry-After")).toBe("1"); }); }); + +// #2108: a Windows reboot can leave the native-main fence closed until `ocx restart`, and the +// reporter could not tell us WHICH gate reason settled because nothing ever logged it. The 503 +// message must stay byte-identical (claude-messages.ts:818 matches it to keep the fence a 503 +// instead of remapping to Anthropic 529), and headers never survive to /api/logs, so stdout is +// the only surface that reaches every path this fence fires on. +describe("native-main fence names its gate reason", () => { + afterEach(() => { + __resetNativeMainFenceReasonLog(); + }); + + test("the thrown fence carries the settled reason and says so once", async () => { + const warn = spyOn(console, "warn").mockImplementation(() => {}); + const fence = blockNativeMainStartupForUnownedServiceHome("ownership-unknown"); + try { + const err = new CodexMainProfileDrainingError(); + + expect(err.reason).toBe("ownership-unknown"); + expect(err.message).toBe(CODEX_MAIN_PROFILE_MAINTENANCE_MESSAGE); + const lines = warn.mock.calls.map(call => call.join(" ")); + expect(lines.filter(line => line.includes("ownership-unknown"))).toHaveLength(1); + // The homeId is derived from a profile directory path and has no business in a log line. + expect(lines.join("\n")).not.toContain("homeId"); + + // A retrying client must not turn the diagnostic into the noise it was meant to cut. + new CodexMainProfileDrainingError(); + new CodexMainProfileDrainingError(); + expect(lines.length).toBe(warn.mock.calls.length); + } finally { + warn.mockRestore(); + await fence.release(); + } + }); + + test("a different fence reports a different reason, so the value is read and not assumed", async () => { + const warn = spyOn(console, "warn").mockImplementation(() => {}); + const fence = blockNativeMainStartupForUnownedServiceHome("foreign-ownership"); + try { + expect(new CodexMainProfileDrainingError().reason).toBe("foreign-ownership"); + expect(warn.mock.calls.map(call => call.join(" ")).join("\n")).toContain("foreign-ownership"); + } finally { + warn.mockRestore(); + await fence.release(); + } + }); + + // auth-context.ts:326 throws the same error for the turn-drain fence (lifecycle.ts:180), which + // is NOT the startup gate: the snapshot there reads `ready`. Inventing a reason for it would + // send the next reboot report chasing a startup gate that never closed. + test("the turn-drain fence stays silent instead of borrowing a startup reason", () => { + const warn = spyOn(console, "warn").mockImplementation(() => {}); + try { + expect(new CodexMainProfileDrainingError().reason).toBeUndefined(); + expect(warn).not.toHaveBeenCalled(); + } finally { + warn.mockRestore(); + } + }); +});