diff --git a/devlog/_plan/260812_five_bug_fix_campaign/000_research.md b/devlog/_plan/260812_five_bug_fix_campaign/000_research.md new file mode 100644 index 000000000..99a7b6fac --- /dev/null +++ b/devlog/_plan/260812_five_bug_fix_campaign/000_research.md @@ -0,0 +1,258 @@ +# 000 — Research: five confirmed bug issues, live code grounding + +Unit: `260812_five_bug_fix_campaign` +Baseline: `origin/dev` at `cbbfdd877` (`fix(release): filter successful gate runs from JSON`). +Date: 2026-08-12. + +## Why this unit exists + +A triage pass over the 16 open `bug`-labelled issues found that **no merged PR +currently closes any of them**: the only merged PR carrying a closing reference +in the recent window is #1501 → #1477, and #1477 is already closed. Two issues +have draft PRs in flight (#1503 → draft #1508, #1497 → stale draft #1008) and +the rest have no implementation at all. + +Five issues were selected because each has a **code-level defect already located +in the current tree**, not a missing-evidence or upstream-attribution problem: + +| Issue | Area | Located in current tree | +|---|---|---| +| #1514 | `openai-chat` adapter | `src/adapters/openai-chat.ts:900-907` | +| #1503 | `google` adapter | `src/adapters/google.ts:615-618`, `:836` | +| #1497 | management usage API | `src/server/management/logs-usage-routes.ts:210-214` | +| #1409 | provider config write | `src/server/management/provider-routes.ts:353-363` | +| #1419 | service lifecycle | `src/lib/abort.ts:125-133` neighbourhood | + +Explicitly out of scope for this unit: #1527, #1524, #1388, #1302, #1296, +#1059, #1049, #1024, #904, #417, #92. Those are either needs-info, upstream +tracking, policy-level enhancements, or CI burn-down work. + +## Per-defect grounding + +### #1514 — `flushToolCalls` can emit a tool call with an empty name + +`parseStream` accumulates streamed tool calls into `PendingToolCall` records +whose `name` starts as the empty string: + +```ts +call = { key: key ?? `seq:${pendingToolCalls.length}`, id: "", name: "", args: "", argsBytes: 0 }; +``` + +`name` is only ever populated from a truthy upstream field: + +```ts +if (tc.function?.name && !call.name) call.name = tc.function.name; +``` + +So an upstream that streams `function.arguments` deltas while never sending +`function.name` leaves `call.name === ""`. At every flush boundary the call is +emitted regardless: + +```ts +const flushToolCalls = function* (): Generator { + for (const call of closeToolCalls()) { + if (!call.id) call.id = `call_${++toolCallSeq}`; + yield { type: "tool_call_start", id: call.id, name: call.name }; // name may be "" + if (call.args.length > 0) yield { type: "tool_call_delta", arguments: call.args }; + yield { type: "tool_call_end" }; + } +}; +``` + +Note the asymmetry that makes this a real bug rather than a style complaint: +the **id** is already defended (`if (!call.id)` synthesizes one) while the +**name** — the field that actually decides which tool runs — is not. A +synthesized id is harmless because the id is an opaque correlation handle; a +synthesized name would be a guess at intent, so the correct treatment is +fail-closed, not invention. + +There are three flush sites, and the fix has to hold at all three: + +1. `[DONE]` frame (`:925`) — the OpenCode Zen / DeepSeek path in the report. +2. `finish_reason` on a choice (`:1028`). +3. Post-loop normal completion (`:1085`). + +Existing invariants that must survive (from #1325 and the truncation work): + +- a non-array `delta.tool_calls` is terminal protocol corruption, not padding; +- a raw EOF with pending tool calls and no terminal signal is a truncation + error and must stay fail-closed; +- a claimed tool call must not be silently dropped when doing so could orphan a + matching `function_call_output` on the next turn. + +External contract check (Luna lane, Gemini lane returned; OpenAI lane pending): +the OpenAI streaming convention is that the **first** chunk of a tool call +carries `id` and `function.name`, with subsequent chunks carrying only +`function.arguments` deltas. An upstream that never sends the name is therefore +non-conforming, which is exactly why provider-specific tolerance is the wrong +fix and fail-closed rejection is the right one. + +### #1503 — Google `thought: true` parts are emitted as visible text + +Streaming parser: + +```ts +for (const part of parts) { + if (part.text) { + emittedContentEvent = true; + yield { type: "text_delta", text: part.text }; + } +``` + +Buffered parser: + +```ts +for (const part of candidates[0].content.parts) { + if (part.text) events.push({ type: "text_delta", text: part.text }); +``` + +Neither branch reads `part.thought`. The part type declared locally is +`{ text?: string; functionCall?: {...} }`, so the flag is not even in the +narrowed shape, while `observeAntigravityReplay` already receives the raw +`parts as unknown[]` and does look at signature fields. + +Primary-source confirmation (Luna lane 2, returned 2026-08-12): + +- `Part.thought` is documented as "whether the part represents the model's + thought process or reasoning" in the Gemini REST reference and the + `@google/genai` `Part` interface. A text-bearing part with `thought: true` is + a **thought summary**, not the answer channel. +- `thoughtSignature` is an opaque encrypted reasoning handle that must be + replayed byte-for-byte in its original part; for Gemini 3 function calling the + first function-call part of each step must carry it or the API returns 400. +- Official SDK examples branch on `part.thought` to render thoughts in a + separate "Thought" channel; proxies (OpenRouter, LiteLLM) normalize the same + distinction into a reasoning field rather than merging it into content. + +So the required behavior — thought text becomes hidden reasoning, ordinary text +stays visible, signatures untouched — is the vendor-documented contract, not a +local preference. + +**Contributor work exists.** Draft PR #1508 by `Ingwannu` +(`agent/fix-1503-google-thought-visibility`, head `219e7f365a`, `MERGEABLE`) +already implements this across `src/adapters/google.ts`, +`tests/google-hardening.test.ts`, and `structure/04_transports-and-sidecars.md`. +The correct action is to review and land that branch, not to reimplement it. + +### #1497 — usage `30d` and `all` share one moving byte tail + +`GET /api/usage` reads first and filters second: + +```ts +const effectiveReadLimit = config.managementUsageMaxReadBytes ?? 64 * 1024 * 1024; +... +const snapshot = await readUsageSnapshotForManagement(effectiveReadLimit); +const summary = { + ...summarizeUsage(snapshot.entries, range, now, surface), + historyTruncated: snapshot.truncatedPrefixBytes > 0 || snapshot.entriesTruncated, +``` + +`readUsageSnapshotForManagement` delegates to +`readUsageEntriesFullCooperatively(path, signal, maxReadBytes)`, which reads the +**newest `maxReadBytes`** of `usage.jsonl`. The range filter inside +`summarizeUsage` then operates on whatever survived that byte cut. On the +reporter's installation the newest 64 MiB covered roughly 39 hours, so `30d` +omitted 73.6% of requests and `range=all` returned the same rows under the label +"Available history". + +The response is not silent about it — `historyTruncated: true` and +`truncatedPrefixBytes` are both returned — but the dashboard labels remain `30d` +and `Available history`, and cumulative totals can *decrease* as old rows fall +out of the moving window. + +Draft PR #1008 proposes a daily rollup sidecar plus raw-tail merge. It is stale, +conflicting, and carries unresolved correctness findings (crash-safe +append/commit validation, truncated-ledger invalidation, partial-day overlap, +request dedup, disabled-rollup behavior). An incorrect derived aggregate is +worse than an honestly truncated one, so this unit does not adopt it. + +### #1409 — user `modelContextWindows` overrides are replaced by registry seeds + +Two write paths exist and they disagree. + +**PATCH** (`applyProviderPatch`, `:187-208`) merges per key and preserves +unmentioned entries: + +```ts +const windows: Record = { ...(next.modelContextWindows ?? {}) }; +``` + +**POST overwrite** (`:353-364`) does not: + +```ts +enrichProviderFromCatalog(name, prov); +... +const existingPool = config.providers[name]?.apiKeyPool; +if (existingPool && !prov.apiKeyPool) prov.apiKeyPool = existingPool; +const existingCosts = config.providers[name]?.modelCosts; +if (existingCosts && !prov.modelCosts) prov.modelCosts = existingCosts; +config.providers[name] = stripRegistryOnlyStaticHeaders(name, prov); +``` + +The dashboard's add/edit form does not send `modelContextWindows`. So on an +overwrite `prov.modelContextWindows` is absent, +`enrichProviderFromCatalog` → `enrichProviderFromRegistry` fills it from the +registry seed: + +```ts +if (!prov.modelContextWindows && seed.modelContextWindows) prov.modelContextWindows = { ...seed.modelContextWindows }; +``` + +and the stored row becomes the registry default. For `opencode-go` the seed is +exactly `{ "kimi-k3": KIMI_K3_STANDARD_CONTEXT_WINDOW }` — which is precisely the +`{"kimi-k3": 262144}` the reporter observed replacing their +`{"deepseek-v4-flash": 900000}`. + +Note the shape of the existing defenses: `apiKeyPool` and `modelCosts` are +carried over with exactly this rationale ("the add/edit form does not send +modelCosts, so an overwrite must not silently erase hand-edited per-model +prices"). `modelContextWindows` is the same class of hand-edited user data and +was simply not included. The reporter's timeline — value lost across an upgrade, +becoming visible after "a later full config write after an unrelated provider +change" — matches an overwrite through this path rather than a defect in +`derive.ts` or `router.ts` merging. + +`enrichProviderFromRegistry` itself is fill-only and correct; the bug is that +the POST path treats "the client omitted the field" as "the user has no value", +for a field the client never sends. + +### #1419 — a TLS/reset failure kills the whole Bun process + +Reported signature: `EXC_BREAKPOINT / SIGTRAP` on the main thread ~0.5-0.6s after +a connection reset followed by `unknown certificate verification error`, twice, +with matching Bun image UUID and stack offsets. Because `ocx gui` serves the +dashboard from the same process, the dashboard dies with the proxy. + +The repository already knows this failure family. `src/lib/abort.ts` documents +an uncatchable native rejection: + +> Bun's HTTP client, when a `fetch(..., { signal })` is aborted AFTER the +> response resolved, tears down the response body stream and rejects any +> in-flight internal read. ... Bun reports it as +> `unhandledRejection: TypeError: null is not an object` (native-only stack) — +> uncatchable by any caller try/catch. + +`cancelBodyOnAbort` exists to absorb that specific orphaned rejection by making +us the consumer that settles the body. `src/lib/eventstream-decoder.ts:211` +carries the same note. + +External check (Luna lane 3, returned 2026-08-12): there is **no** known +`oven-sh/bun` issue establishing that TLS verification failure or a socket reset +aborts with `SIGTRAP`/`EXC_BREAKPOINT`, and the current stable Bun is `1.3.14` +(2026-05-13) — i.e. there is no newer stable release to upgrade into as a fix. +Related but distinct: #31894 (stale pooled socket, 1.3.14) and #31463 +(`ECONNRESET` after `Connection: close`). Conclusion: this cannot be closed by a +runtime bump, and attributing the trap requires the full faulting frame list the +maintainer already asked for. + +That bounds what this unit can honestly deliver for #1419: **process survival +hardening on the paths we own**, plus a documented disposition, rather than a +claimed fix for a native trap we cannot reproduce. See `050`. + +## Verification environment + +All test and typecheck evidence for this unit is produced on `ssh lidge` +(Linux x86_64, 16 cores, 30 GB RAM, `bun` at `~/.bun/bin/bun`), per the +standing rule that CPU-heavy suites do not run on the local workstation. The +remote checkout at `~/Developer/opencodex` is stale and dirty, so a dedicated +clean worktree is provisioned rather than reusing it. diff --git a/devlog/_plan/260812_five_bug_fix_campaign/001_external_contract_evidence.md b/devlog/_plan/260812_five_bug_fix_campaign/001_external_contract_evidence.md new file mode 100644 index 000000000..f15ddf3bd --- /dev/null +++ b/devlog/_plan/260812_five_bug_fix_campaign/001_external_contract_evidence.md @@ -0,0 +1,106 @@ +# 001 — External contract evidence (Luna search lanes) + +Three `gpt-5.6-luna` explorer lanes were dispatched under `cxc-lunasearch`, +each attached to `cxc-search` for the proof ladder. Luna output is discovery; +the claims below were kept only where the lane opened a primary source. One +lane (first OpenAI attempt) errored with `Request blocked` and was respawned. + +All retrievals: **2026-08-12**. Source anchors are given per lane below so each +claim can be re-checked independently (audit blocker B12). + +## Source anchors + +**Lane A (OpenAI streaming tool calls)** + +- `openai-python` chunk schema — `src/openai/types/chat/chat_completion_chunk.py` + +- `openai-python` stream accumulator — `src/openai/lib/streaming/chat/_completions.py` + (accumulate at ~`:329-365`, strict-tool lookup ~`:409-424`, event build ~`:483-496`) + +- `openai-node` accumulator — `src/lib/ChatCompletionStream.ts` (~`:533-679`, `:747-765`) + +- LiteLLM streaming handler — `litellm/litellm_core_utils/streaming_handler.py` + (~`:804-827`, `:1411-1449`, `:1472-1487`) + +- OpenAI API reference (streaming) + +**Lane B (Gemini `thought`)** + +- Gemini `generateContent` REST reference +- `@google/genai` `Part.thought` + +- Gemini thinking / thought summaries +- Thought signatures (Google AI) + +- Thought signatures (Vertex / Gemini Enterprise) + +- OpenRouter reasoning tokens + +- LiteLLM Gemini reasoning issue (lead only, opened 2026-04-24) + + +**Lane C (Bun SIGTRAP)** + +- Bun latest release — `bun-v1.3.14`, 2026-05-13 + +- Bun #31894 stale pooled socket (1.3.14, hang) +- Bun #31463 `ECONNRESET` after `Connection: close` +- Bun #5570 `NODE_TLS_REJECT_UNAUTHORIZED` +- Bun #17325 self-signed CA + +The reviewer independently re-confirmed the Lane C conclusions (Bun 1.3.14 is +current; the adjacent pooled-socket and TLS issues do not establish this +`SIGTRAP`). + +## Lane A — OpenAI-compatible streaming tool-call metadata + +| Claim | Status | Source | +|---|---|---| +| `function.name` normally arrives in the first delta for a tool-call index; later deltas carry only `arguments` fragments | verified | `openai-python` generated chunk schema | +| Both `name` and `arguments` are optional in the wire schema, so an arguments-only delta is structurally representable | verified | same | +| `openai-python` accumulates by index and neither synthesizes a name nor raises a name-specific error; an empty name simply fails to match an input tool | verified | `src/openai/lib/streaming/chat/_completions.py` | +| `openai-node` initializes `function.name` to `''`, overwrites only when a non-empty name arrives, and validates `finish_reason` rather than name presence at completion | verified | `src/lib/ChatCompletionStream.ts` | +| LiteLLM forwards tool-call deltas, repairs a missing `type`, but never synthesizes or rejects a missing `function.name` | verified | `litellm/litellm_core_utils/streaming_handler.py` | + +**Consequence for #1514.** A call that reaches end-of-stream with no non-empty +name is not executable. The reference implementations refuse to invent one; they +simply carry an unusable object and let the caller fail. OpenCodex sits at the +boundary where the unusable object becomes a *Codex tool-call contract event*, +so the equivalent of "let the caller fail" is to not emit the call as usable. +Inventing a name is ruled out by every reference implementation, and +provider-specific tolerance is ruled out because the shape is non-conforming for +every provider, not special to one. + +## Lane B — Gemini `thought` part semantics + +| Claim | Status | Source | +|---|---|---| +| `Part.thought` means "this part represents the model's thought process or reasoning" | verified | Gemini `generateContent` REST reference; `@google/genai` `Part` interface | +| A text-bearing part with `thought: true` is a thought summary, not the answer channel; summaries are opt-in via `includeThoughts` | verified | Gemini thinking guide | +| `thoughtSignature` is an opaque encrypted reasoning handle that must be replayed byte-for-byte in its original part; Gemini 3 function calling returns 400 if the first function-call part of a step omits it | verified | Google AI + Vertex thought-signature guides | +| Official SDK examples branch on `part.thought` and render thought text in a separate channel | verified | `python-genai` examples | +| OpenRouter and LiteLLM normalize Gemini thoughts into a reasoning/thinking field rather than merging into visible content | verified (proxy behavior, not REST semantics) | OpenRouter reasoning docs; LiteLLM issue #26413 | +| Vertex and Cloud Code Assist / Antigravity use the same `Part.thought` / `thoughtSignature` semantics; no separate documented wire contract was found | verified-negative | Vertex guide; no contradicting official doc located | + +**Consequence for #1503.** Routing `thought: true` text to hidden reasoning is +the vendor-documented contract, and preserving `thoughtSignature` untouched is a +hard API requirement for Gemini 3 tool calls — so the fix must classify text +**without** disturbing the existing `observeAntigravityReplay` path. + +## Lane C — Bun SIGTRAP on macOS after TLS failure + +| Claim | Status | Source | +|---|---|---| +| No `oven-sh/bun` issue was found establishing that TLS verification failure or a socket reset aborts the process with `SIGTRAP`/`EXC_BREAKPOINT` | verified-negative | issue search across TLS/fetch/crash families | +| Current stable Bun is `1.3.14`, released 2026-05-13; no 1.3.15+ stable release notes exist to inspect | verified | official GitHub `releases/latest` | +| Known adjacent defects: #31894 stale pooled keep-alive socket (1.3.14, hang not abort); #31463 `ECONNRESET` after `Connection: close` | verified | linked issues | +| Older TLS issues (#5570 `NODE_TLS_REJECT_UNAUTHORIZED`, #17325 self-signed CA) surface certificate *errors*, not process aborts | verified | linked issues | +| "Unhandled rejection in a TLS/fetch callback aborts the process" as a general Bun pattern | **unverified lead** | not established by any opened source | + +**Consequence for #1419.** The issue cannot be closed by a runtime bump: the +reporter is already on the newest stable line, and no upstream fix exists to +point at. Attribution to Bun's TLS stack versus JavaScriptCore's unhandled- +exception path still requires the full faulting frame list the maintainer +requested. This unit therefore treats #1419 as *survivability hardening on the +paths we own* plus an evidence-backed disposition, and explicitly does not claim +a fix for the native trap. diff --git a/devlog/_plan/260812_five_bug_fix_campaign/002_audit_round1_synthesis.md b/devlog/_plan/260812_five_bug_fix_campaign/002_audit_round1_synthesis.md new file mode 100644 index 000000000..af3340db8 --- /dev/null +++ b/devlog/_plan/260812_five_bug_fix_campaign/002_audit_round1_synthesis.md @@ -0,0 +1,172 @@ +# 002 — Audit round 1: synthesis and plan amendments + +Reviewer: independent `gpt-5.6-sol` (medium) explorer, read-only, anchored at +`HEAD == origin/dev == cbbfdd877`. Verdict: **FAIL**, 12 numbered blockers. + +Every blocker below was re-checked against the tree by the main agent before +being accepted or rebutted. The reviewer was right about more than it was wrong +about, and two of its findings invalidate deliverables the plan had promised. + +## Accepted — and what changes + +### B5, B6, B7 (High) — phase 050 proposed work that already exists + +**Verified.** `src/lib/crash-guard.ts:332` `installCrashGuards()` already +registers both `process.on("unhandledRejection")` and +`process.on("uncaughtException")`, records redacted diagnostics, and is called +from `src/cli/index.ts:265`. It even contains a dedicated +`isBenignAbortTeardown` branch for the exact Bun teardown rejection +(`crash-guard.ts:156,185`). + +`cancelBodyOnAbort` is already applied at 8 sites including the OpenAI Responses +path (`src/server/responses/core.ts:3436,3671`), the Anthropic and generic web +search executors, both vision describers, and `codex/auth-api.ts:1748`. + +Service supervision already exists (launchd `KeepAlive`, systemd +`Restart=on-failure`, Windows restart-on-failure). + +So three of the five work items in `050` were **re-proposals of shipped code**. +That is a planning failure: the phase was written from the issue text and the +`abort.ts` comment without auditing what already consumed that helper. Worse, +the reviewer's point about activation is decisive — a test that re-proves +`cancelBodyOnAbort` on an already-guarded path cannot go red before the fix, so +it would have been ceremonial coverage, not a regression test. + +**Amendment:** `050` is rewritten as a disposition-first phase. See `051`. + +### B1 (High) — PR #1508 sets `emittedContentEvent` for thought-only parts + +**Verified in the real diff** (`pr1508@219e7f365a`): + +```ts +const textEvent = googlePartTextEvent(part); +if (textEvent) { + emittedContentEvent = true; + yield textEvent; +} +``` + +`emittedContentEvent` feeds `return emittedContentEvent ? "content" : "continue"` +(`google.ts:645`), which drives heartbeat suppression +(`if (sawLiveness && !sawContentEvent) yield { type: "heartbeat" }`). + +**Partial rebuttal, and the plan was also wrong.** The main agent initially +wrote in `020` that a thought-only part must not set the flag. Reading the +consumer shows that flag is **liveness classification**, not user-visible-content +accounting: a candidate carrying model thinking *is* upstream activity, and +suppressing the synthetic heartbeat for it is arguably correct. + +But the reviewer's underlying complaint stands on a stronger footing than the +one it stated: the PR **changes heartbeat behavior for thought-bearing streams +and has no test asserting which behavior is intended**. Whichever way it goes, +it must be a decided, covered contract rather than an accident of refactoring. + +**Amendment:** `020` drops its incorrect assertion that the flag must not be +set, and instead requires (a) an explicit decision recorded in the PR, and +(b) a test that pins heartbeat/content classification for a thought-only frame. +The main agent's position, to be confirmed with the contributor: keep +`emittedContentEvent = true` for thought parts (they are real upstream +activity), and add the missing test. Also required: an explicit +thought-signature replay regression, not reliance on unchanged fixtures. + +### B2, B3 (High) — `rangeFullyCovered` is not derivable, and 030 does not close #1497 + +**Verified and decisive.** `usage.jsonl` is append-ordered by *completion*, +while the persisted timestamp is the request *start* time. A long-running +request started before a short one can be appended after it. Therefore the +oldest timestamp among retained rows does **not** bound the timestamps in the +dropped prefix, and `rangeFullyCovered: true` could assert completeness that is +false. Shipping a field whose whole value is trustworthiness, in a state where +it can lie, would repeat the exact class of defect #1497 reports. + +The reviewer is also right that the issue's acceptance bar ("`30d` must +aggregate every valid persisted request") is not met by better labeling. + +**Amendment:** `030` is rewritten (see `031`) to drop `rangeFullyCovered` +entirely and report only what is provable: that truncation occurred and what +the retained window is, explicitly framed as a lower bound. The PR is a +**partial mitigation without `Closes #1497`**; the issue stays open for the +rollup work tracked in #1008. + +### B4 (High) — #1409 attribution is not proven + +**Verified.** `gui/src/pages/use-providers-crud.ts` uses `PATCH` at lines 82, +99, 125 for provider edits, and `gui/src/pages/Models.tsx:476` sends +`modelContextWindows` over `PATCH`. Only the Add Provider modal POSTs, and only +a duplicate name reaches the overwrite branch. + +So the plan proved a **real bug** — `buildProviderPayload` +(`gui/src/provider-payload.ts:71-108`) constructs a payload that structurally +cannot carry `modelContextWindows`, and the POST path fills the absent field +from the registry seed — but it did **not** prove that this is the sequence the +reporter hit. Their timeline is upgrade → restart → later unrelated full-config +write, and the maintainer's comment names #1273's stale whole-document writer as +the leading hypothesis. + +**Amendment:** `040` fixes the POST data-loss defect on its own merits and +**does not carry `Closes #1409`**. The issue receives a comment describing the +confirmed POST path, the fix, and what evidence would still be needed to +attribute the reporter's specific loss. This is the honest disposition: fix what +is proven, do not claim the report is resolved. + +### B8, B10 (Medium) — scope statements to tighten + +**B8 verified:** `openai-chat.ts:1157` checks only `typeof name === "string"`, +so a buffered `""` name is emitted. The narrowed issue #1514 is about the +streaming path, but `010`'s claim that the buffered path "is not part of the +defect" is too strong. **Amendment:** `010` extends the nonblank-name check to +the buffered validator, with its own focused test — the same one-line class of +fix, and it removes an obvious follow-up report. + +**B10 verified:** `contextWindow` is also omitted by `buildProviderPayload`, +also user-editable from Models (`Models.tsx:475`), and also registry-seeded +(`derive.ts:404`). **Amendment:** `040` locks the field-ownership matrix at plan +time rather than deferring it to B, and covers `contextWindow` alongside +`modelContextWindows`. + +### B11, B12 (Medium) — gates and evidence hygiene + +**B11 accepted:** `gui/AGENTS.md` requires locale updates plus +`bun test tests`, `lint`, `build`, and `lint:i18n` inside `gui/` for functional +GUI changes. `030`'s command list was root-only. Since `031` now scopes the GUI +work down to copy on an existing surface, the full gate list is recorded and +run; if the GUI change proves to need new visible strings, every locale module +is updated in the same PR. + +**B12 partially accepted.** The security-review point is accepted: `040` touches +a management write boundary, so the PR explicitly flags it for the security +review `src/AGENTS.md` requires. The evidence-URL point is accepted for `001`, +which now carries source URLs and retrieval dates. + +## Rebutted + +### B9 (Medium) — post-loop termination branch "lacks grounded activation" + +**Rebutted with rationale.** The reviewer is right that the post-loop flush is +hard to reach with an unnamed call: raw EOF with pending calls exits through the +truncation branch first (`openai-chat.ts:1071-1080`). But the branch is not +being *added* — `yield* flushToolCalls()` already runs there, and the change is +that its result is now honored. Leaving that one site unhandled would mean an +unnamed call still escapes on whatever path reaches it, which is precisely the +defect. Handling all three sites uniformly is cheaper to reason about than a +two-of-three exception that a future reader must re-derive. + +Concession: `010` no longer claims a distinct activation scenario for site 3. +It states plainly that sites 1 and 2 are the reachable activations and that +site 3 is defensive uniformity on an existing call. + +## Net effect on the goalplan + +| Phase | Before | After | +|---|---|---| +| 010 #1514 | streaming only, `Closes #1514` | streaming + buffered, `Closes #1514` | +| 020 #1503 | land #1508 as-is | land #1508 **with** heartbeat decision + 2 added tests | +| 030 #1497 | `rangeFullyCovered`, `Closes #1497` | provable reporting only, **no** `Closes` | +| 040 #1409 | `Closes #1409` | fix POST loss, **no** `Closes`, issue comment | +| 050 #1419 | 3 work items | disposition-first; shipped code audited, not duplicated | + +Two issues therefore move from "will be closed" to "will be advanced with an +evidence-backed comment". That is a real reduction in what this unit delivers, +and it is the correct call: the alternative was closing #1497 with an aggregate +that still omits rows, and closing #1409 against a path the reporter may never +have taken. diff --git a/devlog/_plan/260812_five_bug_fix_campaign/003_audit_round2_synthesis.md b/devlog/_plan/260812_five_bug_fix_campaign/003_audit_round2_synthesis.md new file mode 100644 index 000000000..a00219b82 --- /dev/null +++ b/devlog/_plan/260812_five_bug_fix_campaign/003_audit_round2_synthesis.md @@ -0,0 +1,129 @@ +# 003 — Audit round 2: synthesis and final amendments + +Same reviewer, re-audit after the round-1 amendments. Verdict: **FAIL**, 5 +remaining blockers. Anchor moved to `origin/dev af2ed77d8` (one CI-only commit +on top of `cbbfdd877`; no source drift in the files this unit touches). + +Round-1 blockers B2, B3, B4, B5, B6, B7, B11 and the evidence half of B12 are +confirmed closed. Both of my rebuttals (B1 semantics, B9 uniformity) were +accepted with line evidence. Five findings remain, and **four of them are real +defects in my amendments** — including one that would have shipped a test that +could never pass. + +## R2-1 (High) — the `contextWindow` carry-over I added cannot execute + +**Verified, and this one is decisive.** My amendment proposed: + +```ts +if (existingContextWindow !== undefined && prov.contextWindow === undefined) { ... } +``` + +But `enrichProviderFromCatalog(name, prov)` runs **before** that line +(`provider-routes.ts:353`), and `derive.ts:404` already did: + +```ts +if (prov.contextWindow === undefined && seed.contextWindow !== undefined) prov.contextWindow = seed.contextWindow; +``` + +So by the time my guard runs, `prov.contextWindow` is the registry seed, never +`undefined`. The guard is dead code and its acceptance test would have stayed +red. + +This is the same trap the `modelContextWindows` fix avoids only by accident: my +map version merges `{...existing, ...(prov.x ?? {})}` unconditionally, so it +works — but it would *also* merge registry-seeded keys into the stored row, +which is the behavior we already have and is not harmful for a fill-only map. +The scalar has no such luck. + +**Amendment.** Capture request ownership **before** enrichment, for every +carried field: + +```ts +// Ownership must be sampled BEFORE enrichProviderFromCatalog: enrichment fills +// absent fields from the registry seed, after which "the client omitted this" +// is indistinguishable from "the registry supplied it" (audit R2-1). +const submittedContextWindow = Object.hasOwn(prov, "contextWindow"); +const submittedModelContextWindows = Object.hasOwn(prov, "modelContextWindows"); +enrichProviderFromCatalog(name, prov); +... +const existing = config.providers[name]; +if (!submittedContextWindow && existing?.contextWindow !== undefined) { + prov.contextWindow = existing.contextWindow; +} +if (existing?.modelContextWindows) { + prov.modelContextWindows = submittedModelContextWindows + ? { ...existing.modelContextWindows, ...(prov.modelContextWindows ?? {}) } + : { ...existing.modelContextWindows }; +} +``` + +Note the second branch also fixes a latent flaw in my round-1 map fix: when the +client omitted the map, the stored value should be the user's map, not the +user's map with registry seeds merged in. + +This pattern — sample ownership pre-enrichment — is the correct general shape +for every field in the ownership matrix, so `040` states it once as the rule. + +## R2-2 (Medium) — `020`'s fallback snippet contradicts the accepted decision + +**Verified.** The reviewer confirmed my B1 reading (the flag drives only +heartbeat suppression at `google.ts:676-689`), but my independent-implementation +fallback snippet still contained `if (textEvent.type === "text_delta") emittedContentEvent = true;` +— the behavior the amended prose now calls wrong. + +**Amendment:** the fallback sets the flag for every emitted event, and the two +required tests (heartbeat classification, signature replay) move into the +numbered acceptance criteria instead of living only in prose. + +## R2-3 (Medium) — `retainedWindow*` has two different populations + +**Verified.** `summarizeUsage` filters by range **and** surface +(`summary.ts:706-716`) *after* the byte-bounded read. My `031` described the +fields as spanning "summarized rows" in one place and rows that "actually fit" +the reader in another. Those are different sets, and publishing the wrong one +next to a summary would be a new small lie of exactly the kind #1497 is about. + +**Amendment:** rename to `snapshotWindowStart` / `snapshotWindowEnd`, defined +unambiguously as min/max timestamp across `snapshot.entries` — the rows the +reader loaded, before range/surface filtering — and label them that way in the +API and the UI. This is the honest, cheap contract: it describes what was read, +which is exactly the thing truncation affects. Tests cover an empty snapshot and +a surface-filtered request to pin that the fields do not track the filtered set. + +## R2-4 (Medium) — `!name` is not "nonblank", and existing test T7 expects the old behavior + +**Verified, both halves.** + +`tests/openai-chat-parallel-stream.test.ts:144` is literally titled +*"T7: name never arrives - call still flushed with empty name (parity, no silent +drop)"* and asserts `{ id: "anon", name: "", args: "{\"q\":1}" }`. My plan would +have added a new test while leaving this one red — and worse, T7 encodes a +deliberate past decision, so changing it needs to be an explicit, argued change +rather than a casualty. + +The argument for changing it: T7's rationale is "no silent drop", and the new +behavior is not a silent drop — it is a loud, terminal error. The invariant T7 +protects (a claimed call never vanishes without a trace) is preserved and +strengthened; only the mechanism changes from "emit unusable" to "fail the +turn". That is the reasoning recorded in the PR. + +`!name` also admits `" "`. **Amendment:** both paths validate +`name.trim().length > 0`, and `010` explicitly updates T7 (retitled to reflect +the terminal-error contract) while keeping T6's late-name coverage untouched. + +## R2-5 (Medium) — the `051` audit table could disclose an unshipped finding + +**Verified against `AGENTS.md`.** The repository rule is explicit: unreleased +failure findings and pre-disclosure patch reasoning stay in scratch (`.tmp/`), +never in a public comment or `devlog/`. My `051` listed "the complete audit +table" as a public issue-comment deliverable, which would publish an unguarded +body-settlement path before its fix ships. + +**Amendment:** `051` orders it explicitly — publish only protections that are +already shipped and public; any newly discovered unguarded site stays in `.tmp/` +until its fix is merged, and only then is it named in the issue. + +## Status + +All five are amended below/in the phase docs. Round 3 re-audit follows; the +plan is not implemented until it passes. diff --git a/devlog/_plan/260812_five_bug_fix_campaign/010_phase1_issue1514_empty_tool_name.md b/devlog/_plan/260812_five_bug_fix_campaign/010_phase1_issue1514_empty_tool_name.md new file mode 100644 index 000000000..7daa52db4 --- /dev/null +++ b/devlog/_plan/260812_five_bug_fix_campaign/010_phase1_issue1514_empty_tool_name.md @@ -0,0 +1,274 @@ +# 010 — Phase 1 (#1514): never emit a tool call with an empty name + +Depends on: nothing (foundation phase — it touches only the adapter's own +flush boundary and establishes the fail-closed pattern the later phases reuse). + +## Scope + +IN + +- `src/adapters/openai-chat.ts` — `parseStream`'s `flushToolCalls`. +- `src/adapters/openai-chat.ts` — the buffered `parseResponse` tool-call + validator (`:1157`). Added after audit blocker B8: that validator checks only + `typeof name === "string"`, so a buffered `""` name is emitted today. The + streamed defect is what #1514 reports, but shipping "never emit an unnamed + tool call" while leaving the buffered twin open would invite the immediate + follow-up report. +- `tests/` — one focused regression test file (extend the nearest existing + openai-chat streaming test module rather than adding a new one if a suitable + one exists). + +OUT + +- The nested `tool_calls` validation from #1325. +- The raw-EOF truncation rules. +- Any provider-specific branch keyed on `opencode-free` or `deepseek`. + +Audit correction (B8): an earlier revision of this document claimed the +buffered path was already safe. It is not — it validates the *type* of `name`, +never its emptiness. + +## Diff-level change map + +### `src/adapters/openai-chat.ts` + +Current: + +```ts +const flushToolCalls = function* (): Generator { + for (const call of closeToolCalls()) { + if (!call.id) call.id = `call_${++toolCallSeq}`; + yield { type: "tool_call_start", id: call.id, name: call.name }; + if (call.args.length > 0) yield { type: "tool_call_delta", arguments: call.args }; + yield { type: "tool_call_end" }; + } +}; +``` + +Target: convert the generator so that an unusable call terminates the turn +through the adapter error channel instead of being emitted. + +```ts +// A streamed tool call is only usable once the upstream has named the function. +// #1325 established that a claimed-but-malformed call is terminal protocol +// corruption rather than droppable padding, and the same reasoning applies when +// the name never arrives: emitting `name: ""` hands the Codex tool-call contract +// a call it cannot dispatch, and dropping it silently can orphan the matching +// result on the next turn. The id is synthesizable because it is an opaque +// correlation handle; the name is not, because inventing one guesses at intent. +const flushToolCalls = function* (): Generator { + for (const call of closeToolCalls()) { + if (call.name.trim().length === 0) { + debugProviderDiagnostic("openai-chat", "tool-call-unnamed", { + hadId: call.id.length > 0, + argsBytes: call.argsBytes, + }); + yield unnamedToolCallEvent(pendingUsage); + return "terminate"; + } + if (!call.id) call.id = `call_${++toolCallSeq}`; + yield { type: "tool_call_start", id: call.id, name: call.name }; + if (call.args.length > 0) yield { type: "tool_call_delta", arguments: call.args }; + yield { type: "tool_call_end" }; + } + return "continue"; +}; +``` + +`closeToolCalls()` is called before the check, so budget reservations for every +pending call are released on the error path exactly as they are on the success +path — including the calls after the offending one, which the early `return` +skips emitting but which `closeToolCalls()` has already closed. The `finally` +block's second `closeToolCalls()` remains a no-op safety net. + +A module-scope helper mirrors the existing `invalidToolCallsEvent`: + +```ts +function unnamedToolCallEvent(usage: OcxUsage | undefined): Extract { + return { + type: "error", + ...(usage ? { usage } : {}), + message: "upstream streamed a tool call without a function name — cannot dispatch", + }; +} +``` + +Confirmed by the reviewer against the real helper: `invalidToolCallsEvent` +carries exactly `type`, `message`, and optional `usage`, so the shape above +matches. + +### Buffered validator (`parseResponse`, `:1157`) + +Current: + +```ts +if (typeof id !== "string" || typeof name !== "string" || typeof args !== "string") { + return [invalidToolCallsEvent(usage)]; +} +``` + +Target — reject a blank name through the same existing error, since a buffered +response that claims a tool call with no name is malformed in exactly the sense +that helper already describes: + +```ts +if (typeof id !== "string" || typeof name !== "string" || typeof args !== "string" || name.trim().length === 0) { + return [invalidToolCallsEvent(usage)]; +} +``` + +Both snippets above use trimmed-length validation (audit R2-4): `!name` would +admit `" "`, and a whitespace-only function name is no more dispatchable than +an empty one. Neither is a legitimate OpenAI tool-call shape. + +### Existing test T7 encodes the old contract and must be updated + +`tests/openai-chat-parallel-stream.test.ts:144` is titled *"T7: name never +arrives — call still flushed with empty name (parity, no silent drop)"* and +asserts `{ id: "anon", name: "", args: "{\"q\":1}" }`. Adding a new test while +leaving T7 in place would land a known-red suite. + +T7 is updated rather than deleted, and the reasoning is recorded in the PR: the +invariant T7 protects is that **a claimed tool call never vanishes without a +trace**. The new behavior preserves that invariant and strengthens it — the call +does not vanish, it fails the turn loudly. Only the mechanism changes, from +"emit an unusable call" to "terminate with a named error". T6's late-arriving +name coverage is untouched, because a name that arrives in a later chunk is +exactly the case that must keep working. + +### The three call sites + +Each site currently does `yield* flushToolCalls();` and must now honor the +returned decision. + +1. `[DONE]` frame — `handleDataLine`, currently: + +```ts +if (payload === "[DONE]") { + yield* flushToolCalls(); + const stopReason = stopReasonFor(finishReason); + yield { type: "done", usage: pendingUsage, ...(stopReason ? { stopReason } : {}) }; + return "terminate"; +} +``` + +becomes: + +```ts +if (payload === "[DONE]") { + if ((yield* flushToolCalls()) === "terminate") return "terminate"; + const stopReason = stopReasonFor(finishReason); + yield { type: "done", usage: pendingUsage, ...(stopReason ? { stopReason } : {}) }; + return "terminate"; +} +``` + +The `done` event must **not** be emitted after the error — a turn that already +reported an undispatchable tool call must not also report clean completion. + +2. `finish_reason` on a choice: + +```ts +if (typeof choice.finish_reason === "string" && choice.finish_reason) yield* flushToolCalls(); +return "continue"; +``` + +becomes: + +```ts +if (typeof choice.finish_reason === "string" && choice.finish_reason) { + if ((yield* flushToolCalls()) === "terminate") return "terminate"; +} +return "continue"; +``` + +3. Post-loop normal completion: + +```ts +yield* flushToolCalls(); +const stopReason = stopReasonFor(finishReason); +yield { type: "done", usage: pendingUsage, ...(stopReason ? { stopReason } : {}) }; +``` + +becomes an early `return` on `"terminate"` before the `done` event, matching +site 1. + +`handleDataLine` already returns `"continue" | "terminate"` and its callers +already `return` on `"terminate"`, so no caller-side plumbing changes. + +## Interaction check with existing invariants + +- **Raw EOF truncation (`!sawFinish && pendingToolCalls.length > 0`)** runs + *before* the post-loop flush and is unchanged: a truncated stream still + reports truncation, not the new unnamed-call error. The new error is reachable + only when the stream *did* reach a terminal signal. +- **#1325 non-array / non-record `tool_calls`** still terminate earlier in + `handleDataLine` and never reach the flush. +- **Orphaned results**: the failure is surfaced as a turn error, so no + half-formed call is handed downstream to be paired later. + +## Activation scenario (C-ACTIVATION-GROUNDING-01) + +The new branch is triggered by driving `parseStream` with a synthetic SSE stream +whose `delta.tool_calls` entry carries `function.arguments` but never +`function.name`, terminated by `[DONE]`: + +``` +data: {"choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"id":"call_x","function":{"arguments":"{\"a\":1}"}}]}}]} +data: [DONE] +``` + +Observable effect proving it ran: the event sequence contains an `error` event +whose message names the missing function name, contains **no** `tool_call_start` +event, and contains **no** `done` event. + +Second activation at the `finish_reason` site: same delta followed by a chunk +with `"finish_reason":"tool_calls"` — same assertions. + +Third site (post-loop) is **not** claimed as a distinct activation. Audit +blocker B9 correctly observed that raw EOF with pending calls exits through the +truncation branch first, so an unnamed call is not reachable there from a state +the system visits. That site is handled anyway because `yield* flushToolCalls()` +already runs there and ignoring its result would leave one escape path open; +this is defensive uniformity on an existing call, not a new branch with its own +trigger. + +Buffered activation (B8): `parseResponse` over +`{"choices":[{"message":{"tool_calls":[{"id":"call_x","function":{"name":"","arguments":"{}"}}]}}]}` +→ the returned events are the existing invalid-tool-calls error, with no +`tool_call_start`. + +## Accept criteria + +1. Unnamed streamed tool call terminated by `[DONE]` → error, no + `tool_call_start`, no `done`. (Red before the change.) +2. Unnamed streamed tool call terminated by `finish_reason` → same. +3. A **named** tool call still emits `tool_call_start` / `tool_call_delta` / + `tool_call_end` / `done` exactly as before, including the id-synthesis path + when `id` is absent. (Guards against over-broad rejection.) +4. A stream with pending tool calls and no terminal signal still produces the + existing truncation error, not the new one. +5. A non-array `delta.tool_calls` still produces the #1325 error. +6. Buffered response with an empty `function.name` produces the invalid + tool-calls error and no `tool_call_start`. (Red before the change.) +7. Buffered response with a valid name is unchanged. +8. A whitespace-only name (`" "`) is rejected on both paths. +9. T7 is updated to the terminal-error contract and passes; T6 still passes + unchanged. +10. `bun run typecheck` exit code 0. +11. Existing openai-chat adapter suites green on `ssh lidge`, including + `tests/openai-chat-parallel-stream.test.ts` and `tests/openai-chat-eof.test.ts`. + +## Verification commands + +```bash +bun x tsc --noEmit +bun test tests/openai-chat*.test.ts tests/adapter*.test.ts +``` + +Exact file globs are resolved in B against the real `tests/` listing. + +## Delivery + +Branch `codex/1514-unnamed-tool-call`, PR against `dev`, template filled, +`Closes #1514`. diff --git a/devlog/_plan/260812_five_bug_fix_campaign/020_phase2_issue1503_google_thought_text.md b/devlog/_plan/260812_five_bug_fix_campaign/020_phase2_issue1503_google_thought_text.md new file mode 100644 index 000000000..ad2cbf57c --- /dev/null +++ b/devlog/_plan/260812_five_bug_fix_campaign/020_phase2_issue1503_google_thought_text.md @@ -0,0 +1,190 @@ +# 020 — Phase 2 (#1503): Google `thought` parts must not become visible text + +Depends on: 010 only for the shared "adapter classifies before emitting" +discipline; the code paths are disjoint (`google.ts` vs `openai-chat.ts`), so +the branches do not conflict. + +## Scope + +IN + +- `src/adapters/google.ts` — streaming part loop and buffered part loop. +- `tests/google-hardening.test.ts` — regression coverage. +- `structure/04_transports-and-sidecars.md` — record the transport decision. + +OUT + +- `observeAntigravityReplay` and every `thoughtSignature` path. Replay must be + byte-identical after this change; Gemini 3 function calling returns 400 when + the first function-call part of a step loses its signature (see `001`). +- Inline image materialization. +- Function-call ordering. +- The Antigravity/Vertex namespace split. + +## Contributor-first delivery decision + +Draft PR **#1508** by `Ingwannu` already implements this fix: + +- branch `agent/fix-1503-google-thought-visibility`, head `219e7f365a`, + state `MERGEABLE`; +- files: `src/adapters/google.ts`, `tests/google-hardening.test.ts`, + `structure/04_transports-and-sidecars.md` — exactly the scope above; +- author-reported verification: Google focused tests 81/81, typecheck pass, + privacy scan pass, rebased onto `dev@4fed8d3fe`. + +`dev` has since advanced to `cbbfdd877`. The plan is therefore **review and +land the contributor branch**, not reimplement it: + +1. Fetch the PR head and diff it against current `dev`. +2. Verify the four properties below against the real diff. +3. Re-run the Google focused suite and typecheck on `ssh lidge` at the PR head + merged onto current `dev`. +4. If the diff is correct → merge it, and #1503 closes via the PR. +5. If the diff is incomplete → push a small correction commit **on top of** the + contributor's commits (never a squash that erases authorship, never a + force-push without `--force-with-lease`), then merge. +6. Only if the branch is unusable → implement independently, and say so in the + PR discussion with the specific reason. + +## Properties the diff must satisfy + +| # | Property | Why | +|---|---|---| +| P1 | A part with `thought === true` and non-empty `text` produces a hidden reasoning event (`reasoning_raw_delta` / `thinking_delta`), never `text_delta` | the reported defect | +| P2 | Both the streaming loop and the buffered `parseResponse` loop are covered by one shared classifier | the issue names both; two copies drift | +| P3 | Ordinary text (`thought` absent or false) still produces `text_delta` | guard against over-broad suppression | +| P4 | `thoughtSignature` observation and replay are untouched | hard API requirement (001, Lane B) | + +Additional checks against the current tree: + +- the locally-declared part shape + `{ text?: string; functionCall?: { name: string; args: unknown } }` + must gain `thought?: boolean` (both loops declare their own shape, so both + need it); +- `reasoning.summary: "none"` must continue to suppress visible rendering + downstream, which follows from routing through the reasoning channel rather + than a new one. + +### `emittedContentEvent`: corrected after audit (B1) + +An earlier revision of this document asserted that a thought-only part must not +set `emittedContentEvent`. That assertion was wrong, and reading the consumer +shows why: the flag feeds +`return emittedContentEvent ? "content" : "continue"` (`google.ts:645`), whose +only consumer is heartbeat suppression — +`if (sawLiveness && !sawContentEvent) yield { type: "heartbeat" }`. It is +**liveness classification**, not user-visible-content accounting. A candidate +carrying model thinking is genuine upstream activity, so suppressing the +synthetic heartbeat for it is correct. + +PR #1508 keeps `emittedContentEvent = true` for both event types, which the +main agent judges correct. What is genuinely missing — and this is the valid +core of audit blocker B1 — is that the PR **changes heartbeat behavior for +thought-bearing streams with no test pinning the intended contract**. The +decision must be recorded and covered rather than inherited from a refactor. + +Required additions before merge: + +1. a test asserting that a thought-only SSE frame classifies as `content` + (no synthetic heartbeat), pinning the decision above; +2. an explicit thought-signature replay regression, rather than relying on + existing fixtures being unchanged. + +## If independent implementation is needed + +Shared classifier near the part loops: + +```ts +// Gemini marks model-internal reasoning with `Part.thought`. That text is a +// thought summary, not the answer channel, so it crosses into the hidden +// reasoning stream instead of visible output. `thoughtSignature` is a separate +// opaque replay handle and is deliberately not read here. +function googlePartTextEvent(part: { text?: string; thought?: boolean }): AdapterEvent | undefined { + if (!part.text) return undefined; + return part.thought === true + ? { type: "reasoning_raw_delta", text: part.text } + : { type: "text_delta", text: part.text }; +} +``` + +Streaming loop: + +```ts +const textEvent = googlePartTextEvent(part); +if (textEvent) { + // Both branches set the flag (audit R2-2). `emittedContentEvent` drives only + // heartbeat suppression (google.ts:645 -> the `sawLiveness && !sawContentEvent` + // check), and a thought delta is real upstream activity, so a synthetic + // heartbeat must not be emitted alongside it. + emittedContentEvent = true; + yield textEvent; +} +``` + +Buffered loop: + +```ts +const textEvent = googlePartTextEvent(part); +if (textEvent) events.push(textEvent); +``` + +## Activation scenario (C-ACTIVATION-GROUNDING-01) + +No credential or live request is needed — the issue supplies the synthetic +candidate: + +```json +{"candidates":[{"content":{"parts":[{"thought":true,"text":"internal reasoning"}]},"finishReason":"STOP"}]} +``` + +Buffered activation: `adapter.parseResponse(new Response(JSON.stringify(payload), {headers:{"content-type":"application/json"}}), budget)` → +the returned events contain no `{type:"text_delta", text:"internal reasoning"}` +and do contain a hidden reasoning event carrying that text. + +Streaming activation: the same candidate delivered as an SSE `data:` frame +through `parseStream` → same assertions on the yielded sequence. + +Mixed activation proving P3: parts +`[{thought:true,text:"secret"},{text:"visible"}]` → exactly one `text_delta` +with `"visible"`, and `"secret"` appears only on the reasoning channel. + +Signature activation proving P4: a candidate carrying both a thought part and a +`functionCall` part with a `thoughtSignature` → the replay observation records +the same signature value as before the change (assert against the existing +Antigravity/Vertex replay test fixtures). + +## Accept criteria + +1. Buffered thought part → no visible `text_delta`, hidden reasoning present. +2. Streaming thought part → same. +3. Ordinary text still visible in both parsers. +4. Thought-signature replay fixtures unchanged. +5. A thought-only SSE frame classifies as `content`, so no synthetic heartbeat + is emitted for that batch. (New test — pins the decision above.) +6. An explicit thought-signature replay regression asserts the observed + signature is byte-identical before and after the change. (New test — not + inferred from unchanged fixtures.) +7. `bun run typecheck` exit code 0. +8. Google focused suite green on `ssh lidge`. +9. PR CI green at the exact merged head (the current aggregate is red from a + macOS timeout). + +## Verification commands + +```bash +bun x tsc --noEmit +bun test tests/google-hardening.test.ts tests/google*.test.ts +``` + +## Delivery + +Preferred: merge PR **#1508** (contributor-authored), which carries +`Closes #1503` or is closed manually against `dev` per the branch policy note in +`AGENTS.md` (PRs target `dev`, so GitHub auto-close does not fire; close the +issue manually citing the merge SHA). + +The two missing tests are pushed **on top of** the contributor's commits +(`agent/fix-1503-google-thought-visibility`, head `219e7f365a`), never as a +squash that erases authorship and never with a force-push lacking +`--force-with-lease`. The reviewer also noted the PR's aggregate CI is currently +red from a macOS timeout; CI must be re-run and green at the exact merged head. diff --git a/devlog/_plan/260812_five_bug_fix_campaign/030_phase3_issue1497_usage_range_truncation.md b/devlog/_plan/260812_five_bug_fix_campaign/030_phase3_issue1497_usage_range_truncation.md new file mode 100644 index 000000000..66b4b8250 --- /dev/null +++ b/devlog/_plan/260812_five_bug_fix_campaign/030_phase3_issue1497_usage_range_truncation.md @@ -0,0 +1,180 @@ +# 030 — Phase 3 (#1497): `30d` and `all` must not silently share one byte tail + +Depends on: nothing in 010/020 (disjoint files). Ordered here because it is the +first phase that changes a **response contract** rather than an adapter's +internal event stream, so it needs the adapter phases settled first to keep each +PR reviewable in isolation. + +## The defect, stated precisely + +`GET /api/usage` reads a bounded newest-bytes window and *then* applies the +range filter: + +```ts +const effectiveReadLimit = config.managementUsageMaxReadBytes ?? 64 * 1024 * 1024; +const snapshot = await readUsageSnapshotForManagement(effectiveReadLimit); +const summary = { + ...summarizeUsage(snapshot.entries, range, now, surface), + historyTruncated: snapshot.truncatedPrefixBytes > 0 || snapshot.entriesTruncated, +``` + +Two distinct user-visible failures follow: + +1. **`30d` is a lie by omission.** The measured installation had 175,818 + requests over Aug 4-11; the bounded read returned 46,417 (~39 hours). 73.6% + of in-range requests were omitted from a view labelled `30d`. +2. **`Available history` is indistinguishable from `30d`.** `range=all` reads + the same tail, so the control that promises "everything" returns the same + incomplete set. + +A third consequence is that cumulative totals can *decrease* between reloads as +older high-usage rows fall out of the moving window — a monotonic counter that +goes backwards. + +## Scope decision: honest reporting, not a derived-aggregate rewrite + +Draft PR #1008 proposes a daily rollup sidecar plus raw-tail merge. That is the +durable direction, but it is stale, conflicting, and carries unresolved +correctness findings (crash-safe append/commit validation, truncated-ledger +invalidation, partial-day range overlap, request dedup, disabled-rollup +behavior). **An incorrect derived aggregate is worse than an honestly truncated +view**, and adopting it here would silently take on those findings. + +This phase therefore does the bounded, provably-correct part: + +**IN** + +- Make the API tell the truth about *what range the returned data actually + covers*, so a client can never present a truncated window as a complete one. +- Make `range=all` and `range=30d` distinguishable when truncation occurred. +- Surface the coverage boundary in the dashboard label. + +**OUT** + +- The rollup sidecar (#1008 keeps that scope). +- Raising or removing `managementUsageMaxReadBytes` as the "fix" — the issue + itself notes that only postpones recurrence and makes every summary parse a + growing file. +- Any change to `usage.jsonl` write paths. + +## Diff-level change map + +### `src/usage/log.ts` + +`readUsageSnapshotForManagement` already returns `truncatedPrefixBytes`, +`entriesTruncated`, `entriesDropped`. Add the one fact the caller cannot derive: +the **timestamp of the oldest entry actually read**. That is the true left edge +of coverage. + +```ts +export async function readUsageSnapshotForManagement(maxReadBytes = MANAGEMENT_USAGE_MAX_READ_BYTES): Promise<{ + entries: PersistedUsageEntry[]; + revision: UsageLogRevision | null; + truncatedPrefixBytes: number; + entriesTruncated: boolean; + entriesDropped: number; +}> +``` + +The oldest-entry timestamp is computable in `logs-usage-routes.ts` from +`snapshot.entries` without changing this signature (entries are append-ordered, +so the first surviving entry is the oldest read). Preferring the caller-side +derivation keeps the shared reader untouched and avoids disturbing the +in-flight-dedup and revision-key logic around it. + +### `src/server/management/logs-usage-routes.ts` + +```ts +const snapshot = await readUsageSnapshotForManagement(effectiveReadLimit); +const historyTruncated = snapshot.truncatedPrefixBytes > 0 || snapshot.entriesTruncated; +// When the bounded reader dropped a prefix, the rows we have do not span the +// requested range: the oldest row we read IS the left edge of what any summary +// over these entries can describe. Reporting it lets the client label the view +// by its real coverage instead of by the range that was asked for. Without it +// `30d` and `all` are indistinguishable on a busy installation, which is the +// defect in #1497. +const coverageStart = historyTruncated ? oldestEntryTimestamp(snapshot.entries) : null; +const summary = { + ...summarizeUsage(snapshot.entries, range, now, surface), + historyTruncated, + truncatedPrefixBytes: snapshot.truncatedPrefixBytes, + entriesTruncated: snapshot.entriesTruncated, + entriesDropped: snapshot.entriesDropped, + coverageStart, + rangeFullyCovered: !historyTruncated || rangeStartsAfter(range, coverageStart, now), +}; +``` + +`rangeFullyCovered` is the field a client can act on without arithmetic: + +- `range=7d` on a tail covering 39 hours → `false`; +- `range=7d` on a tail covering 30 days → `true` even though `historyTruncated` + is `true`, because everything the range asked for is present. This distinction + matters: today a truncated file makes *every* range look suspect. + +`refreshedUsageSummary` (`:119`) re-derives range-dependent fields for cached +entries and must carry these two through consistently; the cache key already +includes `effectiveReadLimit`, so a limit change invalidates correctly. + +`oldestEntryTimestamp` and `rangeStartsAfter` are small local helpers; the +range→start-instant mapping already exists inside `summarizeUsage`/`parseRange` +and is reused rather than duplicated. + +### `gui/src/pages/Usage.tsx` + +When `rangeFullyCovered === false`, the range control must not present the +result as the requested range. Minimum: an inline note stating the covered +window (`"showing Aug 10 18:00 onward — older rows exceed the read limit"`) and +the `Available history` option relabelled to reflect that it is bounded. This is +the user-facing half of the fix; without it the API tells the truth and the UI +still does not. + +A GUI change means the PR must include a screenshot per `AGENTS.md` +(`enforce-target` rejects `gui`-touching PRs without one). + +## Activation scenario (C-ACTIVATION-GROUNDING-01) + +Fixture-driven, no live installation needed: + +1. Write a temporary `usage.jsonl` whose total size exceeds a deliberately small + `managementUsageMaxReadBytes` (e.g. 4 KiB), with entries spanning 40 days and + the newest ~1 day of rows fitting inside the limit. +2. `GET /api/usage?range=30d` → `historyTruncated: true`, + `rangeFullyCovered: false`, `coverageStart` equal to the oldest row that fit. +3. `GET /api/usage?range=all` → same `coverageStart`, `rangeFullyCovered: false`. +4. Raise the limit above the file size and repeat → `historyTruncated: false`, + `rangeFullyCovered: true`, `coverageStart: null`, and the `30d` totals now + match a direct summarization of every in-range row. + +Observable effect proving the branch ran: `rangeFullyCovered` flips between +steps 2 and 4 for the identical request. + +## Accept criteria + +1. Truncated ledger + `range=30d` → `rangeFullyCovered: false` with a + `coverageStart` matching the oldest read row. (Red before the change: the + field does not exist.) +2. Truncated ledger + `range=all` → same, and the client can therefore tell the + two views apart. +3. Untruncated ledger → `rangeFullyCovered: true`, `coverageStart: null`, and + summary numbers identical to the pre-change behavior (no regression in the + normal case). +4. A truncated ledger whose tail still fully covers a short range → + `historyTruncated: true` **and** `rangeFullyCovered: true`. +5. Cached responses (`refreshedUsageSummary`) carry both fields consistently. +6. GUI shows the coverage boundary when `rangeFullyCovered` is false. +7. `bun run typecheck` and `bun run lint:gui` exit 0; usage suites green on + `ssh lidge`. + +## Verification commands + +```bash +bun x tsc --noEmit +bun test tests/usage*.test.ts tests/logs-usage*.test.ts tests/management*.test.ts +bun run lint:gui +``` + +## Delivery + +Branch `codex/1497-usage-range-coverage`, PR against `dev` with +`Closes #1497` and a GUI screenshot. diff --git a/devlog/_plan/260812_five_bug_fix_campaign/031_phase3_revised_usage_coverage.md b/devlog/_plan/260812_five_bug_fix_campaign/031_phase3_revised_usage_coverage.md new file mode 100644 index 000000000..6ef92d78f --- /dev/null +++ b/devlog/_plan/260812_five_bug_fix_campaign/031_phase3_revised_usage_coverage.md @@ -0,0 +1,116 @@ +# 031 — Phase 3 REVISED (#1497): report only what is provable + +Supersedes `030`. Written after audit blockers B2 and B3. + +## Why 030 was wrong + +`030` proposed `rangeFullyCovered`, derived from the oldest retained entry's +timestamp. That is unsound: + +- `usage.jsonl` is appended when a request **completes**; +- the persisted timestamp is the request's **start** time; +- so a long-running request that started early can be appended after later, + shorter requests. + +The bounded reader keeps the newest *bytes*, i.e. the newest *appends*. The +minimum start-timestamp among retained rows therefore does not bound the +start-timestamps inside the dropped prefix. `rangeFullyCovered: true` could +claim a range is complete while an old long-running request sits in the dropped +prefix inside that range. + +A field whose entire purpose is to be trusted, which can be wrong, is worse than +no field. Shipping it would reproduce #1497's own defect class: a label that +asserts more than the data supports. + +## What this phase delivers instead + +Only facts the reader can prove: + +1. **Truncation is already reported** (`historyTruncated`, `truncatedPrefixBytes`, + `entriesTruncated`, `entriesDropped`). Keep as-is. +2. **Add the loaded-snapshot window**, named so it cannot be read as a + completeness claim or as a description of the summarized rows: + +```ts +// The bounded reader keeps the newest BYTES of an append-ordered ledger, and rows +// are appended on completion while their timestamp is the request start. So the +// oldest retained timestamp is NOT a bound on what the dropped prefix contains: +// a long-running request started earlier can be appended later. This field is +// therefore reported as the window of the rows the READER LOADED — never as +// proof that the requested range is fully covered (#1497 audit B2), and never +// as a description of the summarized subset (audit R2-3). +snapshotWindowStart: number | null; // min timestamp across snapshot.entries +snapshotWindowEnd: number | null; // max timestamp across snapshot.entries +``` + +**Population is `snapshot.entries`, before any filtering.** `summarizeUsage` +applies both the range window and the surface predicate +(`src/usage/summary.ts:706-716`), so the summarized subset is strictly smaller +and describes the *query*, not the *read*. Truncation is a property of the read, +which is why the reported window must be the read's. Naming them +`snapshotWindow*` rather than `retainedWindow*` removes the ambiguity that audit +R2-3 caught. + +3. **Make the dashboard stop presenting a truncated view as the requested + range.** When `historyTruncated` is true, the range control shows that the + summary is computed over a bounded tail and names the retained window. The + `Available history` label specifically must stop implying completeness — it + is the more misleading of the two, because `30d` at least states a bound. + +**No `Closes #1497`.** The issue's bar is complete 7d/30d aggregation and +monotonic all-time totals. This phase does not achieve that; it removes the +false presentation while the durable fix (daily rollup sidecar, #1008) is worked +separately. The PR says so explicitly and links #1008. + +## Scope + +IN: `src/server/management/logs-usage-routes.ts` (two derived fields + +`refreshedUsageSummary` passthrough), `gui/src/pages/Usage.tsx` (truncation +disclosure), locale modules for any new visible string, tests. + +OUT: the rollup sidecar; raising `managementUsageMaxReadBytes`; any change to +usage write paths; any completeness claim. + +## Activation scenario (C-ACTIVATION-GROUNDING-01) + +1. Fixture `usage.jsonl` larger than a deliberately small + `managementUsageMaxReadBytes`, rows spanning 40 days. +2. `GET /api/usage?range=30d` → `historyTruncated: true`, and + `snapshotWindowStart` equals the min timestamp across the rows the reader + loaded — asserted against a directly-computed expectation, not against the + reader's own output. +3. Raise the limit above file size → `historyTruncated: false`, + `snapshotWindowStart` spans the whole fixture, and the `30d` totals equal a + direct summarization of every in-range row (proving no regression in the + normal path). +4. GUI: with `historyTruncated: true` the disclosure renders; with `false` it + does not. +5. `surface=claude` on the same fixture → `snapshotWindow*` is **unchanged** + from the `surface=all` request, proving the fields track the read and not the + filtered result (audit R2-3). +6. Empty snapshot → both fields are `null`, not `NaN` or `Infinity`. + +Observable effect: the disclosure element appears and disappears across steps 2 +and 3 for the identical request. + +## Accept criteria + +1. Truncated ledger → `snapshotWindowStart`/`End` match independently computed + values. (Red before: fields do not exist.) +2. Untruncated ledger → summary numbers byte-identical to pre-change behavior. +3. Cached responses carry both fields consistently through + `refreshedUsageSummary`. +4. GUI discloses truncation and no longer labels a bounded tail as complete + history. +5. No field in the response asserts range completeness. +6. Surface and range filters do not move `snapshotWindow*`; an empty snapshot + yields `null` for both. +7. Gates: root `bun x tsc --noEmit`, root usage/management suites, and inside + `gui/`: `bun test tests`, `bun run lint`, `bun run build`, `bun run lint:i18n` + (per `gui/AGENTS.md`), plus every locale module updated for new copy. +8. PR includes a GUI screenshot (`enforce-target` gate). + +## Delivery + +Branch `codex/1497-usage-truncation-disclosure`. PR against `dev`, references +`#1497` and `#1008`, **without** `Closes`. diff --git a/devlog/_plan/260812_five_bug_fix_campaign/040_phase4_issue1409_context_window_overrides.md b/devlog/_plan/260812_five_bug_fix_campaign/040_phase4_issue1409_context_window_overrides.md new file mode 100644 index 000000000..d5b4d5bf5 --- /dev/null +++ b/devlog/_plan/260812_five_bug_fix_campaign/040_phase4_issue1409_context_window_overrides.md @@ -0,0 +1,282 @@ +# 040 — Phase 4 (#1409-adjacent): user-owned fields must survive a provider POST overwrite + +**Attribution correction (audit blocker B4).** This phase proves and fixes a +real data-loss defect on the POST overwrite path. It does **not** prove that +path is what the #1409 reporter hit. See "Attribution" below. The PR therefore +does **not** carry `Closes #1409`. + +Depends on: nothing in 010-030 (disjoint files). Ordered after 030 because both +touch management routes and keeping them in separate PRs keeps each diff small. + +## The defect (proven) + +The report blames "the upgrade", but the upgrade is only the trigger that makes +the user re-save a provider. The deletion happens on the **provider overwrite +path**, and the tree contains the proof. + +`POST /api/providers` (`src/server/management/provider-routes.ts:353-364`): + +```ts +enrichProviderFromCatalog(name, prov); +const { saveConfigPreservingClaudeCode: save } = await import("../../config"); +// Overwriting an existing provider must not drop its multi-key pool: ... +const existingPool = config.providers[name]?.apiKeyPool; +if (existingPool && !prov.apiKeyPool) prov.apiKeyPool = existingPool; +// The same rule applies to user-configured price overlays: the dashboard's +// add/edit form does not send modelCosts, so an overwrite must not silently +// erase hand-edited per-model prices from Logs/Usage estimates. +const existingCosts = config.providers[name]?.modelCosts; +if (existingCosts && !prov.modelCosts) prov.modelCosts = existingCosts; +config.providers[name] = stripRegistryOnlyStaticHeaders(name, prov); +``` + +`enrichProviderFromCatalog` → `enrichProviderFromRegistry` +(`src/providers/derive.ts:405`): + +```ts +if (!prov.modelContextWindows && seed.modelContextWindows) prov.modelContextWindows = { ...seed.modelContextWindows }; +``` + +The dashboard add/edit form does not send `modelContextWindows`. So on an +overwrite the field is absent, the registry seed fills it, and the stored row +becomes the seed. For `opencode-go` the seed is: + +```ts +modelContextWindows: { "kimi-k3": KIMI_K3_STANDARD_CONTEXT_WINDOW }, +``` + +which is exactly the `{"kimi-k3": 262144}` the reporter found in place of their +`{"deepseek-v4-flash": 900000}`. The observed UI change (855k → 950k) follows +because with no override the catalog falls back to the registry +`contextWindow: 1000000` with a 900000 auto-compact limit. + +**The two existing carry-overs are the shipped precedent.** `apiKeyPool` and +`modelCosts` are preserved with exactly this rationale — the form does not send +them, so absence must not mean deletion. `modelContextWindows` is the same class +of hand-edited user data and was simply never added to the list. + +Confirmed in the GUI source: `buildProviderPayload` +(`gui/src/provider-payload.ts:83-108`) builds `ProviderPayload`, whose type +(`:71-81`) has no `modelContextWindows` member at all. The payload therefore +*structurally cannot* carry the field. + +## Attribution: what this explains and what it does not + +The reviewer verified that the dashboard's normal editing surfaces use `PATCH` +(`gui/src/pages/use-providers-crud.ts:82,99,125`), and that `Models.tsx:476` +sends `modelContextWindows` over `PATCH`. The POST overwrite branch is reached +only when the Add Provider modal submits a **duplicate provider name**. + +So the confirmed reproduction is: re-add an existing provider through the Add +Provider modal → the user's `modelContextWindows` is replaced by the registry +seed. A genuine bug, worth fixing on its own merits. + +The #1409 reporter's sequence was upgrade → daemon restart → later unrelated +full-config write, and the maintainer's comment names #1273's stale +whole-document writer as the leading hypothesis. Nothing found here rules that +in or out. **The honest disposition is to fix the proven defect and comment on +#1409 with what was confirmed, what was fixed, and what evidence is still +needed** — not to close it. + +## Field-ownership matrix (locked at plan time, audit blocker B10) + +A field needs carry-over when all three hold: (a) user-editable somewhere in the +product, (b) absent from `ProviderPayload`, (c) registry-seeded by +`enrichProviderFromRegistry`. Carrying over a registry-only field would freeze +stale registry metadata into user config — the opposite failure — so (a) must be +confirmed by reading the editing UI, never assumed. + +| Field | User-editable | In POST payload | Registry-seeded | Carry over? | +|---|---|---|---|---| +| `apiKeyPool` | yes | no | no | already carried | +| `modelCosts` | yes | no | no | already carried | +| `modelContextWindows` | yes (`Models.tsx:476`) | **no** | yes (`derive.ts:405`) | **add** | +| `contextWindow` | yes (`Models.tsx:475`) | **no** | yes (`derive.ts:404`) | **add** | +| `modelInputModalities`, `modelMaxOutputTokens`, `modelReasoningEfforts`, `modelDefaultReasoningEfforts`, `modelReasoningEffortMap`, `reasoningEffortMap`, `noVisionModels`, `noReasoningModels`, `noTemperatureModels`, `defaultMaxOutputTokens` | confirm against the real editing surfaces in B | no | yes | only where (a) is confirmed | + +`contextWindow` joins this phase because it fails the same three tests and is +edited on the same Models surface, one line above `modelContextWindows`. + +### What is *not* the cause (checked, so nobody re-checks it) + +- `derive.ts:405` is fill-only and correct in isolation: it only fills when the + whole map is absent. The bug is that the map *is* absent on this path because + the client never sends it, not because the fill logic is wrong. +- `router.ts:270-272` merges registry values *beneath* user entries + (`mergeRecordFill`) and never overwrites. +- `provider-routes.ts:187-208` (the PATCH path) merges per key and preserves + unmentioned entries. PATCH is already correct; POST is not. +- The stale whole-document writer in #1273 can make the loss *visible* later, + but it is not required to explain the loss. + +## Scope + +IN + +- `src/server/management/provider-routes.ts` — the POST overwrite carry-over. +- `tests/` — regression coverage for the overwrite path. + +OUT + +- `derive.ts` fill semantics. +- `router.ts` merge semantics. +- The PATCH path. +- #1273's whole-document writer (separate issue, separate unit). + +## Diff-level change map + +### Ownership must be sampled BEFORE enrichment (audit R2-1) + +`enrichProviderFromCatalog(name, prov)` runs at `:353`, and +`enrichProviderFromRegistry` fills absent fields from the registry seed. After +that call, "the client omitted this field" and "the registry supplied it" are +indistinguishable. Any carry-over guard written as +`prov.x === undefined` after enrichment is therefore **dead code** — this was a +real defect in an earlier revision of this document, caught in audit round 2. + +The correct shape samples ownership first: + +```ts +// Sample request ownership BEFORE enrichment (audit R2-1): enrichment fills +// absent fields from the registry seed, after which an omitted field is +// indistinguishable from a seeded one, and a post-enrichment `=== undefined` +// guard can never fire. +const submittedContextWindow = Object.hasOwn(prov, "contextWindow"); +const submittedModelContextWindows = Object.hasOwn(prov, "modelContextWindows"); +enrichProviderFromCatalog(name, prov); +``` + +then restores from the stored row where the request did not own the field: + +```ts +const existing = config.providers[name]; +// The add/edit form cannot send these: `ProviderPayload` (gui/src/provider-payload.ts:71) +// has no member for either. Absence in the request means "not carried", never +// "the user deleted it" — deletion goes through PATCH with an explicit null. +if (!submittedContextWindow && existing?.contextWindow !== undefined) { + prov.contextWindow = existing.contextWindow; +} +if (existing?.modelContextWindows) { + prov.modelContextWindows = submittedModelContextWindows + ? { ...existing.modelContextWindows, ...(prov.modelContextWindows ?? {}) } + : { ...existing.modelContextWindows }; +} +``` + +The `submittedModelContextWindows` distinction matters: when the client did not +send the map, the stored value must be the user's map alone. Merging the +registry seed on top would persist seed keys into user config as a side effect +of an unrelated save. + +```ts +const existingCosts = config.providers[name]?.modelCosts; +if (existingCosts && !prov.modelCosts) prov.modelCosts = existingCosts; +// Same rule again for per-model context windows. The add/edit form does not +// send modelContextWindows either, so registry enrichment above would fill the +// absent field with the registry seed and the stored row would lose a +// hand-edited override (#1409: an explicit deepseek-v4-flash entry was replaced +// by the opencode-go seed for kimi-k3). Absence in the request means "the client +// did not carry this field", never "the user deleted their override" — deletion +// goes through the PATCH path, which sends an explicit null. +// SUPERSEDED by the ownership-sampling block above (audit R2-1). Kept only to +// show the shape that was wrong: this merges the registry seed into the stored +// row when the client omitted the map. +const existingWindows = config.providers[name]?.modelContextWindows; +if (existingWindows) { + prov.modelContextWindows = { ...existingWindows, ...(prov.modelContextWindows ?? {}) }; +} +``` + +Merge direction matters: existing user entries form the base and anything the +request *did* send wins per key. That preserves a hand-edited +`deepseek-v4-flash` while still letting an explicit submitted value update a +key, and it survives registry enrichment because enrichment ran before this +line and only ever filled an absent map. + +### The same audit applied to the neighbouring fields + +`enrichProviderFromRegistry` fills these fill-only fields the form may also omit: +`modelInputModalities`, `modelMaxOutputTokens`, `modelReasoningEfforts`, +`modelDefaultReasoningEfforts`, `modelReasoningEffortMap`, `reasoningEffortMap`, +`noVisionModels`, `noReasoningModels`, `noTemperatureModels`, `contextWindow`, +`defaultMaxOutputTokens`. + +The matrix above locks the decision for `modelContextWindows` and +`contextWindow`. For the remaining fields, B confirms condition (a) by reading +the editing UI before extending the carry-over — speculative carry-over of +registry-only metadata would freeze stale registry values into user config. +Fields that are purely registry metadata are deliberately left alone. + +`contextWindow` carry-over: + +```ts +// SUPERSEDED and PROVEN DEAD by audit R2-1: enrichment already assigned +// prov.contextWindow from the registry seed, so this guard never fires. +// Use the ownership-sampling block above instead. +const existingContextWindow = config.providers[name]?.contextWindow; +if (existingContextWindow !== undefined && prov.contextWindow === undefined) { + prov.contextWindow = existingContextWindow; +} +``` + +`stripRegistryOnlyStaticHeaders` runs after and is unaffected. + +## Activation scenario (C-ACTIVATION-GROUNDING-01) + +1. Seed config with + `providers["opencode-go"].modelContextWindows = { "deepseek-v4-flash": 900000 }`. +2. `POST /api/providers` with the body shape the dashboard form sends for an + edit — `name`, `adapter`, `baseUrl`, `apiKey`, `defaultModel` — and **no** + `modelContextWindows`. +3. Read the persisted config. + +Before the change: `{"kimi-k3": 262144}` — the override is gone. +After: `{"deepseek-v4-flash": 900000}` — the user's map is restored intact. + +The registry seed is deliberately **not** persisted here (audit R3-2): the +client did not send the map, so the stored row must be the user's map alone. +Registry values still reach the runtime through `router.ts`'s +`mergeRecordFill(registryEntry.modelContextWindows, provider.modelContextWindows)`, +which fills seed keys *beneath* user entries at resolve time. Persisting seeds +into user config as a side effect of an unrelated save is the behavior this +phase removes, not one it should reproduce. + +Observable effect proving the branch ran: the persisted map contains the user's +key, which is unreachable in the pre-change code for this request shape. + +Second activation (submitted map): the same POST **with** +`modelContextWindows: { "kimi-k3": 300000 }` → persisted +`{"deepseek-v4-flash": 900000, "kimi-k3": 300000}` — the submitted key wins and +the untouched user key survives. + +## Accept criteria + +1. Overwrite without `modelContextWindows` preserves the existing user entry. + The persisted map is exactly the user's map, with no registry seed keys + added. (Red before the change.) +2. Overwrite **with** an explicit `modelContextWindows` value for a key updates + that key and still preserves the other user keys. +3. Overwrite without `contextWindow` preserves the existing user value. + (Red before the change.) +4. Creating a brand-new provider still receives the registry seed (no + regression in enrichment). +5. The PATCH path's existing delete-by-null behavior is unchanged — an explicit + null still deletes. +6. `apiKeyPool` and `modelCosts` carry-overs still behave as before. +7. `bun run typecheck` exit 0; provider/management suites green on `ssh lidge`. +8. The PR flags the management write-boundary change for the security review + `src/AGENTS.md` requires (audit blocker B12). + +## Verification commands + +```bash +bun x tsc --noEmit +bun test tests/provider-routes*.test.ts tests/management*.test.ts tests/config*.test.ts +``` + +## Delivery + +Branch `codex/1409-preserve-context-window-overrides`, PR against `dev`, +referencing `#1409` **without** `Closes`. A comment on #1409 records the +confirmed POST reproduction, the fix, and the outstanding attribution question +(#1273). diff --git a/devlog/_plan/260812_five_bug_fix_campaign/050_phase5_issue1419_tls_crash_survivability.md b/devlog/_plan/260812_five_bug_fix_campaign/050_phase5_issue1419_tls_crash_survivability.md new file mode 100644 index 000000000..081eeb600 --- /dev/null +++ b/devlog/_plan/260812_five_bug_fix_campaign/050_phase5_issue1419_tls_crash_survivability.md @@ -0,0 +1,139 @@ +# 050 — Phase 5 (#1419): a transient TLS failure must not take the process down + +Depends on: 010-040 landed, because this phase is the one whose terminal +outcome may legitimately be a disposition rather than a fix, and it should not +block the four provable fixes. + +## What the evidence supports, and what it does not + +Reported: `EXC_BREAKPOINT / SIGTRAP` on the main thread ~0.5-0.6s after a +connection reset followed by `unknown certificate verification error`, twice, +with matching Bun image UUID and matching main-thread stack offsets. The +dashboard died with the proxy because `ocx gui` serves it from the same process. + +External research (`001`, Lane C): + +- **No** `oven-sh/bun` issue establishes that TLS verification failure or a + socket reset aborts with `SIGTRAP`/`EXC_BREAKPOINT`. +- Current stable Bun is `1.3.14` (2026-05-13) — the version in the report. There + is **no newer stable release to upgrade into**, so "bump Bun" is not available + as a fix. +- Adjacent known defects are hangs and `ECONNRESET`, not aborts. + +So the honest position is: we cannot presently attribute the trap to an +OpenCodex JavaScript path, and we cannot close it by a runtime bump. The +maintainer has already asked the reporter for the full faulting frame list, +which is the correct discriminator between Bun's TLS/fetch implementation and +JavaScriptCore's unhandled-exception path. + +**This phase therefore does not claim to fix the native trap.** It does the +part that is within our authority and is independently valuable. + +## The repository already knows this failure family + +`src/lib/abort.ts`: + +> Bun's HTTP client, when a `fetch(..., { signal })` is aborted AFTER the +> response resolved, tears down the response body stream and rejects any +> in-flight internal read. If our code hasn't attached a reader yet ... Bun +> reports it as `unhandledRejection: TypeError: null is not an object` +> (native-only stack) — **uncatchable by any caller try/catch**. + +`cancelBodyOnAbort` exists precisely to absorb that orphaned rejection by making +us the consumer that settles the body. `src/lib/eventstream-decoder.ts:211` +carries the same note. This is direct in-tree evidence that Bun can surface +**uncatchable** failures originating in HTTP/TLS teardown, and that the working +mitigation is to ensure *we* settle every stream we open rather than relying on +`try/catch`. + +That gives a concrete, testable work item that does not depend on reproducing +the trap. + +## Scope + +IN + +1. **Audit every `fetch` in the request path for an unsettled body on the + failure branch.** Any site that awaits `fetch()` and can leave `response.body` + without a consumer when an error or abort intervenes is a candidate for the + same orphaned-rejection class `cancelBodyOnAbort` was written for. Each site + found gets the existing helper applied — reusing the established mitigation, + not inventing a second one. +2. **Process-level last-resort observability.** A top-level + `process.on("unhandledRejection")` / `uncaughtException` handler that logs an + actionable OpenCodex-side diagnostic (redacted per `privacy:scan` rules — no + URLs with credentials, no bodies) before the runtime decides the process's + fate. This cannot stop a native `SIGTRAP` — nothing in-process can — but it + converts the currently-silent JS-attributable subset into a named error, which + is exactly what the issue asks for as its minimum bar: "at minimum exit with + an actionable OpenCodex error." +3. **Supervision/restart hardening**, only if the service layer does not already + provide it. `src/service.ts` is inspected first; if a supervised restart path + exists, the work is to verify it covers abnormal termination (signal death, + not just `process.exit`) and to record that finding rather than add a second + mechanism. + +OUT + +- Claiming the trap is fixed. +- Vendoring or patching Bun. +- Pinning a Bun version (no newer stable exists). +- Disabling TLS verification anywhere, under any flag. A "fix" that weakens + certificate verification to avoid a crash trades a liveness bug for a security + defect and is refused outright. + +## Activation scenario (C-ACTIVATION-GROUNDING-01) + +For the body-settling work: drive a request whose `fetch` resolves and is then +aborted before a reader attaches, against a local test server, and assert the +process emits no unhandled rejection and the proxy still answers a subsequent +request. The "still answers afterwards" assertion is the one that proves +survival rather than mere absence of a log line. + +For the diagnostic handler: install it, trigger a synthetic unhandled rejection +in a child process running the real entry point, and assert the emitted +diagnostic names the OpenCodex-side context and contains no credential material. + +For supervision: kill a running instance with `SIGTRAP`/`SIGKILL` and observe +whether the supervisor restarts it and whether the dashboard becomes reachable +again — this is the observable that maps directly to the reporter's experience. + +## Terminal-outcome policy for this phase + +- If the audit finds a real unsettled-body site in the request path → fix it, + regression-test it, land it, and report #1419 as **partially addressed** with + the specific path named. The issue stays open pending the crash frames. +- If the audit finds none → the deliverable is the diagnostic handler plus a + documented disposition comment on the issue stating what was checked, what the + external research established (no known Bun fix, already on newest stable), + and precisely which frames would settle attribution. Outcome: **NEEDS_HUMAN** + on the root cause, with the survivability work landed on its own merits. + +Either way, the issue is **not** closed by this unit unless a real in-tree cause +is found and fixed. Closing a crash report without a reproduction or a proven +cause would be exactly the "evidence-free closure" the repository's triage rules +forbid. + +## Accept criteria + +1. The `fetch`-site audit is complete and recorded, naming every site checked + and its settle path. +2. Any unsettled-body site found is fixed and covered by a regression test that + asserts a subsequent request still succeeds. +3. The diagnostic handler emits an actionable, credential-free message. +4. `bun run privacy:scan` exit 0. +5. `bun run typecheck` exit 0; server/service suites green on `ssh lidge`. +6. #1419 receives either a PR link or an evidence-backed disposition comment. + +## Verification commands + +```bash +bun x tsc --noEmit +bun run privacy:scan +bun test tests/server*.test.ts tests/service*.test.ts tests/abort*.test.ts +``` + +## Delivery + +Branch `codex/1419-tls-failure-survivability` if code lands; otherwise a +disposition comment on the issue with the audit table. diff --git a/devlog/_plan/260812_five_bug_fix_campaign/051_phase5_revised_issue1419_disposition.md b/devlog/_plan/260812_five_bug_fix_campaign/051_phase5_revised_issue1419_disposition.md new file mode 100644 index 000000000..16dc1c97b --- /dev/null +++ b/devlog/_plan/260812_five_bug_fix_campaign/051_phase5_revised_issue1419_disposition.md @@ -0,0 +1,93 @@ +# 051 — Phase 5 REVISED (#1419): audit first, disposition, no duplicate hardening + +Supersedes `050`. Written after audit blockers B5, B6, B7. + +## Why 050 was wrong + +`050` proposed three work items. The audit proved that all three already exist +in the tree, and the main agent confirmed each: + +| 050 proposed | Reality | +|---|---| +| add `unhandledRejection` / `uncaughtException` diagnostics | `src/lib/crash-guard.ts:332` `installCrashGuards()` already installs both, records redacted diagnostics, and is invoked at `src/cli/index.ts:265`. It even special-cases the benign Bun abort-teardown rejection at `:156,185`. | +| audit `fetch` sites for unsettled bodies | `cancelBodyOnAbort` is already applied at 8 sites, including the OpenAI Responses path (`src/server/responses/core.ts:3436` and `:3671`), both web-search executors, both vision describers, and `codex/auth-api.ts:1748`. | +| add supervision/restart hardening | launchd `KeepAlive`, systemd `Restart=on-failure`, and Windows restart-on-failure already ship. | + +The reviewer's sharpest point: a test written against an already-guarded path +**cannot go red before the fix**. It would be ceremony, not regression coverage, +and landing it under #1419's number would imply the crash was addressed. + +There is also a hard ceiling the plan under-weighted: **no in-process handler +survives a native `SIGTRAP`**. `crash-guard` catches JS-level failures; the +reporter explicitly observed *no* JS crash log, which is itself evidence that +the fault did not pass through the JS error path. + +## What this phase actually does + +### 1. Audit, and record it + +Enumerate every `fetch` in the request path and record, per site, whether the +response body is settled on the failure/abort branch. + +**Disclosure ordering (audit R2-5).** `AGENTS.md` requires unreleased failure +findings and pre-disclosure patch reasoning to stay in scratch space. So: + +- the working audit table lives in `.tmp/` (gitignored), **not** in `devlog/` + and **not** in a public issue comment; +- the public comment names only protections that already ship and are visible in + public diffs (`installCrashGuards`, the `cancelBodyOnAbort` sites, service + supervision); +- any **unguarded** site discovered is named publicly only after its fix is + merged and therefore already disclosed by the diff. + +If — and only if — an unguarded site is found: + +- it gets `cancelBodyOnAbort` (reuse the existing helper); +- it gets a regression test that is **red before** the change; +- it ships as its own PR, described as hardening found while investigating + #1419, not as a fix for the reported crash. + +### 2. Post an evidence-backed disposition on #1419 + +Contents: + +- what already exists (`crash-guard`, `cancelBodyOnAbort` site list, service + supervision) so the reporter and future triagers stop re-proposing it; +- the external finding: current stable Bun is **1.3.14** (2026-05-13), which is + the version in the report, so **no upstream fix exists to upgrade into**; + adjacent issues [#31894](https://github.com/oven-sh/bun/issues/31894) (stale + pooled socket — hang) and [#17325](https://github.com/oven-sh/bun/issues/17325) + (self-signed CA — error) do **not** establish this abort; +- the discriminator still needed: the full faulting main-thread frame list from + both `.ips` reports, plus the Bun image UUID and load address, to tell Bun's + TLS/fetch implementation apart from JavaScriptCore's unhandled-exception path; +- the one thing the reporter can act on now: `ocx gui` starts an **unsupervised** + background process, so installing the service gives them the restart behavior + they expected. This is a genuine mitigation for the dashboard-death half of + the report, and it is honest about not being a fix for the trap. + +### 3. Do not close the issue + +Terminal outcome for this phase: **NEEDS_HUMAN** on root cause. The issue stays +open awaiting crash frames. Closing a crash report with no reproduction and no +proven cause is exactly the evidence-free closure the repository's triage rules +forbid. + +## Explicitly refused + +- Weakening or bypassing TLS certificate verification anywhere, under any flag. + That trades a liveness bug for a security defect. +- A second crash handler alongside `installCrashGuards`. +- Any test that re-proves `cancelBodyOnAbort` on an already-guarded path. + +## Accept criteria + +1. The complete `fetch`-site audit table exists in `.tmp/` with every site's + settle path named. It is never committed and never pasted publicly. +2. Any unguarded site found ships with a red-before test, in its own PR. +3. The disposition comment is posted containing a **public shipped-protections + table** (only protections already visible in public diffs), the Bun evidence + with URLs, the frame-list request, and the service-install mitigation. + Unshipped findings stay in `.tmp/` until their fix merges (audit R2-5, R3-3). +4. `bun run privacy:scan` exit 0 if any code lands. +5. #1419 remains open, labeled to reflect that it is awaiting reporter evidence. diff --git a/src/adapters/openai-chat.ts b/src/adapters/openai-chat.ts index c3eed13c3..99b62af5e 100644 --- a/src/adapters/openai-chat.ts +++ b/src/adapters/openai-chat.ts @@ -178,6 +178,29 @@ function invalidToolCallsEvent(usage?: OcxUsage): Extract { + return { + type: "error", + message: "upstream streamed a tool call without a function name — cannot dispatch", + ...(usage !== undefined ? { usage } : {}), + }; +} + function isRecord(value: unknown): value is Record { return value !== null && typeof value === "object" && !Array.isArray(value); } @@ -897,13 +920,27 @@ export function createOpenAIChatAdapter(provider: OcxProviderConfig): ProviderAd pendingToolCalls.length = 0; return calls; }; - const flushToolCalls = function* (): Generator { + // Returns "terminate" when a pending call cannot be dispatched, so every flush site + // stops the turn instead of emitting an unusable call. `closeToolCalls()` runs first, + // so budget reservations are released for every pending call even on the early return. + const flushToolCalls = function* (): Generator { for (const call of closeToolCalls()) { + // Ingest already proved `name` is a string; the typeof guard keeps this branch + // total so a future ingest change cannot turn a malformed name into a throw. + if (typeof call.name !== "string" || call.name.trim().length === 0) { + debugProviderDiagnostic("openai-chat", "tool-call-unnamed", { + hadId: call.id.length > 0, + argsBytes: call.argsBytes, + }); + yield unnamedToolCallEvent(pendingUsage); + return "terminate"; + } if (!call.id) call.id = `call_${++toolCallSeq}`; yield { type: "tool_call_start", id: call.id, name: call.name }; if (call.args.length > 0) yield { type: "tool_call_delta", arguments: call.args }; yield { type: "tool_call_end" }; } + return "continue"; }; const terminateWithError = function* ( event: Extract, @@ -922,7 +959,7 @@ export function createOpenAIChatAdapter(provider: OcxProviderConfig): ProviderAd const payload = rawPayload.trim(); if (payload.length === 0) return "continue"; if (payload === "[DONE]") { - yield* flushToolCalls(); + if ((yield* flushToolCalls()) === "terminate") return "terminate"; const stopReason = stopReasonFor(finishReason); yield { type: "done", usage: pendingUsage, ...(stopReason ? { stopReason } : {}) }; return "terminate"; @@ -992,6 +1029,25 @@ export function createOpenAIChatAdapter(provider: OcxProviderConfig): ProviderAd id?: string; function?: { name?: string; arguments?: string }; }; + // That cast is a TypeScript convenience, not a runtime guarantee: this is + // upstream JSON. Validate the fields before they are stored, so a non-string + // name or arguments value fails closed through the #1325 channel here rather + // than escaping later as a TypeError from string handling at flush time. + const rawFunction = (rawToolCall as { function?: unknown }).function; + if (rawFunction !== undefined && rawFunction !== null) { + if (!isRecord(rawFunction)) { + return yield* terminateWithError(invalidToolCallsEvent(pendingUsage)); + } + const rawName = rawFunction.name; + const rawArguments = rawFunction.arguments; + if ((rawName !== undefined && typeof rawName !== "string") + || (rawArguments !== undefined && typeof rawArguments !== "string")) { + return yield* terminateWithError(invalidToolCallsEvent(pendingUsage)); + } + } + if (tc.id !== undefined && typeof tc.id !== "string") { + return yield* terminateWithError(invalidToolCallsEvent(pendingUsage)); + } const key = typeof tc.index === "number" ? `i:${tc.index}` : tc.id @@ -1025,7 +1081,9 @@ export function createOpenAIChatAdapter(provider: OcxProviderConfig): ProviderAd } } - if (typeof choice.finish_reason === "string" && choice.finish_reason) yield* flushToolCalls(); + if (typeof choice.finish_reason === "string" && choice.finish_reason) { + if ((yield* flushToolCalls()) === "terminate") return "terminate"; + } return "continue"; }; @@ -1085,7 +1143,7 @@ export function createOpenAIChatAdapter(provider: OcxProviderConfig): ProviderAd yield { type: "error", message: "upstream stream ended without a terminal signal ([DONE] or finish_reason) — possible truncation" }; return; } - yield* flushToolCalls(); + if ((yield* flushToolCalls()) === "terminate") return; const stopReason = stopReasonFor(finishReason); yield { type: "done", usage: pendingUsage, ...(stopReason ? { stopReason } : {}) }; } catch (error) { @@ -1156,7 +1214,11 @@ export function createOpenAIChatAdapter(provider: OcxProviderConfig): ProviderAd const id = rawToolCall.id; const name = rawToolCall.function.name; const args = rawToolCall.function.arguments; - if (typeof id !== "string" || typeof name !== "string" || typeof args !== "string") { + // A blank name is as undispatchable as a missing one, so it fails closed here + // for the same reason the streamed path refuses it. Trimmed length, not `!name`: + // a whitespace-only function name is not a legitimate tool-call shape either. + if (typeof id !== "string" || typeof name !== "string" || typeof args !== "string" + || name.trim().length === 0) { return [invalidToolCallsEvent(usage)]; } events.push({ type: "tool_call_start", id, name }); diff --git a/tests/openai-chat-eof.test.ts b/tests/openai-chat-eof.test.ts index 4031b1e0a..f87ff4c89 100644 --- a/tests/openai-chat-eof.test.ts +++ b/tests/openai-chat-eof.test.ts @@ -2,7 +2,7 @@ import { describe, expect, test } from "bun:test"; import { createOpenAIChatAdapter as createOpenAIChatAdapterProduction } from "../src/adapters/openai-chat"; import { bridgeToResponsesSSE } from "../src/bridge"; import type { AdapterEvent } from "../src/types"; -import { withTestTranslatorBudget } from "./helpers/translator-budget"; +import { createTestTranslatorBudget, withTestTranslatorBudget } from "./helpers/translator-budget"; const createOpenAIChatAdapter = (...args: Parameters) => withTestTranslatorBudget(createOpenAIChatAdapterProduction(...args)); @@ -293,3 +293,167 @@ describe("openai-chat EOF mid tool call (#735)", () => { expect(events.at(-1)?.type).toBe("done"); }); }); + +describe("openai-chat unnamed tool calls fail closed (#1514)", () => { + // The reported OpenCode Zen / DeepSeek shape: argument deltas arrive, the function name + // never does, and the stream reaches a normal terminal boundary. Emitting that call hands + // the Codex tool-call contract something it cannot dispatch and the turn breaks downstream. + const unnamedDelta = + 'data: {"choices":[{"delta":{"tool_calls":[{"index":0,"id":"call_x","function":{"arguments":"{\\"a\\":1}"}}]}}]}\n\n'; + + function errorMessage(events: AdapterEvent[]): string { + const last = events.at(-1); + return last && last.type === "error" ? last.message : ""; + } + + test("terminated by [DONE]: error, no tool_call_start, no done", async () => { + const response = new Response([unnamedDelta, "data: [DONE]\n\n"].join("")); + const events = await collect(createOpenAIChatAdapter(provider).parseStream(response)); + expect(events.some(e => e.type === "tool_call_start")).toBe(false); + expect(events.some(e => e.type === "done")).toBe(false); + expect(errorMessage(events)).toContain("without a function name"); + }); + + test("terminated by finish_reason: error, no tool_call_start, no done", async () => { + const response = new Response([ + unnamedDelta, + 'data: {"choices":[{"delta":{},"finish_reason":"tool_calls"}]}\n\n', + "data: [DONE]\n\n", + ].join("")); + const events = await collect(createOpenAIChatAdapter(provider).parseStream(response)); + expect(events.some(e => e.type === "tool_call_start")).toBe(false); + expect(events.some(e => e.type === "done")).toBe(false); + expect(errorMessage(events)).toContain("without a function name"); + }); + + test("a whitespace-only name is rejected like a missing one", async () => { + const response = new Response([ + 'data: {"choices":[{"delta":{"tool_calls":[{"index":0,"id":"call_x","function":{"name":" ","arguments":"{}"}}]}}]}\n\n', + "data: [DONE]\n\n", + ].join("")); + const events = await collect(createOpenAIChatAdapter(provider).parseStream(response)); + expect(events.some(e => e.type === "tool_call_start")).toBe(false); + expect(errorMessage(events)).toContain("without a function name"); + }); + + test("a name arriving in a later chunk is still accepted", async () => { + // The guard must not reject a call whose name is simply late: that is the ordinary + // OpenAI streaming shape, where the first chunk may carry only id and arguments. + const response = new Response([ + unnamedDelta, + 'data: {"choices":[{"delta":{"tool_calls":[{"index":0,"function":{"name":"shell"}}]}}]}\n\n', + "data: [DONE]\n\n", + ].join("")); + const events = await collect(createOpenAIChatAdapter(provider).parseStream(response)); + expect(events.some(e => e.type === "error")).toBe(false); + expect(events.filter(e => e.type === "tool_call_end")).toHaveLength(1); + expect(events.at(-1)?.type).toBe("done"); + }); + + test("an unnamed call without any terminal signal still reports truncation", async () => { + // Raw EOF keeps its own fail-closed error: the truncation branch runs before the + // post-loop flush, so the new guard must not steal that diagnosis. + const response = new Response(unnamedDelta); + const events = await collect(createOpenAIChatAdapter(provider).parseStream(response)); + expect(events.some(e => e.type === "done")).toBe(false); + expect(errorMessage(events)).toContain("mid tool call"); + }); + + test("a non-array tool_calls payload still reports the #1325 error", async () => { + const response = new Response([ + 'data: {"choices":[{"delta":{"tool_calls":"nope"}}]}\n\n', + "data: [DONE]\n\n", + ].join("")); + const events = await collect(createOpenAIChatAdapter(provider).parseStream(response)); + expect(errorMessage(events)).toContain("invalid tool calls"); + }); + + // The streamed tool-call shape is upstream JSON behind a TypeScript cast, so a truthy + // non-string name reaches the accumulator unvalidated. Before ingest validation it was + // stored and then thrown on at flush time as `call.name.trim is not a function` — an + // uncatchable-looking TypeError instead of the #1325 terminal error. + test("a non-string function name terminates instead of throwing", async () => { + const response = new Response([ + 'data: {"choices":[{"delta":{"tool_calls":[{"index":0,"id":"call_x","function":{"name":123,"arguments":"{}"}}]}}]}\n\n', + "data: [DONE]\n\n", + ].join("")); + const events = await collect(createOpenAIChatAdapter(provider).parseStream(response)); + expect(errorMessage(events)).toContain("invalid tool calls"); + expect(events.some(e => e.type === "tool_call_start")).toBe(false); + expect(events.some(e => e.type === "done")).toBe(false); + }); + + test("non-string arguments and a non-record function both terminate", async () => { + const badArgs = new Response([ + 'data: {"choices":[{"delta":{"tool_calls":[{"index":0,"id":"call_x","function":{"name":"shell","arguments":42}}]}}]}\n\n', + "data: [DONE]\n\n", + ].join("")); + expect(errorMessage(await collect(createOpenAIChatAdapter(provider).parseStream(badArgs)))) + .toContain("invalid tool calls"); + + const badFunction = new Response([ + 'data: {"choices":[{"delta":{"tool_calls":[{"index":0,"id":"call_x","function":"shell"}]}}]}\n\n', + "data: [DONE]\n\n", + ].join("")); + expect(errorMessage(await collect(createOpenAIChatAdapter(provider).parseStream(badFunction)))) + .toContain("invalid tool calls"); + }); + + // Terminating mid-flush must not strand the reservations of the calls that were never + // emitted. `closeToolCalls()` snapshots and closes every pending key before iteration, + // so the call AFTER the offender is released too — assert that against the budget + // itself rather than inferring it from the emitted events. + test("terminating mid-flush releases every pending call's budget", async () => { + const budget = createTestTranslatorBudget(); + const named = (index: number, id: string, name: string) => + `data: {"choices":[{"delta":{"tool_calls":[{"index":${index},"id":"${id}","function":{"name":"${name}","arguments":"{\\"padding\\":\\"aaaaaaaaaaaaaaaaaaaa\\"}"}}]}}]}\n\n`; + const unnamed = (index: number, id: string) => + `data: {"choices":[{"delta":{"tool_calls":[{"index":${index},"id":"${id}","function":{"arguments":"{\\"padding\\":\\"bbbbbbbbbbbbbbbbbbbb\\"}"}}]}}]}\n\n`; + + const response = new Response([ + named(0, "call_ok", "shell"), + unnamed(1, "call_bad"), + named(2, "call_after", "read"), + "data: [DONE]\n\n", + ].join("")); + + const events = await collect( + createOpenAIChatAdapterProduction(provider).parseStream(response, budget), + ); + + const snapshot = budget.snapshot(); + expect(snapshot.activeCalls).toBe(0); + expect(snapshot.currentBytes).toBe(0); + expect(snapshot.highWaterBytes).toBeGreaterThan(0); + + // The call after the offender is never emitted, and the turn ends on the error. + const started = events.filter(e => e.type === "tool_call_start"); + expect(started.some(e => e.type === "tool_call_start" && e.id === "call_after")).toBe(false); + expect(events.at(-1)?.type).toBe("error"); + expect(events.some(e => e.type === "done")).toBe(false); + }); + + test("buffered response with a blank function name fails closed", async () => { + const response = new Response( + JSON.stringify({ + choices: [{ message: { tool_calls: [{ id: "call_x", type: "function", function: { name: "", arguments: "{}" } }] } }], + }), + { headers: { "content-type": "application/json" } }, + ); + const events = await createOpenAIChatAdapter(provider).parseResponse!(response); + expect(events.some(e => e.type === "tool_call_start")).toBe(false); + expect(events.some(e => e.type === "error")).toBe(true); + }); + + test("buffered response with a valid function name is unchanged", async () => { + const response = new Response( + JSON.stringify({ + choices: [{ message: { tool_calls: [{ id: "call_x", type: "function", function: { name: "shell", arguments: "{}" } }] } }], + }), + { headers: { "content-type": "application/json" } }, + ); + const events = await createOpenAIChatAdapter(provider).parseResponse!(response); + expect(events.some(e => e.type === "error")).toBe(false); + expect(events.filter(e => e.type === "tool_call_end")).toHaveLength(1); + }); +}); diff --git a/tests/openai-chat-parallel-stream.test.ts b/tests/openai-chat-parallel-stream.test.ts index 6706575bb..7c7c9e01b 100644 --- a/tests/openai-chat-parallel-stream.test.ts +++ b/tests/openai-chat-parallel-stream.test.ts @@ -141,12 +141,21 @@ describe("openai-chat parallel tool call stream assembly", () => { expect(assembled(events)).toEqual([{ id: "late", name: "late_name", args: "{\"z\":9}" }]); }); - test("T7: name never arrives - call still flushed with empty name (parity, no silent drop)", async () => { + // Previously this asserted the call was flushed with an empty name, on a "no silent drop" + // rationale. That invariant still holds and is now stronger: an unnamed call cannot vanish + // silently, because the turn fails loudly instead. What changed is the mechanism — emitting + // a call the Codex tool-call contract cannot dispatch was never a usable outcome (#1514). + test("T7: name never arrives - turn fails closed instead of emitting an undispatchable call", async () => { const events = await collect(sse([ chunkOf([{ index: 0, id: "anon", function: { arguments: "{\"q\":1}" } }]), chunkOf([], "tool_calls"), ])); - expect(assembled(events)).toEqual([{ id: "anon", name: "", args: "{\"q\":1}" }]); + expect(assembled(events)).toEqual([]); + expect(events.some(e => e.type === "tool_call_start")).toBe(false); + const last = events.at(-1); + expect(last?.type).toBe("error"); + expect(last && last.type === "error" ? last.message : "").toContain("without a function name"); + expect(events.some(e => e.type === "done")).toBe(false); }); test("T8: text deltas interleaved mid-assembly pass through and never split a call", async () => {