From 7bcd99276f0ba40633cd521dea583ce813ae6dce Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 14:55:40 +0900 Subject: [PATCH 1/3] fix(sse): accept unspaced `data:` fields across six parsers (#1170) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The space after the colon is optional in text/event-stream, so `data:{...}` is as valid as `data: {...}`. Six parsers hardcoded the spaced form and silently dropped every frame from a producer that omits it. To the user that looked like a completed turn with no content. The same wire format was already accepted on the relay path (`sse-decoder.ts`, `relay.ts`, `google.ts`) and rejected on the adapter path. That split is the bug. Adds two primitives beside the decoder whose rule they mirror: - `sseFieldValue(line, field)` for the five string-slicing parsers - `sseFieldOffset(text, start, end, field)` for the live Claude relay, whose translator-budget accounting reserves bytes by offset — materializing the line first would allocate the string the budget exists to bound Call sites fixed: `adapters/openai-chat.ts`, `chat/outbound.ts`, `claude/outbound.ts` (two independent parsers, both `event` and `data`), `web-search/parse.ts`, `server/claude-messages.ts`. Both helpers strip at most one leading space, matching the decoder, so a payload that legitimately begins with whitespace keeps the rest of it. Neither trims: callers own that choice and some intentionally keep trailing bytes. Deferred deliberately, not fixed here: `\r\n\r\n` frame delimiting and multiline `data` joining without the spec's `\n` separator. Both are frame-level rather than field-level and carry a different blast radius. Verification: each new test was confirmed to fail with the fix reverted. --- .../260807_untouched_bug_stack/000_plan.md | 127 ++++++++++++++++++ .../010_sse_unspaced_data_fields.md | 110 +++++++++++++++ .../020_routed_reasoning_effort.md | 111 +++++++++++++++ .../030_windows_acl_harden_envelope.md | 107 +++++++++++++++ .../040_npm_cache_preflight_replacement.md | 107 +++++++++++++++ .../050_adopt_as_is_replacements.md | 52 +++++++ .../060_adopt_with_changes_replacements.md | 102 ++++++++++++++ src/adapters/openai-chat.ts | 6 +- src/chat/outbound.ts | 7 +- src/claude/outbound.ts | 21 ++- src/lib/sse-decoder.ts | 35 +++++ src/server/claude-messages.ts | 7 +- src/web-search/parse.ts | 5 +- tests/claude-messages-endpoint.test.ts | 28 ++++ tests/sse-unspaced-data-fields.test.ts | 126 +++++++++++++++++ 15 files changed, 938 insertions(+), 13 deletions(-) create mode 100644 devlog/_plan/260807_untouched_bug_stack/000_plan.md create mode 100644 devlog/_plan/260807_untouched_bug_stack/010_sse_unspaced_data_fields.md create mode 100644 devlog/_plan/260807_untouched_bug_stack/020_routed_reasoning_effort.md create mode 100644 devlog/_plan/260807_untouched_bug_stack/030_windows_acl_harden_envelope.md create mode 100644 devlog/_plan/260807_untouched_bug_stack/040_npm_cache_preflight_replacement.md create mode 100644 devlog/_plan/260807_untouched_bug_stack/050_adopt_as_is_replacements.md create mode 100644 devlog/_plan/260807_untouched_bug_stack/060_adopt_with_changes_replacements.md create mode 100644 tests/sse-unspaced-data-fields.test.ts diff --git a/devlog/_plan/260807_untouched_bug_stack/000_plan.md b/devlog/_plan/260807_untouched_bug_stack/000_plan.md new file mode 100644 index 0000000000..3da067959b --- /dev/null +++ b/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 (5 call sites, 1 shared primitive) +- `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. diff --git a/devlog/_plan/260807_untouched_bug_stack/010_sse_unspaced_data_fields.md b/devlog/_plan/260807_untouched_bug_stack/010_sse_unspaced_data_fields.md new file mode 100644 index 0000000000..cedab6256b --- /dev/null +++ b/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` — a fifth 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. diff --git a/devlog/_plan/260807_untouched_bug_stack/020_routed_reasoning_effort.md b/devlog/_plan/260807_untouched_bug_stack/020_routed_reasoning_effort.md new file mode 100644 index 0000000000..cffde07fd5 --- /dev/null +++ b/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`, 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` 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. diff --git a/devlog/_plan/260807_untouched_bug_stack/030_windows_acl_harden_envelope.md b/devlog/_plan/260807_untouched_bug_stack/030_windows_acl_harden_envelope.md new file mode 100644 index 0000000000..7062da209d --- /dev/null +++ b/devlog/_plan/260807_untouched_bug_stack/030_windows_acl_harden_envelope.md @@ -0,0 +1,107 @@ +# 030 — #1156: Windows ACL harden envelope is too small + +## Defect, stated precisely + +The original report and our first triage both said PR #1135's retry shares the +5-second budget. That is wrong, and the correction matters. + +Owner-level recovery at `src/codex/native-main-owner.ts:205-210` calls +`hardenSecret` again, and each call creates its own deadline at +`src/lib/windows-secret-acl.ts:651-670` and `:703-720`. The retry does get a +fresh envelope. + +The real defect: **one complete ACL sequence gets 5 seconds total**. The +deadline is created per harden call and every command inside it draws from the +remainder (`:426-470`, `:475-506`). A sequence is `/grant:r`, `/inheritance:r`, +and a verification pass, plus `/findsid` fallbacks when the principal does not +resolve on the first form. On a machine where `icacls` is slow — Defender +real-time scanning, a roaming profile, a domain controller round-trip — the +budget is exhausted mid-sequence and the owner publishes a permanent +`unavailable`, so every native request returns 503. + +`loadConfig` hardens directory, config, and auth sequentially, which is why the +budget is shared per call in the first place: per-attempt budgets would stack +into multi-minute startup stalls. The comment at `:227-234` documents this +trade-off, and it is a real one. + +## Change + +`src/lib/windows-secret-acl.ts:234` — raise `HARDEN_DEADLINE_DEFAULT_MS` from +`5_000` to `30_000`. + +Keep everything else: the 60-second `HARDEN_DEADLINE_MAX_MS` cap, the +`OPENCODEX_ACL_TIMEOUT_MS` override, the clamp, and the shared-envelope +structure. + +Rejected alternative: independent per-command budgets. With `/findsid` +fallbacks and retries, that multiplies into the startup stall the shared budget +was introduced to prevent. Raising one constant preserves the design and fixes +the reported failure. + +## Worst-case cost, stated honestly + +An earlier draft of this doc said "roughly 60 seconds". That was wrong, because +the budget is per harden *call* and `loadConfig()` makes three of them +sequentially — directory, config file, then `auth.json` +(`src/config.ts:1759-1764`): + +``` +hardenConfigDir(); +hardenExistingSecret(configPath); +hardenExistingSecret(join(dir, "auth.json")); +``` + +So the real bounds after raising the default to 30 seconds are: + +| Path | Before (5s) | After (30s) | +|---|---|---| +| `loadConfig()` startup, all three hardens timing out | ~15s | **~90s** | +| native-owner harden + one recovery (250ms delay) | ~10.25s | ~60.25s | + +A 90-second synchronous startup stall is a real cost and has to be justified +rather than glossed over. Two things make it acceptable: + +1. It is the **timeout** path, not the normal path. Reaching 90 seconds requires + all three ACL sequences to exhaust a 30-second envelope, meaning `icacls` is + pathologically slow on that machine. On a healthy machine the sequence + finishes in milliseconds and nothing changes. +2. The alternative is what #1156 reports today: the harden fails, the owner + publishes a permanent `unavailable`, and *every* native request returns 503 + until the user restarts. A slow start is recoverable; a permanent 503 is not. + +If 90 seconds is judged too long, the fallback is a smaller bump (15 seconds, +giving ~45s startup) rather than reverting to per-command budgets. Record which +bound was chosen in the PR body so the reviewer sees the trade explicitly. + +The failure remains fail-closed, which is the security-relevant property. + +## Security review gate + +This phase touches ACL/credential-permission handling, which requires explicit +security review under `MAINTAINERS.md` — CI green is not sufficient. Run +`bun run privacy:scan` and request the security review before marking the PR +ready. + +## Test + +`tests/windows-secret-acl.test.ts`: + +- `slow successful ACL steps fit the default harden envelope (#1156)` — fake + clock consumes 2s on `/grant:r` and 11s on `/inheritance:r`, then succeeds. + Assert `{ok: true}` and that all three core steps ran. Fails under a 5-second + default. +- Update the existing default-budget expectations at `:356-374` and `:438-465`, + which assert the old constant. + +## Blast radius + +Every Windows secret file and directory harden. Behavior is unchanged on +machines where `icacls` is fast; only the failure threshold moves. Memoization +and retry cardinality are untouched. + +## Not fixed here + +#1149 (ACL principal built from `USERDOMAIN`, rejecting workgroup local +accounts) lives in the same file at `:396-408` but is a different defect with a +different fix. Keeping it out of this phase keeps the diff reviewable; it is a +candidate for the next unit. diff --git a/devlog/_plan/260807_untouched_bug_stack/040_npm_cache_preflight_replacement.md b/devlog/_plan/260807_untouched_bug_stack/040_npm_cache_preflight_replacement.md new file mode 100644 index 0000000000..9afec3f9c2 --- /dev/null +++ b/devlog/_plan/260807_untouched_bug_stack/040_npm_cache_preflight_replacement.md @@ -0,0 +1,107 @@ +# 040 — #557 replacement: npm cache preflight and log sanitization + +## Why replace rather than rebase + +PR #557 is 18 commits past its merge base; `dev` is 1,220 commits past the same +point. The diff is 24 files, +2351/-64, and mixes one good idea with machinery +that no longer matches the tree. Current `dev` has independently rebuilt the +restart and service-repair paths and absorbed none of #557's modules. + +Its CI state confirms it: two branch-owned `update-job` failures on Windows +(`must not spawn`), a macOS hang in `update-npm-cache-preflight.test.ts` until +30-minute cancellation, and a Bun 1.3.14 crash on Ubuntu that looks +environmental. A rebase would carry all of that forward. + +## The defect, still live on dev + +`ocx` stops the proxy before it knows whether the install can succeed: + +- `bin/ocx.mjs:116-138` checks the version, then proceeds to shutdown at + `:139-258`; installation only starts at `:260-266`. npm cache access is never + checked. +- `src/update/index.ts:168-179` runs a registry-integrity preflight — not a + cache-access one — then shuts down at `:188-262`. + +So a foreign-owned or unreadable nested cache entry produces a failed install +*after* the proxy is already down. + +Second defect, same area: GUI update output is persisted verbatim. The flow — +corrected after audit, because an earlier draft cited the wrong line: + +- `src/update/job.ts:269-282` — `updateExecutionCommand` only *builds* the + command. It does not invoke anything. +- `src/update/job.ts:1469` — the actual invocation: + `runLoggedCommand(job, cmd.bin, cmd.args, UPDATE_TIMEOUT_MS)`. +- `src/update/job.ts:520-530` — `runLoggedCommand` stores stdout/stderr. +- `src/update/job.ts:241-266` — the write boundary, which does not sanitize. + +Local paths and account names end up in stored logs. + +## Change + +New `src/update/npm-cache-preflight.mjs`: bounded Unix cache inspection +returning structured reason codes. `lstat` nested symlinks and verify ownership, +then **skip traversal** rather than rejecting — normal `_npx`, `node_modules`, +and `.bin` links must not block an update. Never surface arbitrary worker text +into logs. + +Call sites: + +- `bin/ocx.mjs:138` — run the preflight before any tray or proxy stop. +- `src/update/index.ts:181-188` — same gate on the second entry point. +- `src/update/job.ts` — gate the job path before the stop that precedes + `runLoggedCommand` at `:1469`, not merely before the command is constructed at + `:269`. Building a command is free; stopping the proxy is the irreversible + step, and the preflight must sit ahead of it. +- `src/update/job.ts:241-266` — sanitize every persisted field and log line at + the write boundary: Windows and POSIX separators, anchorless profile paths, + multi-word usernames, cache paths, UID/GID. + +Windows is an explicit tested skip, not an accidental gap. + +Excluded from the replacement: `install-process.*`, the recovery-tree +declarations, the PID/config changes, and the recovery rewrites. #557's process +runner is where its crashes live — missing stream `error` handlers, rejecting +cleanup promises, leaked signal listeners — and none of it is needed for the +preflight. + +## Tests + +- `tests/update-npm-cache-preflight.test.ts` (new): foreign or inaccessible + entries abort before stop; normal nested symlinks pass without target + traversal; timeout and malformed worker output fail closed; the Windows skip + does not spawn npm. +- `tests/update-stop-first.test.ts:21`: the gate runs before shutdown. +- `tests/update-job.test.ts`: persisted logs contain no profile path, cache + path, or UID/GID. + +Tests must exercise behavior. #557 had source-text assertions that passed +without running the code they described. + +## Objections the replacement must satisfy + +Carried from the review threads on #557, since a replacement that repeats them +will collect the same objections: + +1. Normal npm-cache symlinks must not block updates. +2. Sanitization covers anchorless Windows paths and multi-word usernames. +3. Windows behavior is explicit policy with a tested no-spawn skip. +4. No process-runner crash surface — excluded entirely. +5. No unreachable branches, no source-text-only tests; docs, declarations, and + runtime behavior agree. + +Update the five lifecycle locales only. Do not import #557's ADR or recovery +prose; it describes a tree that no longer exists. + +## Security review gate + +This phase touches the dependency-install and update path, which requires +explicit security review under `MAINTAINERS.md` — CI green is not sufficient. +Run `bun run privacy:scan` (the sanitization change is exactly what it guards) +and request security review before marking the PR ready. + +## Locale-file dependency + +Phase 060 also edits lifecycle locale files. Whichever lands first, the second +rebases; note it in both PR bodies so the conflict is expected rather than +discovered. diff --git a/devlog/_plan/260807_untouched_bug_stack/050_adopt_as_is_replacements.md b/devlog/_plan/260807_untouched_bug_stack/050_adopt_as_is_replacements.md new file mode 100644 index 0000000000..ae0df03f49 --- /dev/null +++ b/devlog/_plan/260807_untouched_bug_stack/050_adopt_as_is_replacements.md @@ -0,0 +1,52 @@ +# 050 — adopt-as-is replacements: #1159 and #1171 + +Two contributor PRs survived a skeptical read with no required changes. Per the +stack principle, we still do not merge them in place: the change is rebuilt as +our commit on the stack branch, and the original is closed with a pointer. + +## #1159 — Cursor Grok wire model prefix + +Claude-family and Grok models need a `cursor-` prefix with an effort-tier suffix +on the request wire, while parameterized Grok Fast keeps its base id. The PR +adds a request-only helper and leaves discovery normalization alone — the right +seam, since mixing the two would corrupt the model list. + +- `src/adapters/cursor/effort-map.ts:118-128` — the prefix helper +- `src/adapters/cursor/request-builder.ts:130-154` — the call site +- `tests/cursor-effort-suffix.test.ts:86-118` +- `docs-site/src/content/docs/reference/adapters.md`, Cursor model-ID section + +Test: `regular grok-4.5 request ids match the recorded discovery fixture` — +low/medium/high and default/xhigh serialize as `cursor-grok-4.5-{tier}`; Fast +stays on the base id plus parameters. + +Known limitation, worth stating in the PR body rather than discovering later: +the fixture proves the mapping against recorded discovery output, not live +Cursor state. A Cursor rename requires refreshing it. + +This does not fix #1162 (Cursor Claude-family `resource_exhausted`). That issue +has no code-level cause identified and needs a capture. + +## #1171 — A6API unlimited quota keys + +An unlimited A6API key reports zero finite credit totals, and finite-total +validation then hides it, so a working key looks dead in the dashboard. The PR +puts the unlimited branch ahead of that validation and keeps expiry. + +- `src/providers/quota.ts:61-75,201-206,318-367` +- `tests/provider-quota.test.ts:260-316` + +Test: `A6API unlimited keys remain visible even when all finite credit totals +are zero` — unlimited flag, zero totals, expiry propagation, exactly one report, +and an "Unlimited API credits" row. + +Two observations that are not blockers: `creditsUsd` is currently ignored by the +GUI and duplicates the display window, and the string handling recognizes +`"true"` but not `"1"`. Neither breaks an existing consumer. Note them in the +PR body. + +## Stacking + +Neither touches files used by any other phase in this unit. They can be one PR +or two; two is preferable because they close two different originals and a +reviewer should be able to reject one without the other. diff --git a/devlog/_plan/260807_untouched_bug_stack/060_adopt_with_changes_replacements.md b/devlog/_plan/260807_untouched_bug_stack/060_adopt_with_changes_replacements.md new file mode 100644 index 0000000000..8edd6b40d5 --- /dev/null +++ b/devlog/_plan/260807_untouched_bug_stack/060_adopt_with_changes_replacements.md @@ -0,0 +1,102 @@ +# 060 — adopt-with-changes replacements: #1155, #1152, #1169 + +Three PRs with correct intent and a specific defect each. The defect is named +before implementation so the replacement is not a re-post of the original. + +## #1155 — web-search buffered upstream policy + +Intent: preserve the buffered-upstream policy through the web-search loop +instead of forcing streaming (the bypass behind closed issue #1143). + +Two problems in the author's diff: + +1. It routes through `openai-responses.parseResponse`, which is compaction-only + and rejects function-call-only payloads at + `src/adapters/openai-responses.ts:1286-1293`. The PR's test uses OpenAI Chat, + which masks it — a Responses turn carrying only a function call is exactly + the web-search case. +2. Buffered adapter batches are retained but never released when intercepted, so + repeated search iterations accumulate leases. + +Targets: `src/server/responses/core.ts:2519-2533`; +`src/web-search/loop.ts:243-285,364-412,536-560`; +`src/web-search/progress-stream.ts:29-35,139-156,218-230`; +`tests/web-search.test.ts:557`; `tests/web-search-progress-stream.test.ts:124`. + +Tests: + +- `buffered Responses web-search preserves function calls` — an + `openai-responses` function-call-only turn dispatches the sidecar and completes + downstream as SSE. +- `buffered intercepted iterations release translated leases` — repeated + iterations under a small translator budget do not accumulate discarded + batches. + +Also correct the five locale docs, which claim absolutely that all events are +buffered. + +## #1152 — account picker selector initialization + +The namespace and collision-detection foundations are sound and drew no +substantive review objections. But `initializeDefaultCodexAccountNamespaces` has +no production caller, so the PR title promises behavior the diff does not +deliver. + +An earlier draft of this doc said to invoke it "inside the explicit +picker-enable transaction". The audit found no such transaction exists. In the +current tree `codexAccountPickerEnabled` appears only as schema and validation +(`src/config.ts:1065`, `:1720-1722`, `:1957-1981`), a type +(`src/types.ts:807`), and a read helper +(`src/codex/account-namespaces.ts:155-158`). No management route writes it, and +`src/server/management/routing-profile-routes.ts:303-339` creates and updates +routing profiles — an unrelated surface. + +So there are two honest options, and the choice must be made before coding: + +**(a) Foundations-only.** Retitle the replacement to match what it does — add +namespace allocation and collision detection with no caller — and file the +wiring as a follow-up. Small, truthful, reviewable. + +**(b) Build the enable path.** Design the management entry point that writes +`codexAccountPickerEnabled` and allocates namespaces in one atomic +config write. This is a real API surface addition — a new route, its auth scope, +its validation, and its GUI caller — and is a larger change than PR #1152. + +Recommendation: **(a)** for this stack. Option (b) is a feature, and this unit +is a bug-fix stack; smuggling a new management route into it would make the +stack incoherent and expand the review surface for no user-visible bug fix. +File (b) as its own issue and reference it from the PR body. + +Targets for (a): `src/codex/account-namespaces.ts:18-131`; +`src/config.ts:1112-1130`; `src/routing/profile.ts:17,154-178`. + +Tests: opt-in persistence; no mutation when allocation fails; non-empty map +identity and order preserved; pre-save rejection of policy and profile-prefix +collisions without leaking private ids. + +## #1169 — codex-shim readiness warning + +Advisory-only design is right: warn when a shim install cannot prove routing, +without failing the install. Secret-leak coverage is already focused. + +One defect: `currentExternalCodexModelProvider()` can throw on an unreadable or +racing config, which turns a successful install into a failing command. An +advisory probe must never do that. + +Targets: `src/cli/index.ts:1035-1041`; a readiness helper beside `src/cli/`; +`src/codex/inject.ts:83-86`. + +Catch probe failure as "unverifiable" and keep exit 0. Test the unreadable-config +path and assert the warning discloses neither the proxy URL nor credentials. + +## Stacking + +`#1155` overlaps `src/server/responses/core.ts` with the #1095 rewrite, which is +deliberately out of this unit — so within this stack it is free-standing. +`#1152` and `#1169` touch disjoint files. Order: #1152, #1169, #1155, putting +the largest surface last. + +Each phase closes its original PR with a comment naming the replacement number. + +Phase 040 also edits lifecycle locale files that #1169's docs touch; whichever +lands first, the second rebases. diff --git a/src/adapters/openai-chat.ts b/src/adapters/openai-chat.ts index 7e8aff56c5..42a9236574 100644 --- a/src/adapters/openai-chat.ts +++ b/src/adapters/openai-chat.ts @@ -3,6 +3,7 @@ import type { AdapterEvent, OcxAssistantMessage, OcxContentPart, OcxMessage, Ocx import { isAllowedToolChoice, modelInList, namespacedToolName, resolveToolChoiceWireName, toolAllowedByChoice } from "../types"; import { mapReasoningEffort, modelRecordValue } from "../reasoning-effort"; import { debugProviderDiagnostic } from "../lib/debug"; +import { sseFieldValue } from "../lib/sse-decoder"; import { isDebugEnabled } from "../lib/debug-settings"; import { isCyberPolicyCode } from "../lib/errors"; import { redactSecretString } from "../lib/redact"; @@ -947,8 +948,9 @@ export function createOpenAIChatAdapter(provider: OcxProviderConfig): ProviderAd // Yields adapter events and returns "terminate" for a terminal frame ([DONE] / error) that // must end the stream, or "continue" otherwise. Mutates the closure's terminal-signal state. const handleDataLine = function* (line: string): Generator { - if (!line.startsWith("data: ")) return "continue"; - const payload = line.slice(6).trim(); + const rawPayload = sseFieldValue(line, "data"); + if (rawPayload === null) return "continue"; + const payload = rawPayload.trim(); if (payload === "[DONE]") { yield* flushToolCalls(); const stopReason = stopReasonFor(finishReason); diff --git a/src/chat/outbound.ts b/src/chat/outbound.ts index 0b5a064292..e7ad46775c 100644 --- a/src/chat/outbound.ts +++ b/src/chat/outbound.ts @@ -7,7 +7,7 @@ */ type Rec = Record; -import { decodeServerSentEvents } from "../lib/sse-decoder"; +import { decodeServerSentEvents, sseFieldValue } from "../lib/sse-decoder"; import { isTranslatorBudgetExceededError, type TranslatorBudget } from "../lib/translator-budget"; import { classifyError, CYBER_POLICY_ERROR_CODE, isCyberPolicyCode, isCyberPolicyMessage } from "../lib/errors"; @@ -671,8 +671,9 @@ export async function collectChatCompletion( const rawFrame = buffer.slice(0, sep); buffer = replaceRetained(buffer, buffer.slice(sep + 2), "live_transient"); for (const line of rawFrame.split("\n")) { - if (!line.startsWith("data: ")) continue; - const data = line.slice(6).trim(); + const rawData = sseFieldValue(line, "data"); + if (rawData === null) continue; + const data = rawData.trim(); if (!data || data === "[DONE]") continue; let parsed: unknown; try { parsed = JSON.parse(data); } catch { continue; } diff --git a/src/claude/outbound.ts b/src/claude/outbound.ts index e025762f85..494720c497 100644 --- a/src/claude/outbound.ts +++ b/src/claude/outbound.ts @@ -16,6 +16,7 @@ import { TranslatorBudgetExceededError, type TranslatorBudget, } from "../lib/translator-budget"; +import { sseFieldOffset, sseFieldValue } from "../lib/sse-decoder"; type Rec = Record; @@ -588,10 +589,16 @@ export function responsesSseToAnthropicSse( while (lineStart <= rawFrame.length) { const newline = rawFrame.indexOf("\n", lineStart); const lineEnd = newline === -1 ? rawFrame.length : newline; - if (rawFrame.startsWith("event: ", lineStart)) { - eventName = rawFrame.slice(lineStart + 7, lineEnd).trim(); - } else if (rawFrame.startsWith("data: ", lineStart)) { - const fragmentStart = lineStart + 6; + // The space after the colon is optional in text/event-stream (#1170); + // compute the value offset the same way sseFieldValue does, without + // slicing the line first — the byte accounting below is keyed to + // offsets into rawFrame. + const eventOffset = sseFieldOffset(rawFrame, lineStart, lineEnd, "event"); + const dataOffset = sseFieldOffset(rawFrame, lineStart, lineEnd, "data"); + if (eventOffset !== -1) { + eventName = rawFrame.slice(eventOffset, lineEnd).trim(); + } else if (dataOffset !== -1) { + const fragmentStart = dataOffset; const fragmentBytes = utf8SliceBytes(rawFrame, fragmentStart, lineEnd); const fragmentReservation = translatorBudget.reserveTransient(fragmentBytes, { kind: "live_transient" }); let fragmentCommitted = false; @@ -861,8 +868,10 @@ export async function collectAnthropicMessage( let eventName = ""; let dataLine = ""; for (const line of rawFrame.split("\n")) { - if (line.startsWith("event: ")) eventName = line.slice(7).trim(); - else if (line.startsWith("data: ")) dataLine += line.slice(6); + const eventValue = sseFieldValue(line, "event"); + if (eventValue !== null) { eventName = eventValue.trim(); continue; } + const dataValue = sseFieldValue(line, "data"); + if (dataValue !== null) dataLine += dataValue; } if (!eventName || !dataLine) continue; let data: unknown; diff --git a/src/lib/sse-decoder.ts b/src/lib/sse-decoder.ts index a0352157e0..c24c4dad2c 100644 --- a/src/lib/sse-decoder.ts +++ b/src/lib/sse-decoder.ts @@ -13,6 +13,41 @@ export type SseRecord = | { kind: "event"; event?: string; data: string } | { kind: "comment"; comment: string }; +/** + * Extract one SSE field value from a single line, or null when the line is a different field. + * + * The space after the colon is OPTIONAL in text/event-stream: `data:{"a":1}` is as valid as + * `data: {"a":1}`. Parsers that hardcoded `startsWith("data: ")` silently dropped every frame + * from a producer that omits it, which surfaced as a completed turn with no content (#1170). + * + * Strips at most ONE leading space — the same rule `decodeServerSentEvents` applies below — so a + * payload that legitimately begins with whitespace keeps the rest of it. Does not trim the value: + * callers own that choice, and some of them intentionally keep trailing bytes. + */ +export function sseFieldValue(line: string, field: string): string | null { + if (!line.startsWith(field)) return null; + const rest = line.slice(field.length); + if (!rest.startsWith(":")) return null; + return rest.startsWith(": ") ? rest.slice(2) : rest.slice(1); +} + +/** + * Offset-only variant of {@link sseFieldValue} for parsers that index into a larger buffer. + * + * Returns the index where the field's value begins within `text`, or -1 when the line at + * `[lineStart, lineEnd)` is a different field. Slicing nothing matters for the live Claude relay, + * whose translator-budget accounting reserves bytes by offset — materializing the line first would + * allocate the very string the budget exists to bound. + */ +export function sseFieldOffset(text: string, lineStart: number, lineEnd: number, field: string): number { + if (!text.startsWith(field, lineStart)) return -1; + let valueStart = lineStart + field.length; + if (valueStart >= lineEnd || text[valueStart] !== ":") return -1; + valueStart += 1; + if (valueStart < lineEnd && text[valueStart] === " ") valueStart += 1; + return valueStart; +} + /** * Decode text/event-stream records across arbitrary fetch chunk boundaries. * diff --git a/src/server/claude-messages.ts b/src/server/claude-messages.ts index 1b3ed22323..5e3074bec4 100644 --- a/src/server/claude-messages.ts +++ b/src/server/claude-messages.ts @@ -7,6 +7,7 @@ * unchanged. The Responses output (SSE or JSON) is converted back to Anthropic shape. */ import { FORWARD_HEADERS } from "../adapters/openai-responses"; +import { sseFieldValue } from "../lib/sse-decoder"; import { enforceAnthropicImageLimits, sniffImageDimensions } from "../adapters/anthropic-image-guard"; import { normalizeAnthropicImages } from "../adapters/anthropic-image-normalize"; import { AnthropicRequestError, anthropicToResponsesTranslation, extractOcxEffortDirective, extractOcxRouteDirective, resolveInboundModel, type ClaudeCacheKeySource } from "../claude/inbound"; @@ -171,7 +172,11 @@ export function tapAnthropicSseForLog( while ((sep = buffer.indexOf("\n\n")) !== -1) { const frame = buffer.slice(0, sep); buffer = buffer.slice(sep + 2); - const dataLine = frame.split("\n").filter(l => l.startsWith("data: ")).map(l => l.slice(6)).join(""); + const dataLine = frame + .split("\n") + .map(l => sseFieldValue(l, "data")) + .filter((v): v is string => v !== null) + .join(""); if (!dataLine) continue; let data: unknown; try { data = JSON.parse(dataLine); } catch { continue; } diff --git a/src/web-search/parse.ts b/src/web-search/parse.ts index 30946ac1e7..0f6c2229cd 100644 --- a/src/web-search/parse.ts +++ b/src/web-search/parse.ts @@ -1,3 +1,5 @@ +import { sseFieldValue } from "../lib/sse-decoder"; + /** A single web source backing the sidecar's answer. */ export interface WebSearchSource { url: string; @@ -187,7 +189,8 @@ export async function parseSidecarSSE(response: Response): Promise { + const upstream = new ReadableStream({ + start(controller) { + controller.enqueue(sseEncoder.encode(UNSPACED_USAGE_FRAMES)); + controller.close(); + }, + }); + const { calls, finalize } = spyFinalize(); + const ctx = freshLogCtx(); + const tap = tapAnthropicSseForLog(upstream, ctx, finalize, { stallMs: 5_000, maxBytes: 0 }); + const text = await new Response(tap).text(); + + // The bytes pass through untouched either way; what the strict prefix broke was the inspection. + expect(text).toContain("message_start"); + // "terminal" rather than "eof" is itself part of the fix: recognizing the unspaced + // `message_delta` is what lets the tap classify the close as a real terminal frame. + expect(calls).toEqual([{ status: 200, closeReason: "terminal" }]); + expect(ctx.usage).toEqual(expect.objectContaining({ inputTokens: 11, outputTokens: 7 })); +}); + test("A1: stalled upstream body gets an Anthropic timeout_error tail and body_stall close reason", async () => { const upstream = new ReadableStream({ start(controller) { diff --git a/tests/sse-unspaced-data-fields.test.ts b/tests/sse-unspaced-data-fields.test.ts new file mode 100644 index 0000000000..64c8e412c8 --- /dev/null +++ b/tests/sse-unspaced-data-fields.test.ts @@ -0,0 +1,126 @@ +import { describe, expect, test } from "bun:test"; +import { createOpenAIChatAdapter as createOpenAIChatAdapterProduction } from "../src/adapters/openai-chat"; +import { sseFieldOffset, sseFieldValue } from "../src/lib/sse-decoder"; +import { parseSidecarSSE } from "../src/web-search/parse"; +import type { AdapterEvent } from "../src/types"; +import { withTestTranslatorBudget } from "./helpers/translator-budget"; + +// #1170: the space after the colon is optional in text/event-stream. Several parsers required it +// and silently dropped every frame from a compliant producer that omits it, which surfaced to the +// user as a completed turn with no content. + +const createOpenAIChatAdapter = (...args: Parameters) => + withTestTranslatorBudget(createOpenAIChatAdapterProduction(...args)); + +const provider = { adapter: "openai-chat", baseUrl: "https://example.test/v1", apiKey: "key" }; + +async function collect(gen: AsyncGenerator): Promise { + const out: AdapterEvent[] = []; + for await (const e of gen) out.push(e); + return out; +} + +describe("sseFieldValue", () => { + test("accepts both spaced and unspaced field values", () => { + expect(sseFieldValue('data: {"a":1}', "data")).toBe('{"a":1}'); + expect(sseFieldValue('data:{"a":1}', "data")).toBe('{"a":1}'); + expect(sseFieldValue("event: message_start", "event")).toBe("message_start"); + expect(sseFieldValue("event:message_start", "event")).toBe("message_start"); + }); + + test("strips at most one leading space so payload whitespace survives", () => { + expect(sseFieldValue("data: two-spaces", "data")).toBe(" two-spaces"); + }); + + test("returns null for a different field, a bare prefix, or a comment", () => { + expect(sseFieldValue("event: x", "data")).toBeNull(); + expect(sseFieldValue("database: x", "data")).toBeNull(); + expect(sseFieldValue("data", "data")).toBeNull(); + expect(sseFieldValue(": keepalive", "data")).toBeNull(); + }); + + test("an empty value is a value, not an absent field", () => { + expect(sseFieldValue("data:", "data")).toBe(""); + expect(sseFieldValue("data: ", "data")).toBe(""); + }); + + test("does not trim the value — callers own that", () => { + expect(sseFieldValue("data: payload ", "data")).toBe("payload "); + }); +}); + +describe("sseFieldOffset", () => { + const frame = 'event:start\ndata:{"a":1}\nother: x'; + + test("returns the value offset for spaced and unspaced fields", () => { + expect(frame.slice(sseFieldOffset(frame, 0, 11, "event"), 11)).toBe("start"); + expect(frame.slice(sseFieldOffset(frame, 12, 24, "data"), 24)).toBe('{"a":1}'); + }); + + test("returns -1 for a different field", () => { + expect(sseFieldOffset(frame, 25, frame.length, "data")).toBe(-1); + }); + + test("agrees with sseFieldValue on the same line", () => { + for (const line of ["data: x", "data:x", "data:", "data: y", "event:z"]) { + const offset = sseFieldOffset(line, 0, line.length, "data"); + const value = sseFieldValue(line, "data"); + if (value === null) expect(offset).toBe(-1); + else expect(line.slice(offset)).toBe(value); + } + }); +}); + +describe("openai-chat adapter (#1170)", () => { + test("accepts unspaced data frames and finish_reason without [DONE]", async () => { + const response = new Response([ + 'data:{"choices":[{"delta":{"content":"hello"}}]}\n\n', + 'data:{"choices":[{"delta":{},"finish_reason":"stop"}],"usage":{"prompt_tokens":3,"completion_tokens":1}}\n\n', + ].join("")); + const events = await collect(createOpenAIChatAdapter(provider).parseStream(response)); + + const text = events.filter(e => e.type === "text_delta").map(e => (e as { text: string }).text).join(""); + expect(text).toBe("hello"); + expect(events.at(-1)?.type).toBe("done"); + expect(events.some(e => e.type === "error")).toBe(false); + }); + + test("accepts an unspaced [DONE] sentinel", async () => { + const response = new Response([ + 'data:{"choices":[{"delta":{"content":"hi"}}]}\n\n', + "data:[DONE]\n\n", + ].join("")); + const events = await collect(createOpenAIChatAdapter(provider).parseStream(response)); + expect(events.at(-1)?.type).toBe("done"); + }); + + test("still handles the spaced form identically", async () => { + const response = new Response([ + 'data: {"choices":[{"delta":{"content":"hi"}}]}\n\n', + "data: [DONE]\n\n", + ].join("")); + const events = await collect(createOpenAIChatAdapter(provider).parseStream(response)); + const text = events.filter(e => e.type === "text_delta").map(e => (e as { text: string }).text).join(""); + expect(text).toBe("hi"); + expect(events.at(-1)?.type).toBe("done"); + }); +}); + +describe("web-search sidecar parser (#1170)", () => { + function sseStream(body: string): Response { + return new Response(body, { headers: { "content-type": "text/event-stream" } }); + } + + const frames = (prefix: string) => [ + `${prefix}{"type":"response.output_text.delta","delta":"answer"}\n\n`, + `${prefix}{"type":"response.output_text.done","text":"answer"}\n\n`, + `${prefix}[DONE]\n\n`, + ].join(""); + + test("accepts unspaced data frames", async () => { + const spaced = await parseSidecarSSE(sseStream(frames("data: "))); + const unspaced = await parseSidecarSSE(sseStream(frames("data:"))); + expect(spaced.text).toContain("answer"); + expect(unspaced.text).toBe(spaced.text); + }); +}); From 263c07f91ebe1ea4eb975d514bad08c517771d74 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 15:13:11 +0900 Subject: [PATCH 2/3] fix(sse): treat a colonless field line as an empty value, and cover all six call sites Audit found two real gaps in the previous commit. `sseFieldValue("data", "data")` returned null while `decodeServerSentEvents` treats a colonless field as an empty value (`colon < 0` -> valueStart = line.length, sse-decoder.ts:240). Two helpers that mirror the decoder must not disagree with it. Both now return the empty value, and `sseFieldOffset` returns the end-of-line offset for the same case. Regression coverage was also incomplete: `chat/outbound.ts` and `claude/outbound.ts` had no unspaced test, so two of the six fixed call sites were unverified. Both now have one, and each was confirmed to fail with its fix reverted. Also corrects the remaining stale "5 call sites" claims in the plan unit. --- .../260807_untouched_bug_stack/000_plan.md | 2 +- .../010_sse_unspaced_data_fields.md | 2 +- src/lib/sse-decoder.ts | 8 ++- tests/sse-unspaced-data-fields.test.ts | 72 ++++++++++++++++++- 4 files changed, 78 insertions(+), 6 deletions(-) diff --git a/devlog/_plan/260807_untouched_bug_stack/000_plan.md b/devlog/_plan/260807_untouched_bug_stack/000_plan.md index 3da067959b..98a129b7c2 100644 --- a/devlog/_plan/260807_untouched_bug_stack/000_plan.md +++ b/devlog/_plan/260807_untouched_bug_stack/000_plan.md @@ -64,7 +64,7 @@ Fixing only the reported site would leave five live paths broken. Implementation phases, one decade doc each, one PABCD cycle each: -- `010` — #1170 unspaced SSE field parsing (5 call sites, 1 shared primitive) +- `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 diff --git a/devlog/_plan/260807_untouched_bug_stack/010_sse_unspaced_data_fields.md b/devlog/_plan/260807_untouched_bug_stack/010_sse_unspaced_data_fields.md index cedab6256b..28bfff3510 100644 --- a/devlog/_plan/260807_untouched_bug_stack/010_sse_unspaced_data_fields.md +++ b/devlog/_plan/260807_untouched_bug_stack/010_sse_unspaced_data_fields.md @@ -17,7 +17,7 @@ Strict call sites on `origin/dev@6d04574d0`: and `startsWith("data: ", ...)` against a raw frame with byte-budget accounting interleaved - `src/web-search/parse.ts:190` -- `src/server/claude-messages.ts:174` — a fifth site, not named in the report +- `src/server/claude-messages.ts:174` — another site, not named in the report Lenient call sites that prove the intended behavior: diff --git a/src/lib/sse-decoder.ts b/src/lib/sse-decoder.ts index c24c4dad2c..c76623d07b 100644 --- a/src/lib/sse-decoder.ts +++ b/src/lib/sse-decoder.ts @@ -27,6 +27,10 @@ export type SseRecord = export function sseFieldValue(line: string, field: string): string | null { if (!line.startsWith(field)) return null; const rest = line.slice(field.length); + // A colonless field line is the field with an empty value per the SSE rules, and + // `decodeServerSentEvents` below treats it that way (`colon < 0` -> valueStart = line.length). + // These helpers must not disagree with the decoder they mirror. + if (rest.length === 0) return ""; if (!rest.startsWith(":")) return null; return rest.startsWith(": ") ? rest.slice(2) : rest.slice(1); } @@ -42,7 +46,9 @@ export function sseFieldValue(line: string, field: string): string | null { export function sseFieldOffset(text: string, lineStart: number, lineEnd: number, field: string): number { if (!text.startsWith(field, lineStart)) return -1; let valueStart = lineStart + field.length; - if (valueStart >= lineEnd || text[valueStart] !== ":") return -1; + // Colonless field line: empty value, positioned at end-of-line (matches the decoder). + if (valueStart >= lineEnd) return lineEnd; + if (text[valueStart] !== ":") return -1; valueStart += 1; if (valueStart < lineEnd && text[valueStart] === " ") valueStart += 1; return valueStart; diff --git a/tests/sse-unspaced-data-fields.test.ts b/tests/sse-unspaced-data-fields.test.ts index 64c8e412c8..3df1db33f1 100644 --- a/tests/sse-unspaced-data-fields.test.ts +++ b/tests/sse-unspaced-data-fields.test.ts @@ -2,6 +2,9 @@ import { describe, expect, test } from "bun:test"; import { createOpenAIChatAdapter as createOpenAIChatAdapterProduction } from "../src/adapters/openai-chat"; import { sseFieldOffset, sseFieldValue } from "../src/lib/sse-decoder"; import { parseSidecarSSE } from "../src/web-search/parse"; +import { collectChatCompletion } from "../src/chat/outbound"; +import { collectAnthropicMessage } from "../src/claude/outbound"; +import { createTranslatorBudget } from "../src/lib/translator-budget"; import type { AdapterEvent } from "../src/types"; import { withTestTranslatorBudget } from "./helpers/translator-budget"; @@ -14,6 +17,17 @@ const createOpenAIChatAdapter = (...args: Parameters { + return new ReadableStream({ + start(controller) { + controller.enqueue(sseEncoder.encode(body)); + controller.close(); + }, + }); +} + async function collect(gen: AsyncGenerator): Promise { const out: AdapterEvent[] = []; for await (const e of gen) out.push(e); @@ -32,13 +46,19 @@ describe("sseFieldValue", () => { expect(sseFieldValue("data: two-spaces", "data")).toBe(" two-spaces"); }); - test("returns null for a different field, a bare prefix, or a comment", () => { + test("returns null for a different field or a comment", () => { expect(sseFieldValue("event: x", "data")).toBeNull(); expect(sseFieldValue("database: x", "data")).toBeNull(); - expect(sseFieldValue("data", "data")).toBeNull(); expect(sseFieldValue(": keepalive", "data")).toBeNull(); }); + test("a colonless field line is an empty value, matching the decoder", () => { + // decodeServerSentEvents treats `colon < 0` as valueStart = line.length, i.e. an empty + // value rather than a non-match. These helpers must not disagree with it. + expect(sseFieldValue("data", "data")).toBe(""); + expect(sseFieldValue("event", "event")).toBe(""); + }); + test("an empty value is a value, not an absent field", () => { expect(sseFieldValue("data:", "data")).toBe(""); expect(sseFieldValue("data: ", "data")).toBe(""); @@ -61,8 +81,14 @@ describe("sseFieldOffset", () => { expect(sseFieldOffset(frame, 25, frame.length, "data")).toBe(-1); }); + test("a colonless field line yields the end-of-line offset (empty value)", () => { + const bare = "data"; + expect(sseFieldOffset(bare, 0, bare.length, "data")).toBe(bare.length); + expect(bare.slice(sseFieldOffset(bare, 0, bare.length, "data"), bare.length)).toBe(""); + }); + test("agrees with sseFieldValue on the same line", () => { - for (const line of ["data: x", "data:x", "data:", "data: y", "event:z"]) { + for (const line of ["data: x", "data:x", "data:", "data: y", "event:z", "data", "database: x"]) { const offset = sseFieldOffset(line, 0, line.length, "data"); const value = sseFieldValue(line, "data"); if (value === null) expect(offset).toBe(-1); @@ -124,3 +150,43 @@ describe("web-search sidecar parser (#1170)", () => { expect(unspaced.text).toBe(spaced.text); }); }); + +describe("chat/outbound collectChatCompletion (#1170)", () => { + const frames = (prefix: string) => [ + `${prefix}{"choices":[{"index":0,"delta":{"content":"collected"}}]}\n\n`, + `${prefix}{"choices":[{"index":0,"delta":{},"finish_reason":"stop"}]}\n\n`, + `${prefix}[DONE]\n\n`, + ].join(""); + + test("accepts unspaced data frames", async () => { + const spaced = await collectChatCompletion(streamOf(frames("data: ")), "m", createTranslatorBudget()); + const unspaced = await collectChatCompletion(streamOf(frames("data:")), "m", createTranslatorBudget()); + + const textOf = (r: Record) => + ((r.choices as { message?: { content?: string } }[] | undefined)?.[0]?.message?.content) ?? ""; + expect(textOf(spaced)).toBe("collected"); + expect(textOf(unspaced)).toBe(textOf(spaced)); + }); +}); + +describe("claude/outbound collectAnthropicMessage (#1170)", () => { + // sep is "" for the unspaced variant and " " for the spaced one, applied to BOTH fields. + const frames = (sep: string) => [ + `event:${sep}message_start\ndata:${sep}{"type":"message_start","message":{"usage":{"input_tokens":2}}}\n\n`, + `event:${sep}content_block_start\ndata:${sep}{"type":"content_block_start","index":0,"content_block":{"type":"text","text":""}}\n\n`, + `event:${sep}content_block_delta\ndata:${sep}{"type":"content_block_delta","index":0,"delta":{"type":"text_delta","text":"claude"}}\n\n`, + `event:${sep}content_block_stop\ndata:${sep}{"type":"content_block_stop","index":0}\n\n`, + `event:${sep}message_stop\ndata:${sep}{"type":"message_stop"}\n\n`, + ].join(""); + + test("accepts unspaced event and data fields", async () => { + const spaced = await collectAnthropicMessage(streamOf(frames(" ")), "claude-test", createTranslatorBudget()); + const unspaced = await collectAnthropicMessage(streamOf(frames("")), "claude-test", createTranslatorBudget()); + + const textOf = (r: Record) => + ((r.content as { type?: string; text?: string }[] | undefined) ?? []) + .filter(p => p.type === "text").map(p => p.text ?? "").join(""); + expect(textOf(spaced)).toBe("claude"); + expect(textOf(unspaced)).toBe(textOf(spaced)); + }); +}); From 8662a0999cb8da14f1f6ae7fa4b46624cf298d7b Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 15:16:26 +0900 Subject: [PATCH 3/3] test(claude): activate the budgeted raw-frame parser with unspaced fields (#1170) The offset-based parser inside responsesSseToAnthropicSse was covered only indirectly. This drives it head-on: the same six Responses frames in spaced and unspaced form, asserting identical event names, identical translated text, and identical budget accounting. The budget assertion compares the two paths rather than asserting zero. This translator leaves a small residue at stream end on the spaced path too (51 bytes for this fixture), so zero would be asserting something that was never true. Equality is the contract the offset arithmetic must satisfy. Confirmed to fail when the offset fix is reverted. --- tests/claude-outbound.test.ts | 47 +++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/claude-outbound.test.ts b/tests/claude-outbound.test.ts index 8651f779d1..66250f683d 100644 --- a/tests/claude-outbound.test.ts +++ b/tests/claude-outbound.test.ts @@ -46,6 +46,11 @@ function dataOnlySse(data: Record): string { return `data: ${JSON.stringify(data)}\n\n`; } +/** Unspaced counterparts of `sse` / `dataOnlySse` — `data:{...}` is as valid as `data: {...}` (#1170). */ +function unspacedSse(name: string, data: Record): string { + return `event:${name}\ndata:${JSON.stringify(data)}\n\n`; +} + const DONE_SSE = "data: [DONE]\n\n"; function streamFrom(text: string): ReadableStream { @@ -149,6 +154,48 @@ describe("claude outbound SSE", () => { expect(budget.snapshot().currentBytes).toBe(0); }); + test("#1170: the budgeted raw-frame parser accepts unspaced event/data fields and still balances the budget", async () => { + // This drives the offset-based parser inside responsesSseToAnthropicSse, which reserves and + // releases translator budget by offset rather than by slicing each line. A spaced-only prefix + // check dropped every frame here, producing an empty translation. + const frames = [ + { name: "response.created", data: { response: { id: "resp_1" } } }, + { name: "response.output_item.added", data: { output_index: 0, item: { type: "message", id: "msg_1", role: "assistant" } } }, + { name: "response.content_part.added", data: { item_id: "msg_1", output_index: 0, content_index: 0, part: { type: "output_text" } } }, + { name: "response.output_text.delta", data: { item_id: "msg_1", output_index: 0, content_index: 0, delta: "unspaced" } }, + { name: "response.output_item.done", data: { output_index: 0, item: { type: "message", id: "msg_1" } } }, + { name: "response.completed", data: { response: { status: "completed", usage: { input_tokens: 5, output_tokens: 2 } } } }, + ]; + + const spacedBudget = createTestTranslatorBudget(); + const spaced = await collectEvents(responsesSseToAnthropicSse( + streamFrom(frames.map(f => sse(f.name, f.data)).join("")), + "claude-ocx-test", + { translatorBudget: spacedBudget }, + )); + + const unspacedBudget = createTestTranslatorBudget(); + const unspaced = await collectEvents(responsesSseToAnthropicSse( + streamFrom(frames.map(f => unspacedSse(f.name, f.data)).join("")), + "claude-ocx-test", + { translatorBudget: unspacedBudget }, + )); + + const textOf = (events: { name: string; data: Record }[]) => events + .filter(e => e.name === "content_block_delta") + .map(e => e.data?.delta?.text ?? "") + .join(""); + + expect(textOf(spaced)).toBe("unspaced"); + expect(unspaced.map(e => e.name)).toEqual(spaced.map(e => e.name)); + expect(textOf(unspaced)).toBe(textOf(spaced)); + // The offset arithmetic must not change accounting: the unspaced path retains exactly what + // the spaced path retains. (Both leave a small non-zero residue at stream end; that is + // pre-existing behavior of this translator, not something this fix introduces — asserting + // equality is the contract that matters here.) + expect(unspacedBudget.snapshot().currentBytes).toBe(spacedBudget.snapshot().currentBytes); + }); + test("text + thinking + tool call + completed w/ usage -> exact Anthropic sequence", async () => { const upstream = [ sse("response.created", { response: { id: "resp_1", status: "in_progress" } }),