From 75422fa6dba60092fa4cbd18dc51bbfc53a0c5bf Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 24 Jul 2026 01:37:02 +0900 Subject: [PATCH 1/8] =?UTF-8?q?docs(devlog):=20bugfix-train=20roadmap=20un?= =?UTF-8?q?it=20=E2=80=94=20000=20master=20plan=20+=20diff-level=20decade?= =?UTF-8?q?=20docs=20010-061?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Docs-only wp0 cycle for the 260724 bugfix train. Two-round adversarial audit applied (13 blockers folded: live PR336 head refresh, shim repair transactionality, cycle-1 file-set correction, mandatory activation matrices for 010/020/040, lexico split of PR337 investigation into 061, CYCLE6_BASE_SHA baseline rule). --- devlog/_plan/260724_bugfix_train/000_plan.md | 124 +++ .../010_passthrough_state.md | 678 +++++++++++++++ .../260724_bugfix_train/020_pool_retry_400.md | 704 +++++++++++++++ .../030_shim_autorestore.md | 707 +++++++++++++++ .../040_ux_discovery_badge.md | 817 ++++++++++++++++++ .../260724_bugfix_train/050_pr336_takeover.md | 413 +++++++++ .../260724_bugfix_train/060_pr337_takeover.md | 369 ++++++++ .../061_root_suite_investigation.md | 112 +++ 8 files changed, 3924 insertions(+) create mode 100644 devlog/_plan/260724_bugfix_train/000_plan.md create mode 100644 devlog/_plan/260724_bugfix_train/010_passthrough_state.md create mode 100644 devlog/_plan/260724_bugfix_train/020_pool_retry_400.md create mode 100644 devlog/_plan/260724_bugfix_train/030_shim_autorestore.md create mode 100644 devlog/_plan/260724_bugfix_train/040_ux_discovery_badge.md create mode 100644 devlog/_plan/260724_bugfix_train/050_pr336_takeover.md create mode 100644 devlog/_plan/260724_bugfix_train/060_pr337_takeover.md create mode 100644 devlog/_plan/260724_bugfix_train/061_root_suite_investigation.md diff --git a/devlog/_plan/260724_bugfix_train/000_plan.md b/devlog/_plan/260724_bugfix_train/000_plan.md new file mode 100644 index 00000000000..775c5b0f1d2 --- /dev/null +++ b/devlog/_plan/260724_bugfix_train/000_plan.md @@ -0,0 +1,124 @@ +# 000 — 260724_bugfix_train: Plan + +> Roadmap master for the bugfix train. One work-phase = one full PABCD cycle. +> Bound goalplan: `.codexclaw/goalplans/opencodex-bugfix-train-6-pabcd-cycles-bookkeepin/goalplan.json` +> (session 019f8f62-c778-7550-bd4c-aa4419fce259). + +## Objective + +Land every verified-unpatched bug from the 2026-07-23 issue sweep as reviewable PRs +against `dev`, and bring the two open contributor PRs to merge-ready quality. + +Evidence base: three Sol reviewer reports against origin/dev tip `d9e06c8d` +(issues #334/#326, issues #335/#331/#329/#324/#320/#241/#92, PRs #336/#337) plus a +3-Mind contradiction rescan (14 contradictions; 4 high folded into this plan as +corrections, 6 medium recorded as OPEN ASSUMPTIONS below). + +## Loop-spec + +- Loop archetype: verifier-defined (spec-satisfaction repair) for every phase. +- Trigger: owner directive 2026-07-24 — auto-resolve open questions via subagent + opinion, run repeated small PABCD cycles, push + open PR per cycle. +- Goal: cycles 1-6 each closed through D with branch pushed and PR opened; #324 + closed; #92/#241 status comments posted. +- Non-goals: upstream-blocked fixes (#92/#241 root causes), #42 Phase 2/3, all + feature-request issues, merging any PR (owner decision; gui/ merges need fresh + explicit approval). +- Verifier per cycle: `bun run typecheck` + `bun run test` + `bun run privacy:scan` + green; cycle 4/6 add `bun run lint:gui` + `bun run build:gui`; docs-site sync + decision recorded in the D summary. +- Stop condition: all goalplan criteria met, or a stated terminal outcome + (BLOCKED / NEEDS_HUMAN / UNSAFE / BUDGET_EXHAUSTED with evidence). +- Memory artifact: this unit + goalplan + `.codexclaw/ledger.jsonl`. +- Escalation: upward — main reclaims a slice after two distinct agent failures on + the same packet (DISPATCH-RETIRE-01); downward — pushing a slice to a worker is a + P-phase amendment, never mid-B improvisation. +- Resource bounds: tools = local git/gh/bun + GitHub via gh; write scope = the + ocx-bugfix-train worktree + contributor PR branches (maintainerCanModify verified); + push scope = per-cycle `codex/260724-*` branches and the two contributor branches + only (owner pre-approved for this exact scope). + +## Topology (corrected per contradiction scan) + +- One worktree: `/Users/jun/.codex/worktrees/ocx-bugfix-train` (deps installed). +- Per-cycle branches `codex/260724-` cut from CURRENT `origin/dev` — never + stacked on unmerged predecessors (keeps every PR independently reviewable). +- Cycles 5/6 fetch the contributor head branches and push fix commits there + (attribution preserved; no superseding PRs). +- The review worktree `codex-260723-issue-pr-review` stays pinned at d9e06c8d as + read-only reference; all post-cycle-1 verification runs in this worktree. + +## Work-phase map (dependency-ordered, PHASE-SPLIT-01) + +| WP | Doc | Slice | Depends on | +|-----|------|-------|------------| +| wp0 | 000 | This roadmap (docs-only cycle) | — | +| wp1 | 010 | #334 SSE output backfill + #326 idempotent guidance injection (file set per 010: src/server/relay.ts, src/types.ts, src/responses/parser.ts, src/server/responses/collaboration.ts — core.ts/state.ts explicitly untouched) | wp0 | +| wp2 | 020 | #335 bounded single pool retry on allow-listed account-specific 400 (SECURITY GATE) | wp1 | +| wp3 | 030 | #320 codex-shim auto-restore after external npm update (SECURITY GATE) | wp1 | +| wp4 | 040 | #329 discovery-error badge + #331 helper-fallback UX copy (gui) | wp1 | +| wp5 | 050 | PR #336 takeover: refresh to live head (87af85fb, 8 files, Cross-platform CI already GREEN), rebase onto post-wp1 dev, interdiff review, re-run gates. Expected textual overlap with wp1 is LOW (wp1 avoids core.ts); treat any collaboration.ts adjacency case-by-case | wp1 MERGED | +| wp6 | 060 | PR #337 takeover: interactive component tests + split 840-line component + root-suite failures | wp4 merged preferred (i18n overlap) | +| wp7 | — | Bookkeeping: close #324 (docs pointer), status comments on #92/#241 | wp1 | + +Dependency rationale: wp1 is the foundation (passthrough state machine; PR #336 +edits the same subsystem, so it rebases after wp1 lands to keep semantics +consistent even though direct textual conflict is expected to be low). wp2-wp4 +are independent capability lanes. wp5 is integration gated on wp1 merging. wp6 +prefers wp4 merged to avoid i18n hunk collisions. wp7 is hygiene. + +## wp0 artifact map (docs-only cycle, DIFFLEVEL-ROADMAP-01) + +| Action | Path (relative to repo root) | +|--------|------------------------------| +| NEW | devlog/_plan/260724_bugfix_train/000_plan.md (this file) | +| NEW | devlog/_plan/260724_bugfix_train/010_passthrough_state.md | +| NEW | devlog/_plan/260724_bugfix_train/020_pool_retry_400.md | +| NEW | devlog/_plan/260724_bugfix_train/030_shim_autorestore.md | +| NEW | devlog/_plan/260724_bugfix_train/040_ux_discovery_badge.md | +| NEW | devlog/_plan/260724_bugfix_train/050_pr336_takeover.md | +| NEW | devlog/_plan/260724_bugfix_train/060_pr337_takeover.md | +| NEW | devlog/_plan/260724_bugfix_train/061_root_suite_investigation.md (wp6 research artifact, split per LEXICO-SPLIT-01) | +| DELETE | devlog/_plan/260724_bugfix_train/010_phase1.md (scaffold, replaced by 010_passthrough_state.md) | +| DELETE | devlog/_plan/260724_bugfix_train/020_phase2.md (scaffold, replaced by 020_pool_retry_400.md) | +| DELETE | devlog/_plan/260724_bugfix_train/030_phase3.md (scaffold, replaced by 030_shim_autorestore.md) | +| DELETE | devlog/_plan/260724_bugfix_train/040_phase4.md (scaffold, replaced by 040_ux_discovery_badge.md) | +| DELETE | devlog/_plan/260724_bugfix_train/050_phase5.md (scaffold, replaced by 050_pr336_takeover.md) | +| DELETE | devlog/_plan/260724_bugfix_train/060_phase6.md (scaffold, replaced by 060_pr337_takeover.md) | + +No production code changes in wp0. Activation proof for this cycle: the A-gate +reviewer verdict (adversarial repo-grounded audit) + `git status` showing only +devlog/_plan additions + the D attestation in `.codexclaw/ledger.jsonl`. +devlog/ is gitignored with tracked content: force-add these artifacts when +committing (`git add -f devlog/_plan/260724_bugfix_train`). + +## OPEN ASSUMPTIONS (carried from Interview, recorded) + +1. #335 intentionally refines the 35d28a02 "4xx = caller, no failover" policy; PR + description must say so explicitly; the pinned no-penalty test stays green. +2. Done-gate extended beyond the user's "PR open": + privacy:scan + docs-site sync + decision per cycle (AGENTS.md requirements). +3. Cycles 5/6 push fix commits to contributor branches (maintainerCanModify=true); + no superseding PRs; original authors keep attribution. +4. #326 fix = idempotent injection (option a) with replayPrefixLen plumbed through + parseRequest; option (b) strip-at-persist rejected (5 call sites). +5. #334 fix = one shared SSE item-accumulator helper for both consumers; + trackSseForRequestLog needs no backfill (terminal-status only). +6. Cycles 2 and 3 carry security-review checkpoints (MAINTAINERS.md security + boundary); gui/ merges are outside this loop's authority. +7. (superseded assumption, corrected by A-gate round 1) PR #336's required CI was + NOT missing — the initial gap was the first-time-contributor approval gate; + Cross-platform CI is green on live head 87af85fb. wp5 scope is rebase + + interdiff + gates re-run, not CI restoration. + +## Accept criteria (mirrored in goalplan criteria[]) + +- c0: this roadmap closes its own PABCD cycle with reviewer PASS/near-pass. +- c1-c6: each cycle's branch pushed, PR opened, gates output captured. +- c7: #324 closed with docs-pointer comment; #92/#241 status comments posted. + +## SoT sync (SOT-SYNC-01) + +Repo SoT targets: `structure/` maintainer invariants + `docs-site/` user docs. +Each cycle's P names which docs-site page (if any) its behavior change touches; +the D summary records the sync decision. diff --git a/devlog/_plan/260724_bugfix_train/010_passthrough_state.md b/devlog/_plan/260724_bugfix_train/010_passthrough_state.md new file mode 100644 index 00000000000..f3d5996b4e7 --- /dev/null +++ b/devlog/_plan/260724_bugfix_train/010_passthrough_state.md @@ -0,0 +1,678 @@ +# 010 — Cycle 1: passthrough record/replay state machine (#334 + #326) + +> DIFFLEVEL-ROADMAP-01: this is the copy-paste-executable implementation PRD for Cycle 1. The implementer must keep the production and test paths, signatures, branch semantics, and verification gates below unless new repository evidence makes the design impossible. Any expansion is escalated to the main agent before editing. + +## Loop spec + +- **Loop archetype:** spec-satisfaction repair. +- **Trigger:** issue #334 reports that native Responses SSE persistence loses completed output items when `response.completed.response.output` is absent or empty; issue #326 reports that proxy-generated multi-agent guidance is persisted and then appended again on every `previous_response_id` continuation. +- **Goal:** make passthrough continuation recording reconstruct terminal output from `response.output_item.done` events when necessary, and make proxy-generated guidance injection idempotent across replayed prefixes, without altering client-facing SSE bytes or compaction ordering. +- **Non-goals:** redesign the Responses state store; strip guidance at persistence time; merge the inspection and metadata consumer loops; backfill request-log-only SSE tracking; synthesize items from delta events; change upstream payloads, adapter routing, provider continuation state, retention limits, snapshots, compaction behavior, or GUI/docs behavior. +- **Verifier:** `bun run typecheck`; `bun run test`; `bun run privacy:scan`. +- **Stop condition:** all focused regressions below pass, all three repository verifiers exit 0, native client SSE remains byte-identical, empty/missing terminal output is backfilled in `output_index` order for both persistence-capable consumers, non-empty terminal output remains authoritative, and a two-continuation replay contains exactly one proxy guidance item in outbound input and persisted state. +- **Memory artifact:** this file, `devlog/_plan/260724_bugfix_train/010_passthrough_state.md`, updated only by the owning main agent if implementation evidence forces a correction. +- **Expected terminal outcomes:** `PASS` (implementation and all verifiers satisfy this spec); `FAIL` (a verifier or acceptance assertion fails and the cycle returns to implementation); `BLOCKED` (the required behavior cannot be achieved inside the file/scope map and is escalated). +- **Escalation:** upward — main reclaims after two agent failures; downward — none planned. + +## Failure model and invariants + +### #334 — terminal response is not the whole streamed response + +`src/server/relay.ts` currently extracts only the `response` object carried by `response.completed`. Native Responses streams can instead finalize authoritative output items in earlier `response.output_item.done` events while the terminal event has `output: []` or no `output`. `rememberResponseState` accepts any array, including an empty one, so the current terminal-only callback either stores no assistant/reasoning/function-call items or does not store at all when `output` is absent. The next locally expanded continuation is therefore incomplete and can repeat a tool call forever. + +The repair law is: + +1. Observe only `response.output_item.done` events. +2. Store each valid item in one `Map` keyed by integer, non-negative `output_index`; later observations for the same index replace earlier ones. +3. On `response.completed`, leave a non-empty terminal `response.output` exactly untouched. +4. If terminal `output` is missing or an empty array and at least one done item was accumulated, provide a shallow response copy whose `output` is the accumulated items sorted by ascending index. +5. Invoke `onCompletedResponse` only after rule 3 or 4 has been applied. +6. Never rewrite, buffer, re-encode, or emit the client-facing SSE branch. + +The two persistence-capable consumers deliberately keep different control loops: + +- `consumeForInspection` stops inspecting post-terminal payloads only when there is no completion callback (`if (reported && !onCompletedResponse) continue`). Preserve that condition and route each payload that remains eligible through the accumulator. +- `consumeForResponseLogMetadata` never applies that skip. Preserve its loop and route every parsed payload through the same accumulator. +- `trackSseForRequestLog` is terminal-status/request-log tracking only. It has no `onCompletedResponse`, never calls `rememberResponseState`, and must not allocate or use the item accumulator. It needs no backfill. + +### #326 — replay provenance must reach injection + +`expandPreviousResponseInput` already records the replay prefix length in a proxy-private `WeakMap` keyed by the exact expanded request object. `parseRequest` reads that length for compaction-boundary logic, but discards it from `OcxParsedRequest`. `injectDeveloperMessage` therefore cannot distinguish replayed proxy guidance from a new request suffix and always appends another copy to both parsed messages and `_rawBody.input`. Passthrough persistence records the mutated `_rawBody`, producing one extra guidance item per continuation. + +The repair law is: + +1. Add optional proxy-private `OcxParsedRequest._replayPrefixLen?: number`. +2. `parseRequest` copies its already-read `replayedInputPrefixLength` into that field when the value is positive. +3. `injectDeveloperMessage` constructs the exact wire item it would inject, scans every item in `_rawBody.input.slice(0, _replayPrefixLen)`, and skips both parsed-message and raw-input insertion if an exact generated item is present. +4. “Exact generated item” means `{type:"message", role:"developer", content:[{type:"input_text", text}]}` with exactly one content part and the exact requested text. Do not match arbitrary developer messages, substrings, or suffix items. +5. Detection searches the whole replay prefix. It never assumes guidance is first, last, adjacent to a user item, or adjacent to `compaction_trigger`. +6. Fresh insertion retains the existing invariant: if the final raw item is `compaction_trigger`, insert immediately before it; otherwise append. +7. Do not implement the rejected persist-time stripping alternative. It would spread policy across five persistence call sites and enlarge the blast radius. + +## Exact file change map + +### Planning artifact changes for this cycle + +- **NEW** `devlog/_plan/260724_bugfix_train/010_passthrough_state.md` — this diff-level implementation contract. +- **DELETE** `devlog/_plan/260724_bugfix_train/010_phase1.md` — superseded empty scaffold. + +### Production and regression changes to implement + +- **MODIFY** `src/server/relay.ts` — add one shared stateful completion accumulator and use it from `consumeForInspection` and `consumeForResponseLogMetadata`; retain `completedResponseFromSsePayload` and leave `trackSseForRequestLog` unchanged. +- **MODIFY** `src/types.ts` — add `_replayPrefixLen?: number` to `OcxParsedRequest`. +- **MODIFY** `src/responses/parser.ts` — preserve the already-computed replay prefix length on the parsed request. +- **MODIFY** `src/server/responses/collaboration.ts` — detect the exact generated guidance item anywhere in the replay prefix and make dual-write injection idempotent. +- **MODIFY** `tests/responses-state.test.ts` — add focused stream/persistence tests for both relay consumers, terminal-output authority, and the two-continuation guidance regression. + +No other file is part of Cycle 1. In particular, do not modify `src/server/responses/core.ts`, `src/responses/state.ts`, `src/server/request-log.ts`, adapters, snapshots, docs, GUI, workflows, dependencies, or release automation. + +## Diff-level implementation + +### 1. `src/server/relay.ts` + +#### Keep the terminal extractor signature unchanged + +Current code: + +```ts +/** Extract the response object from a `response.completed` SSE payload, or null. */ +export function completedResponseFromSsePayload(payload: string): { id?: unknown; output?: unknown; status?: unknown } | null { + if (payload === "[DONE]") return null; + try { + const json = JSON.parse(payload) as { type?: unknown; response?: unknown }; + if (json.type !== "response.completed") return null; + const response = json.response; + if (!response || typeof response !== "object" || Array.isArray(response)) return null; + return response as { id?: unknown; output?: unknown; status?: unknown }; + } catch { + return null; + } +} +``` + +After: retain this function byte-for-byte. Add the following immediately after it. The new helper is file-private because only the two background consumers own this reconstruction responsibility. + +```ts +type CompletedResponse = { id?: unknown; output?: unknown; status?: unknown }; + +function createCompletedResponseAccumulator( + onCompletedResponse?: (response: CompletedResponse) => void, +): (payload: string | null) => void { + const itemsByOutputIndex = new Map(); + return payload => { + if (!payload || payload === "[DONE]") return; + try { + const event = JSON.parse(payload) as { + type?: unknown; + output_index?: unknown; + item?: unknown; + }; + if (event.type === "response.output_item.done") { + if (Number.isInteger(event.output_index) + && (event.output_index as number) >= 0 + && event.item !== undefined) { + itemsByOutputIndex.set(event.output_index as number, event.item); + } + return; + } + } catch { + return; + } + + let response = completedResponseFromSsePayload(payload); + if (!response) return; + if ((!Array.isArray(response.output) || response.output.length === 0) + && itemsByOutputIndex.size > 0) { + response = { + ...response, + output: [...itemsByOutputIndex.entries()] + .sort(([left], [right]) => left - right) + .map(([, item]) => item), + }; + } + onCompletedResponse?.(response); + }; +} +``` + +Implementation note: the first parse handles item-done observation; non-item events then flow to the existing terminal extractor. A malformed payload remains best-effort/no-throw. Do not export this helper and do not broaden `CompletedResponse` beyond the existing callback contract. + +#### Modify `consumeForInspection` + +Real signature (unchanged): + +```ts +export function consumeForInspection( + body: ReadableStream, + onTerminal: (status: ResponsesTerminalStatus, httpStatusOverride?: number) => void, + signal?: AbortSignal, + onDone?: () => void, + logCtx?: RequestLogContext, + onCancel?: () => void, + onCompletedResponse?: (response: { id?: unknown; output?: unknown; status?: unknown }) => void, + onFirstOutput?: () => void, +): void { +``` + +Current initialization: + +```ts + let reported = false; + let cancelled = false; + const reportFirstOutput = createFirstOutputReporter(onFirstOutput); +``` + +After: + +```ts + let reported = false; + let cancelled = false; + const reportFirstOutput = createFirstOutputReporter(onFirstOutput); + const observeCompletedResponse = createCompletedResponseAccumulator(onCompletedResponse); +``` + +Current EOF completion block: + +```ts + if (onCompletedResponse) { + const response = completedResponseFromSsePayload(payload); + if (response) onCompletedResponse(response); + } +``` + +After: + +```ts + observeCompletedResponse(payload); +``` + +Current normal-loop completion block: + +```ts + if (onCompletedResponse) { + const response = completedResponseFromSsePayload(payload); + if (response) onCompletedResponse(response); + } +``` + +After: + +```ts + observeCompletedResponse(payload); +``` + +Do not alter `if (reported && !onCompletedResponse) continue`, terminal reporting, logging, first-output reporting, cancellation, synthetic status behavior, or `onDone` ordering. The accumulator runs only on payloads the existing loop already permits. The normal event ordering (`output_item.done` before `response.completed`) guarantees backfill is complete before the callback. + +#### Modify `consumeForResponseLogMetadata` + +Real signature (unchanged): + +```ts +export function consumeForResponseLogMetadata( + body: ReadableStream, + logCtx: RequestLogContext, + signal?: AbortSignal, + onDone?: () => void, + onCompletedResponse?: (response: { id?: unknown; output?: unknown; status?: unknown }) => void, + onFirstOutput?: () => void, +): void { +``` + +Current initialization: + +```ts + let buffer = ""; + const reportFirstOutput = createFirstOutputReporter(onFirstOutput); +``` + +After: + +```ts + let buffer = ""; + const reportFirstOutput = createFirstOutputReporter(onFirstOutput); + const observeCompletedResponse = createCompletedResponseAccumulator(onCompletedResponse); +``` + +Replace both identical callback blocks (one in EOF residual handling, one in the normal block loop): + +```ts + if (payload && onCompletedResponse) { + const response = completedResponseFromSsePayload(payload); + if (response) onCompletedResponse(response); + } +``` + +and + +```ts + if (payload && onCompletedResponse) { + const response = completedResponseFromSsePayload(payload); + if (response) onCompletedResponse(response); + } +``` + +with, respectively: + +```ts + observeCompletedResponse(payload); +``` + +and + +```ts + observeCompletedResponse(payload); +``` + +Do not add a reported/terminal skip to this consumer. It must continue inspecting every payload for request-log metadata exactly as before. + +#### Explicit no-change: `trackSseForRequestLog` + +Real signature: + +```ts +export function trackSseForRequestLog( + body: ReadableStream, + onTerminal: (status: ResponsesTerminalStatus) => void, + onCancel: () => void, + logCtx?: RequestLogContext, + onFirstOutput?: () => void, +): ReadableStream { +``` + +Before and after are identical. This function only calls `terminalStatusFromSsePayload`, reports log terminal state, and relays bytes. It has no response-state callback. Adding the accumulator here would create unused state and blur the persistence boundary. + +### 2. `src/types.ts` + +Current excerpt: + +```ts +export interface OcxParsedRequest { + modelId: string; + previousResponseId?: string; + context: OcxContext; + stream: boolean; + options: OcxRequestOptions; + _rawBody?: unknown; + /** True when the proxy expanded a previous_response_id request into a full input replay. */ + _previousResponseInputExpanded?: boolean; +``` + +After: + +```ts +export interface OcxParsedRequest { + modelId: string; + previousResponseId?: string; + context: OcxContext; + stream: boolean; + options: OcxRequestOptions; + _rawBody?: unknown; + /** Number of leading raw input items restored from local previous_response_id state. */ + _replayPrefixLen?: number; + /** True when the proxy expanded a previous_response_id request into a full input replay. */ + _previousResponseInputExpanded?: boolean; +``` + +Keep the field optional so existing hand-built `OcxParsedRequest` fixtures remain valid. It is proxy-private metadata and must never be serialized into `_rawBody` or sent upstream. + +### 3. `src/responses/parser.ts` + +Real signature: + +```ts +export function parseRequest(body: unknown): OcxParsedRequest { +``` + +Current opening already captures provenance and remains unchanged: + +```ts +export function parseRequest(body: unknown): OcxParsedRequest { + const replayedInputPrefixLength = previousResponseReplayPrefixLength(body); + const parsed = responsesRequestSchema.safeParse(body); +``` + +Current return excerpt: + +```ts + return { + modelId: data.model, + ...(data.previous_response_id ? { previousResponseId: data.previous_response_id } : {}), + context, + stream: data.stream === true, + options, + _rawBody: body, + ...(webSearch ? { _webSearch: webSearch } : {}), +``` + +After: + +```ts + return { + modelId: data.model, + ...(data.previous_response_id ? { previousResponseId: data.previous_response_id } : {}), + context, + stream: data.stream === true, + options, + _rawBody: body, + ...(replayedInputPrefixLength > 0 ? { _replayPrefixLen: replayedInputPrefixLength } : {}), + ...(webSearch ? { _webSearch: webSearch } : {}), +``` + +Do not replace the existing `WeakMap`, add a wire field, or change the existing `inputIndex >= replayedInputPrefixLength` compaction-boundary test. This is a second consumer of the provenance already read at function entry. + +### 4. `src/server/responses/collaboration.ts` + +#### Add exact-item predicates immediately above `injectDeveloperMessage` + +After: + +```ts +function isRecord(value: unknown): value is Record { + return !!value && typeof value === "object" && !Array.isArray(value); +} + +function isGeneratedDeveloperItem(item: unknown, text: string): boolean { + if (!isRecord(item) || item.type !== "message" || item.role !== "developer") return false; + if (!Array.isArray(item.content) || item.content.length !== 1) return false; + const [part] = item.content; + return isRecord(part) && part.type === "input_text" && part.text === text; +} +``` + +If this file already has an equivalent record guard at implementation time, reuse it instead of adding a duplicate. The semantic check must remain exact as shown. + +#### Modify `injectDeveloperMessage` + +Real signature: + +```ts +export function injectDeveloperMessage(parsed: OcxParsedRequest, text: string): void { +``` + +Current function: + +```ts +export function injectDeveloperMessage(parsed: OcxParsedRequest, text: string): void { + parsed.context.messages.push({ role: "developer", content: text, timestamp: Date.now() }); + const raw = parsed._rawBody as { input?: unknown } | undefined; + if (raw && Array.isArray(raw.input)) { + const devItem = { type: "message", role: "developer", content: [{ type: "input_text", text }] }; + // compaction_trigger must remain the final input item (codex-rs + ChatGPT backend both + // validate this). Insert the developer message BEFORE the trigger when present. + const last = raw.input[raw.input.length - 1]; + if (last && typeof last === "object" && (last as { type?: string }).type === "compaction_trigger") { + raw.input.splice(raw.input.length - 1, 0, devItem); + } else { + raw.input.push(devItem); + } + } +} +``` + +After: + +```ts +export function injectDeveloperMessage(parsed: OcxParsedRequest, text: string): void { + const raw = parsed._rawBody as { input?: unknown } | undefined; + const devItem = { type: "message", role: "developer", content: [{ type: "input_text", text }] }; + if (raw && Array.isArray(raw.input)) { + const replayPrefixLen = Math.min(parsed._replayPrefixLen ?? 0, raw.input.length); + if (raw.input.slice(0, replayPrefixLen).some(item => isGeneratedDeveloperItem(item, text))) { + return; + } + } + + parsed.context.messages.push({ role: "developer", content: text, timestamp: Date.now() }); + if (raw && Array.isArray(raw.input)) { + // compaction_trigger must remain the final input item (codex-rs + ChatGPT backend both + // validate this). Insert the developer message BEFORE the trigger when present. + const last = raw.input[raw.input.length - 1]; + if (last && typeof last === "object" && (last as { type?: string }).type === "compaction_trigger") { + raw.input.splice(raw.input.length - 1, 0, devItem); + } else { + raw.input.push(devItem); + } + } +} +``` + +The early return is intentionally before the parsed-message push. `parseRequest` has already converted the replayed wire guidance into one developer message in `parsed.context.messages`; adding another parsed-only copy would still duplicate model context. If raw input is a string/non-array, preserve current behavior: parsed context receives the message and raw input is untouched. A matching item in the new suffix does not suppress injection because only replayed proxy state is trusted for idempotence. + +## Regression test plan + +All new tests go in **MODIFY** `tests/responses-state.test.ts`, whose existing `beforeEach`/`afterEach` isolate `OPENCODEX_HOME`, clear memory, and remove snapshots. Extend imports with `injectDeveloperMessage` from `../src/server/responses` and `consumeForInspection`, `consumeForResponseLogMetadata`, plus the request-log context type or a minimal cast from `../src/server`/the existing export surface. Add a local SSE stream builder and use an `onDone` promise rather than timing sleeps. + +Suggested shared fixture: + +```ts +function sseStream(events: Record[]): ReadableStream { + const encoder = new TextEncoder(); + return new ReadableStream({ + start(controller) { + for (const event of events) { + controller.enqueue(encoder.encode(`data: ${JSON.stringify(event)}\n\n`)); + } + controller.close(); + }, + }); +} +``` + +Use a structurally valid minimal `RequestLogContext` fixture already accepted by relay tests, or import its type and cast a local `{}` only if the inspector functions tolerate it for the selected events. Prefer the real existing helper if one is present at implementation time. + +### Test 1 — inspection consumer backfills and persists done items + +Name: + +```ts +test("inspection consumer backfills empty completed output before passthrough persistence (#334)", async () => { ... }); +``` + +Fixture/event order: + +1. `response.output_item.done`, `output_index: 2`, completed `function_call` item. +2. `response.output_item.done`, `output_index: 0`, completed `reasoning` item. +3. `response.output_item.done`, `output_index: 1`, completed assistant `message` item. +4. `response.completed`, response `{id:"resp_334_inspection", status:"completed", output:[]}`. + +Wire `consumeForInspection(..., onCompletedResponse)` so the callback calls `rememberResponseState(requestBody, response, undefined, {force:true})`. Await `onDone`, expand a next request with `previous_response_id: "resp_334_inspection"`, and assert the replayed output types are exactly `reasoning`, `message`, `function_call` after the original input and before the new suffix. + +**C-ACTIVATION-GROUNDING-01:** out-of-order indices activate the sort path; all three valid done events activate `Map.set`; the terminal empty array activates backfill; `rememberResponseState` plus the next expansion proves the callback received the reconstructed array and that saved state contains assistant/reasoning/tool-call items. Reading the untouched tee is not part of this unit, so byte preservation is established structurally by the production diff (inspection-only branch) and by the full passthrough suite. + +### Test 2 — metadata consumer uses the same backfill path + +Name: + +```ts +test("metadata consumer backfills missing completed output before passthrough persistence (#334)", async () => { ... }); +``` + +Use at least a `message` done item at index 0 and a `function_call` done item at index 1, followed by `response.completed` whose response has an id/status but **omits** `output`. Wire `consumeForResponseLogMetadata` to the same persistence callback, await `onDone`, expand by that response id, and assert both items are present in index order. + +**C-ACTIVATION-GROUNDING-01:** omitted `output` activates the `!Array.isArray(response.output)` side of backfill, while invoking the metadata consumer proves its independent payload loop is wired to the shared accumulator. Successful local replay is the observable persistence proof. + +### Test 3 — non-empty terminal output remains authoritative + +Name: + +```ts +test("non-empty completed output remains authoritative over accumulated done items (#334)", async () => { ... }); +``` + +Send a done event containing a sentinel `function_call`, then complete with a non-empty `output` containing a different sentinel assistant message. Capture the callback response and persist it. Assert the callback's `output` is the exact terminal array/reference and the next replay contains the terminal assistant item but not the accumulated sentinel call. + +**C-ACTIVATION-GROUNDING-01:** the prior done event makes the map non-empty, so the only reason the call is excluded is activation of the `Array.isArray(output) && output.length > 0` authoritative path. Reference equality (when captured before stream construction) plus replay content proves the terminal array was not replaced or merged. + +### Test 4 — two chained continuations keep one guidance copy + +Name: + +```ts +test("two previous_response_id continuations keep one replayed guidance item (#326)", () => { ... }); +``` + +Use one stable guidance string and perform this sequence: + +1. Parse request 1 with array input, inject guidance once, and persist response 1. Response 1's `output` must include a completed `function_call` item, modeling the post-#334 persisted shape. +2. Expand request 2 from response 1 with a `function_call_output` suffix, parse it, and assert `_replayPrefixLen` covers the saved request-1 input plus output. The replay prefix order must place the guidance before the backfilled function call, so guidance is not at the prefix edge. Call `injectDeveloperMessage` with the same text and assert raw outbound input and parsed context each contain exactly one copy. Persist response 2. +3. Expand request 3 from response 2 with a new user suffix, parse it, call injection again, and assert raw outbound input and parsed context still contain exactly one copy. Persist response 3, expand an audit-only request from response 3, and assert saved state also contains exactly one exact generated wire item. + +Count only exact wire items matching the generated shape, and separately count parsed developer messages whose content equals the guidance. Retain the existing `injectDeveloperMessage` test `inserts BEFORE compaction_trigger so it stays the final input item` unchanged. + +**C-ACTIVATION-GROUNDING-01:** request 1 activates fresh insertion; request 2's interior replayed guidance activates whole-prefix search and early return; the replayed `function_call` grounds fixture ordering after #334; request 3 activates idempotence on a second chained continuation; raw `_rawBody.input` is the passthrough outbound body observable; audit expansion from response 3 is the saved-state observable. Exact counts of one prove neither dual-write destination accumulated duplicates. + +### Test 5 — duplicate output indices are last-write-wins + +Name: + +```ts +test("duplicate output_index keeps only the final done item (#334)", async () => { ... }); +``` + +Send two valid `response.output_item.done` events at `output_index: 2`: first a sentinel +assistant message, then the final completed `function_call`. Follow them with an empty +`response.completed.response.output`, persist through the inspection consumer, and replay the +saved response. Assert index 2 appears exactly once, contains the final function call, and does not +contain the earlier sentinel. This fixture mandatorily activates the existing `Map.set` replacement +branch rather than merely exercising insertion. + +### Test 6 — malformed done events are rejected + +Name: + +```ts +test("malformed output_item.done events do not enter reconstructed output (#334)", async () => { ... }); +``` + +Interleave one valid index-0 message with done events whose `output_index` is missing, negative, +fractional, and a string, plus one valid-index event whose `item` is missing. Complete with +`output: []`. Assert the callback fires once and reconstructed/persisted output contains only the +valid index-0 item. This is the mandatory activation for every index/item rejection guard. + +### Test 7 — exact guidance in the current suffix does not suppress injection + +Name: + +```ts +test("matching guidance in the current suffix does not suppress replay-prefix injection (#326)", () => { ... }); +``` + +Build an expanded request whose replay prefix contains no exact generated guidance, but whose new +caller suffix contains an item with the exact generated wire shape/text. Set `_replayPrefixLen` to +end immediately before that suffix item, call `injectDeveloperMessage`, and assert a new generated +item is inserted in addition to the suffix item and parsed context receives the injected message. +The observable count is two raw exact-shape items, proving the scan is restricted to trusted replay +provenance rather than all current input. + +### Test 8 — malformed SSE payload is skipped without losing completion + +Name: + +```ts +test("malformed SSE payload is skipped before a valid completed response (#334)", async () => { ... }); +``` + +Use a raw SSE fixture (not the JSON-only helper) containing `data: {not-json}\n\n`, then a valid +index-0 done event and a valid empty-output completion. Run it through each persistence-capable +consumer (table-driven subcases are allowed). Assert no throw, one completion callback per +consumer, and replay contains the valid item. This mandatorily activates the accumulator's JSON +parse catch while proving later frames are still processed. + +### Conditional-path activation matrix + +| New/changed conditional | Activation scenario | Observable proof | +|---|---|---| +| Null or `[DONE]` payload is ignored | Append `[DONE]` to Test 1 and retain existing null/no-payload coverage | Callback fires once and no throw/regression occurs | +| Malformed SSE payload is ignored | Mandatory Test 8 places raw invalid JSON before valid done/completed frames in both consumers | No throw; one callback; later valid item persists | +| Valid `response.output_item.done` index/item | Tests 1–3 send valid done events | Saved/captured output contains the item | +| Duplicate `output_index` uses Map last-write-wins | Mandatory Test 5 sends two valid items at index 2 | Replay contains exactly one final index-2 item and no sentinel | +| Invalid/missing index or missing item is ignored | Mandatory Test 6 covers missing, negative, fractional, string index and missing item | Callback/replay contains only the one valid item | +| Missing terminal output backfills | Test 2 omits `output` | Saved replay has message + function call | +| Empty terminal output backfills | Test 1 uses `output: []` | Saved replay has reasoning + message + function call | +| Non-empty terminal output is untouched | Test 3 supplies a non-empty terminal array while map is populated | Reference equality and no sentinel call in replay | +| Completion callback absent | Existing cancel/incomplete tests call `consumeForInspection` without it | Existing terminal/cancel assertions remain green; no behavior change | +| Replay prefix length is zero | Existing fresh injection tests plus request 1 in Test 4 | One parsed and one raw guidance insertion | +| Exact guidance exists anywhere in replay prefix | Test 4 places it before a backfilled function call | Injection returns early; counts remain one | +| Similar/non-exact developer item does not suppress | Add a required table-driven subcase to Test 7 with different text and an extra content part in the prefix | Requested exact guidance is still inserted once | +| Matching item exists only in current suffix | Mandatory Test 7 places exact shape/text after `_replayPrefixLen` | Prefix-only scan inserts one new item; raw exact-shape count becomes two | +| Raw input is non-array | Existing `string raw input is left alone` test | Parsed message added; raw string unchanged | +| Fresh raw input ends in `compaction_trigger` | Existing `inserts BEFORE compaction_trigger so it stays the final input item` test | Trigger remains final | + +Tests 1–8 and every matrix row marked mandatory/required are acceptance requirements. Table-driven +subcases may share setup, but none may be omitted, folded into an unnamed “existing coverage” claim, +or have its observable weakened. + +## Scope boundary + +### IN + +- Native Responses SSE background inspection used by passthrough response-state recording. +- Reconstruction from finalized output items only. +- Replay-prefix provenance propagation from parser to collaboration injection. +- Exact, prefix-scoped idempotence for proxy-generated developer guidance. +- Focused state-machine regressions and repository-wide verification. + +### OUT + +- Client-facing SSE mutation, buffering, reordering, or synthesis. +- Delta-to-item reconstruction (`response.output_text.delta`, reasoning deltas, function argument deltas). +- Changes to `rememberResponseState`, `expandPreviousResponseInput`, WeakMap ownership, snapshot schema/version, TTL, byte caps, or eviction. +- Persist-time removal of guidance. +- Changes to `trackSseForRequestLog` beyond a clarifying comment if absolutely necessary; no code-path change is authorized. +- Compaction trigger placement or compaction persistence policy changes. +- Routed-provider bridge output, JSON passthrough, provider adapters, request routing, auth, credentials, workflows, dependencies, release scripts, GUI, or docs-site. + +Any need to touch an OUT path or any file not listed in the exact change map is a scope expansion and must be reported to the main agent before editing. + +## Docs-site sync decision + +No docs-site update. Both issues repair internal continuation correctness and preserve the public wire contract. There is no new option, endpoint, model behavior, or user-facing workflow to document. The regression tests and this devlog artifact are the appropriate durable record. + +## Security and privacy + +- No authentication, credential, OAuth, workflow, dependency, release, or permission boundary changes are in scope. +- Never log request bodies, replayed input, guidance text, completed output items, tool arguments, account identifiers, or response-state contents. +- The new replay prefix field remains in the in-process parsed object only; it must not be copied into `_rawBody`, serialized upstream, or persisted as a new snapshot field. +- The accumulator is per-consumer/per-stream local state and is released when the consumer finishes. It does not create cross-request global state. +- `bun run privacy:scan` is mandatory even though no new logging is planned. + +## Implementation and verification sequence + +1. Confirm the worktree is at the intended `origin/dev` baseline and preserve unrelated dirty work. +2. Implement `src/server/relay.ts` helper and consumer wiring without touching the client relay branch. +3. Add `_replayPrefixLen` in `src/types.ts` and populate it in `src/responses/parser.ts`. +4. Implement exact replay-prefix detection in `src/server/responses/collaboration.ts`, retaining fresh compaction-trigger ordering. +5. Add ALL EIGHT named regressions (the four core regressions plus the four + mandatory matrix tests: duplicate-index last-write-wins, malformed done-event + rejection, suffix-guidance non-dedupe, malformed-SSE skip) and their + activation assertions to `tests/responses-state.test.ts`. +6. Run focused tests: `bun test tests/responses-state.test.ts tests/multi-agent-compat.test.ts tests/consume-for-inspection-cancel.test.ts tests/passthrough-abort.test.ts`. +7. Run `bun run typecheck` and require exit 0. +8. Run `bun run test` and require exit 0. +9. Run `bun run privacy:scan` and require exit 0. +10. Inspect `git diff --check`, `git diff --stat`, and the exact changed-path list. Fail if any path outside the implementation map changed. + +## Acceptance checklist + +- [ ] One shared `Map` accumulator is used by both persistence-capable SSE consumers. +- [ ] Empty and missing terminal output are backfilled in ascending index order before `onCompletedResponse`. +- [ ] Non-empty terminal output is passed through unchanged and is never merged with accumulated items. +- [ ] `trackSseForRequestLog` remains terminal-status-only and receives no backfill logic. +- [ ] Client-facing native SSE bytes and relay selection remain unchanged. +- [ ] `OcxParsedRequest` receives proxy-private replay prefix length without changing the wire body. +- [ ] Guidance detection searches the whole replay prefix for the exact generated item shape. +- [ ] A replay hit skips both parsed-context and raw-input insertion. +- [ ] Fresh guidance insertion still precedes a final `compaction_trigger`. +- [ ] #326 fixtures include the post-#334 persisted `function_call` shape. +- [ ] Two chained continuations produce one guidance copy in outbound input and one in persisted/replayed state. +- [ ] Focused tests, typecheck, full test suite, and privacy scan all exit 0. +- [ ] No request-body or response-item logging was introduced. + +## Issue-comment traceability self-check + +- [ ] **#334 reporter — `relay.ts` empty-output backfill:** implemented by the file-private accumulator, `output_index` Map ordering, and both consumer wiring sections above. +- [ ] **#334 reporter — regression test:** covered by the inspection empty-output persistence test, metadata missing-output persistence test, and non-empty authoritative-output test. +- [ ] **#326 reporter kdnsna — idempotent injection or persist exclusion:** implemented using the accepted idempotent injection option; replay provenance is copied to `OcxParsedRequest`, the exact guidance item is searched across the prefix, and the rejected five-call-site persist exclusion is explicitly out of scope. +- [ ] **#326 reporter kdnsna — two-continuation regression:** covered by the request-1 → response-1 → request-2 → response-2 → request-3 sequence, with raw outbound and audit-replayed saved-state counts fixed at one. +- [ ] **Cross-issue ordering:** the #326 fixture deliberately replays a #334-style completed `function_call`, proving guidance detection does not assume a prefix-edge position. +- [ ] **Compaction invariant:** the existing regression requiring generated guidance before final `compaction_trigger` remains mandatory and unchanged. diff --git a/devlog/_plan/260724_bugfix_train/020_pool_retry_400.md b/devlog/_plan/260724_bugfix_train/020_pool_retry_400.md new file mode 100644 index 00000000000..89ce9503a54 --- /dev/null +++ b/devlog/_plan/260724_bugfix_train/020_pool_retry_400.md @@ -0,0 +1,704 @@ +# 020 — Cycle 2: bounded pool retry for account-specific unsupported-model 400 + +> DIFFLEVEL-ROADMAP-01: this is the implementation contract for issue #335. It +> fixes one account/model capability mismatch without changing the general 4xx +> health taxonomy introduced by `35d28a02`. + +## Loop-spec + +- **Work class:** C4 for the affected slice. The implementation changes pooled + account selection and therefore crosses an authentication/security boundary; + the change remains a narrowly bounded spec-satisfaction repair. +- **Loop archetype:** verifier-defined **spec-satisfaction repair**. +- **Specification:** in canonical OpenAI Pool mode, recognize only the captured + ChatGPT account/model rejection, then make at most one additional upstream + dispatch with a different eligible pooled account before any response bytes are + exposed to the client. +- **Primary verifier:** activation tests in `tests/server-auth.test.ts` plus the + focused auth-selection tests in `tests/codex-auth-context.test.ts`. +- **Policy-preservation verifier:** the existing test named `caller and model 4xx + responses do not penalize account health` in `tests/codex-routing.test.ts` must + remain unchanged and green. +- **Completion verifier:** `bun run typecheck`, `bun run test`, and + `bun run privacy:scan`, all exit 0. Run the focused files first for fast + diagnosis, then the repository gates. +- **Escalate up:** stop and return to Plan/Audit if implementation needs to (a) + classify generic `unsupported model`, `model not found`, or `invalid request` + substrings, (b) change `classifyCodexUpstreamOutcome`, health penalties, + cooldowns, or global active-account state, (c) retry after a client-visible + byte or after an SSE `200` terminal event, (d) make more than one + account-switch retry, or (e) expose account IDs/tokens/error bodies in logs. +- **Escalate down:** once the exact-body matcher, one-account exclusion, four + required activation scenarios, streaming safety scenario, and C4 gates are + green, do not add diagnostics, catalog changes, affinity redesign, or broader + provider retry abstractions in this cycle. +- **Budget/bounds:** one logical bug fix; no dependencies, schema/config changes, + GUI changes, release work, or public API additions. + +## Evidence and invariant lock + +### Reproduction evidence + +Issue #335 captured this exact upstream payload for `gpt-5.6-sol`: + +```json +{"detail":"The 'gpt-5.6-sol' model is not supported when using Codex with a ChatGPT account."} +``` + +Repository search found the same ChatGPT/Codex sentence in +`devlog/_plan/issue_017_mobile-thread-bypass-proxy/00_review.md`; no test fixture +or source capture supports broader alternatives such as bare `unsupported model`, +`model is not supported`, or `model not found for this account`. + +### Existing behavior that must remain true + +- `src/codex/auth-context.ts:92-141` resolves one pool account and its credential + before provider dispatch. +- `src/codex/routing.ts:116-125` maps every non-auth/non-quota 4xx to `caller`. +- `src/codex/routing.ts:450-480` returns without mutating health for `caller`. +- `tests/codex-routing.test.ts:217-226` pins 400/404/422 as zero-penalty and keeps + account `a` selectable. +- `src/server/responses/core.ts:920-948` builds a replayable passthrough request + and performs the existing bounded transient/5xx dispatch before returning a + client `Response`. +- `src/server/responses/core.ts:1011-1017` records the selected pool account's + HTTP outcome. The new retry must converge back into this path with the final + account/response; it must not invent a model-health penalty. +- `src/lib/errors.ts:79-190` is response/error-envelope classification. Its broad + `model unavailable` / `model not found` / `unsupported model` bucket at + `src/lib/errors.ts:172-183` is intentionally unsuitable as a retry signal. + +### Policy statement for the PR description + +The PR description must include this sentence (wording may be lightly edited, +meaning may not): + +> This intentionally refines the request-dispatch behavior around the +> `35d28a02` outcome policy: allow-listed account/model 400s may trigger one +> different-account Pool retry, while all 4xx responses remain caller outcomes +> with zero account-health penalty. + +## Scope + +### IN + +- Canonical OpenAI `openai-responses` passthrough with `codexAccountMode: "pool"`. +- HTTP status exactly `400`. +- Top-level JSON `detail` exactly matching the captured account/model sentence + after narrow normalization, with the requested routed model interpolated. +- At most one alternate-account dispatch, selected from the existing eligible + pool while excluding the first account. +- Both `stream: false` and a streaming request whose upstream rejects with HTTP + 400 before the server returns a `Response` to the client. +- Preservation of current status/body/headers when no retry is allowed or the + alternate account also rejects. + +### OUT + +- Generic 400/404/422 retry, substring/keyword heuristics, fuzzy matching, locale + variants, and provider-specific error taxonomies outside canonical ChatGPT. +- Model catalog correction, capability caching, per-model account health, quota + penalty, cooldown, reauth marking, or permanent account quarantine. +- Changing global active-account selection or thread-affinity persistence. This + cycle makes a request-local alternate selection; a successful retry does not + rewrite global account preference. +- Retry of an HTTP `200` SSE stream that later emits `response.failed`, even if + the terminal payload contains similar text. +- WebSocket-specific replay, diagnostics fields, GUI/history account labels, and + issue #335's optional diagnostics suggestion. +- Compact endpoint behavior, release/version changes, and dependency changes. + +## Allow-list contract + +### Accepted phrase template + +The allow-list contains exactly one evidence-backed template: + +```text +The '' model is not supported when using Codex with a ChatGPT account. +``` + +Normalization is deliberately limited to: + +1. parse JSON and require a **top-level string** property named `detail`; +2. trim leading/trailing whitespace; +3. collapse internal ASCII/Unicode whitespace runs to one space; +4. compare case-insensitively to the expected sentence built from + `route.modelId`. + +Do not strip punctuation, remove quotes, search substrings, inspect arbitrary +nested fields, or accept a different model ID. Consequently these remain false: + +```text +unsupported model +model is not supported +model not found for this account +The 'other-model' model is not supported when using Codex with a ChatGPT account. +Invalid request: malformed tool schema +``` + +The three generic phrases are leads only. Add one in a later change only after a +sanitized primary ChatGPT response fixture proves its complete envelope and exact +wording. This is the tightest defensible list from current repository and issue +evidence. + +## Safe retry boundary + +The retry decision occurs after `fetchWithTransientRetry(...)` has returned an +HTTP response but **before** `sanitizePassthroughHeaders`, terminal recorder +installation, body tee/relay, or construction/return of the client-facing +`Response` (`src/server/responses/core.ts:938-963`). At that point: + +- the request body is already a replayable string from `adapter.buildRequest`; +- no upstream body byte has been relayed to the client; +- no client response headers have been committed; +- an HTTP 400 is a pre-stream rejection even when `parsed.stream === true`. + +Inspect only `upstreamResponse.clone()` with `readBoundedResponseBody`, using the +existing 64 KiB/5 s bounds and the request abort signal. A timeout, oversized body, +invalid JSON, missing/non-string `detail`, or normalization mismatch means **no +retry** and returns the untouched original response. The clone prevents matcher +inspection from consuming or rewriting a non-matching response. + +Once a client-facing `Response` has been returned, or once an HTTP 200 SSE body is +being relayed/inspected, the retry window is closed. A later +`response.failed`/EOF/error event never enters this path. This defines +“non-streaming or pre-first-byte” precisely and avoids duplicate visible output. + +The unsupported-model budget is one account switch and one alternate HTTP +dispatch. The initial account is excluded from alternate selection, the alternate +dispatch is deliberately **not** wrapped in `fetchWithTransientRetry`, and its +response is returned directly. Existing transient transport/5xx retry behavior on +the initial dispatch before this classifier is not broadened; it is a separate +pre-existing policy. No recursive call to `handleResponses` is permitted. + +## Exact file change map + +| Action | Path | Diff responsibility | +| --- | --- | --- | +| MODIFY | `src/codex/auth-context.ts` | Add an optional request-local excluded account to `resolveCodexAuthContext`; use existing eligible-pool ranking for the alternate and reuse the exact credential resolution path. | +| MODIFY | `src/server/responses/core.ts` | Add the exact `detail` matcher and bounded clone inspection; perform one alternate-account request rebuild/dispatch before response commitment; preserve final response fidelity and health policy. | +| MODIFY | `tests/codex-auth-context.test.ts` | Prove exclusion selects a different eligible account and fails closed when none exists. | +| MODIFY | `tests/server-auth.test.ts` | Add endpoint activation scenarios for success, one-account, malformed 400, bounded double rejection, and pre-first-byte streaming safety. | +| NEW | none | No new abstraction/module is justified for one private matcher and one optional auth-resolution input. | +| DELETE | none | No production/test file is removed. | + +Explicitly unchanged: + +- `src/codex/routing.ts`: no outcome-class, health, cooldown, affinity, or active + account changes. +- `src/lib/errors.ts`: no retry semantics added to provider-agnostic error + presentation. +- `tests/codex-routing.test.ts`: do not weaken or rewrite the pinned 4xx test; + execute it as a policy-preservation gate. +- `docs-site/`: no user-configurable or public contract change; see Docs sync. + +## Diff-level implementation sketches + +The sketches below use current names/signatures and define the intended diff. +Minor extraction for formatting is allowed only if behavior remains identical. + +### 1. `src/codex/auth-context.ts` — alternate selection with one exclusion + +Before (`src/codex/auth-context.ts:92-105`): + +```ts +export async function resolveCodexAuthContext( + headers: Headers, + config: OcxConfig, + mode: CodexAccountMode, +): Promise { + if (mode === "direct") { + if (!hasCallerCodexBearer(headers)) throw new CodexDirectAuthenticationError(); + return { kind: "main", accountId: null }; + } + const threadId = headers.get("x-codex-parent-thread-id"); + const resolution = resolveCodexAccountForThreadDetailed(threadId, config); + if (resolution.status === "expired") throw new CodexThreadAffinityExpiredError(resolution.accountId); + const accountId = resolution.status === "selected" ? resolution.accountId : null; + if (!accountId) throw new CodexPoolAuthenticationError(); +``` + +After: + +```ts +export interface ResolveCodexAuthContextOptions { + excludeAccountId?: string; +} + +export async function resolveCodexAuthContext( + headers: Headers, + config: OcxConfig, + mode: CodexAccountMode, + options: ResolveCodexAuthContextOptions = {}, +): Promise { + if (mode === "direct") { + if (!hasCallerCodexBearer(headers)) throw new CodexDirectAuthenticationError(); + return { kind: "main", accountId: null }; + } + const threadId = headers.get("x-codex-parent-thread-id"); + const resolution = options.excludeAccountId + ? (() => { + const accountId = pickLowestUsageCodexAccount(config, options.excludeAccountId); + return accountId + ? { status: "selected" as const, accountId } + : { status: "none" as const }; + })() + : resolveCodexAccountForThreadDetailed(threadId, config); + if (resolution.status === "expired") throw new CodexThreadAffinityExpiredError(resolution.accountId); + const accountId = resolution.status === "selected" ? resolution.accountId : null; + if (!accountId) throw new CodexPoolAuthenticationError(); +``` + +Required import change: + +```ts +import { + getCodexAccountCooldownUntil, + pickLowestUsageCodexAccount, + resolveCodexAccountForThreadDetailed, +} from "./routing"; +``` + +The remainder of `resolveCodexAuthContext` at lines 106-140 stays the single +credential owner for main-pool and stored pool accounts. The exclusion branch +must not call `setActiveCodexAccount`, bind/clear thread affinity, bypass +`isCodexAccountUsable`, or fall back to the inbound caller bearer. + +### 2. `src/server/responses/core.ts` — exact matcher + +Add private helpers near the existing Codex passthrough helpers +(`codexLogAccountId`, `usesCodexForwardPoolAuth`): + +```ts +function normalizeCodexUnsupportedModelDetail(value: string): string { + return value.trim().replace(/\s+/gu, " ").toLocaleLowerCase("en-US"); +} + +function isAllowListedCodexAccountModel400( + status: number, + bodyText: string, + modelId: string, +): boolean { + if (status !== 400) return false; + try { + const payload = JSON.parse(bodyText) as unknown; + if (!payload || typeof payload !== "object" || Array.isArray(payload)) return false; + const detail = (payload as { detail?: unknown }).detail; + if (typeof detail !== "string") return false; + const expected = `The '${modelId}' model is not supported when using Codex with a ChatGPT account.`; + return normalizeCodexUnsupportedModelDetail(detail) + === normalizeCodexUnsupportedModelDetail(expected); + } catch { + return false; + } +} + +async function shouldRetryCodexPoolAccountModel400( + response: Response, + modelId: string, + signal?: AbortSignal, +): Promise { + if (response.status !== 400) return false; + try { + const body = await readBoundedResponseBody(response.clone(), { signal }); + return body.displaySafe + && !body.truncated + && isAllowListedCodexAccountModel400(response.status, body.text, modelId); + } catch { + return false; + } +} +``` + +Do not use `classifyError(...)` as the predicate. Its real signature remains: + +```ts +export function classifyError(status: number, type: string, message: string): OcxErrorPayload +``` + +That function intentionally maps presentation/error codes and broadly groups +model text. It has neither account context nor the original structured `detail` +field and cannot safely authorize cross-account dispatch. + +### 3. `src/server/responses/core.ts` — rebuild and dispatch once + +Before (`src/server/responses/core.ts:920-963`): + +```ts +const request = await adapter.buildRequest(parsed, { headers: selectedForwardHeaders }); +// ... +let upstreamResponse: Response; +try { + upstreamResponse = await fetchWithTransientRetry( + recovery => { + noteAttemptSend(logCtx.activeAttempt, passthroughEstimate, recovery); + return fetchWithHeaderTimeout(request.url, applyUpstreamRecoveryInit({ + method: request.method, + headers: request.headers, + body: request.body, + }, recovery), upstream.signal, connectMs, parsed.stream, providerFetch(route.provider)); + }, + { abortSignal: upstream.signal, label: safeHostLabel(request.url) }, + ); +} catch (err) { + // existing transport failure handling +} +const headers = sanitizePassthroughHeaders(upstreamResponse.headers); +``` + +After, retain the existing transport error catch but place one non-recursive +account-switch block before header sanitization: + +```ts +let request = await adapter.buildRequest(parsed, { headers: selectedForwardHeaders }); +// ... existing first dispatch, assigning upstreamResponse ... + +if ( + usesCodexForwardPoolAuth(authCtx, route.provider) + && await shouldRetryCodexPoolAccountModel400( + upstreamResponse, + route.modelId, + options.abortSignal, + ) +) { + const firstAuthCtx = authCtx; + let retryAuthCtx: CodexAuthContext | undefined; + try { + retryAuthCtx = await resolveCodexAuthContext( + req.headers, + config, + "pool", + { excludeAccountId: firstAuthCtx.accountId }, + ); + } catch (error) { + if ( + !(error instanceof CodexPoolAuthenticationError) + && !(error instanceof CodexAuthContextError) + && !(error instanceof CodexAccountCooldownError) + ) throw error; + } + + if (retryAuthCtx && (retryAuthCtx.kind === "pool" || retryAuthCtx.kind === "main-pool")) { + // Preserve 35d28a02 explicitly: first 400 is recorded through the existing + // classifier and remains a caller/no-health-mutation outcome. + recordCodexUpstreamOutcome(config, firstAuthCtx.accountId, 400, { + threadId: req.headers.get("x-codex-parent-thread-id"), + }); + + const retryHeaders = headersForCodexAuthContext(req.headers, retryAuthCtx); + const retryProvider = applyCodexAuthContextToProvider( + stripCodexRuntimeProviderFields(route.provider), + retryAuthCtx, + "pool", + ); + const retryAdapter = resolveAdapter( + resolveWireProtocolOverride(route.providerName, route.modelId, retryProvider), + config.cacheRetention, + ); + request = await retryAdapter.buildRequest(parsed, { headers: retryHeaders }); + + await upstreamResponse.body?.cancel().catch(() => undefined); + authCtx = retryAuthCtx; + selectedForwardHeaders = retryHeaders; + route.provider = retryProvider; + logCtx.provider = formatCodexProviderForLog( + route.providerName, + retryAuthCtx.accountId, + config, + ); + + noteAttemptSend(logCtx.activeAttempt, passthroughEstimate); + upstreamResponse = await fetchWithHeaderTimeout( + request.url, + { + method: request.method, + headers: request.headers, + body: request.body, + }, + upstream.signal, + connectMs, + parsed.stream, + providerFetch(route.provider), + ); + // No transient wrapper and no call back into the matcher: the entire retry + // budget is spent by this one different-account HTTP dispatch. + } +} + +const headers = sanitizePassthroughHeaders(upstreamResponse.headers); +``` + +Implementation audit notes for this sketch: + +- Import and use existing `stripCodexRuntimeProviderFields`; otherwise the second + adapter can retain the first account's runtime override. +- Extracting the duplicated fetch body into a private local dispatch function is + acceptable and preferred if it keeps `handleResponses` readable. The helper + must take the concrete request/provider and must not recurse or own selection. +- The existing catch behavior at lines 949-962 must be extracted/reused around + the second dispatch. A second-attempt connect failure returns the existing 502 + and records against the second account; it must not retry that same account or + select a third account. +- The first 400 response body is canceled only after alternate auth and request + construction succeed. If no alternate exists or alternate auth cannot be + resolved, preserve and return the original 400 unchanged. +- After the switch, all existing quota, terminal-recorder, response logging, and + health logic must observe the final `authCtx`, provider, and response. Do not + record success for the first account. +- `recordCodexUpstreamOutcome(config, firstAuthCtx.accountId, 400, ...)` is a + deliberate no-op health-wise. The existing `caller` early return remains the + policy owner; do not special-case health in the new matcher. +- A second allow-listed 400 passes directly to current response forwarding. There + is no loop and no third `resolveCodexAuthContext` call. +- A 5xx/connect failure from B also ends the request after B's single dispatch; + the pre-existing same-account transient helper remains confined to the initial + dispatch so this repair's retry can never target the same account. + +### 4. `src/codex/routing.ts` — explicit no-diff policy pin + +The following real signatures and branches remain byte-for-byte unchanged: + +```ts +export function classifyCodexUpstreamOutcome( + outcome: CodexUpstreamOutcome, +): CodexUpstreamOutcomeClass { + // ... + if (outcome >= 400 && outcome < 500) return "caller"; + // ... +} + +export function pickLowestUsageCodexAccount( + config: OcxConfig, + excludeId?: string, + now = Date.now(), +): string | null + +export function recordCodexUpstreamOutcome( + config: OcxConfig, + accountId: string | null, + outcome: CodexUpstreamOutcome, + meta: CodexUpstreamOutcomeMeta = {}, +): void { + // ... + if (outcomeClass === "caller") return; + // ... +} +``` + +`pickLowestUsageCodexAccount(config, firstAccountId)` already supplies the exact +eligibility and exclusion semantics needed by auth resolution: reauth-needed, +hard-cooldown, soft-avoided, unusable, and the excluded account are omitted. + +## Test plan and activation scenarios + +### `tests/codex-auth-context.test.ts` + +Add a second stored account fixture only inside the new cases and clear its +credential/reauth state in cleanup. + +1. **Exclusion selects another eligible account.** Configure `pool-a` active and + usable, `pool-b` usable, and deterministic quota scores. Call the real API: + + ```ts + await resolveCodexAuthContext(headers, config, "pool", { + excludeAccountId: "pool-a", + }); + ``` + + Assert `accountId === "pool-b"`, B's token/account header values are returned, + and `config.activeCodexAccountId` remains `pool-a`. + +2. **Exclusion fails closed with no alternate.** With only usable `pool-a`, call + the same API excluding `pool-a`; assert `CodexPoolAuthenticationError`. It must + not return `main`, use the inbound bearer, or retry `pool-a`. + +### `tests/server-auth.test.ts` + +Use `redirectCanonicalCodexTo`, two saved test credentials, deterministic quotas, +and an upstream server that records `authorization` / `chatgpt-account-id` and +returns account-specific responses. Assertions must observe the public +`/v1/responses` behavior, dispatch count/order, response fidelity, and +`getCodexUpstreamHealth`. + +#### Activation A — allow-listed 400 and at least two eligible accounts + +- A receives the exact payload for the requested model and returns 400. +- B returns 200. +- Assert dispatch accounts are exactly `[A, B]`; B differs from A. +- Assert the client gets B's successful response. +- Assert health for A and B is `null` (A's 400 has zero penalty; B's clean success + creates no failure state). +- Assert the configured active account is still A (request-local selection). + +#### Activation B — allow-listed 400 and only one eligible account + +- A returns the exact allow-listed 400; no usable B exists. +- Assert one upstream dispatch only. +- Assert status, sanitized headers, and body equal A's original response. +- Assert A health is `null` and active account remains A. + +#### Activation C — non-allow-listed malformed-input 400 + +- A returns `{"detail":"Invalid request: malformed tool schema"}` with B eligible. +- Assert one upstream dispatch only and B is never contacted. +- Assert the same 400 body reaches the client. +- Assert A and B health are `null` and neither is marked reauth-needed. + +This scenario is mandatory and is the security regression proving broad keyword +or status matching did not turn malformed caller input into a cross-account quota +burn. + +#### Activation D — second account also returns the allow-listed 400 + +- A and B both return the exact sentence for the requested model. +- Assert dispatch order/count is exactly `[A, B]` / 2. +- Assert B's final 400 reaches the client. +- Assert there is no third dispatch, no return to A, and both health states are + `null`. + +#### Activation E — streaming request is retried only before first byte + +- Send `stream: true`; A returns the exact HTTP 400 before any SSE response. +- B returns a valid completed SSE stream. +- Assert the client receives only B's SSE frames and dispatch count is 2. +- Add the negative half: A returns HTTP 200 then an SSE `response.failed` payload + containing the same sentence; assert dispatch count is 1. This proves the + matcher cannot activate after response commitment/stream relay. + +### Mandatory negative activation matrix — hostile/non-authorizing bodies + +Every case below uses two eligible accounts, records dispatch order, and compares the client-visible +status, sanitized headers, and body bytes with account A's original HTTP 400. The invariant is one +dispatch (`[A]`), no request to B, unchanged original 400, and null health for both accounts. Use +`readBoundedResponseBody`'s real timeout/truncation/oversize/display-safety behavior; do not replace +the production reader with a permissive test stub. + +| Name (mandatory test) | Fixture/activation | Observable assertion | +|---|---|---| +| `oversized 400 body never authorizes a pool retry` | A returns a body larger than 64 KiB whose retained prefix resembles or contains the allow-listed JSON sentence | Reader reports unsafe oversized/truncated inspection; dispatches are `[A]`; B is untouched; the original 400 status/headers/full body are returned unchanged | +| `stalled 400 body timeout never authorizes a pool retry` | A's clone-readable body emits a partial allow-listed-looking JSON prefix, stalls beyond the configured inactivity/total deadline, then is released for client read | Timeout/display-unsafe branch executes; dispatches are `[A]`; after release the client receives A's original 400 bytes and headers unchanged | +| `aborted 400 inspection never authorizes a pool retry` | Abort the request signal while bounded clone inspection is pending on A's 400 body | Abort is caught only as matcher-false; B is untouched (no second dispatch). NOTE: the request abort also cancels A's upstream fetch (core.ts links options.abortSignal to upstream.signal and aborts the controller on request-signal fire), so the original body is NOT observable post-abort — assert the client-cancel outcome (aborted response surface), never body fidelity. If original-body fidelity must be exercised, use a matcher-local inspection signal decoupled from the request signal | +| `invalid JSON 400 never authorizes a pool retry` | A returns `{"detail":` or non-JSON text with content type JSON | JSON parse fails; dispatches are `[A]`; exact malformed bytes and original 400 metadata reach the client | +| `missing or non-string detail never authorizes a pool retry` | Table-driven A bodies: `{}`, `{"detail":null}`, `{"detail":400}`, `{"detail":{"message":"..."}}` | Every subcase dispatches once, never contacts B, and returns that exact original body/status | +| `wrong model id in exact sentence never authorizes a pool retry` | Requested route model is `gpt-5.6-sol`, but A's otherwise exact sentence names `other-model` | Model interpolation mismatch keeps dispatches at `[A]`; original 400 is unchanged | +| `normalization near-misses never authorize a pool retry` | Table-driven near misses outside the allow-list: removed terminal period, changed quote characters, extra prefix/suffix text, or punctuation removal; include one whitespace/case-only variant as the positive control | Every near miss returns A unchanged with no B dispatch; only the whitespace/case-only positive control may dispatch `[A,B]`, proving normalization is narrow | + +These seven named tests are additional to Activations A–E. A shared parameterized harness is allowed, +but each hostile-body class must appear by name in test output and assert response fidelity, not only +dispatch count. + +### `tests/codex-routing.test.ts` policy-preservation gate + +Do not change the existing assertions at lines 217-226. Run the test and ensure +400, 404, and 422 still produce no health object and do not move future routing +away from account A. The endpoint scenarios add stronger per-account assertions; +they do not replace this test. + +### Test hygiene + +- Reset both A and B credentials, reauth flags, quota state, upstream health, and + thread map after each scenario. +- Use synthetic account IDs/tokens only; never include a real email, token, + ChatGPT account ID, or raw local path in fixtures/log snapshots. +- Do not add skips, weaken assertions, lower coverage, or introduce test-only + production branches. + +## Security-review checkpoint + +This checkpoint is mandatory before merge under `MAINTAINERS.md` because the diff +changes authentication/account-selection behavior. + +### Threat model + +- **Assets:** pooled OAuth credentials, account quota, caller request integrity, + thread/account routing state, and privacy-safe logs. +- **Entrypoint:** an untrusted ChatGPT backend HTTP 400 body plus the caller's + requested model ID. +- **Trust boundary:** upstream free text authorizes one additional credentialed + request under another account. A false positive can spend another account's + quota; an unbounded retry can amplify cost. +- **Attacker/failure capability:** malformed caller input can induce arbitrary + 400s; an upstream/proxy can return large, stalled, invalid, or crafted bodies; + account credentials can become unusable between selection and dispatch. +- **Controls:** exact status, exact top-level field, model-bound full-sentence + equality, bounded clone read, one excluded-account selection, one non-recursive + retry, existing eligibility checks, zero health penalty, no body/token logging, + and no post-first-byte activation. +- **Residual risk:** upstream wording changes cause a false negative (no retry), + which preserves current behavior and is safer than a false-positive quota burn. + +### Reviewer checklist + +- [ ] A maintainer explicitly performs security review; author self-approval is + not sufficient. +- [ ] PR targets `dev`, not `main`. +- [ ] Matcher accepts only the evidence-backed `detail` template and requested + model; generic phrase cases are negative tests. +- [ ] Alternate selection excludes the first account and uses existing + eligibility checks; no caller bearer fallback occurs. +- [ ] Dispatch count is bounded and cannot recurse; double rejection is exactly + two account dispatches. +- [ ] No response byte/header is client-visible before the retry decision. +- [ ] `classifyCodexUpstreamOutcome` and caller-health behavior are unchanged. +- [ ] No credential, account identifier, request body, or raw upstream body is + added to logs or error diagnostics. +- [ ] Full typecheck/test/privacy gates pass on the reviewed commit. + +## Privacy notes + +- The matcher reads an already-received bounded clone in memory and retains no + new data. +- Do not log `detail`, model/account tuples, access tokens, ChatGPT account IDs, + email addresses, request bodies, or authorization headers. +- Existing provider log labels may update to the final account through the normal + sanitized formatter; this cycle adds no dedicated diagnostics field. +- Test fixtures use `pool-a`, `pool-b`, fake bearer values, and `.example.test` + identities only. +- `bun run privacy:scan` is a hard completion gate. + +## Docs-site sync decision + +**No `docs-site/` change.** This repairs internal Pool dispatch behavior without a +new setting, command, endpoint, model catalog entry, error contract, or user action. +The upstream 400 or alternate response remains the public wire response when the +bounded retry cannot recover. Documenting exact private backend wording would also +create a brittle public contract. If a future cycle adds configurable retry policy +or diagnostics fields, that cycle must update the English docs source and verify +translations do not contradict it. + +## Verification (C) + +Run from repository root at the implementation commit: + +```bash +bun test tests/codex-auth-context.test.ts +bun test tests/server-auth.test.ts +bun test tests/codex-routing.test.ts +bun run typecheck +bun run test +bun run privacy:scan +``` + +Expected result for every command: exit code 0. The focused endpoint test output +must name all activation scenarios A-E and all seven mandatory hostile-body negatives. Completion evidence records command, +commit SHA, exit code, pass/fail counts, and the explicit security-review result. + +## Acceptance criteria + +- [ ] Exact captured `detail` from A with at least two eligible accounts dispatches + once to B and can recover. +- [ ] The retry account is eligible and not A. +- [ ] One eligible account returns the original 400 without another dispatch. +- [ ] Malformed-input/non-allow-listed 400 never contacts B. +- [ ] A then B allow-listed rejection stops at two dispatches and returns B's 400. +- [ ] Streaming requests retry only on an HTTP 400 before response handoff; HTTP + 200/SSE terminal failures never retry. +- [ ] A's allow-listed 400 creates no health penalty, cooldown, soft avoid, reauth + state, active-account mutation, or global affinity rewrite. +- [ ] `classifyCodexUpstreamOutcome` and its pinned 4xx test are unchanged and green. +- [ ] Response body/status/header fidelity is retained whenever no retry occurs. +- [ ] No secrets/PII/account IDs or raw bodies are newly logged. +- [ ] Security reviewer approves the auth-boundary refinement and the PR states + its intentional relationship to `35d28a02`. +- [ ] Focused tests, full typecheck/test, and privacy scan all exit 0. diff --git a/devlog/_plan/260724_bugfix_train/030_shim_autorestore.md b/devlog/_plan/260724_bugfix_train/030_shim_autorestore.md new file mode 100644 index 00000000000..3a4f46fee23 --- /dev/null +++ b/devlog/_plan/260724_bugfix_train/030_shim_autorestore.md @@ -0,0 +1,707 @@ +# 030 — Cycle 3: codex-shim auto-restore after external Codex update + +> Evidence baseline: `origin/dev` / `d9e06c8dd08df6635f5ca042bf6aa469fe1a10a8` (2026-07-24). +> This is a diff-level implementation design, not an implementation patch. + +## Loop spec + +- Loop archetype: **spec-satisfaction repair** for issue #320 part 2. +- Success condition: a previously installed OpenCodex Codex shim that a completed external + `@openai/codex` npm update replaced is restored by the next real `ocx` command without requiring + `ocx codex-shim install`. +- Verifier: `bun run typecheck`, `bun run test`, and `bun run privacy:scan`, plus the focused + activation scenarios and the healthy-probe timing check in this document. +- Bounds: one bugfix slice; no daemon, watcher, dependency, auth-flow, release, or GUI work. +- Escalation: return to A/maintainer review rather than widening the patch if (a) the healthy probe + cannot stay below 5 ms typical, (b) a platform requires process enumeration or a filesystem + watcher, (c) replacement stability cannot be established without waiting, or (d) any proposed + diagnostic would read or print credential contents. A security reviewer must explicitly approve + the final diff before merge because the shim state is colocated with `auth.json`. + +## Objective and evidence + +Issue #320 part 2 is a lifecycle gap, not a missing repair primitive: + +- `installCodexShim()` already recognizes a tracked wrapper replaced by a non-shim, moves the new + launcher into the owned backup, and writes a fresh shim (`src/codex/shim.ts:445-487`). +- `diagnoseCodexShim()` already reports the exact stale condition as `present but not an opencodex + shim` (`src/codex/shim.ts:546-577`). +- The repair path is only called explicitly from `ocx codex-shim install` + (`src/cli/index.ts:659-665`) or after OpenCodex's own updater succeeds + (`src/update/index.ts:255-267`). An external Codex npm update reaches neither call site. +- The published `ocx` entry always reaches `src/cli/index.ts`; the runtime SOT identifies that file + as the CLI entry and `src/codex/shim.ts` as the shim lifecycle owner + (`structure/01_runtime.md:7-16`). +- Credential handling and other security-boundary changes require explicit security review + (`MAINTAINERS.md:16-25`). `loadConfig()` hardens both config and adjacent `auth.json` + (`src/config.ts:645-650`), so the startup probe must use the read-only diagnostics loader and must + never read auth data. + +## Scope + +### IN + +- Auto-restore only a **previously installed, validly tracked** shim after its tracked wrapper path + has been replaced by a complete, stable, non-OpenCodex launcher. +- Run a bounded, read-only probe on ordinary `ocx` CLI startup and synchronously execute the existing + repair transaction only on the rare confirmed-replacement path. +- Add opt-out controls: `config.codexShimAutoRestore` and + `OPENCODEX_CODEX_SHIM_AUTO_RESTORE=0`. +- Preserve macOS/Linux, native Windows, Git-Bash, and WSL safety behavior. +- Warn on successful automatic repair and on repair failure; never change the requested command's + exit behavior because repair failed. +- Update runtime SOT, root README copies, and docs-site CLI/configuration copies in every maintained + locale so no published page retains the old “manual install/update only” claim. + +### OUT + +- **Issue #320 part 1, the native Codex login gate, is UPSTREAM and explicitly OUT.** No auth/login, + `auth.json`, account-pool, or token behavior changes. +- Fresh shim installation. Missing/corrupt state, a missing wrapper, a missing backup, a platform + mismatch, and an untracked Codex executable remain explicit/manual repair cases. +- `codex.exe` wrapping. Windows discovery intentionally refuses a real `.exe` + (`src/codex/shim.ts:176-207`). +- Background filesystem watcher, npm-process scanner, polling timer, sleep/retry loop, service + install, proxy lifecycle, GUI control, dependency, release, or schema migration. +- Changing `codexAutoStart`; it controls whether an installed shim runs `ocx ensure` + (`src/config.ts:772-774`, `src/types.ts:530-531`) and is not the lifecycle-repair opt-out. + +## Design decisions + +### 1. Hook point: ordinary CLI startup + +**Choose:** invoke a best-effort helper in `src/cli/index.ts` after the existing version/help early +exits and before `switch (command)` (`src/cli/index.ts:44-60`, `src/cli/index.ts:540`). Skip explicit +destructive/repair lifecycle commands: top-level `uninstall`/`remove`, and `codex-shim install`, +`uninstall`, or `remove`. Keep `codex-shim status` eligible so it reports the repaired state. + +| Candidate | Coverage | Cost / invasiveness | Decision | +| --- | --- | --- | --- | +| Every real `ocx` CLI startup | The next user command, even when no proxy starts | One state read plus bounded wrapper-prefix/metadata reads on the healthy path; no writes | **Chosen** | +| `ocx start` / serve hook | Only commands that start the foreground proxy | Misses `status`, `login`, `doctor`, service-managed/no-start workflows | Reject | +| Proxy startup | Only a newly created server process | Couples launcher mutation to server/auth startup and misses commands using an existing proxy | Reject | +| Filesystem watcher | Continuous detection | Long-lived resource, platform-specific semantics, update races, teardown and permission surface | Reject | + +The hook is deliberately not placed in `bin/ocx.mjs`: the runtime SOT assigns that file bundled-Bun +bootstrap only (`structure/01_runtime.md:7-9`). Shim lifecycle remains owned by +`src/codex/shim.ts` (`structure/01_runtime.md:16`). + +“Never block” means no network, subprocess, watcher, sleep, polling, or retry on startup, and no +repair failure may block/fail the requested command. The healthy check is synchronous but bounded so +the shim is known-restored before command dispatch. The rare repair transaction may add local rename +and write latency; making that detached would violate the “next command restored it” contract. + +### 2. Restore reuses install behavior inside a real multi-wrapper transaction + +**Choose:** extract the body of current `installCodexShim()` into a private +`installCodexShimInternal(options)` and keep the public no-argument signature unchanged. Explicit +install calls the internal function without a replacement guard. Auto-restore calls the same +internal function with the stable fingerprints captured by the probe. Do not copy the +backup/rename/write logic. + +The current helpers preserve one path on failure, but `installCodexShim()` mutates siblings +sequentially (`src/codex/shim.ts:471-483`). That is not sufficient for the promised all-or-nothing +multi-wrapper repair. The guarded auto-restore path must therefore wrap the existing branch behavior +in a transaction-wide preflight and rollback journal: + +1. `refreshShimFile()` recognizes a tracked non-shim wrapper (`src/codex/shim.ts:445-468`). +2. `replaceOwnedBackup()` remains the single-path primitive, but guarded multi-wrapper restore must + retain its staged prior backup until the whole sibling set commits; deleting it after each sibling + would make later rollback impossible. +3. `writeShim()` emits `.cmd`, `.ps1`, Git-Bash sh, or Unix sh content and applies Unix execute bits + (`src/codex/shim.ts:397-419`). +4. `installCodexShim()` writes canonical state only after every planned sibling mutation succeeds. + On any mid-sequence mismatch/write/rename failure, reverse the journal before returning/throwing + and leave state bytes unchanged. + +The accepted residual TOCTOU window is explicit: filesystem contents can change after transaction- +wide preflight. Revalidate each source fingerprint immediately before its own first rename; if that +late check fails after an earlier sibling was repaired, roll back the earlier sibling(s) in reverse +order and return `deferred`. A rollback failure is an explicit filesystem repair failure reported by +the CLI warning path, never a false `restored` result. + +### 3. Opt-out names and precedence + +- Config: `codexShimAutoRestore?: boolean`, default `true`. +- Environment: `OPENCODEX_CODEX_SHIM_AUTO_RESTORE=0` disables auto-restore for that process. +- Effective rule: enabled only when the config value is not `false` **and** the env value is not + exactly `"0"`. The env variable is an opt-out, not a force-enable; `=1` does not override config + `false`. + +The names follow current boundaries: persisted config uses camelCase boolean fields such as +`codexAutoStart` (`src/types.ts:526-531`), while public OpenCodex environment controls use the +`OPENCODEX_` prefix (`src/config.ts:308`, `src/cli/index.ts:134-135`). Internal shim recursion remains +the separate `OCX_SHIM_BYPASS` concern (`src/codex/shim.ts:254-260`, `src/codex/shim.ts:348-350`). + +The common healthy/not-installed path never parses config. The shim owner first probes tracked state; +only a stable restore candidate invokes a lazy `enabled()` callback. That callback uses +`readConfigDiagnostics().config` (`src/config.ts:730-752`), not `loadConfig()`, so startup does not +chmod, back up, or read adjacent credential files. + +### 4. Cross-platform contract + +- macOS/Linux: preserve the executable-bit health check (`src/codex/shim.ts:91-99`) and Unix shim + writer/chmod (`src/codex/shim.ts:415-418`). Symlink launchers are allowed only when the symlink and + resolved target fingerprints are stable and the target is a non-empty regular file. +- Windows: operate only on the state-tracked `.cmd`, `.ps1`, and extensionless Git-Bash launcher set; + writer selection and UTF-8 BOM behavior stay centralized in `writeShim()` + (`src/codex/shim.ts:397-414`). Never discover or rename `codex.exe`. +- WSL: auto-restore performs no PATH rediscovery. It requires `state.platform === process.platform`, + so Windows-owned state is not repaired from WSL. Fresh/manual discovery retains the existing WSL + interop refusal (`src/codex/shim.ts:132-173`). +- Multi-wrapper state: all changed tracked launchers must pass stability checks before any one is + mutated. A mixed “npm is still replacing siblings” state is deferred as one unit. + +### 5. Completed/stable replacement check + +Do not sleep and stat twice over time. A candidate is safe only when all of these hold in one bounded +probe: + +1. `codex-shim.json` parses through existing `readState()` and its platform equals the runtime. +2. Every non-`preserveOnly` tracked entry still has its owned backup (or valid `realPath`), and every + wrapper exists. Corrupt/missing state is not auto-repaired. +3. At least one tracked wrapper lacks the marker in a bounded prefix; intact wrappers remain healthy. +4. For each changed wrapper, `lstat` before and after the bounded read has the same + device/inode/type/size/mtime; for symlinks, the followed target's corresponding fingerprint is + also unchanged and is a non-empty regular file. +5. The newest `mtime`/`ctime` in those snapshots is at least `100 ms` old. This catches a just-created + partial launcher without adding a startup wait. A younger/changing candidate returns `deferred` + silently and is retried by the next command. +6. Immediately before **any** mutation, `installCodexShimInternal` rechecks every captured wrapper + fingerprint transaction-wide. Any mismatch aborts with zero mutation. During application it also + rechecks each source immediately before that sibling's first rename; a later mismatch rolls back + all already-mutated siblings in reverse order. Concurrent `ocx` or npm activity therefore + degrades to a later retry rather than leaving a partially refreshed wrapper set. + +Use a named `CODEX_SHIM_REPLACEMENT_STABLE_MS = 100` and a bounded marker read (16 KiB is sufficient: +the generated marker is at the top of every builder at `src/codex/shim.ts:222-224`, +`src/codex/shim.ts:287-289`, and `src/codex/shim.ts:329-331`). Do not use current `isShim()` for the +startup probe because it reads the entire path (`src/codex/shim.ts:83-89`), which can be an updated +native launcher/binary. Explicit install and full diagnostics may retain their current behavior. + +This is a stability check, not an npm-brand check: OpenCodex does not need to parse npm launcher +contents or enumerate npm processes. A replacement that is still being written is deferred; a stable +non-shim at an OpenCodex-owned tracked path is the exact existing repair trigger. + +## Structural map + +```text +src/cli/index.ts + -> src/cli/codex-shim-autorestore.ts best-effort CLI boundary + -> src/config.ts lazy effective opt-out + -> src/codex/shim.ts probe + guarded shared install transaction + -> src/lib/bun-runtime.ts existing shim writer dependency + -> src/lib/service-secrets.ts existing token-file path baked into shim + +src/types.ts persisted config type +tests/config.test.ts flag/env contract +tests/codex-shim.test.ts filesystem/platform/stability transaction +tests/codex-shim-autorestore.test.ts CLI policy + activation boundary +``` + +Dependency direction remains CLI policy -> config/domain owners. No server/proxy module imports the +CLI helper, and shim code does not import CLI policy. Coupling is functional and one-way. No new +public package export is introduced. + +## Exact file change map + +### Production and tests + +| Action | Exact path | Diff responsibility | +| --- | --- | --- | +| MODIFY | `src/types.ts` | Add typed persisted `codexShimAutoRestore?: boolean`. | +| MODIFY | `src/config.ts` | Validate/default the field; export env constant and effective opt-out helper. | +| MODIFY | `src/codex/shim.ts` | Add bounded probe, stable fingerprints, guarded internal install reuse, and `autoRestoreCodexShim()`. | +| NEW | `src/cli/codex-shim-autorestore.ts` | Own skip policy, lazy config callback, warnings, and catch-all non-fatal boundary. | +| MODIFY | `src/cli/index.ts` | Call helper after help/version exits and before command dispatch. | +| MODIFY | `tests/config.test.ts` | Cover default, config false, env `0`, and precedence/type validation. | +| MODIFY | `tests/codex-shim.test.ts` | Cover intact fast path, stable replacement, young/changing defer, platform guard, and real repair transaction. | +| NEW | `tests/codex-shim-autorestore.test.ts` | Cover CLI skip policy, warning-only failures, opt-out, and a spawned next-command activation. | + +### Source of truth and public docs + +| Action | Exact path(s) | Diff responsibility | +| --- | --- | --- | +| MODIFY | `structure/01_runtime.md` | Record CLI startup auto-restore ownership and no-watcher/stable-replacement invariant. | +| MODIFY | `README.md`, `README.ko.md`, `README.ja.md`, `README.zh-CN.md`, `README.ru.md` | Replace the contradictory “repair only on install/update” row and name both opt-outs. | +| MODIFY | `docs-site/src/content/docs/reference/cli.md`, `docs-site/src/content/docs/ko/reference/cli.md`, `docs-site/src/content/docs/ja/reference/cli.md`, `docs-site/src/content/docs/zh-cn/reference/cli.md`, `docs-site/src/content/docs/ru/reference/cli.md` | Describe next-command automatic repair, stable-update defer, warning-only failure, and manual fallback. | +| MODIFY | `docs-site/src/content/docs/reference/configuration.md`, `docs-site/src/content/docs/ko/reference/configuration.md`, `docs-site/src/content/docs/ja/reference/configuration.md`, `docs-site/src/content/docs/zh-cn/reference/configuration.md`, `docs-site/src/content/docs/ru/reference/configuration.md` | Add `codexShimAutoRestore` and `OPENCODEX_CODEX_SHIM_AUTO_RESTORE=0`. | + +### Deletes + +- No production/test/docs file is deleted by the implementation. +- Planning scaffold `devlog/_plan/260724_bugfix_train/030_phase3.md` is replaced by this document, + `devlog/_plan/260724_bugfix_train/030_shim_autorestore.md`. + +## Diff-level code sketches + +Line anchors below refer to baseline `d9e06c8d`; implementation line numbers will shift. + +### `src/types.ts` — config contract + +Before (`src/types.ts:526-531`): + +```ts +/** Advertise supports_websockets so Codex opens the WS endpoint. Default false; set true to opt in. */ +websockets?: boolean; +/** Generated API keys for external access to the proxy's /v1/responses endpoint. */ +apiKeys?: Array<{ id: string; name: string; key: string; createdAt: string }>; +/** Auto-start/sync the proxy from the Codex shim before launching Codex. Default true. */ +codexAutoStart?: boolean; +``` + +After: + +```ts +/** Advertise supports_websockets so Codex opens the WS endpoint. Default false; set true to opt in. */ +websockets?: boolean; +/** Generated API keys for external access to the proxy's /v1/responses endpoint. */ +apiKeys?: Array<{ id: string; name: string; key: string; createdAt: string }>; +/** Auto-start/sync the proxy from the Codex shim before launching Codex. Default true. */ +codexAutoStart?: boolean; +/** Restore a previously installed shim after a stable external Codex update replaces it. Default true. */ +codexShimAutoRestore?: boolean; +``` + +### `src/config.ts` — schema/default/effective flag + +Before (`src/config.ts:437-445`, `src/config.ts:772-805`): + +```ts +const configSchema = z.object({ + // ... + multiAgentGuidanceEnabled: z.boolean().optional(), +}).passthrough().superRefine((config, ctx) => { + +export function codexAutoStartEnabled(config: Pick): boolean { + return config.codexAutoStart !== false; +} + +export function getDefaultConfig(): OcxConfig { + return { + // ... + codexAutoStart: true, + }; +} +``` + +After: + +```ts +export const CODEX_SHIM_AUTO_RESTORE_ENV = "OPENCODEX_CODEX_SHIM_AUTO_RESTORE"; + +const configSchema = z.object({ + // ... + multiAgentGuidanceEnabled: z.boolean().optional(), + codexShimAutoRestore: z.boolean().optional(), +}).passthrough().superRefine((config, ctx) => { + +export function codexShimAutoRestoreEnabled( + config: Pick, + env: NodeJS.ProcessEnv = process.env, +): boolean { + return config.codexShimAutoRestore !== false && env[CODEX_SHIM_AUTO_RESTORE_ENV] !== "0"; +} + +export function getDefaultConfig(): OcxConfig { + return { + // ... + codexAutoStart: true, + codexShimAutoRestore: true, + }; +} +``` + +Place the schema key beside other top-level booleans, not as a post-parse cast. Keep the exported env +name canonical so docs/tests do not duplicate the literal. + +### `src/codex/shim.ts` — probe and shared guarded install + +Before (`src/codex/shim.ts:445-517`): + +```ts +function refreshShimFile(file: ShimFileState): boolean { + // ... current replacement/backup/write branches ... +} + +export function installCodexShim(): { installed: boolean; message: string } { + const existing = readState(); + if (existing) { + const files = stateFiles(existing); + let refreshed = false; + for (const file of files) refreshed = refreshShimFile(file) || refreshed; + // ... current allInstalled/state write/result ... + } + // ... current discovery/fresh install ... +} +``` + +After (signatures and control flow; retain current branch bodies verbatim inside the internal owner): + +```ts +const CODEX_SHIM_PROBE_BYTES = 16 * 1024; +export const CODEX_SHIM_REPLACEMENT_STABLE_MS = 100; + +interface ShimPathFingerprint { + dev: number; + ino: number; + kind: "file" | "symlink"; + size: number; + mtimeMs: number; + ctimeMs: number; + target?: Omit; +} + +interface InstallCodexShimInternalOptions { + expectedReplacements?: ReadonlyMap; + allowFreshInstall: boolean; +} + +export type CodexShimAutoRestoreResult = + | { status: "not-installed" | "healthy" | "ineligible" | "deferred" | "disabled" } + | { status: "restored"; message: string }; + +function readShimProbePrefix(path: string): string { + // open/read/close at most CODEX_SHIM_PROBE_BYTES; never read a replaced binary in full. +} + +function stableReplacementFingerprint( + path: string, + nowMs: number, +): ShimPathFingerprint | null { + // lstat -> bounded read -> lstat; follow and fingerprint a symlink target. + // Return null for changing, too-young, empty, non-file, unreadable, or dangling paths. +} + +function refreshShimFile( + file: ShimFileState, + expectedReplacement?: ShimPathFingerprint, +): boolean { + // Existing branch classification stays authoritative for explicit/manual install. +} + +interface GuardedRefreshOperation { + file: ShimFileState; + expectedReplacement: ShimPathFingerprint; + sourcePath: string; + writesWrapper: boolean; +} + +interface GuardedRefreshJournalEntry { + operation: GuardedRefreshOperation; + stagedOldBackupPath?: string; + stagedOldWrapperPath?: string; + replacementMovedToBackup: boolean; + shimWritten: boolean; +} + +function planGuardedRefreshTransaction( + files: readonly ShimFileState[], + expectedReplacements: ReadonlyMap, +): GuardedRefreshOperation[] | null { + // BEFORE ANY rename/write: classify every changed sibling and fingerprint-check ALL + // expected source paths. Return null on any mismatch/ineligible branch. +} + +function applyGuardedRefreshTransaction( + operations: readonly GuardedRefreshOperation[], +): boolean { + // For each operation, recheck its source fingerprint immediately before mutation. + // Stage any prior backup and any wrapper that would be overwritten under unique + // transaction-owned names; move the replacement to backup; write the new shim; append + // every completed rename/write to the journal. + // On mismatch or failure, reverse the journal: remove the generated shim, move the + // replacement backup back to sourcePath, restore the staged wrapper, then restore the + // staged prior backup. Only after all siblings succeed may staged files be removed. + // Return false for a cleanly rolled-back mismatch; throw an AggregateError if apply or + // rollback has an unrecovered filesystem failure. +} + +function installCodexShimInternal( + options: InstallCodexShimInternalOptions, +): { installed: boolean; message: string } { + // Existing installCodexShim body. When expectedReplacements exists, build the complete + // operation list with planGuardedRefreshTransaction(), then apply it atomically through + // applyGuardedRefreshTransaction(). Never call the old sequential refresh loop, never + // write state before commit, and never enter fresh discovery when allowFreshInstall=false. +} + +export function installCodexShim(): { installed: boolean; message: string } { + return installCodexShimInternal({ allowFreshInstall: true }); +} + +export function autoRestoreCodexShim(options: { + enabled: () => boolean; + nowMs?: number; +}): CodexShimAutoRestoreResult { + const state = readState(); + if (!state) return { status: "not-installed" }; + if (state.platform !== process.platform) return { status: "ineligible" }; + + // Bounded marker/health probe. Return healthy without calling options.enabled(). + // Require complete tracked state; collect all stable non-shim fingerprints. + // Return deferred before mutation if any changed sibling is young/changing. + + if (!options.enabled()) return { status: "disabled" }; + const result = installCodexShimInternal({ + allowFreshInstall: false, + expectedReplacements, + }); + return result.installed + ? { status: "restored", message: result.message } + : { status: "deferred" }; +} +``` + +Implementation constraint: fingerprint mismatch must be represented as a non-mutating deferred +outcome, not be mistaken for “already installed.” A mismatch found after a sibling mutation must +first restore every journaled sibling and the prior state bytes, then return deferred. Throw only for +actual apply/rollback filesystem failures; the CLI boundary catches those. + +### `src/cli/codex-shim-autorestore.ts` — non-fatal policy boundary (NEW) + +Complete intended shape: + +```ts +import { autoRestoreCodexShim } from "../codex/shim"; +import { + codexShimAutoRestoreEnabled, + readConfigDiagnostics, +} from "../config"; + +export interface CodexShimAutoRestoreCliDeps { + env: NodeJS.ProcessEnv; + warn: (message: string) => void; + restore: typeof autoRestoreCodexShim; + readConfig: typeof readConfigDiagnostics; +} + +const DEFAULT_DEPS: CodexShimAutoRestoreCliDeps = { + env: process.env, + warn: message => console.warn(message), + restore: autoRestoreCodexShim, + readConfig: readConfigDiagnostics, +}; + +export function skipsCodexShimAutoRestore(command: string | undefined, args: string[]): boolean { + if (command === "uninstall" || command === "remove") return true; + return command === "codex-shim" && ["install", "uninstall", "remove"].includes(args[1] ?? ""); +} + +export function maybeAutoRestoreCodexShim( + command: string | undefined, + args: string[], + deps: CodexShimAutoRestoreCliDeps = DEFAULT_DEPS, +): void { + if (skipsCodexShimAutoRestore(command, args)) return; + try { + const result = deps.restore({ + enabled: () => codexShimAutoRestoreEnabled(deps.readConfig().config, deps.env), + }); + if (result.status === "restored") { + deps.warn(`⚠️ ${result.message} (automatic repair after Codex update)`); + } + } catch (error) { + deps.warn( + `⚠️ Codex shim auto-restore failed; continuing without it: ${ + error instanceof Error ? error.message : String(error) + }. Run 'ocx codex-shim install' after the Codex update finishes.`, + ); + } +} +``` + +The final warning text may be tightened, but it must remain secret-free, identify the manual fallback, +and contain no stack, config contents, launcher contents, token, or account identifier. + +### `src/cli/index.ts` — hook + +Before (`src/cli/index.ts:39-60`, `src/cli/index.ts:540`): + +```ts +import { maybeShowStarPrompt } from "./star-prompt"; +import { maybeShowUpdatePrompt } from "../update/notify"; + +if (command !== undefined && command !== "help" && hasHelpFlag(args.slice(1))) { + printSubcommandUsage(command); + process.exit(0); +} + +// ... +switch (command) { +``` + +After: + +```ts +import { maybeAutoRestoreCodexShim } from "./codex-shim-autorestore"; +import { maybeShowStarPrompt } from "./star-prompt"; +import { maybeShowUpdatePrompt } from "../update/notify"; + +if (command !== undefined && command !== "help" && hasHelpFlag(args.slice(1))) { + printSubcommandUsage(command); + process.exit(0); +} + +maybeAutoRestoreCodexShim(command, args); + +// ... +switch (command) { +``` + +Do not put the hook before version/help exits: those paths currently terminate at +`src/cli/index.ts:47-60` and should stay read-only/instant. Do not put it inside `handleStart()`; that +would reduce coverage to the rejected start-only design. + +### Docs sketches + +Configuration row, translated equivalently in every docs-site locale: + +```md +| `codexShimAutoRestore?` | `boolean` | `true` | Restore a previously installed Codex shim when a completed external Codex update replaces it. Set `false`, or set `OPENCODEX_CODEX_SHIM_AUTO_RESTORE=0` for a process-level opt-out. | +``` + +CLI/README behavior replacement: + +```md +If a completed external Codex update overwrites an installed shim, the next ordinary `ocx` command +backs up the new launcher and restores the shim. A launcher that is still changing is left untouched +and retried on a later command. Failures warn without failing the requested command; manual fallback: +`ocx codex-shim install`. +``` + +## Test plan + +### Focused unit and integration tests + +#### `tests/config.test.ts` (MODIFY) + +1. Default config contains `codexShimAutoRestore: true`. +2. `codexShimAutoRestoreEnabled({})` and explicit `true` are enabled. +3. Config `false` disables with env unset or `=1`. +4. Env `OPENCODEX_CODEX_SHIM_AUTO_RESTORE=0` disables config unset/true. +5. Invalid persisted values (`null`, `"false"`, `0`) fail diagnostics at + `codexShimAutoRestore`, matching current boolean validation tests at `tests/config.test.ts:106-134`. + +#### `tests/codex-shim.test.ts` (MODIFY) + +Use temp `PATH`/`OPENCODEX_HOME`, install through public `installCodexShim()`, and restore environment +and files in `finally`, following the current real filesystem setup at +`tests/codex-shim.test.ts:280-320`. + +1. Healthy installed shim -> `healthy`; `enabled()` is never called; wrapper/backup/state bytes and + mtimes do not change. +2. Stable non-shim replacement (mtime/ctime beyond the 100 ms gate) -> `restored`; the replacement + becomes the backup and wrapper contains the marker. +3. Just-written replacement -> `deferred`; no path changes. Age it, retry, then restore. +4. Fingerprint mismatch before guarded rename -> `deferred`; no backup or wrapper mutation. +5. **Mandatory transactional race — second sibling changes after the first is applied.** Name: + `test("multi-wrapper restore rolls back when a later sibling fingerprint changes", () => { ... })`. + Create stable changed `.cmd` and `.ps1` siblings and capture both fingerprints. Inject a narrow + filesystem seam/hook that changes the second sibling after the first sibling's guarded repair has + completed but before the second sibling's immediate pre-rename recheck. Assert result is + `deferred`; first and second wrapper bytes, both prior backup bytes, execute bits where relevant, + and state JSON are byte-for-byte/metadata-equivalent to their pre-transaction values; no staged + transaction path remains; no `restored` message is emitted. This activates mid-sequence rollback, + not only transaction-wide zero-mutation preflight. +6. Missing backup, missing wrapper, corrupt state, directory/dangling symlink, and state platform + mismatch -> `ineligible`/`deferred`; never fresh-install. +7. Windows CI verifies `.cmd`, `.ps1`, and extensionless tracked siblings restore as one transaction; + Unix CI verifies execute bit and symlink-target stability. WSL guard remains covered by existing + tests at `tests/codex-shim.test.ts:323-370`. + +#### `tests/codex-shim-autorestore.test.ts` (NEW) + +1. Helper skips top-level uninstall/remove and shim install/uninstall/remove; it does not skip + `codex-shim status` or ordinary commands. +2. Injected `restore` throw -> exactly one warning with manual fallback; helper returns normally. +3. Injected `restored` -> exactly one automatic-repair warning. +4. Injected `healthy`, `not-installed`, `disabled`, or `deferred` -> no warning. +5. Lazy config assertion: `readConfig` is not called when core reports healthy/not-installed. +6. Spawned activation: install a temp shim, replace it with a stable real launcher, run the next + `ocx codex-shim status`, assert exit 0, repair warning on stderr, healthy shim status on stdout, + marker at wrapper, and new launcher bytes in backup. + +### Required activation scenarios + +| Scenario | Setup | Trigger | Required observation | +| --- | --- | --- | --- | +| Shim replaced by real Codex binary | Valid installed state; overwrite tracked wrapper(s) with stable non-shim launcher(s) | Next ordinary `ocx` command | Wrapper auto-restored before dispatch; new launcher backed up; one warning; requested command exits normally. | +| Shim intact / zero-mutation overhead path | Valid healthy state | Any ordinary `ocx` command | Bounded state/prefix/metadata reads only; no config parse, rename, write, warning, subprocess, timer, or network. Typical probe <5 ms. | +| Restore failure | Stable candidate; injected/real filesystem failure | Ordinary `ocx` command | Warning includes manual fallback; command's own output and exit status are unchanged. | +| Opt-out set | Config false and, separately, env `=0` | Ordinary `ocx` command | Candidate remains untouched; no restore warning; explicit `ocx codex-shim install` still works. | +| npm update in progress | Changed wrapper is too young or fingerprint changes | Ordinary `ocx` command | Silent defer; no rename/write; later command after stability restores. | + +### Performance proof + +Add a small focused benchmark script invocation to the implementation devlog (not a permanent timing +assertion, which would be flaky in shared CI): create one Unix tracked wrapper or the three Windows +tracked wrappers, warm the module, run the healthy probe 1,000 times, report median and p95 using +`performance.now()`. Acceptance: median and p95 both `< 5 ms` on the maintainer test machine; no sample +performs a write. Keep the permanent test structural: assert `enabled()`/config and repair paths are not +called for healthy state. + +## Verification (C) + +Run in this order from repository root: + +```bash +bun test --isolate tests/config.test.ts tests/codex-shim.test.ts tests/codex-shim-autorestore.test.ts +bun run typecheck +bun run test +bun run privacy:scan +``` + +Expected: every command exits `0`; full tests report zero failures; privacy scan reports no credential, +personal-path, token, or account leakage. Also run the focused healthy-probe benchmark above on at least +one Unix host and use Cross-platform CI for Windows behavior. Do not claim cross-platform completion +from a macOS-only local run. + +## Security-review checkpoint + +This checkpoint is mandatory before merge under `MAINTAINERS.md:22-25`. + +### Threat model + +- Assets: the real Codex launcher, OpenCodex-owned launcher backup, shim state, nearby OpenCodex config + directory, and credentials stored separately under that directory. +- Entrypoints: external npm launcher replacement, ordinary CLI startup, persisted config flag, env + opt-out, and state JSON read from disk. +- Attacker/failure capabilities: interrupted or concurrent npm update; local process modifying tracked + paths/state; symlink swap; concurrent `ocx` commands; malformed state; filesystem permission error. +- Trust boundary: filesystem contents and environment are runtime inputs. Auto-restore may mutate only + paths already recorded by a valid prior shim install; it must not rediscover arbitrary PATH targets. +- Blast radius: tracked Codex launcher and owned backup only. No auth/account/token file may be read, + written, serialized, or logged. + +### Must-pass reviewer checks + +- [ ] Auto-restore requires valid prior state, same platform, complete backups, stable fingerprints, + and at least one tracked non-shim; it cannot fresh-install. +- [ ] All sibling fingerprints are revalidated before any mutation; a preflight mismatch causes zero + mutation. +- [ ] Each sibling is revalidated again immediately before its first rename; a later mismatch rolls + back already-mutated siblings, leaves state unchanged, and returns `deferred`. +- [ ] Symlink target must be a stable non-empty regular file; no directory/dangling-link traversal. +- [ ] Existing single-path rollback in `replaceOwnedBackup()` remains intact for manual install, and + guarded auto-restore adds transaction-wide reverse-order sibling rollback. +- [ ] Windows `.exe` refusal and WSL interop guard remain intact (`src/codex/shim.ts:132-207`). +- [ ] Startup opt-out uses `readConfigDiagnostics()`, not `loadConfig()`, and never opens `auth.json`. +- [ ] Warning/error strings contain status and path-safe context only; no file contents, env dump, + stack, token, account id, request body, or credential identifier. +- [ ] `OPENCODEX_CODEX_SHIM_AUTO_RESTORE=0` and config false prevent automatic writes but do not weaken + explicit install/uninstall semantics. +- [ ] `bun run privacy:scan`, focused negative tests, full tests, and Cross-platform CI are green. + +## Acceptance checklist + +- [ ] The chosen hook is before ordinary CLI dispatch and after help/version exits. +- [ ] Healthy/not-installed probes are bounded, read-only, lazy-config, and <5 ms typical. +- [ ] Stable external update replacement is restored through the same internal transaction used by + `ocx codex-shim install`. +- [ ] Partial/changing update, race, or failure never breaks the requested command. +- [ ] Config and env opt-outs are typed, documented, and tested. +- [ ] macOS/Linux/Windows/Git-Bash/WSL guards are preserved. +- [ ] Issue #320 part 1 remains untouched and documented as upstream/out of scope. +- [ ] Runtime SOT, all root README locales, and all docs-site CLI/config locales agree. +- [ ] Explicit security review is recorded before merge. diff --git a/devlog/_plan/260724_bugfix_train/040_ux_discovery_badge.md b/devlog/_plan/260724_bugfix_train/040_ux_discovery_badge.md new file mode 100644 index 00000000000..7f2dd4800be --- /dev/null +++ b/devlog/_plan/260724_bugfix_train/040_ux_discovery_badge.md @@ -0,0 +1,817 @@ +# 040 — Cycle 4: discovery failure visibility and honest helper fallback copy + +> DIFFLEVEL-ROADMAP-01: this is the implementation SSOT for Cycle 4 of +> `260724_bugfix_train`. Re-verify every cited line against the cycle-start SHA before B; +> do not implement from the issue summaries alone. + +## Loop spec + +- **Loop archetype:** verifier-defined, spec-satisfaction repair. +- **Trigger:** issue #329's core zero-model grouping/manual-add repair is present, but + live discovery failures such as HTTP 401 remain log-only; issue #331's empty + `smallFastModel` runtime behavior leaves both helper overrides unset while the GUI says + “Claude default (Haiku).” +- **Goal / user-visible outcome:** an active provider whose latest live model discovery + failed exposes a safe machine-readable reason through `GET /api/providers`; when that + provider has no model rows, Models shows a compact failure badge and reason beside the + existing recovery guidance. The Claude page accurately says that an unset helper model + delegates selection to Claude Code and warns that native Sonnet usage may incur native + provider cost. +- **Non-goals:** no retry button, no new discovery request, no persistence of transient + discovery state, no upstream response body/status-text exposure, no routing or fallback + behavior change, no provider editor redesign, no broad Claude settings redesign, and no + #337 auto-switch takeover work. +- **Verifier:** `bun run typecheck`, `bun run test`, `bun run lint:gui`, + `bun run build:gui`, and `bun run privacy:scan` must each exit 0. Focused tests prove + the status contract and both conditional UI states. A real Vite render and observed + screenshot prove the empty-provider badge is visible and legible. +- **Stop condition:** stop only when the 401, successful discovery, helper-unset, and + helper-set activation scenarios below pass; all five repository gates pass; the Vite + screenshot has been opened and observed; docs/locales agree; and the final diff still + avoids #337's changed keys and CSS hunks. +- **Memory artifact:** this document, later moved by the parent workflow to the unit's + `_fin/` location with C/D evidence. The delegated writer does not operate parent loop or + goal state. +- **Expected terminal outcomes:** `DONE`; `BLOCKED` if a required existing seam cannot + represent last-attempt status without changing discovery semantics; `NEEDS_HUMAN` if + product owners reject the native-Sonnet/cost wording. +- **Escalation:** upward to the parent before expanding the API beyond an additive optional + field, exposing provider-controlled text, touching auth/token material, altering runtime + selection, or editing a #337-owned key/hunk. No downward delegation is planned; any such + delegation is a P-phase amendment owned by the parent, not a B-phase improvisation. + +## Grounding and necessity gate + +Cycle-start base is `origin/dev` / `d9e06c8dd08df6635f5ca042bf6aa469fe1a10a8`. + +- `src/codex/catalog/provider-fetch.ts:207-388` owns live discovery, fallback, cooldown + activation, logs, and successful cache writes. HTTP non-OK is reduced to a warning at + `:307-313`; valid discovery is cached at `:343-380`. +- `src/codex/model-cache.ts:20-61` already owns per-provider cache and failed-fetch + cooldown lifecycle. `clearModelCache()` is therefore the correct owner for clearing the + associated last-attempt diagnostic; a second independent map module would drift. +- `src/server/management/provider-routes.ts:71-81` builds the additive provider DTO. +- `gui/src/models-groups.ts:1-53` is the pure join between `/api/providers` and model rows; + configured zero-row providers are retained at `:29-37`. +- `gui/src/pages/Models.tsx:180-224` loads both endpoints; `:729-795` renders groups and + passes empty providers to `EmptyProviderHint`; `:1111-1121` owns the current generic + guidance and is the smallest render seam. Because the current `Promise.all` starts + `/api/models` and `/api/providers` concurrently, the provider DTO can win the race before + discovery records its result; order the DTO read after the model read so the first render + is grounded in the attempt it displays. +- `src/claude/context-windows.ts:152-173` proves `tierModels.haiku ?? smallFastModel` is + written to both helper env variables only when non-empty. Existing runtime regression + coverage includes the unset case at `tests/claude-cli.test.ts:214`. +- `gui/src/pages/ClaudeCode.tsx:141-144` labels the empty option with + `claude.slotUnset`; `:348-356` renders the misleading helper copy and picker. +- The six GUI dictionaries are type-locked by English `TKey` through + `gui/src/i18n/shared.ts:1-12`; every new English key must be added to de/ja/ko/ru/zh. +- Existing `.badge`, `.badge-amber`, and `.notice-warn` rules at + `gui/src/styles.css:498-502,730-731` satisfy the visual need. Reuse them; do not add or + edit CSS. + +No-code options rejected: doing nothing leaves #329 log-only and #331 factually wrong; +configuration cannot surface a runtime HTTP result; deleting copy would hide the cost +consequence; reusing only the generic empty hint cannot distinguish an authoritative empty +catalog from a failed request. The plan reuses the model-cache lifecycle, provider DTO, +grouping seam, existing badge/warning classes, and existing i18n system; it introduces no +dependency or background polling. + +## Scope + +### IN + +1. Ephemeral last-attempt discovery status in the model-cache lifecycle. +2. Additive optional discovery metadata on each `GET /api/providers` row. +3. Empty-provider failure badge/reason in Models, with successful/unknown/static states + retaining the current non-error guidance. +4. Accurate helper description, unset-option label, and conditional native Sonnet/cost + warning in all six GUI locales. +5. English plus all existing translated Claude Code guides kept semantically aligned. +6. Focused backend/API/SSR tests, full gates, and a real rendered screenshot observation. + +### OUT + +- Discovery retries, timestamps, history, telemetry, disk persistence, status for disabled + or forward-auth providers, and showing a badge when stale/static rows are already present. +- Any raw response body, URL, API key, account identifier, `statusText`, or thrown message in + the API/UI. +- Changes to `src/claude/context-windows.ts` or Claude Code's actual model choice. +- Changes to provider setup, account auto-switch, global visual tokens, or release files. + +## API contract + +`GET /api/providers` remains an array of provider rows. Add one optional field: + +```ts +export type ProviderModelDiscoveryFailureReason = + | "http" + | "blocked" + | "invalid_response" + | "network" + | "provider"; + +export type ProviderModelDiscoveryStatus = + | { status: "ok" } + | { + status: "failed"; + reason: ProviderModelDiscoveryFailureReason; + httpStatus?: number; + }; + +// GET /api/providers row (new field only) +{ + name: "example", + // ...existing fields unchanged... + discovery?: ProviderModelDiscoveryStatus +} +``` + +Example 401 row: + +```json +{ + "name": "example", + "liveModels": true, + "models": [], + "discovery": { + "status": "failed", + "reason": "http", + "httpStatus": 401 + } +} +``` + +Contract rules: + +- `status: "ok"` means the most recent attempted live discovery returned a valid models + payload, including an authoritative empty array. It does not promise that models exist. +- `status: "failed"` means the most recent attempted live discovery took a fallback path. + `httpStatus` is present only for `reason: "http"` and is the integer HTTP status. +- No status exists before an attempt or for paths that intentionally do not discover + (`liveModels: false`, forward auth, or OAuth without a token). Because object properties + with `undefined` are omitted by JSON serialization, old consumers receive their existing + row shape plus, at most, an unknown additive field. +- Cache hits/cooldown reads preserve the last attempted status. A later valid live response + replaces `failed` with `ok`. `clearModelCache(provider)` and global clear remove both cache + and status, preventing a provider edit from retaining stale diagnostics. +- `reason` is a closed server-owned code. The UI localizes it; it never parses or displays + a provider-controlled message. For issue #329 the visible detail is derived as `HTTP 401`. + +## Exact change map + +### Backend and management API + +#### MODIFY `src/codex/model-cache.ts` + +Before: + +```ts +const cache = new Map(); +const failureAt = new Map(); + +export function markModelsFetchFailure(provider: string, now = Date.now()): void { + failureAt.set(provider, now); +} + +export function clearModelCache(provider?: string): void { + if (provider) { + cache.delete(provider); + failureAt.delete(provider); + } else { + cache.clear(); + failureAt.clear(); + } +} +``` + +After: + +```ts +export type ProviderModelDiscoveryFailureReason = + | "http" | "blocked" | "invalid_response" | "network" | "provider"; + +export type ProviderModelDiscoveryStatus = + | { status: "ok" } + | { status: "failed"; reason: ProviderModelDiscoveryFailureReason; httpStatus?: number }; + +const discoveryStatus = new Map(); + +export function markProviderDiscoveryOk(provider: string): void { + discoveryStatus.set(provider, { status: "ok" }); +} + +export function markProviderDiscoveryFailed( + provider: string, + failure: Omit, "status">, +): void { + discoveryStatus.set(provider, { status: "failed", ...failure }); +} + +export function getProviderDiscoveryStatus( + provider: string, +): ProviderModelDiscoveryStatus | undefined { + return discoveryStatus.get(provider); +} + +// Existing cache/failureAt clearing remains, plus discoveryStatus delete/clear. +``` + +Keep `markModelsFetchFailure(provider, now?)` unchanged for cooldown compatibility. Do not +make `setCached()` imply discovery success because tests and non-fetch callers seed cache +entries directly. + +#### MODIFY `src/codex/catalog/provider-fetch.ts` + +Extend the existing model-cache import with +`markProviderDiscoveryOk` and `markProviderDiscoveryFailed`. + +Before: + +```ts +const failedDiscoveryFallback = (): { models: CatalogModel[]; fallback: "stale" | "configured" } => { + markModelsFetchFailure(name); + // ... +}; + +if (!res.ok) { + const { models, fallback } = failedDiscoveryFallback(); + // log only + return models; +} + +setCached(name, live); +return live; +``` + +After: + +```ts +type DiscoveryFailure = Omit< + Extract, + "status" +>; + +const failedDiscoveryFallback = ( + failure: DiscoveryFailure, +): { models: CatalogModel[]; fallback: "stale" | "configured" } => { + markModelsFetchFailure(name); + markProviderDiscoveryFailed(name, failure); + // existing stale/configured fallback is byte-for-byte unchanged +}; + +if (!res.ok) { + const { models, fallback } = failedDiscoveryFallback({ + reason: "http", + httpStatus: res.status, + }); + // existing sanitized warning remains + return models; +} + +markProviderDiscoveryOk(name); +setCached(name, live); +return live; +``` + +Use the same helper for every generic failure path: + +| Existing branch | Recorded failure | +|---|---| +| destination-policy rejection (`:295-305`) | `{ reason: "blocked" }` | +| HTTP non-OK (`:307-314`) | `{ reason: "http", httpStatus: res.status }` | +| invalid JSON 2xx (`:316-332`) | `{ reason: "invalid_response" }` | +| malformed `data` 2xx (`:333-342`) | `{ reason: "invalid_response" }` | +| thrown fetch/timeout (`:381-387`) | `{ reason: "network" }` | +| Cursor `liveResult.ok === false` (`:249-261`) | `{ reason: "provider" }` | + +Call `markProviderDiscoveryOk(name)` in both successful live seams: Cursor immediately +before its `setCached(name, result)` and generic discovery immediately before +`setCached(name, live)`. Do not mark static, forward-auth, missing-token, fresh-cache, or +cooldown-only returns as newly successful; they did not perform an attempt. + +#### MODIFY `src/server/management/provider-routes.ts` + +Import `getProviderDiscoveryStatus` from `../../codex/model-cache` and append one property +to the existing DTO at `:72-81`: + +```ts +return jsonResponse(Object.entries(config.providers).map(([name, p]) => ({ + // existing fields unchanged + codexAccountMode: providerCodexAccountMode(name, p), + discovery: getProviderDiscoveryStatus(name), +}))); +``` + +Do not trigger discovery from this route. The Models implementation below orders +`/api/providers` after `/api/models` on each existing load (`gui/src/pages/Models.tsx:180-219`), +so the route reports the completed fetch lifecycle rather than duplicating it. + +#### MODIFY `tests/codex-catalog.test.ts` + +- Extend the existing HTTP non-OK case at `:1567-1594` to use 401 and assert + `getProviderDiscoveryStatus(provider)` equals + `{ status: "failed", reason: "http", httpStatus: 401 }` while configured fallback rows + remain unchanged. +- Extend a valid live-discovery case (the private-network opt-in case at `:1460-1485` is + suitable) to assert `{ status: "ok" }`. +- Retain the existing sanitized-log assertions. Cleanup continues through + `clearModelCache(provider)`, now also proving status isolation between tests. + +#### MODIFY `tests/management-provider-validation.test.ts` + +Add a direct management contract test using the existing `handleManagementAPI` seam: + +```ts +markProviderDiscoveryFailed("auth-broken", { reason: "http", httpStatus: 401 }); +const response = await handleManagementAPI( + new Request("http://127.0.0.1/api/providers"), + new URL("http://127.0.0.1/api/providers"), + { providers: { "auth-broken": providerFixture } }, +); +expect(await response!.json()).toContainEqual(expect.objectContaining({ + name: "auth-broken", + discovery: { status: "failed", reason: "http", httpStatus: 401 }, +})); +``` + +Also include an unattempted provider and assert it has no own `discovery` property after +JSON serialization. Clear status in `finally` with `clearModelCache()`. + +### Models GUI + +#### MODIFY `gui/src/models-groups.ts` + +Before, `ConfiguredProviderSummary` and `ProviderModelGroup` carry only live/static model +metadata. After, add the API union and thread it through the pure join: + +```ts +export type ProviderDiscoverySummary = + | { status: "ok" } + | { + status: "failed"; + reason: "http" | "blocked" | "invalid_response" | "network" | "provider"; + httpStatus?: number; + }; + +export interface ConfiguredProviderSummary { + // existing fields + discovery?: ProviderDiscoverySummary; +} + +export interface ProviderModelGroup { + // existing fields + discovery?: ProviderDiscoverySummary; +} + +// map result +discovery: configured?.discovery, +``` + +No new client fetch, store, parser, or component-level status map. + +#### MODIFY `gui/src/pages/Models.tsx` + +Make the load order explicit. Keep `/api/models` and context caps parallel, then read +`/api/providers` only after `/api/models` has completed and recorded discovery status: + +```ts +const [data, capsData] = await Promise.all([ + fetch(`${apiBase}/api/models`).then(r => r.json()) as Promise, + fetch(`${apiBase}/api/provider-context-caps`).then(r => r.json()) + as Promise, +]); +const providerData = await fetch(`${apiBase}/api/providers`) + .then(r => r.json()) as ConfiguredProviderSummary[]; +``` + +This changes request ordering only; it adds no request, retry, or timer. Retain the existing +ten-second refresh and busy guard. The ordering is required so a first-load 401 reaches the +same render rather than waiting for the next poll. + +Destructure `discovery` from each group and pass it only to the zero-row hint: + +```tsx +groups.map(({ provider, rows, native: isNative, liveModels, discovery }) => { + // ... + {rows.length === 0 && ( + + )} +}) +``` + +Before: + +```tsx +export function EmptyProviderHint({ liveModels }: { liveModels: boolean }) { + // generic status row only +} +``` + +After: + +```tsx +export function EmptyProviderHint({ + liveModels, + discovery, +}: { + liveModels: boolean; + discovery?: ProviderDiscoverySummary; +}) { + const t = useT(); + const failed = liveModels && discovery?.status === "failed"; + const reason = !failed + ? "" + : discovery.reason === "http" && discovery.httpStatus !== undefined + ? t("models.discoveryFailedHttp", { status: discovery.httpStatus }) + : discovery.reason === "blocked" + ? t("models.discoveryFailedBlocked") + : discovery.reason === "invalid_response" + ? t("models.discoveryFailedInvalidResponse") + : discovery.reason === "network" + ? t("models.discoveryFailedNetwork") + : discovery.reason === "provider" + ? t("models.discoveryFailedProvider") + : t("models.discoveryFailedGeneric"); + + return ( +
+ + + {failed && ( + {t("models.discoveryFailedBadge")} + )} + {failed ? `${reason} ` : t(liveModels + ? "models.emptyDiscovery" + : "models.emptyDiscoveryDisabled") + " "} + {t("models.openProviderSettings")} + +
+ ); +} +``` + +Keep failure meaning in text as well as color. Use existing `.badge badge-amber`; **no +`gui/src/styles.css` change**. Exact JSX spacing may use nested spans rather than string +concatenation, but rendered text and accessible order must be: badge → reason → settings +link. An `ok` or absent discovery status renders no badge. + +#### MODIFY `gui/tests/models-empty-provider.test.tsx` + +Change `renderHint` to accept optional `ProviderDiscoverySummary`, then cover: + +1. `liveModels=true` + failed/http/401 renders `Discovery failed`, `HTTP 401`, existing + settings link, `badge badge-amber`, and `role="status"`. +2. `liveModels=true` + `{ status: "ok" }` renders existing generic empty guidance and no + failure badge. +3. Existing `liveModels=false` test remains and renders no failure badge even if no status + exists. + +#### MODIFY `tests/models-page-groups.test.ts` + +Extend the configured zero-row provider fixture with a failed discovery object and assert +the resulting group preserves it. This prevents the API metadata from being dropped at the +pure grouping seam. + +### Claude helper UX + +#### MODIFY `gui/src/pages/ClaudeCode.tsx` + +Replace the helper-only use of the misleading old keys with new keys; keep the old keys in +dictionaries for compatibility and to avoid #337-adjacent edits. + +Extract a focused render seam: + +```tsx +export function SmallFastModelSetting({ + value, + options, + onChange, +}: { + value: string; + options: SelectOption[]; + onChange: (value: string) => void; +}) { + const t = useT(); + return ( + <> +
{t("claude.smallFastModel")}
+

+ {t("claude.smallFastModelAccurateHint")} +

+