diff --git a/devlog/_plan/260819_unclaimed_bug_selection/000_investigation.md b/devlog/_plan/260819_unclaimed_bug_selection/000_investigation.md new file mode 100644 index 0000000000..0c5b92dc6e --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/000_investigation.md @@ -0,0 +1,308 @@ +# 000 — Unclaimed bug issues: investigation + +Date: 2026-08-19. Scope: open `bug` issues with **no open PR planning to close +them**, for work starting after stage 3d of `260819_next_roadmap/070`. + +## Candidate derivation (re-derived live, not copied) + +Method: list every open `bug` issue, then scan every open PR's title+body for +`#NNNN` references and subtract. + +``` +open bug issues (19): + 1049 1225 1419 1527 1587 1688 1730 1852 1924 1933 + 1939 2047 2074 2092 2097 2106 2107 2108 2114 + +referenced by some open PR: + 1225(#2041) 1527(#2054) 1688(#2032) 1852(#1876) 1924(#2027) 1939(#2029) + 2047(#2056,#2062) 2074(#2082) 2092(#2099) 2097(#2101) 2106(#2112) + +first pass => UNCLAIMED (8): 1049 1419 1587 1730 1933 2107 2108 2114 +``` + +Timestamp of the derivation: `2026-08-19T11:45:42Z`. + +### Correction: reference-counting is not claim-counting (9, not 8) + +An audit lane re-derived this independently and returned PARTIAL. Nothing was +missing from the union, but **one exclusion was wrong**. + +A `#NNNN` in a PR body proves a *mention*, not an intent to close. PR **#2054** +mentions #1527 and then says, verbatim: + +> Does not close #1527. ... Refs #1527 (residual) + +Its purpose is Cursor conversation-checkpoint reuse, which relieves some of the +token pressure behind #1527 but is not the reported failure (large-context +turns collapsing or rate-limiting while direct Cursor stays healthy). The +author explicitly reserved the residual. + +**`#1527` is therefore still unclaimed. The set is 9, not 8.** + +The other ten pairs were each read individually and are strong — every one is +"Fixes/Closes/Implements #N" with a diff whose whole purpose is that issue: +#2041→1225, #2032→1688, #1876→1852, #2027→1924, #2029→1939, #2056 and #2062 +both→2047, #2082→2074, #2099→2092, #2101→2097, #2112→2106. + +**Method note worth keeping.** The cheap derivation (scan for `#NNNN`, +subtract) is a *starting* filter, not the answer. It over-excludes exactly +where an author was being honest about scope — which is the opposite of what a +triage pass should punish. Any future run of this must read the referencing PR +and ask whether it intends to close the issue. + +## Method + +Eight read-only subagent lanes, one per candidate. Each was told to read the +full issue thread, locate the responsible code in the current tree, and report +a mechanism with `file:line` — or say CANNOT-DETERMINE rather than guess. + +Two lanes had to be re-dispatched (the first batch went silent past three wait +cycles, DISPATCH-RETIRE-01). One candidate, `#1587`, ended up with two +independent lanes, which turned out to be useful: they agreed on the mechanism +and one of them produced a measurement the other did not. + +## Findings + +### #2114 — native-main 503 when systemctl exists but the user bus does not + +**Mechanism (confirmed, two independent lanes).** `inspectSystemd()` maps only +`spawnFailed` to `absent`. Any non-zero `systemctl --user show` exit becomes +`unknown`: + +``` +src/service-manager-probe.ts:267 if (shown.spawnFailed) return { kind: "absent" }; // #1612 fix +src/service-manager-probe.ts:269 if (shown.status !== 0) return unknown(...) // this bug +``` + +That `unknown` then closes native traffic for the life of the process: +`ownership-preflight.ts:155` → `ownership: "unknown"` → `server/index.ts:702` +→ `blockNativeMainStartupForUnownedServiceHome` → `auth-context.ts:313` → +`CodexMainProfileDrainingError` → 503. + +**Why #1612 missed it.** #1612 covered *spawn* failure — `systemctl` not on +PATH. Here spawn succeeds and the bus is unreachable, so the escape hatch does +not apply. + +**Regression, in two steps.** `a2e4fcf47` (2026-08-11) made +`ownership: unknown` block native-main at all; `bb45902ef` (2026-08-15, #1612) +relieved only the spawn branch. This environment has been broken since the +fence shipped. + +**Blast radius.** Linux only, Codex integration enabled. Any host where +`systemctl` is present but the user bus is not: systemd-containing Docker / +devcontainer images under tini, and plausibly WSL without `systemd=true`. +Affected users get **100% native-OpenAI failure**, not degradation. + +**Workaround.** Remove `systemctl` from the proxy's PATH — verified by the +reporter as a single-variable control. Setting `XDG_RUNTIME_DIR` does not help. + +**Evidence.** Strong. Exact stderr, exit code, single-variable isolation, and a +traced chain that matches the source line for line. The repo currently **pins +the bug**: `tests/codex-service-manager-probe.test.ts:277` asserts that +`status: 1, stderr: "Failed to connect to bus"` is `unknown`. + +### #2108 — Windows reboot leaves the native-main gate stuck + +**Mechanism (partially determined — and the lane was right to say so).** The +503 is the same process-wide fence as #2114, but the *trigger* is not logged, +so two candidates remain: + +1. **Owner ACL fail-closed.** A second `ETIMEDOUT` in the icacls hardening is + terminal (`native-main-owner.ts:272`), and `observeOwner()` settles the gate + to `owner-unavailable` and stops (`native-profile-startup.ts:227`). The ACL + module's own comment already records this exact symptom. The reporter's first + 503 is ~74s after wrapper start, past the ~60s owner budget. +2. **Probe fail-closed.** `SERVICE_PROBE_TIMEOUT_MS` is 2000ms. A + scheduler-only install still runs `sc.exe query` for WinSW; if that times out + with the WinSW assets absent, `walkWinswChain()` returns `unknown` instead of + `absent`. + +**The important structural finding:** `startServer()` takes a **one-shot** +ownership verdict and never retries it (`server/index.ts:702-710`). That is why +the gate stays closed until `ocx restart` and why waiting does not help. + +**What it is not.** The lane disproved two plausible readings: the +"did not shut down cleanly" log line is the *injection* journal +(`codex/journal.ts:209`), not the native-profile journal; and disk +`manual-recovery` residue would survive a restart, which contradicts the +reporter's restart-cures-it observation. + +**Relationship to #2114.** Same fence, different trigger. #2114 is deterministic +and restart does not help; #2108 is transient and restart does help. They share +the *unknown → permanent fence* layer, which is the reusable fix. + +### #2107 — WSL 502 after service install + +**Mechanism (confirmed, and it is not what the title suggests).** This is +**not** the #2108 gate and **not** a WSL loopback problem. Codex reached +OpenCodex fine; OpenCodex could not reach ChatGPT. + +`buildUnit()` (`service.ts:2418-2444`) bakes `OCX_SERVICE`, Bun provenance, +`PATH`, `CODEX_HOME`, `CODEX_SQLITE_HOME`, `OPENCODEX_HOME` — and **no proxy +variables**. systemd does not inherit the installing shell's environment, and +`ExecStart=/bin/sh -lc` is dash on Ubuntu WSL, which reads `.profile`, not +`.bashrc`. So a user whose proxy lives in `.bashrc` gets a service that talks +to ChatGPT directly, and the socket is reset → `fetchWithResetRetry` exhausts → +502 `Provider unreachable`. + +The distinguishing evidence is the status code itself: #2108 is **503** with +the native-main string; this is **502** with `recoveryKinds: ["connection-reset"]`. + +**Same hole in launchd and the Windows wrapper** (`service.ts:392-407`, +`1516-1533`), though Windows logon tasks often already carry user env. + +**Regression.** No — `git log -S HTTP_PROXY -- src/service.ts` is empty. This +has always been true; it only shows up when a proxy is required. + +### #1933 — Windows tray registration reported foreign/stale + +**Mechanism (confirmed).** Not a missing-file problem despite the title. The +title is a *collapsed summary string*, and the real cause is a text encoding +bug. + +`runRegistry`/`runRegistryAsync` decode `reg.exe` output with +`encoding: "utf8"` (`src/tray/windows.ts:120-125`, `335-345`). Redirected +`reg query` emits the console ACP, not UTF-8. The reporter's username is +`MötzJensen`; `ö` is `0xF6` in Windows-1252 and decodes to `U+FFFD`. The +round-trip comparison `registered === state.runCommand` then fails, and +`registrationOwned` goes false → the stale summary. + +**This is a known class with an existing fix that was never wired here.** +`decodeWindowsTextBytes` (`src/lib/windows-text.ts:75`) already solves exactly +this for `schtasks` (#1573, with a `C:\Users\Jörg` fixture). The tray reader was +missed. + +**Blast radius.** Windows users with non-ASCII in the profile path or +`OPENCODEX_HOME` on a non-UTF-8 ACP. Also blocks the GUI repair path: Install +is hidden when `tray.stale`, and uninstall refuses on a mismatched parse. + +### #1587 — routed first-turn tool catalog is 3-5x native + +**Mechanism (confirmed by two independent lanes).** `buildTools()` +(`src/responses/parser.ts:155`) never reads Codex's `defer_loading` flag. +`pushFn` and the namespace flattener copy every tool's full `parameters` into +`OcxTool`, and the flag is not on the type, so it is gone by parse time. The +routed adapters then serialize all of them (`openai-chat.ts:1197`, +`anthropic.ts:740`, `google.ts:270`). + +The native path is asymmetric **on purpose**: `openai-responses.ts:406` +preserves `defer_loading` and strips it only when a `tool_search_output` +actually loads the tool. + +**Measured, not asserted.** One lane ran this tree's real `parseRequest` +against a captured Codex Desktop catalog: + +| Sample | Deferred tools | Deferred bytes | After parse | +|---|---|---|---| +| 2026-08-12 rollout | 8 of 8 | 32,927 / 34,404 (**95.7%**) | all 8 emitted with full schemas, 32,887 bytes (~8.2k tokens), zero defer flags surviving | +| second sample | 4 namespaces / 10 tools | 24,227 bytes (~6.1k tokens) | same | + +**A caveat both lanes raised.** The headline "3-5x" is not a clean byte +multiplier: the thread's numbers compare *different tokenizers* (OpenAI vs Kimi +vs Claude) and the Opus row also carried a repo `AGENTS.md`. The mechanism is +real and measured; the exact ratio is not. + +**Regression.** No. Flattening was added 2026-06-19 so chat models could call +MCP tools; the routed path never honored deferral. + +### #1730 — Camel DeepSeek V4 Flash first-round tool call + +**Already half-fixed, and the reporter withdrew the rest.** The shared +conversion half — every converted custom tool getting a generic +`input.description`, which broke `exec` — was fixed by `ea0608611` (#1763), an +ancestor of the current head. The current tree special-cases `exec` at +`custom-tool-compat.ts:73`. + +The remaining claim (a first-round structured-tool miss) is **CANNOT-DETERMINE +as an OpenCodex defect**: there is no Camel code in the tree, the passthrough +forwards the client's `tool_choice` unchanged, and the reporter's own local +`required` patch proves the *model* will tool-call when forced — not that we +dropped a call. The reporter later attributed it to a config error (Responses +override against a Chat Completions host) and asked to close. + +**Proposed action: close as reporter-withdrawn.** Do not implement the +suggested `stream.camelai.com` + `deepseek-v4-flash` hardcode: a hostname/model +special case changing tool-selection semantics for every user of that route, +with no public contract and the reporter now opposing it. + +### #1419 — macOS Bun SIGTRAP after TLS failure + +**CANNOT-DETERMINE as our defect, and the lane was right to refuse.** The crash +is a native `EXC_BREAKPOINT` in Bun after a TLS handshake failure. +`installCrashGuards()` only hooks `unhandledRejection`/`uncaughtException` +(`crash-guard.ts:332`), so a native trap never reaches JS — which matches the +reporter seeing no `crash.log`. `unknown certificate verification error` is a +Bun string, not ours. + +**Bundled Bun is still 1.3.14** (`package.json:65`), and upstream's latest +release is still `bun-v1.3.14`, so there is nothing to bump into. #1691's Bun +1.4 train is blocked for the same reason. + +**One real, separable gap the lane found:** `ocx gui` spawns the proxy +detached and unsupervised (`cli/dispatch.ts:255`), while launchd `KeepAlive` +exists only for `ocx service`. That is a *survivability* fix we own, and it is +testable, unlike the trap. + +### #1049 — adopt pre-substrate Codex homes into the write coordinator + +**Still real; the substrate did not make it moot — it created the leftover +class.** `codexWriteCoordinationEligibility` returns `legacy-uncoordinated` +when there is no coordinator file and residue is not `clean` +(`inject-coordination.ts:46`), and inject/restore then write directly. The +specified adoption path is entirely absent: `adoption-pending` matches **zero** +times in `src/` and `tests/`, and the live schema CHECK does not include it. + +**Important for scheduling:** the lane split this into two phases and warned +that phase 2 is the invasive one — a wrong publish can corrupt the user's Codex +home, and Windows needs a real no-replace primitive rather than a POSIX +hardlink. It is also **not a field incident**: no user logs, no crash. The +`bug` label here marks a known gap, not a live failure. + +### #1527 — Cursor large-context collapse (added after the candidate correction) + +Investigated once the audit established that #2054 does not claim it. + +**What #2054 actually covers.** Process-local checkpoint reuse, so validated +linear follow-ups send `continuationMode=checkpoint` with `rootBytes=0` instead +of rebuilding history. Its own body says "Not run: #1527 large-context / 429 / +kimi-k3", and its live-transport change is capture-only. + +**Residual after it lands — five items, and they are not one bug.** + +1. `kimi-k3` premature completion at ~79-95k input: HTTP 200 with 4-36 output + tokens while direct `cursor-agent --model kimi-k3` produces ~10k at the same + scale. Not re-run on the checkpoint branch. +2. `claude-fable-5` 429 asymmetry vs direct Cursor. Unprovable either way today: + Connect does not expose `cache_read_tokens`, so usage stays estimated and + `cached_tokens: 0` cannot distinguish a cache hit from a miss. +3. **Teardown misclassification.** Normal completion never sets + `expectedClose` — only `cancelCursorRun()` does + (`src/adapters/cursor/live-transport.ts:738`) — while the abort listener + unconditionally `failAndClear("Cursor request was aborted")` (`:1157`), and + `"aborted"` is not benign (`cursor-errors.ts:74`). So a turn that already + emitted `turnEnded` still logs `turn-failed` / `expectedClose:false`. +4. First turn, restart, compaction and helper isolation still full-replay into + the 512 KiB / 192-blob envelope (`protobuf-request.ts:70`, `:68`). +5. Request-shape parity with official Cursor (e.g. `maxMode: false` at + `protobuf-request.ts:879`) is untested. + +**The useful finding:** item 3 is small, low-risk, and independently testable — +it is a misclassification in the abort listener, not a context-window mystery. +Items 1 and 2 are acceptance work that cannot start until #2054 lands, and item +2 may not be provable at all without an upstream field. + +**Evidence.** Strong that the OpenCodex Cursor path diverges from direct Cursor +and that abort classification is wrong. Partial that full replay *causes* the +429/kimi-k3 symptoms — #2054 assumes it and did not re-run the workload. + +## Cross-issue observation + +Three of the eight (#2114, #2108, and the ownership half of #1939, which +already has PR #2029) are the **same architectural fault**: a probe that cannot +answer produces `unknown`, and `unknown` is treated as permanent evidence of +foreign ownership for the life of the process. #2114 is the deterministic case, +#2108 the transient one. + +That is worth naming before ranking, because it changes what "fix #2114" means: +the cheap fix is one classification branch, but the shared fix is making the +fence retryable. They are different sizes and different risks. diff --git a/devlog/_plan/260819_unclaimed_bug_selection/010_ranking.md b/devlog/_plan/260819_unclaimed_bug_selection/010_ranking.md new file mode 100644 index 0000000000..2bfb39bc13 --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/010_ranking.md @@ -0,0 +1,249 @@ +# 010 — Ranking and selection + +Evidence: `000_investigation.md`. Criteria are stated first, then applied. + +> **Revised after audit.** Two corrections landed here: `#1527` was wrongly +> excluded from the candidate set (PR #2054 says "Does not close #1527"), and +> `#2114` is not greenfield — open PR #2029 already edits the same function and +> is `CHANGES_REQUESTED` for the exact hazard this unit rediscovered. Both are +> folded in below. + +## Criteria (stated before the ranking) + +1. **Severity of user-visible breakage.** Total loss of a path outranks + degradation, which outranks cost. +2. **Workaround availability.** A user who is stuck with no way out ranks above + one who has a documented escape. +3. **Blast radius.** Platform reach and what fraction of that platform's users + can hit it. +4. **Regression or long-standing.** A path that *used to work* and now does not + ranks above a gap that was never filled — we broke it, and someone upgraded + into it. +5. **Evidence quality.** Can a fix be written and verified from what is in the + thread today, without waiting on a reporter. +6. **Fix cost and risk.** Cheap and containable outranks invasive, at equal + severity. Risk of *causing* a worse failure counts against. + +Deliberately **not** criteria: issue age, comment count, or how loud the thread +is. Two of the strongest candidates here were filed today. + +## Ranked + +| # | Issue | Sev | Workaround | Radius | Regression | Evidence | Cost/Risk | +|---|---|---|---|---|---|---|---| +| 1 | **#2114** systemd bus | total native loss | yes, obscure | Linux containers/WSL | **yes** | strong | low, but **owned by PR #2029** | +| 2 | **#2107** proxy env | total upstream loss | yes | Linux/WSL behind a proxy | no | partial-strong | low | +| 3 | **#2108** Windows reboot | total native loss | yes, restart | Windows scheduler installs | **yes** | partial | medium | +| 4 | **#1587** deferred catalog | cost, every turn | lossy only | all platforms | no | strong+measured | medium, product risk | +| 5 | **#1933** tray encoding | feature unusable | partial | Windows non-ASCII paths | no | strong | **very low** | +| 6 | **#1527** Cursor residual | collapse at 80k+ | yes, use CLI | Cursor adapter users | no | strong on teardown, partial on cause | split: one small, rest acceptance | +| 7 | **#1049** substrate adoption | none observed | n/a | pre-substrate homes | no | strong mechanism, no incident | **high** | +| 8 | **#1419** Bun SIGTRAP | process death | yes, service | macOS + local TLS proxy | no | partial | not ours | +| 9 | **#1730** Camel | none | yes | one custom provider | no | withdrawn | close, do not patch | + +## Selected: #2114, #2107, #2108, #1587, #1933, and one slice of #1527 + +### Why #2114 is first + +It is the only candidate that scores worst-case on severity **and** regression +**and** evidence at once. A user in a systemd-containing container gets 100% +native-OpenAI failure, the path worked before `a2e4fcf47`, and the only escape +is to notice that hiding `systemctl` from PATH fixes it — which no one will +guess. + +The clincher is that the repository **currently asserts the bug is correct +behavior**: `tests/codex-service-manager-probe.test.ts:277` pins +`status: 1, "Failed to connect to bus"` → `unknown`. That test has to be +amended deliberately, which makes this a decision rather than a patch, and it +is the kind of decision that quietly ages badly if deferred. + +**But "first" means unblocking, not opening.** PR **#2029** (fixes #1939) +already edits `inspectSystemd()` and its test, and is `CHANGES_REQUESTED` +because a reviewer objected that a missing bus is not proof the unit file is +absent — the same fail-open hazard `020` independently arrived at. + +So #2114 and #1939 are one probe-policy decision with two symptoms: a refused +sync and a native 503. Ranking #2114 first is right; treating it as a fresh PR +would mean two people making the same fail-closed call in two places. + +### #1527 — added to the selected set, but only one slice + +It reached the candidate set late, so it is ranked on the same criteria rather +than grandfathered in. The residual after #2054 is five items, and they do not +share a cost: + +- **Take now:** the abort-teardown misclassification. Normal completion never + sets `expectedClose` (`live-transport.ts:738`) while the abort listener + unconditionally fails the turn (`:1157`), so a turn that already emitted + `turnEnded` still logs `turn-failed`. Small, independently testable, and + independent of #2054. +- **Defer:** the kimi-k3 collapse and the 429 asymmetry are **acceptance work** + that cannot start until #2054 lands, and the 429 half may not be provable at + all — Connect does not expose `cache_read_tokens`, so `cached_tokens: 0` + cannot distinguish a cache hit from a miss. + +Splitting it this way is the point: the issue as filed is unfixable in one +step, and one third of it is a clean small fix hiding behind two thirds that +need a live workload. + +### Why #2107 is second despite not being a regression + +Same severity class — the proxy cannot reach upstream at all — and the +mechanism is the cleanest of the eight: `buildUnit()` bakes six environment +variables and no proxy ones. It is a small, well-bounded change to a file we +own, and the same hole exists in the launchd and Windows builders, so one fix +closes three surfaces. + +It ranks below #2114 only because it is long-standing rather than a regression, +and because the affected population needs a proxy in the first place. + +### Why #2108 is third and not first + +Higher-profile platform, and the reporter is a Windows user hitting it on every +reboot. But: the trigger is **not identified** — the lane found two plausible +paths and could not distinguish them because the gate reason is never logged. + +That makes the honest first step *logging the reason*, not fixing a mechanism +we have not confirmed. It also shares the fence layer with #2114, so doing +#2114 first produces the retryable-fence groundwork this one needs. + +### Why #1587 is fourth + +It is the only candidate with a hard measurement: 95.7% of a real captured +catalog was deferred, and all of it was emitted anyway. That is a permanent tax +on every routed first turn for every user with connectors installed. + +It ranks below the three outages because it is cost rather than breakage, and +because the fix has genuine product risk in both directions: strip too much and +routed models lose plugin visibility (the #1522 class), strip too little and +nothing improves. The headline "3-5x" also does not survive scrutiny — the +thread compares three different tokenizers — so the goal should be stated in +bytes we control, not in a ratio. + +### Why #1933 is fifth despite being the cheapest + +The fix is close to trivial: route two `reg.exe` reads through +`decodeWindowsTextBytes`, which already exists and already has a +`C:\Users\Jörg` fixture from #1573. It is fifth only because the tray is not on +the request path — nobody's requests fail because of it. + +It is worth doing precisely *because* it is cheap: it closes a +known-class-missed-a-site bug, and leaving a fixed class half-applied is how +the next one gets missed too. + +## Deferred, with reasons + +## Audit challenge to this ranking, and what changed + +An audit lane argued the ranking is "wrong as a user-harm ordering — it listed +blast radius third, then let evidence and *we can patch today* pick the +winner." Three specific challenges. Two are accepted, one is not. + +### Accepted: #2108 outranks #2114 + +The lane is right. Both are total native loss and both are regressions. Windows +scheduler installs dwarf "Linux host where systemctl is present but the user +bus is not", and #2108 recurs **on every reboot** rather than once at install. +Ranking #2114 first because its trigger is known and a test pins it is +maintainer convenience dressed as impact. + +The "do #2114 first for fence groundwork" argument was also refuted separately +(see `040`): it was a preference, not a dependency. With that gone, nothing +defends the original order. + +**Revised: 1 #2108, 2 #2114, 3 #2107, 4 #1587.** Partial evidence on #2108 is +the reason its phase 1 is *logging*, not the reason to bury it at rank 3. + +### Accepted with a change of shape: #1049 returns as detection-only + +"Integrity bugs are silent; the first report is a corrupted Codex home" is a +better argument than the one this doc made. Waiting for a field incident is the +wrong posture for a data-integrity gap, and `000` already grades the mechanism +as strong. + +But the original deferral was not only about the incident count — the invasive +half can corrupt the thing it protects. Both concerns are satisfied by +splitting it: + +- **In:** phase 1, the atomic no-clobber publish for the ordinary clean + `{0,null}` row, plus refusing to write when adoption state is indeterminate. + That is detect-and-refuse; it reduces risk rather than adding it. +- **Out for now:** phase 2 (`adoption-pending` schema, the native handoff), + which is where the corruption risk lives and which needs a Windows + no-replace primitive we do not have. + +### Not accepted: drop #1933 + +The lane called #1933 "the ranking's worst trade" and would fold it into the +Windows pass. Folding it in is fine. **Dropping it is not**, and the reason is +not severity: + +`decodeWindowsTextBytes` already exists and is already wired into the service +probe. #1933 is that same class at a site that was missed. A class fix left +half-applied is how the *next* site gets missed, and the cost here is two call +sites plus a test that reuses an existing fixture. + +It also is not costless to the user: with a stale tray the GUI offers **no +repair path** (Install hidden, Uninstall refuses), so the person who hits it is +stuck without a documented manual registry edit. + +Accepting the fold: it rides the #2108 Windows pass rather than occupying its +own slot. + +### Final selection + +`#1527` has since been investigated (see `000`), and a fourth audit round found +that `#2114` is not greenfield: open PR **#2029** already edits +`inspectSystemd()` and is `CHANGES_REQUESTED` for the same fail-open hazard +`020` rediscovered. Folding both in: + +``` +1 #2108 Windows reboot gate phase 1 = log the reason; coordinate with PR #2101 +2 #2114 systemd bus UNBLOCK PR #2029 — do not open a parallel PR +3 #2107 service proxy env clean of open work +4 #1527 abort-teardown slice small, independent of #2054 +5 #1587 deferred catalog last: most contested files +6 #1049 phase 1 only atomic publish + refuse-on-indeterminate + #1933 folded into the #2108 Windows pass +``` + +Still deferred: `#1049` phase 2 (schema + native handoff, where the corruption +risk lives), `#1527`'s kimi-k3 and 429 halves (acceptance work that cannot +start until #2054 lands, and the 429 half may be unprovable while Connect hides +`cache_read_tokens`), `#1419` (upstream-blocked), `#1730` (close as withdrawn). + +**This supersedes the header table at the top of this document**, which records +the first-pass ordering before the audit rounds moved it. The table is kept +deliberately — the movement from it to here is the useful part. + +**#1049 — defer, and say why in the issue.** The mechanism is real and well +traced, but there is no field incident behind it: no user logs, no crash, no +report. Meanwhile the lane's phase 2 carries the highest risk in this entire +set — a wrong publish can corrupt a user's Codex home, and Windows needs a real +no-replace primitive rather than a POSIX hardlink. Spending that risk budget on +a gap with no observed failure, while three total-outage bugs are open, is the +wrong trade. Revisit when either a real incident arrives or the split program +has settled and there is appetite for careful substrate work. + +**#1419 — defer as upstream-blocked, keep needs-info.** The trap is inside Bun. +Bundled Bun is 1.3.14 and upstream's latest release is still 1.3.14, so there +is nothing to bump into, and the reporter never supplied the `.ips` frames that +would let us file a useful upstream issue. Do **not** weaken TLS verification to +work around it. + +One separable piece *is* ours and should be split out rather than lost: `ocx +gui` spawns the proxy detached and unsupervised while launchd `KeepAlive` only +covers `ocx service`. That is a survivability fix with a real test, and it is +worth its own small issue instead of riding a crash we cannot reproduce. + +**#1730 — close as reporter-withdrawn.** The half that was ours (`exec` losing +its description in custom-tool conversion) shipped in `ea0608611`. The remaining +claim has no OpenCodex mechanism, and the reporter attributed it to their own +Responses-vs-Chat-Completions misconfiguration and asked to close. The proposed +fix — a `stream.camelai.com` + `deepseek-v4-flash` first-round +`tool_choice: required` hardcode — would change tool-selection semantics for +every user of that route based on one host, with no public contract and the +reporter now opposing it. + +Closing it is a real outcome, not a dodge: it removes a `bug`-labelled issue +that would otherwise keep re-surfacing in triage as unclaimed. diff --git a/devlog/_plan/260819_unclaimed_bug_selection/020_2114_systemd_bus.md b/devlog/_plan/260819_unclaimed_bug_selection/020_2114_systemd_bus.md new file mode 100644 index 0000000000..df9cbc2024 --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/020_2114_systemd_bus.md @@ -0,0 +1,239 @@ +# 020 — #2114: native-main 503 when systemctl exists but the user bus does not + +**FIRST ACTION: extend or rebase open PR [#2029], do not open a parallel PR.** + +Starts after stage 3d of `260819_next_roadmap/070`. + +## This is not greenfield work — #2029 already owns this function + +An audit lane caught what the candidate filter could not: the filter asked +"does an open PR mention issue #2114", and the answer was no. It never asked +"does an open PR already edit `inspectSystemd()`", and the answer to that is +**yes**. + +``` +PR #2029 fix(probe): classify a missing user session bus as absent + reviewDecision: CHANGES_REQUESTED + files: src/service-manager-probe.ts, tests/codex-service-manager-probe.test.ts +``` + +#2029 fixes #1939 by classifying two D-Bus messages as `absent`, and +**deliberately keeps** `Failed to connect to bus` → `unknown` — which is +exactly the pin #2114 needs changed. It is blocked on a review objection that +this document independently rediscovered and wrote down as "test 3": a missing +bus is not proof that the unit file is absent, so a naive widening fails open +when a foreign unit is still on disk. + +So #2114 and #1939 are **one probe-policy decision with two user-visible +symptoms** — a refused sync (#1939) and a native 503 (#2114). Treating them as +two units means two people making the same fail-closed security-adjacent call +in two PRs, with the second one silently overwriting the first. + +**Consequence for ranking:** the work is real and still first, but it is +"unblock #2029 by supplying the containment its reviewer asked for", not "open +a new PR". The container/foreground gate below is the shape of that answer. + +## Failure mechanism + +> **STOP — read this before planning any work on this issue.** +> +> **Open PR #2029 already rewrites `inspectSystemd()`'s non-zero branch**, and +> it deliberately keeps the #2114 case as `unknown`. This doc was written as if +> that function were unowned. It is not. +> +> What #2029 actually does: +> +> ``` +> + err.includes("Failed to get D-Bus connection: No such file or directory") +> + ... "System has not been booted with systemd" +> + return { kind: "absent" }; +> + return unknown(...) // "other bus failures stay unknown" +> ``` +> +> and it adds a test that **cements the #2114 shape as `unknown`**: +> +> ``` +> + test("other bus failures stay unknown — the user manager may be running", () => { +> + stderr: "Failed to connect to bus: $DBUS_SESSION_BUS_ADDRESS not set", +> + expect(...kind).toBe("unknown"); +> ``` +> +> The #2114 reporter's stderr is +> `Failed to connect to user scope bus via local transport: $DBUS_SESSION_BUS_ADDRESS and $XDG_RUNTIME_DIR not defined` +> — which is exactly the family #2029 is choosing to leave closed. +> +> **Consequence: this is not a fresh patch, it is a conversation with #2029.** +> Either extend that PR's classifier to cover this stderr family, or land it and +> follow up on the same branch. Opening a competing PR means two changes fighting +> over one function, and the later one may silently re-pin the bug. +> +> This also changes the ranking: a fail-closed probe change is not "cheap" when +> an overlapping PR is already open on it. See `010`. + +``` +src/service-manager-probe.ts:267 if (shown.spawnFailed) return { kind: "absent" }; +src/service-manager-probe.ts:269-272 if (shown.status !== 0) { ... return unknown(...) } +``` + +(:269 is the `if`; the `return unknown(...)` is :272.) + +The comment on :270 says a non-zero status means "the question never reached the +bus" — which is exactly right, and is exactly why returning `unknown` is wrong. +A question that never reached the bus is evidence about the bus, not evidence +that a foreign service owns this home. + +From there the verdict is terminal for the process: + +| Step | File | +|---|---| +| `manager.kind === "unknown"` → `ownership: "unknown"` | `src/integrations/native/ownership-preflight.ts:155` | +| not `owned` → `blockNativeMainStartupForUnownedServiceHome("ownership-unknown")` | `src/server/index.ts:702-708` (:702 is the probe call, the block is :706) | +| snapshot blocked → `isNativeMainTrafficBlocked()` true | `src/codex/native-profile-startup.ts:351` | +| throws `CodexMainProfileDrainingError` | `src/codex/auth-context.ts:313`, `:318` | +| 503 `OpenCodex local native-main profile maintenance is active` | `src/codex/auth-context.ts:125-126` | + +## Why the existing #1612 fix does not cover it + +`bb45902ef` mapped **spawn** failure to `absent` — `systemctl` missing from +PATH. Here spawn succeeds and returns exit 1 with +`Failed to connect to user scope bus via local transport`. Same user-visible +outcome, different branch. + +## Fix shape + +**Primary change: one classification branch in `inspectSystemd()`.** + +Match bus-unreachable stderr specifically rather than widening every non-zero +exit, and gate the widening on an environment that already cannot host a user +service. The product already owns that signal and does not pass it to the +probe: `service.ts:3122` refuses service install when `/.dockerenv` exists and +reports `unsupported in Docker`. + +``` +if (shown.status !== 0) { + if (busUnreachable(shown.stderr) && deps.serviceHostingUnsupported()) { + return { kind: "absent" }; + } + return unknown(...) // unchanged for every other case +} +``` + +Thread the signal through `ProbeDeps` — the probe is already injectable +(`ProbeRunner`, `ProbeDeps`), so there is no call-site churn. + +### That snippet is unsafe as written — corrected + +An audit lane found the flaw and it is the important finding of this doc. +**With the bus down, `systemctl` cannot see a foreign unit either.** The +snippet returns `absent` on stderr + container signal alone, with no other +ownership evidence. A temporary bus outage inside a container that *does* host +a user service is exactly the fail-open the risk section warns about — and +test 3 below asserts a behavior the code shape cannot deliver. + +The classification must consult the **filesystem**, which does not need the +bus: + +``` +if (shown.status !== 0) { + if (!busUnreachable(shown.stderr) || !deps.serviceHostingUnsupported()) { + return unknown(...); // unchanged for every other case + } + // The bus could not answer. Ask the disk instead: a unit file is proof of + // installation that does not require a running bus. + const unit = deps.readUnitFile?.(UNIT_PATH); + if (unit === undefined) return { kind: "absent" }; // no unit, no owner + return unitOwnershipFrom(unit); // foreign stays foreign +} +``` + +`inspectSystemd()` already parses `FragmentPath` for the bus-answered path, so +the unit-file reader and the "does this unit name our home" logic exist in some +form; this reuses them on the offline path rather than inventing a second +notion of ownership. + +**Open decisions the implementer must make, which this doc cannot make for +them:** + +- `serviceHostingUnsupported()` **does not exist**. `service.ts:3122` is an + *install-time* `/.dockerenv` check. Whether the probe signal is Docker-only + or the broader "container/foreground" the risk section mentions is unset — + and it matters, because Podman and Kubernetes often have no `/.dockerenv`. +- `busUnreachable()` does not exist, and the locale policy is unchosen: match + strings, ignore stderr entirely, or force `LC_ALL=C` on the probe. +- The unit path constant and reader are not named here. + +**Stderr variants to match.** At minimum +`Failed to connect to user scope bus` and +`$DBUS_SESSION_BUS_ADDRESS and $XDG_RUNTIME_DIR not defined`. #1939 reports a +third shape, `Failed to get D-Bus connection`. Locale sensitivity is a real +weakness of string matching here and should be called out in the PR rather than +papered over; a non-English systemd will not match. If that is unacceptable, +the alternative is to key only on the container/foreground signal and ignore +stderr entirely — narrower, but locale-proof. + +**Files:** `src/service-manager-probe.ts`, `src/integrations/native/ownership-preflight.ts` +(signal plumbing only), `tests/service-probe-docker.test.ts`, +`tests/codex-service-manager-probe.test.ts`. + +**Coordination:** those are the same two files #2029 already changes. Rebase on +it or fold this into it; do not race it. + +## The test that must change, deliberately + +`tests/codex-service-manager-probe.test.ts:277` currently asserts +`status: 1, stderr: "Failed to connect to bus"` → `unknown`. **Amend it, do not +delete it**: keep that assertion for the non-container case, so the widening +stays honest. + +## Regression tests + +Red today, green after: + +1. Container signal set + `systemctl` spawn ok + exit 1 bus error + no unit + file → `{ kind: "absent" }` → `inspectNativeCodexOwnership` `owned` → + native-main not blocked. + +Must stay red (guards against over-widening): + +2. Same stderr, **no** container signal → still `unknown`. +3. Container signal + an installed unit naming a foreign home → still blocked. + +A `startServer` test injecting that probe result should return 503 today and +200 after. + +## Verification + +``` +bun test tests/service-probe-docker.test.ts tests/codex-service-manager-probe.test.ts +bun x tsc --noEmit +``` + +## Risk + +This is a **fail-closed security-adjacent boundary**. The failure mode of a bad +fix is admitting native-main on a host where a genuinely foreign unit exists but +was temporarily unqueryable. The container/foreground gate plus test 3 is what +keeps that closed. Do not widen all non-zero exits. + +**Correction:** the container gate alone does *not* keep that closed — that was +the audit's finding above. The disk check is what keeps it closed; the +container gate only limits where the offline path is taken at all. + +**Coverage limit to state plainly.** Even corrected, this fix only relieves +hosts that hit the container signal. WSL without `systemd=true`, bare SSH +sessions with no `XDG_RUNTIME_DIR`, CI runners, and Podman/k8s without +`/.dockerenv` keep returning `unknown` and keep 503-ing. If those matter, the +answer is #2108 phase 2's retryable fence, not a wider classifier here — which +is an argument for doing that work regardless of this fix. + +## Explicitly out of scope + +Two secondary bugs surfaced in the same thread. Both are real; neither should +ride this fix: + +- `ocx ready` / `/readyz` ignores `isNativeMainTrafficBlocked()` + (`src/server/index.ts:850`), so readiness can read ready while every native + request 503s. +- Codex CLI renders the local 503 as "Selected model is at capacity", because + the code is remapped to `server_is_overloaded` (`src/lib/errors.ts:229`). + The body is correct; the user-facing sentence is not. diff --git a/devlog/_plan/260819_unclaimed_bug_selection/030_2107_service_proxy_env.md b/devlog/_plan/260819_unclaimed_bug_selection/030_2107_service_proxy_env.md new file mode 100644 index 0000000000..4a186d55b4 --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/030_2107_service_proxy_env.md @@ -0,0 +1,93 @@ +# 030 — #2107: service unit drops outbound proxy env + +Rank 2. + +## Failure mechanism + +The title says "502 after service install in WSL", and both obvious readings +are wrong. Codex **did** reach OpenCodex; OpenCodex could not reach ChatGPT. + +`buildUnit()` (`src/service.ts:2418-2444`) bakes exactly `OCX_SERVICE`, Bun +provenance, `PATH`, `CODEX_HOME`, `CODEX_SQLITE_HOME`, `OPENCODEX_HOME`. No +`HTTP_PROXY` / `HTTPS_PROXY` / `ALL_PROXY` / `NO_PROXY`. systemd does not +inherit the installing shell's environment. + +`ExecStart=/bin/sh -lc` (`service.ts:457`) makes it worse in a way that is easy +to miss: on Ubuntu WSL `/bin/sh` is dash, and login dash reads `.profile`, not +`.bashrc` — where proxy exports usually live. + +`applyProxyEnv()` (`src/config.ts:3441`) only fills from `config.proxy`, so a +shell-only proxy is invisible to the service. + +Result: outbound TLS goes direct, the socket is reset, +`fetchWithResetRetry` exhausts (`src/lib/upstream-retry.ts:175`), and +`core.ts:2587` returns **502 `Provider unreachable`** with +`recoveryKinds: ["connection-reset"]`. + +## How to tell it apart from #2108 + +The status code is the discriminator, and it is worth writing down because the +two reports look identical in prose: + +| | #2107 | #2108 | +|---|---|---| +| status | **502** | **503** | +| body | `Provider unreachable` | `native-main profile maintenance is active` | +| log | `recoveryKinds: ["connection-reset"]` | native-main gate | +| cure | shim/direct start, or set `config.proxy` | `ocx restart` | + +## Why the shim works and the service does not + +`src/codex/shim.ts:692` runs `ocx ensure` in the interactive Codex shell, and +`src/cli/index.ts:431` spawns with `{ ...process.env }` — so `.bashrc` proxy +vars survive. That asymmetry is the whole bug. + +## Fix shape + +Bake the proxy keys into the generated unit, reusing what already exists: +`PROXY_ENV_KEYS` from `src/lib/proxy-env.ts` and the existing +`systemdEnvironmentAssignment()`. + +The same hole exists in `buildPlist` (`service.ts:392-407`) and the Windows +wrapper (`service.ts:1516-1533`). Fix all three in one change — they are the +same omission, and splitting them means two more reports. + +**Rules the implementation must follow:** + +- Do not emit empty `Environment=` lines for unset keys. +- Keep loopback on `NO_PROXY` the way `applyProxyEnv()` already does, or the + proxy will hairpin its own dashboard traffic. +- Do **not** switch `ExecStart` to an interactive `bash -ic`. That would fix + the symptom by making service startup depend on the user's interactive shell, + which is worse than the bug. + +**Two risks to state in the PR rather than discover later:** + +1. A WSL `WIN_HOST=$(ip route ...)` value is snapshotted at install time and can + change after `wsl --shutdown`. +2. Proxy URLs can carry credentials, and baking them writes those into a unit + file on disk. That is a privacy decision, not a detail — either redact, or + prefer `config.proxy` and document why. + +**Files:** `src/service.ts`, `tests/service.test.ts`. + +## Regression test + +With `HTTP_PROXY`/`HTTPS_PROXY`/`ALL_PROXY`/`NO_PROXY` set, `buildUnit()` +contains the matching `Environment=` lines; with them unset, those keys are +absent. Fails on current `dev`. + +## Verification + +``` +bun test tests/service.test.ts +bun x tsc --noEmit +``` + +## Documented workaround for the issue thread + +Set OpenCodex `config.proxy` — service start still runs `applyProxyEnv()`, so +this works today without any code change. `ocx doctor` already prints +"Current doctor process proxy env" vs "Running proxy process proxy env" +(`src/cli/doctor.ts:859`), which is the fastest way for a user to confirm the +diagnosis themselves. diff --git a/devlog/_plan/260819_unclaimed_bug_selection/031_2107_implementation.md b/devlog/_plan/260819_unclaimed_bug_selection/031_2107_implementation.md new file mode 100644 index 0000000000..290501098d --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/031_2107_implementation.md @@ -0,0 +1,80 @@ +# 031 — #2107 implementation record + +Branch: `fix/service-proxy-env` off `origin/dev` @ `18e072c8d`. +Commit: `eb910776a`. PR: **#2116** → `dev`. + +## What the plan said vs what the tree said + +`030` was written against a read of the code and held up on every point, with one +correction worth recording. + +**The plan asked for a guard that already exists.** It said "do not emit empty +`Environment=` lines". All three builders already drop falsy values before +joining — `systemdEnvironmentAssignment` returns `null`, +`buildPlist` uses ternaries, `windowsBatchSet` returns `null` — and each list is +`.filter(Boolean)`ed. So the risk was real in principle and already handled in +practice; adding a second guard would have been noise. + +That is the useful shape of this correction: the plan named a hazard from +reading a diff, and the tree had already solved it structurally. + +## The change + +One helper plus three call sites: + +``` +resolvedProxyEnv(env = process.env): { name, value }[] + for each of PROXY_ENV_KEYS (HTTP_PROXY, HTTPS_PROXY, ALL_PROXY, NO_PROXY) + value = env[KEY]?.trim() || env[key]?.trim() // either case + if (value) push({ name: KEY, value }) // canonical name only +``` + +| Builder | Line of insertion | +|---|---| +| `buildUnit` | after `opencodexHome`, mapped through `systemdEnvironmentAssignment` | +| `buildPlist` | after `OPENCODEX_HOME`, mapped to `/` | +| `buildWindowsServiceScript` | after `OPENCODEX_HOME`, mapped through `windowsBatchSet` | + +Reading both letter cases but writing only the upper-case name matters: curl-style +tooling sets `http_proxy`, and emitting both spellings into one definition would +leave two sources of truth for one setting. + +## Verification + +``` +bun test tests/service.test.ts 126 pass / 0 fail +bun x tsc --noEmit exit 0 +``` + +**Red-drive, recorded.** With the three `resolvedProxyEnv()` call sites stripped +out, the primary test fails on exactly the missing line: + +``` +Expected to contain: "Environment=\"HTTP_PROXY=http://127.0.0.1:7890\"" +(fail) bakes outbound proxy env into the unit ... (#2107) + 1 pass 1 fail +``` + +and nothing else in the file breaks. Restoring the fix returns 126/0. + +The companion test (no proxy in the shell → no proxy keys emitted) passes in both +states by design. It is not an oracle for the fix; it is a guard that the fix +cannot start emitting empty assignments later. + +## What this deliberately does not do + +- **No interactive `ExecStart`.** `bash -ic` would make service startup depend on + the user's interactive shell — a worse failure mode than the bug. +- **No `NO_PROXY` synthesis.** The runtime's `applyProxyEnv` already keeps + loopback off the proxy path; inventing a value here could diverge from it. +- **No credential handling decision.** A proxy URL can carry credentials, and + baking it writes that to disk. `config.proxy` already works for that case + without this change. Raised in the PR body as an open question rather than + silently resolved. + +## CI posture + +Not consulted. `dev` is mid-merge-train (30 commits in the window this work +started) and its checks are noisy by construction. Verification here is local +and complete for the changed surface; CI becomes the gate once the train +settles. diff --git a/devlog/_plan/260819_unclaimed_bug_selection/040_2108_windows_reboot_gate.md b/devlog/_plan/260819_unclaimed_bug_selection/040_2108_windows_reboot_gate.md new file mode 100644 index 0000000000..9b78b06300 --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/040_2108_windows_reboot_gate.md @@ -0,0 +1,127 @@ +# 040 — #2108: Windows reboot leaves the native-main gate stuck + +Rank 3. **This doc deliberately does not prescribe a mechanism fix first.** + +## What is confirmed + +The 503 is the process-wide native-main fence — same layer as #2114, different +trigger. `/healthz` never consults the gate (`src/server/index.ts:813`), which +is why the reporter sees 200 health + 200 Anthropic + 503 GPT and reasonably +concludes the proxy is fine. + +The structural cause is one line of policy: + +``` +src/server/index.ts:702-710 + owned -> startNativeMainStartupLifecycle() + anything else -> blockNativeMainStartupForUnownedServiceHome(...) // for the process lifetime +``` + +**`startServer()` takes a one-shot ownership verdict and never retries it.** +That is why waiting does not help and `ocx restart` does. + +## What is NOT confirmed, and why that matters + +The investigation found two plausible triggers and could not distinguish them, +because **the settled gate reason is never logged**: + +1. **Owner ACL fail-closed.** A second `ETIMEDOUT` in icacls hardening is + terminal — it publishes `{ status: "unavailable", reason: "lock-unavailable" }` + at `src/codex/native-main-owner.ts:205-212` — and `observeOwner()` settles to + `owner-unavailable` and stops (`native-profile-startup.ts:225-236`). The ACL + module's own comment already describes this exact symptom. Timing fits: the + first 503 is ~74s after wrapper start, past the ~60s owner budget. +2. **Probe fail-closed.** `SERVICE_PROBE_TIMEOUT_MS` is 2000ms. A + scheduler-only install still runs `sc.exe query` for WinSW; if that times out + with WinSW assets absent, `walkWinswChain()` returns `unknown` rather than + `absent` (`service-manager-probe.ts:732-736`). + +**Line numbers matter here.** An earlier draft cited `native-main-owner.ts:272`, +which is `if (released) return` inside `release()` — a fixer grepping that line +lands in the wrong function entirely. + +Two readings were **disproved** and should not be re-raised: the +"did not shut down cleanly" line is the injection journal +(`src/codex/journal.ts:209`), not the native-profile journal; and disk +`manual-recovery` residue would survive `ocx restart`, which contradicts the +reporter's own observation that restart cures it. + +## Phase 1 — log the reason (do this first, alone) + +Emit the concrete gate reason when the fence settles and when the 503 is +returned (`src/codex/auth-context.ts`, `src/codex/native-profile-startup.ts`). + +This is not a placeholder task. Without it the next reboot report is exactly as +ambiguous as this one, and we will be guessing between the same two candidates. +A shipped diagnostic converts the next occurrence into evidence. + +**Test:** the 503 log line includes the settled reason. + +## Phase 2 — make boot-time `unknown` retryable (after phase 1 has data) + +Treat `unknown` that came from a *timeout or unaskable manager* as retryable +while `OCX_SERVICE=1`, instead of a process-lifetime fence. Keep genuine +`foreign` fail-closed, and keep a retry cap. + +Two narrower fixes fall out and are worth doing regardless: + +- If WinSW xml **and** exe are absent, a timed-out `sc.exe query` must not mark + the machine `unknown`. +- A second ACL `ETIMEDOUT` on the service child should back off and retry rather + than settle terminal, so a warm icacls reopens the gate without `ocx restart`. + +## Tests that are currently green and encode the bug + +| Test | Asserts today | +|---|---| +| `tests/native-main-owner-lifetime.test.ts` | second `ETIMEDOUT` → terminal `unavailable` | +| `tests/codex-service-manager-probe.test.ts` | schtasks timeout → `unknown` | +| `tests/native-profile-startup.test.ts` | `ownership-unknown` blocks for the process | + +## Collision with open work + +**PR #2101** (`fix(codex): gate account-native models by entitlement`, 1397 +lines) already edits `src/server/index.ts` and `src/codex/auth-context.ts` — +both files phase 1 and phase 2 touch. Check its state before starting; the +reason-logging change in phase 1 is small enough to be folded in rather than +raced. + +A red-today regression: `startServer` on win32 with scheduler assets present, +first probe timed out and/or two owner ACL timeouts, then a later successful +probe in the **same** process — `POST /v1/responses` for a native model must go +503 → 200 without `process.exit`. Keep a control that a real foreign home stays +503. + +## Verification + +``` +bun test tests/native-profile-startup.test.ts tests/native-main-owner-lifetime.test.ts tests/codex-service-manager-probe.test.ts +bun x tsc --noEmit +``` + +Windows CI is authoritative here; a green macOS/Linux run proves little about +scheduler and icacls paths. + +## Sequencing note — corrected + +The first version of this section said "do #2114 first" and called it a +dependency. **An audit lane refuted that, and it was right.** The two fixes +touch different seams: + +- #2114 narrows one Linux classification in `inspectSystemd()`. +- #2108 phase 2 changes *fence policy* — a one-shot `unknown` becomes + retryable. + +Neither needs the other. #2114 does not implement retryability; phase 2 does not +classify the systemd bus. The shared `unknown → permanent fence` chain is a +shared *symptom*, and the overlap in `service-manager-probe.ts` is merge +convenience, not a prerequisite. + +So: **they can land in either order.** The preference for #2114 first was +"don't design the general rule from the instance we understand least", which is +a reasonable working habit and not a constraint. Stated as a dependency it +would have delayed the Windows fix for no technical reason. + +The one real ordering constraint here remains internal: **phase 1 before phase +2**, because the trigger is unidentified and phase 2 aims at one of two +candidates. diff --git a/devlog/_plan/260819_unclaimed_bug_selection/050_1587_deferred_catalog.md b/devlog/_plan/260819_unclaimed_bug_selection/050_1587_deferred_catalog.md new file mode 100644 index 0000000000..236d7d5ea9 --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/050_1587_deferred_catalog.md @@ -0,0 +1,124 @@ +# 050 — #1587: routed first-turn catalog ignores `defer_loading` + +Rank 4. The only candidate with a hard measurement. + +## Failure mechanism + +`buildTools()` (`src/responses/parser.ts:155`) never reads Codex's +`defer_loading` flag. `pushFn` (:163) and the namespace flattener (:195-207) +copy every tool's full `parameters` into `OcxTool`, and the flag is not on the +type — so it is gone by parse time. + +The routed adapters then serialize all of it: + +| Adapter | Site | +|---|---| +| chat | `src/adapters/openai-chat.ts:1197` | +| anthropic | `src/adapters/anthropic.ts:740` | +| google | `src/adapters/google.ts:270` | + +Non-OpenAI chat/Anthropic/Google additionally inject a `tool-catalog-nudge` +listing every flattened wire name in the system prompt +(`openai-chat.ts:636`). + +**The native path is asymmetric on purpose.** `openai-responses.ts:406` +preserves `defer_loading` and strips it only when a `tool_search_output` +actually loads the tool, so the ChatGPT backend keeps deferred schemas out of +the prompt. Chat and Anthropic wires have no server-side deferral, so the same +bytes land in billed context. + +## Measured, on a real captured catalog + +A lane ran this tree's actual `parseRequest` against a Codex Desktop catalog +captured from a session rollout: + +| Sample | Deferred | Catalog bytes | After parse | +|---|---|---|---| +| 2026-08-12 rollout | 8 of 8 tools | 32,927 / 34,404 = **95.7%** | all 8 emitted with full schemas, 32,887 bytes (~8.2k tokens), **zero** defer flags surviving | +| second sample | 4 namespaces / 10 tools | — | 24,227 bytes (~6.1k tokens) | + +## State the goal in bytes, not in the headline ratio + +The issue title says 3-5x. Both lanes independently flagged that this number +does not survive scrutiny: the thread compares **three different tokenizers** +(OpenAI 21,081 vs Kimi 62,319 vs Claude 98,402), and the Opus row additionally +carried a repo `AGENTS.md`. + +The mechanism is real and measured; the multiplier is not a clean +apples-to-apples figure. Success criteria should therefore be +**"deferred tools contribute no schema bytes to the routed catalog"**, verified +in serialized bytes we control — not "routed matches native within N%". + +## Fix shape + +1. Add `deferred?: boolean` to `OcxTool` and set it in `buildTools` + (`parser.ts:155-241`, both `pushFn` and the namespace path). +2. Clear it where `loadedToolSpecs` promotes a tool (`parser.ts:691-706`, which + already tracks `loadedFromToolSearch`). +3. In the three adapters, emit a **name + one-line description stub with empty + `parameters`/`input_schema`** for deferred tools instead of the full schema. + +**The constraint that makes this delicate:** `parser.ts:633` requires exact +wire names stay listed, or the model guesses names. So the stub must keep the +name and drop only the schema. A model may still call a stubbed tool with wrong +arguments before `tool_search` loads it — the `tool_search` round-trip +(`parser.ts:626-654`) has to be the recovery path, not an optional extra. + +**Files:** `src/responses/parser.ts`, `src/types.ts`, +`src/adapters/openai-chat.ts`, `src/adapters/anthropic.ts`, +`src/adapters/google.ts`, plus conformance tests. + +**Note on the split program — corrected.** `OcxTool` moves in **WP1 (#2019)**, +not WP1b: #2019's diff creates `src/types/tools.ts` and relocates `OcxTool` +there (verified: `git show origin/codex/split-wp1-types:src/types/tools.ts` +contains `interface OcxTool`). #2023 moves the accounts/config/provider/request +clusters. So the `deferred` field lands in `src/types/tools.ts` once **#2019** +is in, one PR earlier than this doc first said. + +**Bigger collision this doc originally missed.** The split trio is not the only +moving code on this surface. Live overlaps on #1587's exact files: + +| PR | Overlaps | +|---|---| +| **#1934** | `parser.ts`, `types.ts`, **and all three adapters** — #1587's entire surface | +| #2040 | `parser.ts` | +| #2115 | the three adapters | + +`#1934` is the real hazard, not the split. The 070 roadmap already schedules it +in phase B precisely because it overlaps. **#1587 should be planned after +#1934 lands**, or the two will conflict across five files. + +The original framing — "only #1587 collides, and only with the split" — was a +consequence of checking against the split branches and nothing else. + +## The test that currently pins the wrong behavior + +`tests/responses-tool-conformance.test.ts` **asserts** that namespace children +are flattened into top-level tools (`github.search` becomes top-level). The +correct regression is the opposite shape and must be added alongside an amended +version of that one. + +Red today, green after: a `defer_loading: true` namespace with fat MCP schemas +plus `exec`/`tool_search` → `toolsToChatFormat`/`toolsToAnthropicFormat` must +not include those children's schemas, while still listing their exact +namespaced wire names; serialized catalog bytes stay near the compact size; and +a `tool_search_output` promoting one restores its full schema on the next turn. + +## Verification + +``` +bun test tests/responses-tool-conformance.test.ts tests/responses-parser.test.ts +bun x tsc --noEmit +``` + +## Risk + +Real product risk in both directions. Strip too much and routed models lose +plugin visibility — the #1522/#1529 class, which is why the flattening was +added in the first place (2026-06-19, `6998fcaad`, so chat models could call +MCP tools). Strip too little and nothing improves. + +**Do not** re-stamp `supports_search_tool=false` as a shortcut: `fcbef381e` +showed that regresses `exec.description` from 96,699 to 258,929 chars. That is +a different expansion and would make the problem worse while appearing to +address it. diff --git a/devlog/_plan/260819_unclaimed_bug_selection/060_1933_tray_encoding.md b/devlog/_plan/260819_unclaimed_bug_selection/060_1933_tray_encoding.md new file mode 100644 index 0000000000..65cf0f2ec4 --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/060_1933_tray_encoding.md @@ -0,0 +1,99 @@ +# 060 — #1933: Windows tray registration misread as foreign/stale + +Rank 5. Cheapest fix in the selected set. + +## The title is a symptom string, not a diagnosis + +"startup registration is foreign, stale, or points to missing package files" is +a single collapsed summary produced by `trayStatusFrom()` +(`src/tray/windows.ts:440-462`). It does not mean three things were checked and +one failed; it means one boolean went false. + +## Failure mechanism + +`runRegistry` / `runRegistryAsync` decode `reg.exe` output with +`encoding: "utf8"` (`src/tray/windows.ts:120-125`, `335-345`). Redirected +`reg query` emits the console ANSI code page, not UTF-8. + +The reporter's username is `MötzJensen`. `ö` is `0xF6` in Windows-1252 and +decodes to `U+FFFD` under UTF-8. The round-trip check +`registered === state.runCommand` then fails, `registrationOwned` goes false, +and the stale summary is printed — even though regedit shows a correct, +well-formed, owned Run value. + +**The write side is fine** (CreateProcess is UTF-16). Only the read is broken. + +## This is a known class with an existing fix that was never wired here + +`decodeWindowsTextBytes` (`src/lib/windows-text.ts:75`) already solves exactly +this for `schtasks` — that was #1573, and it ships with a `C:\Users\Jörg` +fixture in `tests/windows-text-decoding.test.ts`. The tray registry reader was +simply missed. + +That is the argument for doing it now despite the low severity: a half-applied +fix for a known class is how the next site gets missed too. + +## Fix shape + +Capture a Buffer in `runRegistry`/`runRegistryAsync` and decode through +`decodeWindowsTextBytes`, the same helper the service probe uses. + +**Files:** `src/tray/windows.ts`, `tests/` (new case reusing the existing +fixture shape). + +**Known limitation to state, not hide:** `decodeWindowsTextBytes` does not +cover ja/zh code pages by design. This fix closes 1252 and CP949, not every +ACP. A fuller answer is `reg export` (UTF-16) instead of `reg query`, which is +a larger change and should be its own decision. + +**Do not** loosen the foreign-Run refusal (`tray/windows.ts:587`) to make the +symptom go away. That check is correct; it is being fed corrupted input. + +## Regression test + +Feed Windows-1252 (and CP949) `reg query` bytes for a path like +`C:\Users\MötzJensen\.opencodex\opencodex-tray.vbs` through the tray registry +reader and assert +`parseWindowsTrayRunValue(...) === buildWindowsTrayRunCommand(...)`. + +Today UTF-8-decoding those bytes makes +`windowsTrayRegistrationIsStale({ registered: true, registrationOwned: false })` +true. After the fix it must round-trip. + +## Verification + +``` +bun test tests/windows-text-decoding.test.ts tests/windows-tray.test.ts +bun x tsc --noEmit +``` + +(The tray suite is `tests/windows-tray.test.ts` — an earlier draft of this doc +named a `tests/tray-windows.test.ts` that does not exist. Related files: +`windows-tray-restart-hardening.test.ts`, `windows-tray-run-limit.test.ts`.) + +## Secondary UX gap worth a follow-up, not this fix + +The GUI cannot repair this state: Install is hidden when `tray.stale` +(`gui/src/pages/startup-sections.tsx:195`), and Uninstall is shown but also +refuses on a mismatched parse (`tray/windows.ts:687`). So a user in this state +has no in-product action. Worth splitting into its own issue — a stale tray +should always offer a repair path regardless of why it is stale. + +## Honesty note: the mechanism is proven, the attribution is inferred + +The encoding **mechanism** is verified in code — the tray reads `reg.exe` as +utf8 while the service probe already routes the same output through +`decodeWindowsTextBytes`. + +**Pinning this specific issue to it is an inference.** An audit lane checked +the thread: the reporter's GitHub *display name* is `Mötz Jensen`, the actual +profile path was never posted, and `C:\Users\MötzJensen` is reconstructed +rather than observed. The screenshots show the collapsed stale summary and a +German UI; the issue's own earlier review said they cannot distinguish a +foreign Run value from missing package files. + +Consequence for whoever picks this up: make the fix on class-hygiene grounds +(the helper exists, the site was missed), but **do not close #1933 on it** +without asking the reporter for `ocx tray status --json` and the raw Run value. +If their profile path is pure ASCII, this is the wrong diagnosis and the issue +stays open. diff --git a/devlog/_plan/260819_unclaimed_bug_selection/070_sequencing.md b/devlog/_plan/260819_unclaimed_bug_selection/070_sequencing.md new file mode 100644 index 0000000000..4a58560fa9 --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/070_sequencing.md @@ -0,0 +1,113 @@ +# 070 — Sequencing against the release roadmap + +This unit starts **after stage 3d** of `260819_next_roadmap/070` — that is, +after `#2019` and `#2023` merge, after the first preview soak, and after +`#2036` lands alone. + +## Collision analysis — corrected after audit + +The first version of this section asked only "does this fix touch a file the +**split** rewrites". That is the wrong question, and it produced a wrong +answer: "only #1587 collides." + +The right question is **"does an open PR already own this code"**, regardless of +whether that PR mentions our issue. Re-derived: + +| Fix | Files | Collides with | +|---|---|---| +| #2114 | `service-manager-probe.ts` + its test | **PR #2029** — same function, `CHANGES_REQUESTED` | +| #2108 | `server/index.ts`, `codex/auth-context.ts` | **PR #2101** (1397 lines, same two files) | +| #1587 | `types.ts`, `parser.ts`, 3 adapters | **#2019 and #2023** (split), plus #2112/#1934 on types, #2040/#2083 on parser, #2115/#2080/#2075/#2071/#2070 on the adapters | +| #2107 | `service.ts` | clean | +| #1933 | `tray/windows.ts` | clean | + +Three corrections fall out of that table: + +1. **#2114 is not greenfield.** See `020` — the first action is to extend or + unblock #2029, not to open a parallel PR. +2. **#1587 is worse than "after WP1b".** WP1 (**#2019**) already rewrites + `src/types.ts`, and the adapter files it touches are among the most + contested in the queue. It is the last of the five to start, not merely the + one that waits for the split. +3. **#2107 and #1933 are the only genuinely clean ones.** That strengthens the + case for running them in parallel rather than queueing them behind #2114. + +### Why still after stage 3d + +Unchanged for #1587 (`types.ts` is being replaced by a barrel). For the rest it +is scheduling, not correctness — the split train owns review attention until 3d +closes. + +## Order + +``` +1. #2114 unblock PR #2029 with the containment its reviewer asked for +2. #2107 bake proxy env into service units (clean, parallel-safe) +3. #1933 tray registry decoding (clean, parallel-safe) +4. #2108 phase 1 log the gate reason (coordinate with #2101) +5. #1527 residual: abort-teardown misclassification (small, independent) +6. #1587 deferred catalog (last: most contested files) +7. #2108 phase 2 retryable fence (after phase 1 produces data) +``` + +### Dependencies, stated explicitly + +- **#2114 before #2108 phase 2.** They share the `unknown → permanent fence` + layer. #2114 settles how a probe that cannot answer should be classified at + the boundary; phase 2 generalizes that into retryability. Designing the + general rule from #2108 first means deriving it from the instance we + understand least. +- **#2108 phase 1 before phase 2.** The trigger is not identified and the gate + reason is not logged. Phase 2 without phase 1 is a fix aimed at one of two + candidates with no way to confirm which. +- **#1587 after the split AND after the adapter PRs settle.** `#2019` rewrites + `types.ts`; `parser.ts` and the three adapters each have open PRs. This is the + one place where starting early guarantees a rewrite. +- **#1527 residual is independent of everything.** It only touches the Cursor + abort listener. It can slot anywhere, and should not wait for #2054 — the + teardown misclassification is orthogonal to checkpoint reuse. +- **#2107, #1933 are independent.** They can slot anywhere; they are placed by + cost, not constraint. + +### What can run in parallel + +#2107 and #1933 touch nothing the others touch and nothing each other touches. +If there is review capacity, they are the two to run alongside #2114 rather +than after it. + +## Relationship to the preview soak + +`#2114`, `#2107` and `#2108` are all "the proxy cannot serve a path" bugs, and +all three are hard to catch in CI: they need a container without a user bus, a +shell-only proxy, and a Windows reboot respectively. None of those exist on a +runner. + +That makes them **good soak candidates and bad CI candidates**. The 070 roadmap +already establishes a preview window with a named exercise set; these three +should extend it: + +- a container run with `systemctl` present and no user bus (#2114) +- a service install where the proxy env lives only in the shell (#2107) +- a Windows reboot with the scheduler backend (#2108) + +Adding those three to the soak checklist is cheaper than trying to simulate +them in CI, and it converts the next occurrence into a dated observation +instead of another ambiguous report. + +## What this unit does not do + +No `src/` changes, no PR merges, no GitHub mutations. The three deferred +candidates keep their disposition from `010`: #1049 waits for a real incident, +#1419 stays upstream-blocked with a separable supervision follow-up, and #1730 +is a close-as-withdrawn once someone is authorized to close it. + +## Follow-ups this unit identified but does not own + +Both were named in passing and would otherwise be lost. Each deserves its own +issue rather than riding a fix: + +1. **Unsupervised `ocx gui`** (`src/cli/dispatch.ts:255`) spawns the proxy + detached while launchd `KeepAlive` covers only `ocx service`. Separable from + #1419's untestable Bun trap, and unlike it, testable. +2. **Stale tray has no in-product repair path** — GUI hides Install when + `tray.stale` and Uninstall also refuses on a mismatched parse. diff --git a/devlog/_plan/260819_unclaimed_bug_selection/075_verification.md b/devlog/_plan/260819_unclaimed_bug_selection/075_verification.md new file mode 100644 index 0000000000..f2f7159f2b --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/075_verification.md @@ -0,0 +1,168 @@ +# 080 — Verification of this unit's own claims + +> Renumbered to `075` — this is the verification record that sits between the +> sequencing doc and the outcome. `080_outcome.md` is the close-out. +> +> **Update:** the retired lane was replaced. Two later lanes landed +> (`01a019f5` narrow, `01a019ed` wide) and their findings are folded into +> `000`, `010`, `020`, `040`, `060` and `070`. The direct re-verification below +> stands and was independently confirmed by those lanes; what it could not +> catch on its own was the code-ownership collision with PR #2029, which the +> wide lane found. + +An adversarial audit lane was dispatched against `000`-`070` and **went silent +past three wait cycles**. Under DISPATCH-RETIRE-01 that is a failed dispatch, +not a pass. Recording it as failed rather than quietly proceeding, and +re-verifying the load-bearing claims directly instead. + +## Directly re-verified + +### #2114 — the test really does pin the bug + +`tests/codex-service-manager-probe.test.ts`: + +``` +/** + * systemd does NOT signal absence through the exit code — a missing unit + * prints not-found and exits ZERO. A non-zero status means the question never + * reached the bus, which is the opposite conclusion. + */ +test("a non-zero systemctl status is unknown even though a missing unit exits zero", () => { + const { run } = recorder(() => ({ status: 1, stderr: "Failed to connect to bus" })); + expect(inspectServiceManagerInstallation({ run, platform: "linux", home }).kind).toBe("unknown"); +}); +``` + +CONFIRMED, and the comment is worth reading closely: the reasoning is +**correct** and the conclusion is still wrong for this environment. "The +question never reached the bus" is precisely why `unknown` is the wrong verdict +— an unanswerable question is evidence about the bus, not about who owns the +service home. The test is not sloppy; it encodes a genuine judgment that needs +revisiting for the container case, which is exactly why `020` says amend it +rather than delete it. + +### #2107 — `buildUnit` really omits proxy env + +`grep -n 'HTTP_PROXY\|HTTPS_PROXY' src/service.ts` returns **nothing**. +CONFIRMED. There is no proxy key anywhere in the service builder, which also +confirms the doc's claim that launchd and the Windows wrapper share the hole. + +### #1587 — the flag really is discarded + +`grep -n 'defer' src/types.ts` → empty. `OcxTool` has no such field. +`rg 'defer_loading' src/responses/` → empty. The parser never reads it. + +CONFIRMED on both halves, which is the part that matters: the measurement +(95.7% of a captured catalog) came from a lane and cannot be re-run here, but +the *code claim* it rests on is directly verifiable and holds. + +### #1933 — the encoding asymmetry is real + +``` +src/tray/windows.ts:122 encoding: "utf8", +src/tray/windows.ts:338 encoding: "utf8", +src/service-manager-probe.ts:29 import { decodeWindowsTextBytes } from "./lib/windows-text"; +src/service-manager-probe.ts:476 decodeWindowsTextBytes(queried.stdout, ...) +``` + +CONFIRMED. The helper exists, the service probe already uses it, and the tray +reader does not. This is the clearest "known class, missed site" in the set. + +### Sequencing — the collision analysis is complete + +Checked each selected fix's files against what the split PRs rewrite: + +| File | In split diff | +|---|---| +| `src/service-manager-probe.ts` | no | +| `src/service.ts` | no | +| `src/tray/windows.ts` | no | +| `src/codex/native-profile-startup.ts` | no | +| `src/codex/native-main-owner.ts` | no | + +CONFIRMED: `#1587` is the only collision, via `src/types.ts`. + +## What remains unverified, and is labelled as such + +- **The #1587 byte measurement.** 32,927 / 34,404 came from a lane replaying a + captured catalog through the real parser. Not reproduced here. The mechanism + is confirmed; treat the exact percentage as one sample. +- **#2108's actual trigger.** Two candidates, and the doc says so plainly. This + is a genuine gap, not an oversight — it is *why* `040` puts logging first. +- **The candidate-set completeness re-derivation.** The list was derived once + live (`2026-08-19T11:45:42Z`) and not independently re-derived by a second + party. A PR opened after that timestamp could claim one of these eight. Cheap + to re-check at start of work, and `070` should be re-read then rather than + trusted. + +## Note on lane reliability in this unit + +## Second audit round — the one that landed + +The first audit lane was retired as silent. It **returned late**, and four +narrow lanes were dispatched in parallel. All five verdicts are in, and they +found more than the direct grep pass did. Everything below was folded back. + +### The finding that changes the plan: #2114 is already owned + +**Open PR #2029 rewrites the exact function this unit planned to change**, and +deliberately leaves the #2114 case closed: + +``` ++ err.includes("Failed to get D-Bus connection: No such file or directory") ++ ... "System has not been booted with systemd" ++ return { kind: "absent" }; ++ return unknown(...) // "other bus failures stay unknown" + ++ test("other bus failures stay unknown — the user manager may be running", () => { ++ stderr: "Failed to connect to bus: $DBUS_SESSION_BUS_ADDRESS not set", ++ expect(...kind).toBe("unknown"); +``` + +#2114's reporter stderr is `Failed to connect to user scope bus via local +transport...` — the family #2029 is choosing to keep `unknown`. + +This invalidated three things at once: "#2114 is a cheap first fix", the +no-collision table, and the whole "do #2114 first" sequence. All three shared +one cause — **nobody checked who already owns `inspectSystemd()`.** + +### Corrections applied + +| Finding | Where | Fix | +|---|---|---| +| #1527 wrongly excluded (PR #2054 says "Does not close #1527") | `000` | set corrected to 9; method note added | +| The 020 fix snippet **fails open** — with the bus down, systemctl cannot see a foreign unit either | `020` | rewritten to consult the unit file on disk before returning `absent` | +| "#2114 before #2108 phase 2" is preference, not dependency | `040` | retracted; they can land in either order | +| #2108 should outrank #2114 (bigger platform, every reboot) | `010` | accepted; order revised | +| #1049 "no incident" is the wrong test for a silent integrity gap | `010` | accepted as detection-only phase 1 | +| `OcxTool` moves in **WP1 (#2019)**, not WP1b | `050` | corrected | +| #1934 overlaps **all five** of #1587's files | `050` | recorded as the real hazard | +| `tests/tray-windows.test.ts` does not exist | `060` | corrected to `windows-tray.test.ts` | +| `C:\Users\MötzJensen` was reconstructed, not observed | `060` | honesty note; do not close on the inference | + +### Rejected, with reason + +**"Drop #1933 from the selected set."** Folding it into the Windows pass is +accepted; dropping it is not. The helper already exists and is already wired +elsewhere — a half-applied class fix is how the next site gets missed — and the +GUI offers no repair path, so the affected user is stuck. + +### Still open after this round + +- **#1527** is unclaimed and **not investigated**. It arrived after the lanes + were dispatched. The selected set cannot be called final until it is. +- The **#1587 measurement** (95.7%) is a lane result that cannot be replayed + here. Mechanism confirmed; treat the number as one sample. +- Several **line citations drifted** (`native-main-owner.ts:272` is + `release()`; the second ACL timeout is nearer `:205`). Verify before quoting. +- Follow-ups this unit names and then drops: `/readyz` ignoring the native-main + fence, the Codex "at capacity" remap, and the `ocx gui` supervision split from + #1419 — none has an issue number. + +## Note on lane reliability in this unit + +Of eleven dispatches, two went silent in the first batch and one audit lane +went silent at the end. That is a meaningful failure rate and it changed how +this unit was built: the surviving evidence is per-issue lane reports plus +direct verification, not a single audited pass. Where a claim rests only on a +lane, this document says so. diff --git a/devlog/_plan/260819_unclaimed_bug_selection/080_outcome.md b/devlog/_plan/260819_unclaimed_bug_selection/080_outcome.md new file mode 100644 index 0000000000..d6c8ae6acb --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/080_outcome.md @@ -0,0 +1,95 @@ +# 080 — Outcome + +Terminal outcome: **DONE.** Selection and roadmap only; no `src/` change, no PR +merge, no GitHub mutation, no push. + +## Result + +Nine open `bug` issues have no PR intending to close them. Six are selected for +work after stage 3d, three are deferred with reasons. + +| Rank | Issue | Disposition | +|---|---|---| +| 1 | #2114 systemd bus | **unblock PR #2029**, not a new PR | +| 2 | #2107 service proxy env | take, clean of open work | +| 3 | #1933 tray encoding | take, clean of open work | +| 4 | #2108 Windows reboot gate | phase 1 (log the reason) first; coordinate with #2101 | +| 5 | #1527 residual | take only the abort-teardown slice | +| 6 | #1587 deferred catalog | last — most contested files | +| — | #1049 | defer: no field incident, phase 2 can corrupt `CODEX_HOME` | +| — | #1419 | defer: native Bun trap, no upstream release to move to | +| — | #1730 | close as reporter-withdrawn | + +## What the audits changed + +Two rounds ran. Neither merely agreed, and both errors were in the *method* +rather than in any individual finding. + +**The candidate filter was wrong in kind, not in execution.** It asked "does an +open PR mention `#NNNN`" and subtracted the matches. That over-excludes exactly +where an author was honest about scope: PR #2054 mentions #1527 and says +"Does not close #1527", so the filter counted an explicit disclaimer as a +claim. Reference-counting is not claim-counting, and the set was 9 rather +than 8. + +**The collision analysis asked the wrong question.** It checked "does this fix +touch a file the split rewrites" and concluded only #1587 collides. The +question that matters is "does an open PR already own this code", and the +answer changes the top of the ranking: **PR #2029 already edits +`inspectSystemd()`** — the exact function #2114 needs — and is +`CHANGES_REQUESTED` for the same fail-open hazard that `020` independently +rediscovered and wrote down as "test 3". + +So #2114 is still first, but "first" means supplying the containment #2029's +reviewer asked for. Left uncorrected, this unit would have sent someone to open +a second PR against a blocked one and make the same fail-closed +security-adjacent decision twice. + +Smaller corrections, worth recording because they are the kind that waste an +hour: `040` cited `native-main-owner.ts:272`, which is `if (released) return` +inside `release()` — the terminal `unavailable` is at `:205-212`. `060` named a +verification file that does not exist (`tests/tray-windows.test.ts`; the real +one is `tests/windows-tray.test.ts`). + +## What the investigation found that the titles did not + +Three of nine issues do not describe their own cause: + +- **#2107** reads as a WSL networking problem. It is `buildUnit()` baking six + environment variables and no proxy ones, so the service talks direct while + the shim inherits the user's proxy. The discriminator is the status code: + 502 with `connection-reset`, not #2108's 503. +- **#1933** reads as "missing package files". That phrase is a collapsed + summary string; the cause is `reg.exe` output decoded as UTF-8 when the + console code page is Windows-1252, and `decodeWindowsTextBytes` already fixes + this class for `schtasks`. +- **#1730** reads as an OpenCodex tool-call bug. The half that was ours shipped + in `ea0608611`; the reporter attributed the rest to their own configuration + and asked to close. + +And one issue produced a measurement rather than an argument: **#1587** — a +lane ran this tree's real `parseRequest` against a captured Codex Desktop +catalog and found **32,927 of 34,404 bytes (95.7%) deferred and emitted +anyway**. The issue's own "3-5x" headline does not survive scrutiny (it +compares three tokenizers), so the success criterion should be stated in bytes +we control. + +## Follow-ups this unit identified but does not own + +Both were named in a lane report and would otherwise vanish: + +1. Unsupervised `ocx gui` spawns the proxy detached while launchd `KeepAlive` + covers only `ocx service` (`src/cli/dispatch.ts:255`). Separable from + #1419's untestable trap, and unlike it, testable. +2. A stale tray has no in-product repair path: the GUI hides Install when + `tray.stale`, and Uninstall also refuses on a mismatched parse. + +## Method note for the next triage pass + +The cheap derivation — scan PR bodies for `#NNNN`, subtract — is a starting +filter, not an answer. Two checks have to follow it: + +1. **Read the referencing PR.** Does it intend to close the issue, or does it + say it does not? +2. **Check code ownership, not just issue references.** An issue with no PR + mentioning it can still have a PR sitting on the function that must change. diff --git a/src/service.ts b/src/service.ts index 8ba2b417b1..37e77506dd 100644 --- a/src/service.ts +++ b/src/service.ts @@ -19,6 +19,7 @@ import { BUN_RUNTIME_PATH_ENV, BUN_RUNTIME_SOURCE_ENV, durableBunRuntime } from import type { BunRuntimeSource } from "./lib/bun-runtime"; import { isProcessAlive, stopProxy } from "./lib/process-control"; import { serviceApiTokenFilePath } from "./lib/service-secrets"; +import { PROXY_ENV_KEYS } from "./lib/proxy-env"; import { randomUUID } from "node:crypto"; import { ELEVATION_REQUEST_TIMEOUT_MS, @@ -389,7 +390,7 @@ function writeServiceApiTokenFile(): string | null { return path; } -export function buildPlist(): string { +export function buildPlist(proxyEnv: { name: string; value: string }[] = resolvedProxyEnv()): string { const { bun, bunRuntimeSource, cli } = cliEntry(); const log = logPath(); const path = process.env.PATH ?? "/usr/local/bin:/usr/bin:/bin"; @@ -404,6 +405,8 @@ export function buildPlist(): string { codexHome ? ` CODEX_HOME${plistString(codexHome)}` : null, codexSqliteHome ? ` CODEX_SQLITE_HOME${plistString(codexSqliteHome)}` : null, opencodexHome ? ` OPENCODEX_HOME${plistString(opencodexHome)}` : null, + ...proxyEnv.map(({ name, value }) => + ` ${name}${plistString(value)}`), ].filter((line): line is string => Boolean(line)).join("\n"); const command = buildServiceShellCommand(bun, cli); return ` @@ -640,6 +643,31 @@ function systemdEnvironmentAssignment(name: string, value: string | undefined): return `Environment=${systemdQuote(`${name}=${value}`)}`; } +/** + * Outbound proxy settings the installing shell had, resolved for baking into a service + * definition. + * + * A service manager does not inherit the environment of the shell that installed it, and + * `ExecStart=/bin/sh -lc` is dash on Ubuntu/WSL — login dash reads `.profile`, not + * `.bashrc`, which is where proxy exports usually live. So a user who needs a proxy to + * reach the upstream got a service that dialed direct: the socket was reset, the retry + * budget drained, and the request surfaced as `502 Provider unreachable` (#2107). The + * same install driven through `ocx codex-shim` worked, because that path spawns with + * `{ ...process.env }`. + * + * Lower-case variants are honored because curl-style tooling sets them and the runtime's + * own `applyProxyEnv` already treats both cases as equivalent. Only the canonical + * upper-case name is baked, so a definition never carries two spellings of one setting. + */ +export function resolvedProxyEnv(env: NodeJS.ProcessEnv = process.env): { name: string; value: string }[] { + const resolved: { name: string; value: string }[] = []; + for (const key of PROXY_ENV_KEYS) { + const value = env[key]?.trim() || env[key.toLowerCase()]?.trim(); + if (value) resolved.push({ name: key, value }); + } + return resolved; +} + function systemdOutputTarget(value: string): string { // StandardOutput/StandardError use output specifiers such as append:/path. // Quoting the full specifier makes systemd reject it as an invalid output target. @@ -1531,6 +1559,7 @@ export function buildWindowsServiceScript(entry = cliEntry(), port = resolveServ windowsBatchSet("CODEX_HOME", process.env.CODEX_HOME?.trim(), "path"), windowsBatchSet("CODEX_SQLITE_HOME", currentCodexSqliteHomeAbsolute("windows"), "path"), windowsBatchSet("OPENCODEX_HOME", process.env.OPENCODEX_HOME?.trim(), "path"), + ...resolvedProxyEnv().map(({ name, value }) => windowsBatchSet(name, value)), windowsBatchSet("OCX_API_TOKEN_FILE", serviceApiTokenFilePath(), "path"), windowsBatchSet("OCX_SERVICE_LOG", serviceLogPath(), "path"), windowsBatchSet("OCX_BUN", bun, "path"), @@ -2415,7 +2444,7 @@ function unitPath(): string { return join(unitDir(), `${TASK}.service`); } -export function buildUnit(): string { +export function buildUnit(proxyEnv: { name: string; value: string }[] = resolvedProxyEnv()): string { const { bun, bunRuntimeSource, cli } = cliEntry(); const log = logPath(); const path = process.env.PATH ?? "/usr/local/bin:/usr/bin:/bin"; @@ -2430,6 +2459,7 @@ export function buildUnit(): string { codexHome, codexSqliteHome, opencodexHome, + ...proxyEnv.map(({ name, value }) => systemdEnvironmentAssignment(name, value)), ].filter((line): line is string => Boolean(line)).join("\n"); return `[Unit] Description=OpenCodex Proxy Server diff --git a/tests/service.test.ts b/tests/service.test.ts index d4e6b17465..32ba00c47c 100644 --- a/tests/service.test.ts +++ b/tests/service.test.ts @@ -7,6 +7,7 @@ import { saveConfig } from "../src/config"; import { windowsEnvIndirectBatchValue } from "../src/lib/win-paths"; import { assertServiceAuthEnvironment, assertServiceEnvironmentMatchesInstall, bakedServicePathsDiagnostic, confirmServiceServing, launchdListenPort, systemdListenPort, buildPlist, buildUnit, buildWindowsLauncherVbs, buildWindowsSchtasksCreateArgs, buildWindowsSchtasksCreateArgsForXml, buildWindowsServiceScript, buildWindowsTaskXml, deriveWindowsServiceDiagnostic, installFreshWindowsSchedulerSafely, installServiceSafely, launchctlLoadFailed, launchdJobMatchesPlist, normalizeServiceSubcommand, parseServiceInstallState, prepareServiceInstall, readWindowsSchedulerXmlState, registerFreshWindowsSchedulerTask, removeNativeWindowsServiceForScheduler, repairService, resolveServiceListenPort, runLaunchctl, serviceLogPath, serviceStartableFromTray, serviceStatusReport, serviceRetryCommand, serviceStatusSummary, systemdNeedsDaemonReload, windowsListenPort, winswListenPort, startLaunchd, windowsTaskRegistrationHealthy } from "../src/service"; import type { ServiceDiagnostic } from "../src/service"; +import { resolvedProxyEnv } from "../src/service"; import { buildWinswXml } from "../src/lib/winsw"; import { CONFIG_OWNER_FILE, CONFIG_UNINSTALL_MANIFEST, recordOwnedConfigPath, removeOwnedConfigState } from "../src/lib/config-ownership"; import { serviceApiTokenFilePath } from "../src/lib/service-secrets"; @@ -110,6 +111,52 @@ describe("systemd service unit", () => { expect(unit).not.toContain('StandardError="append:'); }); + test("bakes outbound proxy env into the unit so the service is not cut off from upstream (#2107)", () => { + // systemd does not inherit the installing shell's environment, and ExecStart runs + // /bin/sh -lc — which is dash on Ubuntu/WSL and reads .profile, not .bashrc. A user + // whose proxy lives in the shell therefore gets a service that dials upstream direct, + // the socket is reset, and the request surfaces as 502 Provider unreachable. + // + // The shell is passed in rather than assigned onto `process.env`. Mutating the real + // environment here leaked `HTTP_PROXY` out of this file: Bun runs a `bun test a b` + // invocation in ONE process, and the Lab sandbox calls `rejectProxyEnvironment()` on + // the live `process.env`, so every Lab file that loaded afterwards died with + // `harness_failure`. That was 73 failures on the unsharded macOS lane and zero when + // the Lab suites ran alone. + const proxyEnv = resolvedProxyEnv({ + HTTP_PROXY: "http://127.0.0.1:7890", + HTTPS_PROXY: "http://127.0.0.1:7890", + NO_PROXY: "localhost,127.0.0.1", + }); + + const unit = buildUnit(proxyEnv); + expect(unit).toContain('Environment="HTTP_PROXY=http://127.0.0.1:7890"'); + expect(unit).toContain('Environment="HTTPS_PROXY=http://127.0.0.1:7890"'); + expect(unit).toContain("NO_PROXY="); + // An unset key must not produce an empty assignment. + expect(unit).not.toContain('Environment="ALL_PROXY="'); + + const plist = buildPlist(proxyEnv); + expect(plist).toContain("HTTP_PROXYhttp://127.0.0.1:7890"); + expect(plist).not.toContain("ALL_PROXY"); + }); + + test("omits proxy env entirely when the installing shell has none (#2107)", () => { + const unit = buildUnit(resolvedProxyEnv({})); + for (const key of ["HTTP_PROXY", "HTTPS_PROXY", "ALL_PROXY"]) { + expect(unit).not.toContain(`${key}=`); + } + }); + + test("lower-case shell spellings are baked under the canonical name (#2107)", () => { + // curl-style tooling sets the lower-case pair; only the upper-case name is emitted so a + // definition never carries two spellings of one setting. + const unit = buildUnit(resolvedProxyEnv({ http_proxy: "http://127.0.0.1:7890" })); + + expect(unit).toContain('Environment="HTTP_PROXY=http://127.0.0.1:7890"'); + expect(unit).not.toContain("http_proxy="); + }); + test("preserves custom Codex and OpenCodex homes", () => { const oldCodexHome = process.env.CODEX_HOME; const oldCodexSqliteHome = process.env.CODEX_SQLITE_HOME;