From 163d7e50ee4862e4213eddaf059afac057701446 Mon Sep 17 00:00:00 2001 From: jun Date: Wed, 19 Aug 2026 22:33:06 +0900 Subject: [PATCH 1/5] docs(devlog): record the post-split rebase and which deferrals still stand --- .../100_post_split_rebase.md | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 devlog/_plan/260819_unclaimed_bug_selection/100_post_split_rebase.md 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. From b2f6cede16210f3f52aafe327d181321f48e40ac Mon Sep 17 00:00:00 2001 From: jun Date: Wed, 19 Aug 2026 22:35:28 +0900 Subject: [PATCH 2/5] docs(devlog): record the 2114 disposition and why it is a comment not a PR --- .../110_2114_disposition.md | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 devlog/_plan/260819_unclaimed_bug_selection/110_2114_disposition.md 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. From f5da1b29c4d5ad8f6c17d5cf72338967e88f60ff Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Wed, 19 Aug 2026 23:04:57 +0900 Subject: [PATCH 3/5] fix(codex): name the native-main gate reason when the fence returns 503 A Windows reboot can leave native-main admission fenced until the user runs ocx restart, and the report of it was unactionable: every 503 carries the same generic maintenance message, so the settled gate reason was never written anywhere a user could read it. Two candidate triggers settle to different reasons - an ACL fail-closed settles owner-unavailable, a probe fail-closed settles ownership-unknown - and nothing on the request path could tell them apart. Capture the reason in the CodexMainProfileDrainingError constructor and warn once per distinct reason. The constructor is the last moment the reason 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. Three surfaces were considered and rejected. The message cannot carry it - claude-messages.ts matches that string exactly to keep the fence a 503 instead of remapping it to an Anthropic 529, so a changed message would tell Claude Code to back off from an upstream that was never involved. A response header cannot reach /api/logs, which reads only error.message out of the body, and the Claude surface rebuilds its headers from scratch. Threading a reason into codexMainProfileDrainingResponse would have meant editing all five of its zero-arg call sites. stdout reaches every path this fence fires on, and is the stream the original report was already quoting from. The turn-drain claim race throws this same error while the gate reads ready. That site stays silent rather than borrowing a startup reason, which makes a reasonless 503 mean "not the startup fence" - a distinction the report could not make. Only the reason is logged; the snapshot's homeId is derived from a profile directory path. Refs #2108 Verification: red-driven with the fix staged as a no-op export (2 fail on reason === undefined, 1 pass on the silence case), then 80 pass / 0 fail / 470 expect() across the five native-main gate suites, 124 pass / 0 fail on the Claude and chat surfaces, tsc --noEmit exit 0, privacy:scan exit 0. An independent reviewer mutation-tested the new assertions in a scratch tree and confirmed each goes red when the behavior it pins is removed. Suites ran on macOS, so the owner-unavailable icacls branch is proven as plumbing rather than field behavior. --- .../120_2108_phase1.md | 174 ++++++++++++++++++ src/codex/auth-context.ts | 46 ++++- src/codex/native-profile-startup.ts | 4 + tests/codex-auth-context.test.ts | 61 ++++++ 4 files changed, 284 insertions(+), 1 deletion(-) create mode 100644 devlog/_plan/260819_unclaimed_bug_selection/120_2108_phase1.md 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/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(); + } + }); +}); From 50c6ccdbe534e11a7b598a14c1ec256958d18633 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Wed, 19 Aug 2026 23:07:32 +0900 Subject: [PATCH 4/5] docs(devlog): record the 2108 phase-1 implementation and the header the audit killed --- .../121_2108_implementation.md | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 devlog/_plan/260819_unclaimed_bug_selection/121_2108_implementation.md 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`. + From 745f6f6354578d293500281d8d7b61b821313c64 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Wed, 19 Aug 2026 23:27:11 +0900 Subject: [PATCH 5/5] docs(devlog): record the CI proxy-env leak and the ablation that killed the first theory --- .../130_ci_proxy_env_leak.md | 110 ++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100644 devlog/_plan/260819_unclaimed_bug_selection/130_ci_proxy_env_leak.md 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. +