From 3792b371c4b5b9378e16666b41b18a58618578f1 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Wed, 12 Aug 2026 19:30:09 +0900 Subject: [PATCH 1/2] docs(devlog): plan the 260812 five-bug campaign Roadmap unit for issues #1514, #1503, #1497, #1409, #1419, grounded in the live tree and closed through three independent audit rounds. Two phases were superseded by the audit: - 030 -> 031: the proposed rangeFullyCovered field was unsound. usage.jsonl appends on request completion while entries carry the request start time, so the oldest retained row does not bound the dropped prefix. - 050 -> 051: installCrashGuards() and cancelBodyOnAbort already ship at 8 sites, so the proposed hardening was a re-proposal of existing code. #1497 and #1409 are downgraded from Closes to evidence-backed dispositions: neither fix meets the reporter's acceptance bar on its own. --- .../000_research.md | 258 ++++++++++++++++ .../001_external_contract_evidence.md | 106 +++++++ .../002_audit_round1_synthesis.md | 172 +++++++++++ .../003_audit_round2_synthesis.md | 129 ++++++++ .../010_phase1_issue1514_empty_tool_name.md | 274 +++++++++++++++++ ...20_phase2_issue1503_google_thought_text.md | 190 ++++++++++++ ...phase3_issue1497_usage_range_truncation.md | 180 +++++++++++ .../031_phase3_revised_usage_coverage.md | 116 +++++++ ...ase4_issue1409_context_window_overrides.md | 282 ++++++++++++++++++ ...hase5_issue1419_tls_crash_survivability.md | 139 +++++++++ ...51_phase5_revised_issue1419_disposition.md | 93 ++++++ 11 files changed, 1939 insertions(+) create mode 100644 devlog/_plan/260812_five_bug_fix_campaign/000_research.md create mode 100644 devlog/_plan/260812_five_bug_fix_campaign/001_external_contract_evidence.md create mode 100644 devlog/_plan/260812_five_bug_fix_campaign/002_audit_round1_synthesis.md create mode 100644 devlog/_plan/260812_five_bug_fix_campaign/003_audit_round2_synthesis.md create mode 100644 devlog/_plan/260812_five_bug_fix_campaign/010_phase1_issue1514_empty_tool_name.md create mode 100644 devlog/_plan/260812_five_bug_fix_campaign/020_phase2_issue1503_google_thought_text.md create mode 100644 devlog/_plan/260812_five_bug_fix_campaign/030_phase3_issue1497_usage_range_truncation.md create mode 100644 devlog/_plan/260812_five_bug_fix_campaign/031_phase3_revised_usage_coverage.md create mode 100644 devlog/_plan/260812_five_bug_fix_campaign/040_phase4_issue1409_context_window_overrides.md create mode 100644 devlog/_plan/260812_five_bug_fix_campaign/050_phase5_issue1419_tls_crash_survivability.md create mode 100644 devlog/_plan/260812_five_bug_fix_campaign/051_phase5_revised_issue1419_disposition.md 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. From 15c6b53ca5ee7b67c0817c42ac60a825b8b1c57d Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Wed, 12 Aug 2026 19:55:39 +0900 Subject: [PATCH 2/2] fix(openai-chat): refuse to emit an unnamed streamed tool call An upstream that streams function.arguments deltas without ever sending function.name left the pending call with an empty name, and every flush site emitted it anyway. The Codex tool-call contract then received a call it cannot dispatch and the turn failed downstream (#1514). Fail closed instead. The id stays synthesizable because it is an opaque correlation handle; a function name is a guess at intent, so an unnamed call terminates the turn through the adapter error channel. Dropping it silently was not an option either: #1325 established that a claimed call which disappears can orphan the matching result on the next turn. Ingest now validates the streamed shape at runtime. The tool_calls cast is a TypeScript convenience over upstream JSON, so a truthy non-string name reached the accumulator and later threw from string handling; that path now terminates through the existing invalid-tool-calls error. The buffered parseResponse validator checked the type of name but not its emptiness, so it is tightened the same way. Closes #1514 --- src/adapters/openai-chat.ts | 72 +++++++++- tests/openai-chat-eof.test.ts | 166 +++++++++++++++++++++- tests/openai-chat-parallel-stream.test.ts | 13 +- 3 files changed, 243 insertions(+), 8 deletions(-) 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 () => {