fix(sse): accept unspaced data: fields across six parsers (#1170)

devlog/_plan/260807_untouched_bug_stack/000_plan.md

@@ -0,0 +1,127 @@
+# 260807 — untouched-bug stack: research and roadmap
+
+Base: `codex/260807-stack-base` at `origin/dev@6d04574d0`.
+Cycle: docs-first. This unit writes the plan; no production code changes land here.
+
+## Why this unit exists
+
+A sweep over the 60 open issues and 24 open PRs found two distinct backlogs that
+the merged bug campaign did not reach.
+
+The first is a **CI admission backlog**. Eight bug-fix PRs were reported as
+"never ran CI", which reads like contributor neglect but is not: 524 workflow
+runs sat in `action_required`, waiting on maintainer approval. Thirty-nine of
+them belonged to branches with an open PR. The readiness gate cannot verify the
+`ci` check on a run that was never allowed to start, so those PRs could not
+leave draft no matter what their authors did. Approving the open-PR subset is
+the precondition for every disposition below; approving all 524 is not, because
+most belong to branches already merged or abandoned.
+
+The second is a set of **defects with no PR at all** — issues where a reporter
+filed evidence and nothing was ever opened against it.
+
+## Disposition summary
+
+Every verdict below was reached by reading the diff and the current tree, not
+the PR description.
+
+| Target | Verdict | Reason |
+|---|---|---|
+| #557 npm cache preflight | rewrite | dev is 1,220 commits past the merge base; diff mixes the useful preflight with obsolete recovery machinery |
+| #1095 DeepSeek progressive streaming | rewrite | 2,184-line diff carries an unsafe terminal-repair state machine and a raw-fragment race |
+| #1155 web-search buffered policy | adopt with changes | correct intent; `parseResponse` misuse and a lease leak must be fixed |
+| #1159 Cursor Grok wire prefix | adopt as-is | request-only helper, correctly isolated from discovery |
+| #1171 A6API unlimited quota | adopt as-is | unlimited branch ordered before finite validation, focused coverage |
+| #1163 combo catalog fallback | rewrite | resolver cannot distinguish missing rows from deliberately filtered ones |
+| #1152 account picker selectors | adopt with changes | foundations are sound but the entry point has no production caller |
+| #1169 codex-shim readiness warning | adopt with changes | advisory design is right; the probe can throw and fail a good install |
+| #1131 in-place restart identity | rewrite | 35 files with unresolved lifecycle defects; CI red was a GitHub outage, not the code |
+| #1056 desktop picker (#241) | rewrite | 54-file branch with backup poisoning and lost-alias defects |
+| #1170 unspaced SSE frames | new fix | strict `"data: "` prefix in six parsers |
+| #1100 routed reasoning effort | new fix | routed rows advertise ladders, then lose summary support |
+| #1156 Windows ACL budget | new fix | a complete ACL sequence gets only five seconds |
+
+## Two corrections to the initial triage
+
+Recording these because both changed the plan.
+
+**#1156 was described imprecisely.** The first pass said PR #1135's retry shares
+the 5-second budget. It does not — owner-level recovery at
+`src/codex/native-main-owner.ts:205-210` calls `hardenSecret` again and receives
+a fresh deadline. The real defect is narrower and still real: one complete ACL
+sequence (grant, inheritance, verify, with `/findsid` fallbacks) must finish
+inside a single 5-second envelope. The fix is the envelope size, not the retry
+structure.
+
+**#1170 has six call sites, not one.** The reporter named the OpenAI Chat
+adapter. The same strict prefix also sits in `src/chat/outbound.ts`,
+`src/web-search/parse.ts`, `src/server/claude-messages.ts`, and — twice —
+`src/claude/outbound.ts`, which contains two independent parsers (`:591-605`
+and `:864-865`). The second one was missed on our first pass and found in audit.
+Fixing only the reported site would leave five live paths broken.
+
+## Roadmap
+
+Implementation phases, one decade doc each, one PABCD cycle each:
+
+- `010` — #1170 unspaced SSE field parsing (6 call sites, 2 shared primitives)
+- `020` — #1100 routed reasoning-effort propagation
+- `030` — #1156 Windows ACL harden envelope
+- `040` — #557 replacement: npm cache preflight + log sanitization
+- `050` — adopt-as-is PR replacements (#1159, #1171)
+- `060` — adopt-with-changes PR replacements (#1155, #1152, #1169)
+
+Rewrite-class targets (#1095, #1163, #1131, #1056) are deliberately not in this
+roadmap. Each is a full unit of work with its own defect list, and folding four
+rewrites into this stack would produce a chain no reviewer can follow. They are
+recorded here so the next unit can pick them up with the audit already done.
+
+## Stack shape
+
+Sequential stacked PRs. Each targets `dev` or the previous PR's head branch, per
+the stacked-child workflow that `enforce-target` already supports.
+
+`010` and `020` and `030` touch disjoint production files, so their order is a
+review convenience rather than a dependency. `040` is independent of all three.
+`050` and `060` follow because they replace existing PRs and their originals
+must be closed with a pointer to the replacement.
+
+One real overlap: `040` and `060` both edit lifecycle locale files. Whichever
+lands first, the other rebases.
+
+## Review gates beyond CI
+
+`MAINTAINERS.md` requires explicit security review for credential/permission
+handling and for the dependency-install path. Two phases are in that class and
+cannot go ready on green CI alone:
+
+- `030` — Windows ACL permission handling
+- `040` — npm install path plus log sanitization
+
+Both run `bun run privacy:scan` and request security review before leaving
+draft.
+
+## Out of scope
+
+No promotion to `main` or `preview`, no npm publish, no release tag, and no
+merge. Merging is a separate authorization; this unit stops at open PRs with
+green CI.
+
+## Audit record
+
+This plan failed its first independent audit with six blockers, all corrected
+in place:
+
+1. `010` missed a second parser in `src/claude/outbound.ts` and did not address
+   CRLF framing or multiline `data` joining.
+2. `020` did not specify Record merge semantics; a whole-Record fill-if-undefined
+   would let one user override suppress every registry default.
+3. `030` claimed a ~60s worst case; the real load-time bound is ~90s because
+   `loadConfig()` hardens three paths sequentially (`src/config.ts:1759-1764`).
+4. `040` cited `src/update/job.ts:269-280` as the launcher invocation; that
+   builds the command, and the invocation is at `:1469`.
+5. `060` proposed wiring into a picker-enable transaction that does not exist.
+6. Security-review gates for `030` and `040` were missing.
+
+Recording this because the corrections changed what gets built, not just how it
+is described.

devlog/_plan/260807_untouched_bug_stack/010_sse_unspaced_data_fields.md

@@ -0,0 +1,110 @@
+# 010 — #1170: SSE parsers reject unspaced `data:` fields
+
+## Defect
+
+The SSE spec makes the space after `data:` optional; a compliant producer may
+send `data:{"choices":[...]}`. Four adapter-side parsers require the space and
+silently drop every frame without it, so a stream from such a provider looks
+like a completed turn with no content.
+
+Strict call sites on `origin/dev@6d04574d0`:
+
+- `src/adapters/openai-chat.ts:950` — `if (!line.startsWith("data: ")) return "continue";`
+- `src/chat/outbound.ts:674`
+- `src/claude/outbound.ts:865` (and the `event: ` sibling at `:864`)
+- `src/claude/outbound.ts:591-605` — a **second, separate** parser in the same
+  file, missed on the first pass; it does its own `startsWith("event: ", ...)`
+  and `startsWith("data: ", ...)` against a raw frame with byte-budget
+  accounting interleaved
+- `src/web-search/parse.ts:190`
+- `src/server/claude-messages.ts:174` — another site, not named in the report
+
+Lenient call sites that prove the intended behavior:
+
+- `src/lib/sse-decoder.ts:193-208` — strips at most one leading ASCII space
+- `src/server/relay.ts:262-269`
+- `src/adapters/google.ts:618-620`
+
+The split is the bug: the same wire format is accepted on the relay path and
+rejected on the adapter path.
+
+## Change
+
+Export one pure helper from `src/lib/sse-decoder.ts`, beside the decoder whose
+semantics it mirrors:
+
+```ts
+export function sseFieldValue(line: string, field: string): string | null;
+```
+
+Returns `null` when the line is not that field. Otherwise returns the value with
+at most one leading ASCII space removed — one, not `trimStart()`, because
+leading whitespace beyond the first character is payload.
+
+Then replace the strict prefix checks with calls to it — **six sites, not
+five**. `src/claude/outbound.ts` has two independent parsers (`:591-605` and
+`:864-865`); both need it, and both need it for `event` as well as `data`, since
+the `event: ` check carries the identical defect.
+
+The `:591-605` site is the delicate one: it reserves and commits translator
+budget per fragment, so the edit must change only which offset the fragment
+starts at, leaving every `reserveTransient` / `commitRetained` /
+`releaseRetained` call and its byte accounting untouched.
+
+Deliberately not doing: migrating these collectors to `decodeServerSentEvents`.
+They own different buffering, budget accounting, heartbeat, and EOF-failure
+behavior. Replacing six short prefix checks is the whole change; swapping
+six stream state machines is not.
+
+Four local copies of `slice(5)` would also work and would be worse — this class
+of bug is exactly what happens when the same parsing rule is written six times.
+
+## Preserve
+
+Each caller's existing `.trim()` on the extracted payload stays where it is. The
+helper does not trim, so callers that intentionally keep payload whitespace are
+unaffected.
+
+## Two adjacent defects, deliberately not fixed here
+
+The audit surfaced two more spec deviations in the same parsers. Naming them so
+the next reader does not assume this phase covered them:
+
+1. **Frame delimiting.** `src/claude/outbound.ts:567` and
+   `src/server/claude-messages.ts:171` split on `\n\n` only, so a CRLF producer
+   (`\r\n\r\n`) is not framed correctly.
+2. **Multiline `data` joining.** `src/claude/outbound.ts:605` and
+   `src/server/claude-messages.ts:174` concatenate consecutive `data` fragments
+   with no separator; the spec joins them with `\n`.
+
+Both are real, and both are frame-level rather than field-level — fixing them
+means changing buffering and joining semantics, which is a different blast
+radius from swapping a prefix check. This phase stays field-level so the diff
+stays reviewable. The tests below add CRLF and multiline cases as
+**characterization** tests that record current behavior, so whoever fixes the
+framing has a baseline and cannot regress the prefix fix while doing it.
+
+## Tests
+
+Each asserts real content arrives rather than a silently empty turn.
+
+| File | Test | Assertion |
+|---|---|---|
+| `tests/openai-chat-hardening.test.ts` | `accepts unspaced data fields and finish_reason without DONE (#1170)` | text delta, `done`, stop reason, usage |
+| `tests/chat-completions-endpoint.test.ts` | `collectChatCompletion accepts unspaced data fields` | final `message.content` |
+| `tests/claude-outbound.test.ts` | `collectAnthropicMessage accepts unspaced event and data fields` | completed text and stop reason |
+| `tests/web-search-parse.test.ts` | `parseSidecarSSE accepts unspaced data fields` | completed text and source extraction |
+| `tests/claude-outbound.test.ts` | `raw-frame parser accepts unspaced event and data fields` | the `:591-605` parser, with budget accounting intact |
+| `tests/claude-messages-endpoint.test.ts` | `usage extraction accepts unspaced data fields` | the `src/server/claude-messages.ts:174` site: usage extraction and finalization |
+| `tests/claude-outbound.test.ts` | `characterizes CRLF framing and multiline data joining` | records today's behavior for the two deferred defects |
+
+Every one of these fails before the change: the frames are dropped and the
+assertions see empty output. Each of the six production call sites has a test
+that covers it.
+
+## Blast radius
+
+Unknown-line handling, multiline `data` joining, CRLF, `[DONE]`, translator
+accounting, and fail-closed EOF. The helper is additive and pure, so the risk is
+concentrated in whether each call site's replacement preserves its own trim and
+continue/terminate semantics. Read each of the six in full before editing.

devlog/_plan/260807_untouched_bug_stack/020_routed_reasoning_effort.md

@@ -0,0 +1,111 @@
+# 020 — #1100: reasoning effort never reaches routed DeepSeek and GLM
+
+## Defect
+
+A user picks a reasoning effort in Codex Desktop for a routed DeepSeek or GLM
+model. The proxy forwards the turn without it, so the provider runs at its
+default and the picker appears inert.
+
+The contradiction is inside catalog generation:
+
+1. `src/codex/catalog/effort.ts:144-180` — `applyReasoningLevels` advertises the
+   effort ladder on routed rows.
+2. `src/codex/catalog/parsing.ts:341-353` — `normalizeRoutedCatalogEntry` then
+   deletes `supports_reasoning_summaries`.
+3. `src/codex/catalog/parsing.ts:262-267` — strict normalization defaults it to
+   `false`.
+
+Codex reads a row that offers effort levels but declares no reasoning-summary
+support, and omits the entire inbound reasoning object. The adapter would
+serialize `reasoning_effort` correctly if it ever arrived
+(`src/adapters/openai-chat.ts:759-806`) — nothing is broken downstream.
+
+The `delete` is not careless. Routed rows are cloned from native templates, and
+inheriting OpenAI-only summary delivery would be wrong. The comment at
+`parsing.ts:351-352` says exactly that and anticipates per-model opt-in.
+
+## Constraint from PR #1119
+
+PR #1119 is tests-only and does not fix this, but it pins the contract any fix
+must satisfy: an explicit `modelSupportsReasoningSummaries: true` survives
+template normalization, and a ladder without that opt-in stays `false`.
+
+So the fix must not infer `true` from a non-empty ladder. That would flip every
+routed model including providers that reject summary fields, and would break
+#1119's second assertion.
+
+## Change
+
+Supply the opt-in as registry metadata for providers we have evidence for.
+
+The config-level field **already exists** at `src/types.ts:1235-1239` as
+`modelSupportsReasoningSummaries?: Record<string, boolean>`, documented as the
+per-model escape hatch for backends that reject summary fields. So this phase
+does not invent a field — it supplies registry-side defaults for a field users
+currently have to set by hand.
+
+1. `src/providers/registry.ts` — add the same
+   `Record<string, boolean>` shape to `ProviderRegistryEntry`.
+2. Populate it for the canonical DeepSeek V4 models and the GLM models with
+   confirmed support: entries `deepseek`, `opencode-go`, `zai`, and only the
+   Zhipu models with evidence. No speculative entries.
+3. `src/providers/derive.ts:279-282` — backfill in `enrichProviderFromRegistry`.
+
+### The merge must be per-key, not per-Record
+
+Every backfill at `derive.ts:279-282` today is scalar and uses
+`if (prov.X === undefined && entry.X !== undefined)`. Copying that shape for a
+Record would be a real bug: a user who sets one model's flag creates a defined
+Record, and the whole-object `undefined` check then suppresses **every**
+registry default for that provider. One hand-edit would silently disable the
+fix.
+
+So the merge is per-key — start from the registry map, then let explicit user
+keys win:
+
+```ts
+// registry defaults first, explicit user keys override — including explicit false
+if (entry.modelSupportsReasoningSummaries) {
+  prov.modelSupportsReasoningSummaries = {
+    ...entry.modelSupportsReasoningSummaries,
+    ...(prov.modelSupportsReasoningSummaries ?? {}),
+  };
+}
+```
+
+Explicit `false` must survive. A user who disabled summaries for one model
+because their backend 400s on it has to keep that, and a spread-based merge
+preserves it while `undefined`-checking would not.
+
+Deep-clone the registry side so saved config never aliases the registry
+constant — the same precaution `responsesItemIdRepair` already takes at
+`derive.ts:286`.
+
+Arbitrary custom providers stay conservative and keep the existing per-model
+configuration workaround. This is a deliberate asymmetry: we ship the opt-in
+where we have proof and leave it manual where we do not.
+
+## Tests
+
+`tests/codex-catalog.test.ts`:
+
+- `built-in DeepSeek and GLM effort models opt into Codex reasoning propagation (#1100)`
+  — gather registry-enriched models, build with `nativeTemplate()`, assert each
+  row carries both the expected effort levels and
+  `supports_reasoning_summaries === true`.
+- Keep #1119's no-opt-in assertion in the same file so a future global flip
+  fails here rather than in production.
+- `explicit per-model overrides survive registry backfill` — a provider with a
+  user-set `{modelA: false}` keeps `modelA` false and still receives the
+  registry's `modelB: true`. This fails under a whole-Record fill-if-undefined
+  merge, which is the specific mistake this test exists to catch.
+
+## Blast radius
+
+Provider derivation, generated Codex catalogs, and Responses summary
+sanitization. An over-broad opt-in would forward summary fields to providers
+that reject them, which is why the metadata is model-scoped rather than
+provider-scoped or ladder-inferred.
+
+`tests/codex-catalog.test.ts` is also touched by PR #1119. If that PR lands
+first, rebase onto it rather than duplicating its cases.