From 63fbe6690718ece8e42b2b7d5eae770cb490d70b Mon Sep 17 00:00:00 2001 From: JUN Date: Mon, 14 Sep 2026 02:38:34 +0900 Subject: [PATCH 1/3] [agent] fix: normalize inbound Chat images before route selection and preserve an explicit reasoning disable The native Chat fast path recognized only `image_url` parts, while the translated path also understood Pi/MCP `{type:"image", data, mimeType}` and Anthropic-shaped `{type:"image", source}` parts. Two failures followed from that single gap: a text-only routed model kept an image-bearing body because `isNativeChatRouteEligible` could not see the image, and the native whitelist passthrough forwarded the foreign part verbatim to an OpenAI-compatible upstream that does not accept it. Recognition now lives once in `src/chat/image-parts.ts`, and `normalizeChatImageParts` runs before `routeModel` so the diversion decision and the forwarded wire observe the same parts. A body with no foreign image part is returned by reference and stays byte-identical. A remote reference is recognized and rewritten, never fetched. Separately, the Chat inbound effort allowlist dropped `none`. That is the runtime's disable sentinel, not an unknown value: `src/reasoning-effort.ts` maps it to omitting the wire parameter and the Pi export maps Pi's `off` level onto it. Dropping it let a provider default re-enable thinking the caller had turned off, which is not neutral for Anthropic families that think by default. Audit findings F1 and F7 (2026-09-14). Local verification NOT RUN BY USER INSTRUCTION. --- .../260914_provider_parity_stack/000_plan.md | 128 +++++++++ .../001_audit_evidence.md | 260 ++++++++++++++++++ .../002_architect_dispositions.md | 127 +++++++++ .../003_blocker_corrections.md | 161 +++++++++++ .../010_phase1_ingress_normalization.md | 174 ++++++++++++ .../020_phase2_chat_responses_controls.md | 172 ++++++++++++ .../030_phase3_provider_wire_contracts.md | 176 ++++++++++++ .../040_phase4_modality_fidelity.md | 207 ++++++++++++++ .../050_residuals.md | 58 ++++ scripts/test-layout/layout.json | 2 + src/chat/image-parts.ts | 118 ++++++++ src/chat/inbound.ts | 44 +-- src/server/chat-completions.ts | 7 +- src/server/chat-native.ts | 14 +- structure/data-planes/inbound-compat.md | 30 ++ tests/fixtures/test-layout-expected.json | 2 + .../chat-inbound-reasoning-none.test.ts | 56 ++++ .../chat-native-image-normalization.test.ts | 163 +++++++++++ 18 files changed, 1852 insertions(+), 47 deletions(-) create mode 100644 devlog/_plan/260914_provider_parity_stack/000_plan.md create mode 100644 devlog/_plan/260914_provider_parity_stack/001_audit_evidence.md create mode 100644 devlog/_plan/260914_provider_parity_stack/002_architect_dispositions.md create mode 100644 devlog/_plan/260914_provider_parity_stack/003_blocker_corrections.md create mode 100644 devlog/_plan/260914_provider_parity_stack/010_phase1_ingress_normalization.md create mode 100644 devlog/_plan/260914_provider_parity_stack/020_phase2_chat_responses_controls.md create mode 100644 devlog/_plan/260914_provider_parity_stack/030_phase3_provider_wire_contracts.md create mode 100644 devlog/_plan/260914_provider_parity_stack/040_phase4_modality_fidelity.md create mode 100644 devlog/_plan/260914_provider_parity_stack/050_residuals.md create mode 100644 src/chat/image-parts.ts create mode 100644 tests/server/chat-inbound-reasoning-none.test.ts create mode 100644 tests/server/chat-native-image-normalization.test.ts diff --git a/devlog/_plan/260914_provider_parity_stack/000_plan.md b/devlog/_plan/260914_provider_parity_stack/000_plan.md new file mode 100644 index 0000000000..8cc5224e3f --- /dev/null +++ b/devlog/_plan/260914_provider_parity_stack/000_plan.md @@ -0,0 +1,128 @@ +# Provider parity stack — plan + +Unit opened 2026-09-14. Base `origin/dev` `df7dc1be53`. Worktree +`/Users/jun/.codex/worktrees/provider-parity-260914/opencodex`. + +## Objective + +Fix the provider-compatibility defects independently reproduced in the 2026-09-14 +audit, and publish them as a dependency-ordered manual stack of four pull requests. +Every layer is standalone: its own thesis, its own tests, its own docs. + +Evidence and source anchors are in [`001_audit_evidence.md`](001_audit_evidence.md). +Architect proposals and their dispositions are in +[`002_architect_dispositions.md`](002_architect_dispositions.md). + +**[`003_blocker_corrections.md`](003_blocker_corrections.md) is authoritative over +every decade doc below.** Independent review found material errors in the first +draft — F2 was corrected from an ingress strip to final-target sanitization, F3 from +silently ignoring a schema to an explicit error, F5 from a claimed fix to an explicit +residual, F9 from a one-branch fix to both branches, F8 to a deliberate ordering +change. Read it before executing any layer. + +Deliberate non-coverage is in [`050_residuals.md`](050_residuals.md). + +## Constraints + +- Branches `agent/provider-parity-*` only, in this worktree. The original checkout + and every other open PR stay untouched. +- Commits use an `[agent] :` subject prefix and explicit staging. No + `[skip ci]`, no workflow-file edits, no dependency or lockfile edits. +- No merge, no release, no self-approval. A layer whose full gate cannot be + obtained is published as a draft and reported as such. +- Desired-behavior regressions go red before they go green. The prior audit probe + suite asserts the defects and is not reused as the fix gate. +- No new remote fetch is introduced on any request path. + +## Stack topology + +Four layers, merged bottom-up. Each child's base is the preceding open parent head. + +| # | Branch | Base | Thesis | Findings | +|---|--------|------|--------|----------| +| 1 | `agent/provider-parity-01-ingress` | `dev` | Normalize inbound Chat images before route selection; preserve an explicit reasoning disable | F1, F7 | +| 2 | `agent/provider-parity-02-controls` | layer 1 | Scope the Responses control strip to canonical ChatGPT; carry assistant reasoning and penalties through translation | F2, F6 | +| 3 | `agent/provider-parity-03-wire` | layer 2 | Google structured output onto the `generateContent` wire; Anthropic parallel-tool disable | F3, F4 | +| 4 | `agent/provider-parity-04-modality` | layer 3 | Preserve tool-result images, and refuse unsupported modalities explicitly instead of silently | F8, F5, F9, Kiro | + +## Dependency order + +Two different things order these layers, and the distinction is stated rather than +blurred (architect D8, `002_architect_dispositions.md`). + +**Real source dependency — layers 1 and 2.** Layer 1 owns the inbound boundary: it +decides which pipeline a Chat request enters and what an effort value means once it +is inside. Layer 2 edits the same two files: both change +`src/server/chat-completions.ts` and `src/chat/inbound.ts`, so layer 2 cannot be +reviewed or merged independently of layer 1. + +**Serialization, not dependency — layers 3 and 4.** Layer 3 reads +`options.textFormat` and `options.parallelToolCalls`, neither of which layers 1-2 +touch; at source level it could open against `dev` in parallel. It is stacked +because every layer edits the same two test-registration files, and layers 2-4 all +edit `structure/providers/chat-compat.md` — four parallel PRs would conflict on +each of them. Layer 3's PR body states this plainly instead of implying a +dependency it does not have. + +Layer 4 is last on its own merit: an explicit refusal is only honest once the +preceding layers have stopped losing payloads for unrelated reasons. + +Files touched by more than one layer: + +| File | Layers | +|---|---| +| `src/server/chat-completions.ts` | 1, 2 | +| `src/chat/inbound.ts` | 1, 2 | +| `scripts/test-layout/layout.json` | 1, 2, 3, 4 | +| `tests/fixtures/test-layout-expected.json` | 1, 2, 3, 4 | +| `structure/providers/chat-compat.md` | 2, 3, 4 | + +`src/adapters/openai-chat.ts` and `src/responses/parser-content.ts` are touched by +layer 4 only. + +## Work-phase map + +| Work phase | Cycle | Output | +|---|---|---| +| wp1 | docs only | this unit; no production patch | +| wp2 | layer 1 | branch, tests, docs, PR | +| wp3 | layer 2 | branch, tests, docs, PR | +| wp4 | layer 3 | branch, tests, docs, PR | +| wp5 | layer 4 | branch, tests, docs, PR | + +## Verification status + +**No local product check runs on this Mac, by standing user instruction.** The full +status table, the coordinator's baseline observations at `df7dc1be53`, and what +`structure:check` does and does not observe are in +[`003_blocker_corrections.md`](003_blocker_corrections.md) §C0. + +In short: every gate for this unit's changes is **NOT RUN BY USER INSTRUCTION** and +is never reported as passing or provisional. Layers publish as DRAFT. Evidence comes +from hosted GitHub Actions at the exact pushed head and from independent static +review. Red-first execution is impossible under this restriction, so regressions are +written to assert desired behavior and reviewed statically. + +## Source-of-truth sync + +`structure/INDEX.md` maps each changed source area to the docs that must move with +it. The bindings this unit will touch: + +- `src/chat/` and `src/server/` -> `structure/data-planes/inbound-compat.md` +- `src/adapters/` -> `structure/providers/chat-compat.md`, `structure/adapters/registry.md` +- Google -> `structure/providers/google.md` +- Kiro -> `structure/providers/kiro.md` + +Public user-visible behavior changes also update `docs-site/`. + +## Out of scope + +- `#4501` / PR `#4511` (operator `modelCapabilities` text-only in the native + describer, audit F10). Already owned elsewhere; this unit must not duplicate it. +- `#4505` gateway modality metadata — the audit found a display/policy + inconsistency, not evidence of that gateway's native vision behavior. +- `#4513` Devin image passthrough — already fixed. +- `#4528` — adjacent to F2; this unit fixes the adapter-scope defect, not that PR's subject. +- Cursor native/external image path differences — not confirmed as a real loss. +- Qoder's deliberate image refusal and the CodeBuddy/Qoder vendor-tools-disabled + policy. Both are intended behavior and stay. diff --git a/devlog/_plan/260914_provider_parity_stack/001_audit_evidence.md b/devlog/_plan/260914_provider_parity_stack/001_audit_evidence.md new file mode 100644 index 0000000000..5a50a85c2b --- /dev/null +++ b/devlog/_plan/260914_provider_parity_stack/001_audit_evidence.md @@ -0,0 +1,260 @@ +# Audit evidence and vendor contracts + +Research for the provider parity stack. Source anchors and external contracts only; +the diffs live in the decade documents. + +## Provenance + +The 2026-09-14 audit classified 93 providers across 13 base adapters at `dev` +`cb2e15ba6f`, using static tracing and offline mocks with no live account inference. +Its confirmed findings are at +`/Users/jun/.aside/u/0/opencodex-provider-audit-20260914-012106/AUDIT-SUMMARY.ko.md`, +with reproduction output in `probe-results.json` and `codex-audio-probe.json`. + +The individual reports behind that summary contain speculative candidates and some +wrong scope estimates. Only the independently reproduced table is treated as input +here, and every anchor below was re-read in this worktree at `df7dc1be53`. + +A passing probe in that suite means the defect reproduces. Those probes are not +reused as this unit's acceptance gate: they assert current behavior, so they would +pass before a fix and fail after it. + +## Confirmed findings + +### F1 — native Chat image recognition is narrower than the translated path + +`isNativeChatRouteEligible` diverts an image-bearing body away from the native fast +path when the routed model is text-only (`src/server/chat-native.ts:155`), but the +predicate it calls only recognizes `image_url` +(`chatBodyCarriesImage`, `src/server/chat-native.ts:168-177`). + +The translated path is strictly wider. `imageUrlFromPart` +(`src/chat/inbound.ts:48-79`) also accepts Pi/MCP-style `{type:"image", data, +mimeType}` parts and Anthropic-shaped `{type:"image", source:{...}}` parts, in both +base64 and URL form. + +Two consequences follow from the same gap. A text-only routed model keeps a body +carrying a Pi or Anthropic image, because the eligibility check cannot see it. And +because the native path is a whitelist passthrough +(`buildOpenAIChatPassthroughRequest`, `src/adapters/openai-chat.ts:115-134`), the +non-OpenAI-shaped part is forwarded to the upstream verbatim rather than in the +`image_url` form an OpenAI-compatible endpoint accepts. + +Probe: `probe-results.json` `F1` records `nativeEligible:true` for two `image` +parts and `false` for `image_url`. + +### F7 — an explicit reasoning disable is dropped at the Chat boundary + +`OUTPUT_CONFIG_EFFORTS` (`src/chat/inbound.ts:28`) is the allowlist +`resolveReasoningEffort` filters against (`src/chat/inbound.ts:243-253`). It holds +`minimal` through `ultra` and omits `none`. + +`none` is a real sentinel elsewhere in the runtime, not an unknown string. +`src/reasoning-effort.ts:41` accepts it as a valid effort and `:196` maps it to +"omit the reasoning parameter". The Pi client export depends on that meaning: +`src/clients/config-export.ts:909-916` maps Pi's `off` level to `none`. + +So a Pi user who turns thinking off sends `reasoning_effort:"none"`, the allowlist +drops it as if nothing was requested, and a provider default takes over. For +Anthropic families that think by default, omission is not neutral — +`src/adapters/anthropic.ts:960-966` documents that `"none" is not the same as +absent`, because only an explicit `thinking:{type:"disabled"}` turns thinking off. + +Probe: `probe-results.json` `F7` — the Responses ingress yields +`thinking:{type:"disabled"}`, the Chat ingress yields `thinking:{type:"adaptive"}` +and an output config of `effort:"high"` from the same caller intent. + +### F2 — the Responses control strip is adapter-wide, not ChatGPT-scoped + +`src/server/chat-completions.ts:223-230` deletes `max_output_tokens`, +`temperature`, `top_p`, `stop` and `user` whenever +`settledRoute?.provider.adapter === "openai-responses"`, with the comment +"ChatGPT backend rejects store:true and unsupported sampling knobs". + +The restriction is real for the canonical ChatGPT backend. The condition is not: +`provider-inventory.json` lists seven providers on that adapter — `openai`, +`openai-apikey`, `meta-model`, `meta-muse`, `zai`, +`zhipu-bigmodel-responses`, `volcengine-agent-plan`. A generic API-key Responses +endpoint loses the caller's output cap and sampling controls for no upstream reason. + +Probe: `probe-results.json` `F2` — same request, Responses ingress keeps +`max_output_tokens:123 / temperature:0.2 / top_p:0.8`, Chat ingress yields null for +all three. + +`stop` is deliberately not treated as universally supported: it is not part of the +claim this unit makes. + +### F6 — translated Chat loses assistant reasoning and penalties + +`assistantContentToBlocks` (`src/chat/inbound.ts:121-137`) keeps `text` and +`output_text` only. An assistant turn's `reasoning_content` or +`reasoning_details` is dropped before the Responses projection exists. + +The outbound direction is already implemented: `src/adapters/openai-chat.ts:800-843` +reconstructs `reasoning_content` or `reasoning_details` for providers listed in +`preserveReasoningContentModels`, falling back to a replay cache. So the runtime +can express the field; the inbound translation is the asymmetry. + +Penalties are the second half. `src/responses/schema.ts:162-163` accepts +`presence_penalty` and `frequency_penalty`, `src/responses/parser.ts:544-545` +parses them into `options.presencePenalty`/`frequencyPenalty`, and +`src/adapters/openai-chat.ts:1600-1603` writes them back to the wire. The Chat +inbound body builder (`src/chat/inbound.ts:337-362`) never copies them, so the +chain is broken only at its first link. + +Probe: `probe-results.json` `F6` — `projectedPenaltyPresent:false` while the +native path reports `nativePenalty:0.4` and `nativeReasoning:"prior analysis"`. + +Boundary: a thinking signature and cross-provider opaque reasoning metadata are not +representable from a plain Chat string and must never be forged. Only plaintext and +the numeric controls are in scope; opaque replay is recorded as residual. + +### F3 — Google structured output never reaches the wire + +`src/adapters/google.ts:816-849` builds `generationConfig` from +`maxOutputTokens`, `temperature`, `topP`, `stopSequences`, `thinkingConfig` +and `responseModalities`. It never reads `parsed.options.textFormat`, which the +Responses parser populates at `src/responses/parser.ts:561-562`. + +`compileGenerationConfig` (`src/adapters/google-wire-compiler.ts:118-151`) +whitelists the same six keys, so the defect is two-layer: adding a field in the +adapter alone would still be dropped before the wire. + +Probe: `probe-results.json` `F3` — a well-formed `irFormat` with +`googleGenerationConfig:null` and `compiledGenerationConfig:null`. + +Google `tool_choice` is already implemented +(`toolChoiceToGeminiToolConfig`, used at `src/adapters/google.ts:822-823`); the +individual reports' claim that it is missing is not adopted. + +### F4 — Anthropic parallel-tool disable is not mapped + +`src/adapters/anthropic.ts:1015-1022` maps `toolChoice` onto Anthropic's +`tool_choice` object and never emits `disable_parallel_tool_use`. +`parsed.options.parallelToolCalls` (`src/types/request.ts:250`) carries the +caller's intent and has no Anthropic consumer. + +The block is also gated on `parsed.options.toolChoice` being set, so a request +that sends only `parallel_tool_calls:false` emits no `tool_choice` at all. + +Probe: `probe-results.json` `F4` — `inputParallel:false` produces +`toolChoice:{type:"auto"}` with no disable flag. + +### F8 — CodeBuddy keeps user images and flattens tool-result images + +`buildConversationInput` (`src/adapters/coding-agent/protocol.ts:415-462`) is +shared by the CodeBuddy and Qoder adapters. A current `user` message's image parts +become real image blocks through `imagePart` (`:423`), and history user images are +collected the same way (`:446`). + +A `toolResult` message takes a different branch (`:431-436`): its content parts are +mapped with `p.type === "text" ? p.text : "[image]"` and joined into prose. The +image carrier is discarded and replaced by a literal marker. A current user message's +non-image media takes the same shape at `:425` with `"[video]"`. + +Probe: `probe-results.json` `F8` — `userImageParts:1`, +`currentToolImageParts:0`, `historicalToolImageParts:0`. + +Qoder's explicit 400 on original images and the vendor-tools-disabled policy on both +adapters are deliberate and stay. + +### F5 — file and audio payloads disappear in the translated IR + +`OcxContentPart` (`src/types/request.ts:189-204`) is `text | image | video`. +There is no file or audio member. + +`inputContentParts` (`src/responses/parser-content.ts:33-60`) converts +`input_file` into a `[file: name]` text marker and has no `input_audio` branch at +all, so an audio part is silently dropped. `outputToToolResultContent` +(`:94-120`) has the same gap on the tool-output side. + +The upstream Codex wire shape was checked directly rather than assumed: +`git show HEAD:codex-rs/protocol/src/models.rs` in the Codex mirror carries +`input_audio` with an `audio_url` field, in both user content and tool output. + +Probe: `codex-audio-probe.json` — `rawUserPreserved:true` and +`rawToolPreserved:true` against `irUserPreserved:false` and +`irToolPreserved:false`. The raw passthrough keeps the payload; only the translated +IR loses it. + +This unit prefers a scoped explicit refusal over a speculative universal audio +implementation, and native raw passthrough keeps its existing capability. No raw +media bytes may appear in an error message. + +### F9 — translated Chat video vanishes or becomes a malformed part + +The IR does carry video: `parser-content.ts:48-50` produces +`{type:"video", videoUrl}`. The loss is in the Chat adapter's serialization +(`src/adapters/openai-chat.ts:770-792`), where a non-text timeline part +"serializes to nothing", and the image-bearing branch maps every non-image part to +`{type:"text", text: (p as OcxTextContent).text}` — for a video part `text` is +`undefined`, producing a text part with no text. + +Probe: `probe-results.json` `F9` — `textVideoMessages` shows the video gone and +`imageVideoHasMissingTextField:true`. + +Native Chat passthrough and Google inline video behavior are unaffected and must stay. + +### Kiro remote image + +Recorded by the audit as a static-path loss where both the bytes and any marker are +absent. Treated here as a candidate for an explicit refusal or fallback with a +regression test. No fetching is introduced to resolve a remote reference. + +## Vendor contracts + +### Google — structured output on `generateContent` + +Sources: `https://ai.google.dev/gemini-api/docs/generate-content/structured-output` +and `https://ai.google.dev/api/generate-content`. + +Structured output is configured inside `generationConfig`, on `generateContent` +itself. There is no separate Interactions API involved. + +- `responseMimeType: "application/json"` selects JSON output. +- `responseJsonSchema` accepts an ordinary JSON Schema object — lowercase type + names, `required`, `additionalProperties` — the shape produced by + `zodToJsonSchema` or Pydantic. +- `responseSchema` accepts Gemini's own typed `Schema` form with uppercase type + names such as `"OBJECT"` and `"STRING"`. + +`parsed.options.textFormat.schema` is already an OpenAI-style JSON Schema with +lowercase types, so `responseJsonSchema` is the matching field and no type-case +translation is required. + +Two cautions carry into the diff. The response type does not change: the model still +returns text, and that text contains the conforming JSON, so Google response parsing +stays untouched. And `sanitizeGeminiToolParameters` exists to coerce schemas into +the tool-declaration subset — applying it to an output schema would corrupt a valid +JSON Schema, so the output path needs its own handling. + +Mode support is not assumed uniform. AI Studio and Vertex `generateContent` are in +scope. The Cloud Code Assist envelope used by Antigravity, and Claude models served +through it, are not verified for this field, so they get an explicit refusal rather +than a silent drop. + +### Anthropic — parallel tool use + +Source: `https://platform.claude.com/docs/en/agents-and-tools/tool-use/parallel-tool-use` +and the tool-use implementation guide. + +`disable_parallel_tool_use` is a boolean nested inside the `tool_choice` object. +Its per-mode meaning: + +| `tool_choice` | with `disable_parallel_tool_use: true` | +|---|---| +| `{"type":"auto"}` | zero or more tools -> at most one tool call | +| `{"type":"any"}` | must call one -> exactly one call | +| `{"type":"tool","name":...}` | must call that tool -> exactly one call | +| `{"type":"none"}` | tool use is off; the flag is irrelevant | + +That table settles every branch the adapter has. An implicit auto needs a +synthesized `{"type":"auto", disable_parallel_tool_use:true}`, because today no +`tool_choice` is emitted at all. `required` maps to `any` and a named choice maps +to `tool`; both accept the flag. `none` does not get the flag, and a request with +no tools emits no `tool_choice`. + +The flag constrains the model's output, not execution ordering — sequential tool +use is enforced by the caller's own loop returning each `tool_result` before the +next request. The PR states that boundary rather than claiming general parallelism +control. diff --git a/devlog/_plan/260914_provider_parity_stack/002_architect_dispositions.md b/devlog/_plan/260914_provider_parity_stack/002_architect_dispositions.md new file mode 100644 index 0000000000..0511b5cb41 --- /dev/null +++ b/devlog/_plan/260914_provider_parity_stack/002_architect_dispositions.md @@ -0,0 +1,127 @@ +# Architect consultation — proposals and dispositions + +Read-only architect dispatched during P with `cxc-dev` and `dev-architecture` +attached. Its decision IDs are D1-D8. Every claim below was re-verified against the +source in this worktree before being folded in; the verification command and the +confirming line are recorded with each one. + +| ID | Proposal | Disposition | +|---|---|---| +| D1 | `src/chat/image-parts.ts` is the right seam | ACCEPTED as planned | +| D2 | Placement is correct; two amendments needed | ACCEPTED with amendments | +| D3 | Predicate correct; `stop` is inert, and the combo guard should be mirrored | ACCEPTED, plan corrected | +| D4 | Item shape correct; the "no signature" claim is false | ACCEPTED, claim rewritten | +| D5 | Field choice correct; the image-model exclusion is a plan error | ACCEPTED, condition replaced | +| D6 | Implicit-auto synthesis is safe | ACCEPTED, no change | +| D7 | Right layer; two ordering hazards and one wrong acceptance row | ACCEPTED, rows corrected | +| D8 | Layer 3 is not actually dependent on layers 1-2 | ACCEPTED, topology claim corrected | + +## D2 — amendments folded + +Verified readers of `chatBody.messages` after the normalization site: `evidenceFromBody` +(`src/server/chat-completions.ts:147`), the Cursor/Kiro token estimate (`:164`), +`isNativeChatRouteEligible` (`:170`), the native passthrough +(`src/adapters/openai-chat.ts:129`), and the Responses projection (`:196`). All +should see the normalized form. + +Policy routing is unaffected either way: `inputContainsImage` is already shape-wide +and reads `record.messages` (`src/routing/request-evidence.ts:40`), so it matched +Pi-shaped parts before this change. The plan now says so instead of leaving it open. + +Two real consequences are now acceptance rows rather than assumptions: + +- The Cursor/Kiro token estimate shifts, because a Pi part re-serialized as a data + URI gains the `data:;base64,` prefix and loses the `mimeType` key. +- `normalizeChatImageParts` copies each matched base64 payload once, and that + allocation is not metered by `translatorBudget` (which meters only the body read + at `:112`). On the translated path the identical copy already happens inside + `imageUrlFromPart`, so this is new peak memory on the **native** path only. The + plan states the bound rather than silently adding unmetered allocation. + +## D3 — `stop` evidence replaces a speculative risk + +The plan originally justified keeping `stop` as "an honest upstream error is better +than a silent drop". That was weaker than the truth. `rg stop +src/adapters/openai-responses.ts` returns no match: the adapter never writes `stop` +to any wire, so retaining it in `internalBody` cannot reach an upstream at all and +cannot cause a 400 on this adapter. + +The new site also mirrors the `!settledRoute.combo` guard already used with this +same predicate at `src/server/chat-completions.ts:245`, so an unresolved combo +parent is never classified as the canonical backend. + +## D4 — the signature claim was wrong and is corrected + +Verified at `src/responses/parser.ts:304`: + +```ts +signature: envelope?.sig ?? JSON.stringify(reasoning), +``` + +A reasoning item with no signed envelope therefore *does* receive a fabricated +signature inside the IR. The plan's "no signature is produced" was false. + +It never reaches Anthropic, but only because +`isLikelyRealAnthropicThinkingSignature` (`src/adapters/anthropic.ts:247-251`) +requires `/^[A-Za-z0-9+/_=-]+$/`, which a string starting with `{` fails. That is a +charset regex standing in for a design guarantee. + +Corrected claim: **no signature is forwarded**. Layer 2 adds a regression asserting +that filter holds for a synthesized item, so the guarantee stops being incidental. + +## D5 — the image-model exclusion would have reintroduced F3 + +The plan copied `!isImageCapableModel(parsed.modelId)` from the `thinkingConfig` +gate. That exclusion exists there for a specific reason — the `responseModalities` +fallback is gated on `!generationConfig.thinkingConfig` +(`src/adapters/google.ts:845-847`) — and `responseMimeType`/`responseJsonSchema` +do not touch that gate. + +As written it would have silently dropped a caller's schema for an image-capable +model: unconstrained prose returned as success, which is exactly the defect F3 +fixes. Copying a condition without its reason is the failure here. + +Replacement: an image-capable model asked for structured output gets an explicit +error, not a silent drop. Requesting JSON-constrained text from a model configured +to return `["TEXT","IMAGE"]` is a contradiction the caller should see. The phrasing +follows the established surface at `src/adapters/kiro/conversation.ts:33`. + +## D7 — ordering hazards + +Image order is current-then-history, because `imageBlocks` is filled from the +current message before the history loop runs +(`src/adapters/coding-agent/protocol.ts:412`, `:424`, `:443-451`), and +`historyMessages = nonDev.slice(0, -1)` (`:408`) confirms there is no double +counting. Acceptance row 3 claimed "in message order", which is not achievable +without reordering an array that governs existing user-image behavior. The row is +corrected to describe real behavior; reordering is out of scope for this layer. + +Kiro: the marker must be appended before `rawGroupText` is computed +(`src/adapters/kiro/payload.ts:292-293`), because adjacency grouping rebuilds the +turn's content from `texts` and would otherwise discard it. No credential risk — +the marker carries no URL — and the base64 budget counts `KiroImage[]`, not text. + +F9: the image-bearing branch was correct, but a **video-only** user message still +vanishes at `src/adapters/openai-chat.ts:786-787`, where `[undefined].join("")` +yields `""`. Layer 4 now fixes both branches rather than shipping the asymmetry. + +## D8 — the topology claim is corrected + +The architect is right that layer 3 is not source-dependent on layers 1-2: F3 and F4 +read `options.textFormat` and `options.parallelToolCalls`, which layers 1-2 never +touch. The original wording ("consumes the IR that layers 1 and 2 made faithful") +was narrative, not a dependency. + +The stack is kept, for a stated and checkable reason rather than an implied one: + +- real source dependency: layers 1 and 2 share `src/server/chat-completions.ts` and + `src/chat/inbound.ts` +- registration serialization: `scripts/test-layout/layout.json` and + `tests/fixtures/test-layout-expected.json` are edited by all four layers, and + `structure/providers/chat-compat.md` by layers 2-4; parallel PRs would conflict + on every one of them + +Layer 3's PR says plainly that it is independent at source level and stacked for +serialization. Also corrected: `000_plan.md` claimed layer 1 touches +`src/adapters/openai-chat.ts`, which its own file map contradicts — that file +belongs to layer 4 only. diff --git a/devlog/_plan/260914_provider_parity_stack/003_blocker_corrections.md b/devlog/_plan/260914_provider_parity_stack/003_blocker_corrections.md new file mode 100644 index 0000000000..722e39e038 --- /dev/null +++ b/devlog/_plan/260914_provider_parity_stack/003_blocker_corrections.md @@ -0,0 +1,161 @@ +# Blocker corrections — authoritative over the decade docs + +Independent coordinator review raised material blockers against the first draft of +this unit. Where this document and a decade doc disagree, **this document wins**. +Each correction was re-verified against source in this worktree before adoption. + +## C0 — verification status, corrected + +The first draft of `000_plan.md` presented a verifier table with exit codes as if +this session had run them. It had not. That prefill is withdrawn. + +**Standing user instruction: no local product check may run on this Mac.** No +`bun test`, `bun run test`, `typecheck`, `build`, `lint`, `structure:check`, +`privacy:scan`, prepush script, or `cxc receipt test` is executed by this session. +Every such gate is reported as **NOT RUN BY USER INSTRUCTION**, never as passing, +provisional, or assumed. Layers publish as **DRAFT** on that basis. + +What real evidence exists: + +| Check | Result | Provenance | +|---|---|---| +| `bun run typecheck` | exit 0 | **coordinator baseline** at `df7dc1be53`, before this unit's changes — `.tmp/provider-parity-control/baseline-typecheck.log` | +| `bun run structure:check` | exit 0, "structure/ SSOT checks passed" | coordinator baseline, same commit — `baseline-structure.log` | +| `bun run privacy:scan` | exit 0, "Privacy scan passed" | coordinator baseline, same commit — `baseline-privacy.log` | +| this unit's changes | **NOT RUN** | forbidden on this host | + +Those three are a baseline of unmodified source, not coverage of anything this unit +adds. Documents were untracked at that observation, so they do not evidence new-file +coverage either. + +`structure:check` is also described more narrowly now: it validates the structure +index, path and invariant integrity. It does **not** prove that every owning doc was +updated alongside its source area. The earlier wording overclaimed what it observes. + +Remaining evidence paths, in the order this unit uses them: hosted GitHub Actions at +the exact pushed head, and independent static review through Aside. Red-first +execution is impossible under this restriction, so regression tests are written to +assert desired behavior and reviewed statically instead of being run red. + +## C1 — F2 belongs at the final target, not the ingress + +The draft deleted controls in `src/server/chat-completions.ts` based on +`settledRoute`. That is wrong, and it is a data-loss bug in both directions. + +`settledRoute` is the route settled at Chat ingress. A combo or policy route +resolves its concrete child later in the Responses pipeline, so an ingress-time strip +mutates shared intent before the real target is known: a canonical-first combo that +falls back to a key gateway has already lost the caller's controls, and a +non-canonical-first combo that falls back to canonical still ships them. + +Verified final-target site: `stripUnsupportedForwardParams` +(`src/adapters/openai-responses.ts:1279-1287`) is applied to `outBody` at +`:2253`, inside `if (forward)`, after the concrete provider is known. It currently +drops only `max_output_tokens` and `metadata`, and it returns a copy, so +`parsed._rawBody` stays caller-owned. + +**Corrected design.** + +1. `src/server/chat-completions.ts`: keep `internalBody.store = false` for every + `openai-responses` route. Remove the `delete` of `max_output_tokens`, + `temperature`, `top_p`, `stop` and `user`. No adapter-string branch remains at + the ingress. +2. `src/adapters/openai-responses.ts`: extend the existing final-target sanitizer so + the canonical ChatGPT backend still rejects nothing it rejects today. The + `max_output_tokens`/`metadata` drop stays applied to every `forward` provider, + because that is its current, separately-owned behavior and widening or narrowing + it would collide with `#4528`. The sampling controls `temperature`, `top_p`, + `stop` and `user` are removed **only** under + `isCanonicalOpenAiForwardProvider(provider)`, alongside the existing + canonical-only block at `:2255-2260`, and non-mutatingly. + +This preserves generic key-gateway and custom-forward compatibility, and it decides +on the provider that actually receives the body. + +Directional combo tests are required, not optional: canonical-first falling back to a +key gateway must retain the caller's controls, and non-canonical-first falling back +to canonical must have them removed. + +## C2 — F3 must not silently ignore an explicit schema + +Two draft errors. `!isImageCapableModel(...)` silently dropped a caller's schema on +image-capable models, reintroducing the exact defect F3 fixes. And Cloud Code Assist +was described as upstream-unsupported, which is not established. + +**Corrected.** An image-capable model with **no** schema keeps today's +`responseModalities` image behavior untouched. An image-capable model **with** an +explicit schema gets a scoped, content-free error rather than silence. Cloud Code +Assist is described exactly as it is: **not implemented or verified by opencodex**, +not proven impossible upstream. A malformed or absent `json_schema.schema` is +validated through the existing parser path rather than silently downgraded to plain +JSON mode. + +**Field contract, recorded explicitly.** Google's current guide shows REST +`generationConfig.responseFormat.text.{mimeType,schema}`, while the same guide's Go +examples, the Firebase `GenerationConfig` reference, and the Gemini Enterprise tool +reference all still document `responseMimeType` + `responseJsonSchema`; Google Cloud +REST marks the older pair deprecated but not removed. This unit emits +`responseMimeType: "application/json"` plus `responseJsonSchema`, because that pair +is documented as raw JSON Schema — matching the IR's OpenAI-style schema without a +type-case translation — and is still accepted. `responseSchema` is deliberately +omitted, as the Firebase reference requires when `responseJsonSchema` is used. The +tool-parameter sanitizer is not applied to an output schema. + +## C3 — F5 must not throw in the shared parser, and a marker is not a fix + +The draft claimed native raw passthrough never enters the parser. That is false: +`src/responses/parser.ts:570` sets `_rawBody: body`, and the Responses adapter +forwards `parsed._rawBody` (`src/adapters/openai-responses.ts:2226`, `:2410`). +The request does pass through `parseRequest`; the adapter simply forwards the raw +body afterwards. + +Consequence: a throw inside `inputContentParts` would regress legitimate raw +Responses passthrough, including `runTurn`, compaction and sidecar paths, not only +HTTP `buildRequest`. + +**Corrected.** The shared parser stays non-throwing and gains recognition only. +Refusal belongs to the adapters that cannot carry the payload, as a content-free, +target-local error naming the modality and never echoing bytes, a URL, or a +client-controlled format string. Unknown and malformed parts keep their existing +tolerant behavior deliberately. + +**A marker is not payload support.** F5 audio is therefore **not** claimed as fixed. +Recognition plus explicit target-local refusal is the deliverable; real audio +transport stays an explicit residual (`050_residuals.md` R2). + +## C4 — F9 must fix both branches + +The draft fixed only the image-bearing branch and called the text-only branch +correct. It is not: at `src/adapters/openai-chat.ts:786-787` a video-only or +text-plus-video message joins `(p as OcxTextContent).text` over a video part, +yielding `""`, and the message is then dropped. That is the reported defect left in +place, and the draft's acceptance row asserted the silent loss as success. + +**Corrected.** Both branches handle a video part. No acceptance row may assert a +silent drop as success. No universal "upstream does not support video" claim is +made — the statement is scoped to this adapter's Chat wire. + +## C5 — F8 chronological provenance is an ordering change + +Collecting history tool images without changing order does not produce chronological +provenance: current-message images are appended to `imageBlocks` before the history +loop runs (`src/adapters/coding-agent/protocol.ts:412`, `:424`, `:443-451`). + +**Corrected.** Ordering is fixed deliberately so blocks follow conversation order, +and the regression uses two distinguishable images — one historical, one current — +asserting their relative position rather than only their count. + +## C6 — file-map gaps + +`src/adapters/kiro/payload.ts` was missing from the Layer 4 file map and is required: +the marker must be appended before `rawGroupText` is computed (`:292-293`), or +adjacency grouping rebuilds the turn from `texts` and discards it. + +## C7 — scope boundary held open deliberately + +"전부 수정" means no straightforward confirmed loss is left unaddressed. It does not +mean inventing vendor support. Actual vendor-tool execution stays off for +CodeBuddy and Qoder; a strict unsupported request is rejected rather than faked. A +full native client-tool bridge, and unverified gateway capabilities, are separate +feature work and are recorded as a boundary, not delivered here. No unverified +all-model vision declaration is added to any catalog. `#4511` stays untouched. diff --git a/devlog/_plan/260914_provider_parity_stack/010_phase1_ingress_normalization.md b/devlog/_plan/260914_provider_parity_stack/010_phase1_ingress_normalization.md new file mode 100644 index 0000000000..ac05166294 --- /dev/null +++ b/devlog/_plan/260914_provider_parity_stack/010_phase1_ingress_normalization.md @@ -0,0 +1,174 @@ +# Phase 1 — inbound normalization and explicit reasoning disable + +Branch `agent/provider-parity-01-ingress`, base `dev`. Findings F1 and F7. + +Thesis: the Chat ingress must recognize the same image shapes the translated path +already understands, before it decides which pipeline the request enters, and it +must not discard an explicit request to disable reasoning. + +## Scope + +IN: inbound Chat image-part normalization, the native-route eligibility predicate +it feeds, and the reasoning-effort allowlist. + +OUT: vision describer policy (`#4501`/PR `#4511`), any remote fetch of an image +reference, adapter-side image handling, reasoning replay. + +## File change map + +| File | Action | +|---|---| +| `src/chat/image-parts.ts` | NEW — shared recognizer and normalizer | +| `src/chat/inbound.ts` | MODIFY — consume the shared recognizer; allow `none` | +| `src/server/chat-completions.ts` | MODIFY — normalize before route selection | +| `src/server/chat-native.ts` | MODIFY — predicate reads the shared recognizer | +| `tests/server/chat-native-image-normalization.test.ts` | NEW | +| `tests/server/chat-inbound-reasoning-none.test.ts` | NEW | +| `scripts/test-layout/layout.json` | MODIFY — register both test files | +| `tests/fixtures/test-layout-expected.json` | MODIFY — register both test files | +| `structure/data-planes/inbound-compat.md` | MODIFY — record both behaviors | +| `docs-site/` | MODIFY — reasoning `none` is user-visible | + +## NEW `src/chat/image-parts.ts` + +Moves the existing recognizer out of `inbound.ts` unchanged in behavior, and adds +the normalizer the ingress needs. Keeping one implementation is the point of the +layer: the two call sites diverged precisely because the logic was duplicated. + +```ts +type Rec = Record; + +function isRec(v: unknown): v is Rec { + return !!v && typeof v === "object" && !Array.isArray(v); +} + +/** + * The image reference carried by a Chat content part, in URL or data-URI form. + * Accepts OpenAI `image_url`, Pi/MCP `{type:"image", data, mimeType}`, and + * Anthropic-shaped `{type:"image", source:{...}}`. Returns null for anything else. + */ +export function chatImageUrlFromPart(part: Rec): string | null + +/** The `detail` hint, when the part carries a recognized one. */ +export function chatImageDetailFromPart(part: Rec): "auto" | "low" | "high" | undefined + +/** True when any messages[].content[] part carries a recognized image. */ +export function chatBodyCarriesImage(rawBody: Rec): boolean + +/** + * Rewrite every recognized non-OpenAI image part into `image_url` form. + * Returns the same object reference when nothing matched, so a body with no + * image — and a body whose images are already `image_url` — is untouched. + */ +export function normalizeChatImageParts(rawBody: Rec): Rec +``` + +`chatImageUrlFromPart` is `imageUrlFromPart` from `src/chat/inbound.ts:48-79` +moved verbatim. `chatBodyCarriesImage` is `src/server/chat-native.ts:168-177` +widened to call it instead of testing `part.type === "image_url"`. + +`normalizeChatImageParts` produces, for a matched part: + +```ts +{ type: "image_url", image_url: { url, ...(detail ? { detail } : {}) } } +``` + +Identity rules, all of which get a test: + +- no image anywhere -> the same object reference is returned, nothing is copied +- every image already `image_url` -> the same object reference is returned +- a matched part is replaced; every sibling part, every other message field, and + every top-level body field keep their exact value and order + +The copy is structural and shallow per level: only the `messages` array, the +message objects that contain a matched part, and their `content` arrays are +rebuilt. This is what "preserve native Chat fields" requires — the native path is a +whitelist passthrough, so an incidental deep clone would be a behavior change. + +## MODIFY `src/server/chat-completions.ts` + +Before, at `:111-115`: + +```ts + const rawBody = await readChatBody(req, translatorBudget, resolveInboundBodyLimitBytes(config.maxInboundBodyBytes)); + assertChatCompletionsRoutingBody(rawBody); + chatBody = rawBody; +``` + +After: + +```ts + const rawBody = await readChatBody(req, translatorBudget, resolveInboundBodyLimitBytes(config.maxInboundBodyBytes)); + assertChatCompletionsRoutingBody(rawBody); + // Normalize before routing: isNativeChatRouteEligible below decides the pipeline + // from the image parts it can see, and the native path forwards the body as-is. + chatBody = normalizeChatImageParts(rawBody) as typeof rawBody; +``` + +This single site is why the layer is placed at the bottom of the stack. It runs +ahead of `routeModel` (`:145`) and ahead of `isNativeChatRouteEligible` +(`:169`), so both the diversion decision and the forwarded wire see the same parts. + +## MODIFY `src/server/chat-native.ts` + +Delete the local `chatBodyCarriesImage` (`:168-177`) and import the shared one. +`isNativeChatRouteEligible` at `:155` is otherwise unchanged. + +## MODIFY `src/chat/inbound.ts` — F1 half + +Delete `imageUrlFromPart` (`:48-79`); import `chatImageUrlFromPart` and use it in +`userContentToBlocks`. Behavior is identical, and the existing tests for the +translated path are the proof of that. + +## MODIFY `src/chat/inbound.ts` — F7 half + +Before, at `:28`: + +```ts +const OUTPUT_CONFIG_EFFORTS = new Set(["minimal", "low", "medium", "high", "xhigh", "max", "ultra"]); +``` + +After: + +```ts +// "none" is the runtime's disable sentinel, not an unknown value: src/reasoning-effort.ts +// treats it as valid and maps it to "omit the reasoning parameter", and the Pi client +// export maps Pi's "off" level onto it. Dropping it here let a provider default +// re-enable thinking the caller explicitly turned off. +const OUTPUT_CONFIG_EFFORTS = new Set(["none", "minimal", "low", "medium", "high", "xhigh", "max", "ultra"]); +``` + +`reasoningConfigSchema.effort` is `z.string().optional()` +(`src/responses/schema.ts:136-139`), so the produced body still validates. The +downstream consumers already understand the sentinel: +`src/reasoning-effort.ts:196` omits the wire parameter for it, and +`src/adapters/anthropic.ts:964-966` sends `thinking:{type:"disabled"}`. + +## Acceptance criteria + +Each row names the activation scenario and the observable effect. + +| # | Scenario | Observable effect | +|---|---|---| +| 1 | user message with a Pi `{type:"image", data, mimeType}` part, text-only routed model | request is diverted off the native path; `isNativeChatRouteEligible` returns false | +| 2 | same with an Anthropic `{type:"image", source:{type:"base64"}}` part | diverted | +| 3 | same with `source:{type:"url"}` | diverted; no fetch is attempted | +| 4 | tool message carrying a Pi image part | diverted | +| 5 | vision-capable routed model, Pi image part | stays native; forwarded body carries `image_url`, not the raw part | +| 6 | image-only content (no text part) | image survives normalization | +| 7 | body with no image | `normalizeChatImageParts` returns the identical object reference | +| 8 | body whose images are already `image_url` | identical object reference; `detail` preserved | +| 9 | `reasoning_effort:"none"` over Chat | projected body carries `reasoning.effort === "none"` | +| 10 | the same against an Anthropic-adapter route | wire body carries `thinking:{type:"disabled"}` | +| 11 | `reasoning:{effort:"none"}` nested form | same as 9 | + +Rows 1-4 and 9-11 are the red-first regressions: they fail on `dev` today. + +## Bypass record + +Enforcement tier: E7, agent-followed plus test coverage. Executing surface: the +focused test files above and `bun run typecheck`. Known bypass: a future call site +that reads `messages` before `handleChatCompletionsWithBudget` normalizes, or a +third image shape neither recognizer knows. Residual risk: accepted — the shared +module makes the next shape a one-file change. Wording was not downgraded; this is +a normalization, and it is not claimed to be a schema guarantee. diff --git a/devlog/_plan/260914_provider_parity_stack/020_phase2_chat_responses_controls.md b/devlog/_plan/260914_provider_parity_stack/020_phase2_chat_responses_controls.md new file mode 100644 index 0000000000..376e81afc7 --- /dev/null +++ b/devlog/_plan/260914_provider_parity_stack/020_phase2_chat_responses_controls.md @@ -0,0 +1,172 @@ +# Phase 2 — Chat to Responses control fidelity + +Branch `agent/provider-parity-02-controls`, base `agent/provider-parity-01-ingress`. +Findings F2 and F6. + +Thesis: a Chat request translated into the Responses pipeline should keep the +controls the caller actually sent, and the restrictions that exist for the +canonical ChatGPT backend should apply to that backend rather than to every +provider sharing its adapter string. + +## Scope + +IN: the `openai-responses` control strip, and the inbound projection of assistant +reasoning text and sampling penalties. + +OUT: `stop` support claims, opaque reasoning-signature replay, `#4528`'s subject, +Azure (`azure-openai` is a different adapter and never matched this condition). + +## File change map + +| File | Action | +|---|---| +| `src/server/chat-completions.ts` | MODIFY — scope the strip | +| `src/chat/inbound.ts` | MODIFY — carry reasoning text and penalties | +| `tests/server/chat-responses-control-scope.test.ts` | NEW | +| `tests/server/chat-inbound-reasoning-replay.test.ts` | NEW | +| `scripts/test-layout/layout.json` | MODIFY | +| `tests/fixtures/test-layout-expected.json` | MODIFY | +| `structure/data-planes/inbound-compat.md` | MODIFY | +| `structure/providers/chat-compat.md` | MODIFY | + +## MODIFY `src/server/chat-completions.ts` — F2 + +Before, at `:221-231`: + +```ts + if (settledRoute?.provider.adapter === "openai-responses") { + // ChatGPT backend rejects store:true and unsupported sampling knobs. + internalBody.store = false; + delete internalBody.max_output_tokens; + delete internalBody.temperature; + delete internalBody.top_p; + delete internalBody.stop; + delete internalBody.user; + } else if (internalBody.store === undefined) { + internalBody.store = false; + } +``` + +After: + +```ts + if (settledRoute?.provider.adapter === "openai-responses") { + // store:false is correct for every Responses route here — the proxy never wants + // upstream-side retention for a translated Chat turn. + internalBody.store = false; + // The sampling and output-cap restrictions belong to the canonical ChatGPT + // backend, which rejects them. Seven providers share this adapter string + // (openai, openai-apikey, meta-model, meta-muse, zai, zhipu-bigmodel-responses, + // volcengine-agent-plan); a generic API-key Responses endpoint accepts the + // caller's controls, so stripping them there silently discards caller intent. + if (isCanonicalOpenAiForwardProvider(settledRoute.provider)) { + delete internalBody.max_output_tokens; + delete internalBody.temperature; + delete internalBody.top_p; + delete internalBody.stop; + delete internalBody.user; + } + } else if (internalBody.store === undefined) { + internalBody.store = false; + } +``` + +`isCanonicalOpenAiForwardProvider` is already imported in this file and already +used at `:247` for exactly this "is this really the ChatGPT backend" question, so +the scope test reuses the module's existing authority rather than inventing a +second provider classification. + +The forward restrictions themselves are not relaxed. `store` stays pinned false on +every Responses route. `stop` is preserved for non-canonical providers because the +caller sent it, which is not a claim that every provider on this adapter supports +it — an upstream that rejects it still rejects it, and that is a truthful upstream +error rather than a silent proxy-side drop. + +Two neighbouring paths are checked, not assumed: + +- the synthetic effort-row override (`:127-129`) rewrites `chatBody.model` before + routing, so `settledRoute` is the post-override route and the condition reads the + settled provider +- a combo or policy route sets `routeMayChangeCredentialDomain` (`:161`) and its + concrete child is selected later in the Responses pipeline; the strip here applies + to the settled parent, which is the same object the existing code read + +## MODIFY `src/chat/inbound.ts` — F6 reasoning + +`assistantContentToBlocks` (`:121-137`) gains a reasoning branch. The inbound +direction becomes the inverse of the outbound reconstruction that +`src/adapters/openai-chat.ts:800-843` already performs. + +```ts +// Assistant turns replayed by a Chat client carry the model's prior thinking as +// reasoning_content (string) or reasoning_details (array of segments). Both are +// plaintext this proxy can represent; keeping them lets an interleaved-thinking +// provider see its own prior reasoning instead of a bare continuation. +function assistantReasoningText(msg: Rec): string | undefined +``` + +Accepted shapes, both already produced by the outbound path: + +- `reasoning_content: string` +- `reasoning_details: [{ type: "reasoning.text", text: string }, ...]` — the + `text` fields are joined in order + +The extracted text becomes a `{type:"reasoning", content:[{type:"reasoning_text", +text}]}` input item, which `reasoningItemSchema` +(`src/responses/schema.ts:56-60`) already accepts, emitted immediately before the +assistant message it belongs to so ordering is preserved. + +What is deliberately not done: no `signature`, no `encrypted_content`, no item id +is synthesized. A signature is a provider-issued attestation over content this +proxy did not receive, and fabricating one would either be rejected upstream or, worse, +accepted as a false claim of provenance. Cross-provider opaque metadata is likewise +not copied. Opaque reasoning replay across a Chat boundary needs its own design and +is recorded as residual in `050_residuals.md`. + +## MODIFY `src/chat/inbound.ts` — F6 penalties + +The body builder (`:337-362`) gains two lines beside the existing `temperature` +and `top_p` handling: + +```ts + if (typeof raw.presence_penalty === "number") body.presence_penalty = raw.presence_penalty; + if (typeof raw.frequency_penalty === "number") body.frequency_penalty = raw.frequency_penalty; +``` + +The rest of the chain exists already and is the reason this is a two-line fix +rather than a feature: `src/responses/schema.ts:162-163` accepts both, +`src/responses/parser.ts:544-545` parses them into +`options.presencePenalty`/`frequencyPenalty`, and +`src/adapters/openai-chat.ts:1600-1603` writes them back to the Chat wire. Only +the first link was missing. + +Provider opt-outs stay authoritative: `noPenaltyModels` +(`src/adapters/openai-chat.ts:147-150`) still deletes both for models that reject +them. + +## Acceptance criteria + +| # | Scenario | Observable effect | +|---|---|---| +| 1 | Chat request to a non-canonical `openai-responses` provider with `max_tokens`, `temperature`, `top_p` | all three survive into `internalBody` | +| 2 | same request to the canonical ChatGPT backend | all three are stripped, as today | +| 3 | both cases | `store === false` | +| 4 | non-canonical provider with `stop` and `user` | both survive | +| 5 | request carrying a synthetic effort-row model id | scope decision reads the settled post-override route | +| 6 | assistant turn with `reasoning_content` | a `reasoning` input item precedes the assistant message, carrying the text | +| 7 | assistant turn with `reasoning_details` segments | segments joined in order into one item | +| 8 | either case | no `signature`, no `encrypted_content` field is produced | +| 9 | assistant turn with no reasoning | input item list is byte-identical to today | +| 10 | `presence_penalty`/`frequency_penalty` sent over Chat | both reach `options` and the outbound wire | +| 11 | the same against a `noPenaltyModels` model | both are dropped at the adapter, as today | + +Rows 1, 4, 6, 7 and 10 are the red-first regressions. + +## Bypass record + +Tier E7. Executing surface: the two new test files plus `bun run typecheck`. +Known bypass: a provider that shares the `openai-responses` adapter string and +genuinely rejects sampling controls would now receive them and return an upstream +error. Residual risk: accepted and stated — an honest upstream 400 is preferable to +a silent proxy-side drop, and the canonical backend keeps its strip. No wording was +downgraded; this is a scope correction, not a capability claim. diff --git a/devlog/_plan/260914_provider_parity_stack/030_phase3_provider_wire_contracts.md b/devlog/_plan/260914_provider_parity_stack/030_phase3_provider_wire_contracts.md new file mode 100644 index 0000000000..a362275965 --- /dev/null +++ b/devlog/_plan/260914_provider_parity_stack/030_phase3_provider_wire_contracts.md @@ -0,0 +1,176 @@ +# Phase 3 — Google structured output and Anthropic parallel-tool disable + +Branch `agent/provider-parity-03-wire`, base `agent/provider-parity-02-controls`. +Findings F3 and F4. + +Thesis: two request options the IR already carries have no consumer in their +vendor adapter. Both vendors document the exact field; map to it, and refuse +explicitly where the field is not verified rather than dropping it in silence. + +The vendor contracts and their citations are in +[`001_audit_evidence.md`](001_audit_evidence.md#vendor-contracts). + +## Scope + +IN: `options.textFormat` onto the Gemini `generateContent` wire, and +`options.parallelToolCalls` onto Anthropic `tool_choice`. + +OUT: Google `tool_choice` (already implemented), Google response parsing (the +response type does not change), the Interactions API (not used), Antigravity and +Claude-through-CCA structured output (not verified — refused instead). + +## File change map + +| File | Action | +|---|---| +| `src/adapters/google.ts` | MODIFY — build structured-output config | +| `src/adapters/google-wire-compiler.ts` | MODIFY — pass it to the wire | +| `src/adapters/anthropic.ts` | MODIFY — emit the disable flag | +| `tests/adapters/google/google-structured-output.test.ts` | NEW | +| `tests/adapters/anthropic/anthropic-parallel-tool-disable.test.ts` | NEW | +| `scripts/test-layout/layout.json` | MODIFY | +| `tests/fixtures/test-layout-expected.json` | MODIFY | +| `structure/providers/google.md` | MODIFY | +| `structure/providers/chat-compat.md` | MODIFY | +| `docs-site/` | MODIFY — structured output is user-visible | + +## MODIFY `src/adapters/google.ts` — F3 + +Added to the `generationConfig` construction at `:826-849`, after the +`thinkingConfig`/`responseModalities` block: + +```ts + // Structured output travels in generationConfig on generateContent itself. + // responseJsonSchema takes ordinary JSON Schema (lowercase types), which is + // exactly what options.textFormat.schema already holds; responseSchema would + // require Gemini's uppercase typed Schema form. The response type is + // unchanged — the model returns text containing conforming JSON — so no + // response-parsing change belongs in this diff. + const textFormat = parsed.options.textFormat; + if (textFormat && !isImageCapableModel(parsed.modelId)) { + if (provider.googleMode === "cloud-code-assist") { + throw new Error( + "structured output is not supported on this Google mode (cloud-code-assist); " + + "remove response_format or route the model to AI Studio or Vertex", + ); + } + generationConfig.responseMimeType = "application/json"; + if (textFormat.type === "json_schema" && textFormat.schema) { + generationConfig.responseJsonSchema = textFormat.schema; + } + } +``` + +Four decisions, each with a reason: + +- **`responseJsonSchema`, not `responseSchema`.** The IR schema is an OpenAI-style + JSON Schema with lowercase type names; `responseSchema` expects Gemini's typed + form with `"OBJECT"`/`"STRING"`. Choosing the matching field avoids a lossy + case translation. +- **No sanitizer.** `sanitizeGeminiToolParameters` narrows a schema to the + tool-declaration subset. Running it over an output schema would strip valid + constructs; the output path carries the schema through as-is. +- **`json_object` sets only the MIME type.** That is the whole of the contract for + schemaless JSON. +- **Cloud Code Assist refuses.** The CCA envelope is proprietary and this field is + not verified there, for Gemini or for Claude models served through it. An explicit + error tells the caller their constraint was not applied; silence would return + unconstrained prose that looks like success. + +Image-capable models are excluded for the same reason `thinkingConfig` excludes +them at `:845-847`: the `responseModalities` fallback must keep working. + +## MODIFY `src/adapters/google-wire-compiler.ts` — F3 + +`compileGenerationConfig` (`:118-151`) is a whitelist, so the adapter change alone +would be dropped before the wire. This is the half that makes the field real. + +Added before the final `return`: + +```ts + if (typeof value.responseMimeType === "string" && value.responseMimeType.length > 0) { + out.responseMimeType = value.responseMimeType; + } + // Carried through unmodified: this is a caller-authored output schema, not a tool + // declaration, so the tool-parameter sanitizer must not touch it. + if (isObject(value.responseJsonSchema)) out.responseJsonSchema = value.responseJsonSchema; +``` + +## MODIFY `src/adapters/anthropic.ts` — F4 + +Before, at `:1015-1022`: + +```ts + if (parsed.options.toolChoice && (tools || parsed.options.toolChoice === "none")) { + const tc = parsed.options.toolChoice; + if (tc === "auto") body.tool_choice = { type: "auto" }; + else if (tc === "none") body.tool_choice = { type: "none" }; + else if (tc === "required") body.tool_choice = { type: "any" }; + else if (isAllowedToolChoice(tc)) body.tool_choice = { type: tc.mode === "required" ? "any" : "auto" }; + else if (typeof tc === "object" && "name" in tc) body.tool_choice = { type: "tool", name: toolNames.toWire(resolveToolChoiceWireName(parsed.context.tools, tc.name)) }; + } +``` + +After — the mapping is unchanged; the flag is attached afterwards, and an implicit +auto is synthesized so a caller who sent only `parallel_tool_calls:false` is heard: + +```ts + if (parsed.options.toolChoice && (tools || parsed.options.toolChoice === "none")) { + /* ...unchanged mapping... */ + } else if (tools && parsed.options.parallelToolCalls === false) { + // No explicit choice, but the caller asked for one tool at a time. Anthropic + // carries that intent inside tool_choice, so auto must be stated to hold it. + body.tool_choice = { type: "auto" }; + } + // disable_parallel_tool_use is nested in tool_choice and caps the model at one + // tool call for auto/any/tool. It is irrelevant under type "none" (tool use is + // already off) and meaningless with no tools on the wire. + if (parsed.options.parallelToolCalls === false + && isRec(body.tool_choice) + && body.tool_choice.type !== "none") { + body.tool_choice = { ...body.tool_choice, disable_parallel_tool_use: true }; + } +``` + +Branch behavior, matching the documented table: + +| caller | emitted | +|---|---| +| `parallel=false`, no `tool_choice`, tools present | `{type:"auto", disable_parallel_tool_use:true}` | +| `parallel=false`, `auto` | `{type:"auto", disable_parallel_tool_use:true}` | +| `parallel=false`, `required` | `{type:"any", disable_parallel_tool_use:true}` | +| `parallel=false`, named tool | `{type:"tool", name, disable_parallel_tool_use:true}` | +| `parallel=false`, allowed-tools `auto`/`required` | `auto`/`any` + flag | +| `parallel=false`, `none` | `{type:"none"}`, no flag | +| `parallel=false`, no tools | no `tool_choice` at all | +| `parallel` unset or true | byte-identical to today | + +The PR description states the boundary the vendor doc states: the flag constrains +the model's output, not execution ordering. Sequential tool use is enforced by the +caller's loop returning each `tool_result` before the next request. + +## Acceptance criteria + +| # | Scenario | Observable effect | +|---|---|---| +| 1 | AI Studio route, `text.format` `json_schema` | wire `generationConfig.responseMimeType === "application/json"` and `responseJsonSchema` equals the caller's schema | +| 2 | same, through `compileGenerationConfig` | both fields survive compilation | +| 3 | Vertex route, `json_schema` | same as 1 | +| 4 | `json_object` | MIME type only, no schema key | +| 5 | schema containing `additionalProperties:false` and nested `required` | reaches the wire unmodified; the tool sanitizer is not applied | +| 6 | cloud-code-assist route with `text.format` | explicit error naming the unsupported mode; no silent drop | +| 7 | image-capable model | `responseModalities` fallback still emitted | +| 8 | no `text.format` | `generationConfig` byte-identical to today | +| 9-15 | each row of the Anthropic table above | the stated `tool_choice` object | + +Rows 1-6 and 9-14 are the red-first regressions. + +## Bypass record + +Tier E7. Executing surface: the two new test files plus `bun run typecheck`. +Known bypass: a Vertex model or API version that rejects `responseJsonSchema` +returns an upstream error rather than being caught locally — there is no local +capability table for this field and inventing one would be a guess. Residual risk: +accepted; the CCA path refuses explicitly, which is the case actually known to be +unsupported. No wording downgraded — this is a wire mapping, and support is claimed +only for the two modes named. diff --git a/devlog/_plan/260914_provider_parity_stack/040_phase4_modality_fidelity.md b/devlog/_plan/260914_provider_parity_stack/040_phase4_modality_fidelity.md new file mode 100644 index 0000000000..1c8fb6cf1c --- /dev/null +++ b/devlog/_plan/260914_provider_parity_stack/040_phase4_modality_fidelity.md @@ -0,0 +1,207 @@ +# Phase 4 — modality fidelity and explicit refusal + +Branch `agent/provider-parity-04-modality`, base `agent/provider-parity-03-wire`. +Findings F8, F5, F9 and the Kiro remote-image loss. + +Thesis: where a payload can be carried, carry it; where it cannot, say so. The +common defect in all four is that the proxy currently does neither — it drops the +content and returns success. + +This layer is last because an explicit refusal is only honest once the layers below +have stopped losing the payload for unrelated reasons. + +## Scope + +IN: CodeBuddy/Qoder tool-result images, audio and file presence in the translated +IR, translated Chat video, Kiro remote images. + +OUT: any remote fetch; a speculative universal audio wire implementation; Qoder's +deliberate image refusal; the vendor-tools-disabled policy on both coding-agent +adapters; native raw passthrough behavior; Google inline video. + +No raw media bytes may appear in any error message or marker this layer produces. + +## File change map + +| File | Action | +|---|---| +| `src/adapters/coding-agent/protocol.ts` | MODIFY — carry tool-result images | +| `src/responses/parser-content.ts` | MODIFY — audio presence markers | +| `src/adapters/openai-chat.ts` | MODIFY — no malformed video part | +| `src/adapters/kiro-images.ts` | MODIFY — remote image marker | +| `tests/adapters/coding-agent-tool-result-images.test.ts` | NEW | +| `tests/responses/parser-content-audio.test.ts` | NEW | +| `tests/adapters/openai-chat-video-part.test.ts` | NEW | +| `tests/adapters/kiro-remote-image.test.ts` | NEW | +| `scripts/test-layout/layout.json` | MODIFY | +| `tests/fixtures/test-layout-expected.json` | MODIFY | +| `structure/providers/kiro.md` | MODIFY | +| `structure/providers/chat-compat.md` | MODIFY | +| `structure/adapters/registry.md` | MODIFY | + +## MODIFY `src/adapters/coding-agent/protocol.ts` — F8 + +`buildConversationInput` already knows how to carry an image: `imagePart` +(`:300-305`) encodes a `data:` URL or an `https` URL as a real image block, and +the current-user branch (`:423`) and history branch (`:446`) both use it. Only the +`toolResult` branch does not. + +Before, at `:431-436`: + +```ts + } else if (currentMessage.role === "toolResult") { + const text = typeof currentMessage.content === "string" + ? currentMessage.content + : currentMessage.content.map(p => (p.type === "text" ? p.text : "[image]")).join(""); + const status = currentMessage.isError ? " (error)" : ""; + currentRequestText = \`TOOL RESULT (call_id: \${currentMessage.toolCallId})\${status}:\n\${text}\n\nPlease proceed based on the above tool result.\`; +``` + +After — the image carriers join `imageBlocks` in order, and the prose keeps a +bounded provenance marker in their place so the text still reads coherently: + +```ts + } else if (currentMessage.role === "toolResult") { + let text: string; + if (typeof currentMessage.content === "string") { + text = currentMessage.content; + } else { + const segments: string[] = []; + for (const p of currentMessage.content) { + if (p.type === "text") { segments.push(p.text); continue; } + if (p.type === "image") { + // Carry the real image rather than flattening it to a marker. The + // provenance note stays so the model can tell which attachment the + // tool produced; the bytes travel as an image block, never as text. + const image = imagePart(p.imageUrl); + if (image) { imageBlocks.push(image); segments.push("[image attached below]"); } + else segments.push("[image omitted: unsupported reference]"); + continue; + } + segments.push("[video]"); + } + text = segments.join(""); + } + const status = currentMessage.isError ? " (error)" : ""; + currentRequestText = \`TOOL RESULT (call_id: \${currentMessage.toolCallId})\${status}:\n\${text}\n\nPlease proceed based on the above tool result.\`; +``` + +The history loop (`:443-451`) gains the matching `toolResult` case so a historical +tool image is carried too, in the same order the messages appear. + +Preserved exactly: the `(error)` label, the `TOOL RESULT (call_id: ...)` framing, +the "Please proceed" trailer, and message ordering. Qoder's explicit 400 on original +images happens upstream of this function and is untouched; both adapters keep +vendor tools disabled. + +## MODIFY `src/responses/parser-content.ts` — F5 + +`inputContentParts` (`:33-60`) handles `input_text`, `input_image`, +`input_video` and `input_file`, and has no `input_audio` branch — so an audio +part vanishes with no trace. `outputToToolResultContent` (`:94-120`) has the same +gap on the tool-output side. + +Upstream Codex sends `input_audio` with an `audio_url` field in both positions +(`codex-rs/protocol/src/models.rs`), and `codex-audio-probe.json` shows the raw +body keeping it while the IR loses it. + +This layer preserves *presence*, not audio capability. It follows the convention +the file already uses for files at `:53-59`: record that an attachment existed, +never inline the bytes. + +```ts + } else if (block.type === "input_audio") { + // The IR has no audio carrier and no adapter consumes one, so a silent drop + // would tell the model nothing was sent. Record presence only — never the + // payload, which is large base64 and would explode the token count. + const b = block as { audio_url?: string; format?: string }; + const format = nonEmptyString(b.format); + if (nonEmptyString(b.audio_url)) { + parts.push({ type: "text", text: format ? \`[audio: \${format}]\` : "[audio]" }); + } + } +``` + +The same branch is added to `outputToToolResultContent`. + +Native raw passthrough is untouched and keeps forwarding `input_audio` verbatim — +that path never enters this parser. Real audio transport through the translated IR +needs a carrier type, per-provider capability data and a wire mapping for each +vendor; it is recorded as residual rather than guessed at here. + +## MODIFY `src/adapters/openai-chat.ts` — F9 + +The image-bearing branch at `:790-794` maps every non-image part through +`(p as OcxTextContent).text`. For a video part that property does not exist, so the +wire receives `{type:"text", text: undefined}` — a malformed part, which is worse +than a drop because it can fail schema validation upstream. + +```ts + const chatParts = parts!.map(p => { + if (p.type === "image") { + return { type: "image_url", image_url: { url: p.imageUrl, ...(p.detail ? { detail: p.detail } : {}) } }; + } + // OpenAI's Chat Completions wire has no video content part. Emitting a + // bounded marker keeps the turn well-formed and tells the model an + // attachment it cannot see was sent; the previous code produced a text + // part whose text was undefined. + if (p.type === "video") return { type: "text", text: "[video omitted: unsupported by this provider]" }; + return { type: "text", text: (p as OcxTextContent).text }; + }); +``` + +The text-only branch at `:781-786` already breaks on an empty serialization, which +is correct and stays: a message whose only content was a video produces no empty +system message. + +Native Chat passthrough and Google inline video are not touched by this diff. + +## MODIFY `src/adapters/kiro-images.ts` — Kiro remote image + +`extractKiroImages` (`:27-36`) calls `parseDataUrlImage`, which returns undefined +for anything that is not a `data:` URL (`:13-14`). A remote `https` image is +therefore dropped with neither bytes nor marker — the payload and the evidence that +it existed both disappear. + +Kiro's wire carries base64 bytes only, so a remote reference genuinely cannot be +inlined, and this layer introduces no fetch. The fix is to stop losing it silently: + +```ts +/** Remote image references Kiro cannot inline, reported so the loss is never silent. */ +export function extractKiroUnsupportedImageCount(content: string | OcxContentPart[]): number +``` + +The payload builder (`src/adapters/kiro/payload.ts:237` and `:285`) appends a +bounded marker to that turn's text when the count is non-zero: +`[image omitted: remote image references are not supported by this provider]`. +No URL is included — a remote URL can carry a signed token, and this proxy does not +log or echo credentials. + +## Acceptance criteria + +| # | Scenario | Observable effect | +|---|---|---| +| 1 | CodeBuddy turn, current tool result with a data-URL image | image block reaches the wire; `[image]` no longer appears | +| 2 | same with a remote https image | image block with `source.type === "url"` | +| 3 | historical tool result with an image | carried, in message order | +| 4 | tool result with `isError: true` | `(error)` label preserved alongside the image | +| 5 | tool result mixing text and image | text order preserved; provenance marker in place | +| 6 | tool result with an unsupported image reference | `[image omitted: unsupported reference]`, no crash | +| 7 | `input_audio` in user content | `[audio: ]` text part; no base64 in the output | +| 8 | `input_audio` in tool output | same | +| 9 | no audio | parts byte-identical to today | +| 10 | translated Chat, video beside an image | `{type:"text"}` with a real string; no undefined text | +| 11 | translated Chat, video only | message dropped cleanly, no empty system message | +| 12 | Kiro turn with a remote image | bounded marker present; no URL in the text; no fetch attempted | +| 13 | Kiro turn with a data-URL image | unchanged from today | + +Rows 1-8, 10 and 12 are the red-first regressions. + +## Bypass record + +Tier E7. Executing surface: the four new test files plus `bun run typecheck` and +`bun run privacy:scan`. Known bypass: a marker is advisory — a model may ignore it, +and no schema enforces its presence. Residual risk: accepted; the alternative is the +current silent loss. Wording was deliberately downgraded in one place and it is +stated plainly: the audio change is presence preservation, **not** audio support, +and the PR says so rather than implying the modality now works. diff --git a/devlog/_plan/260914_provider_parity_stack/050_residuals.md b/devlog/_plan/260914_provider_parity_stack/050_residuals.md new file mode 100644 index 0000000000..326c9a4f28 --- /dev/null +++ b/devlog/_plan/260914_provider_parity_stack/050_residuals.md @@ -0,0 +1,58 @@ +# Residuals + +Work this unit deliberately does not do, with the reason and what would be needed. +Recorded so the PRs can point at it instead of implying coverage they do not have. + +## R1 — opaque reasoning replay across a Chat boundary (from F6) + +Phase 2 carries assistant reasoning **plaintext** into the Responses projection. It +does not carry a thinking signature, an `encrypted_content` blob, or any +provider-issued item id. + +A signature is an attestation the issuing provider computed over content this proxy +never received. Synthesizing one is either rejected upstream or, worse, accepted as +a false provenance claim. Cross-provider opaque metadata has the same problem in +the other direction: the blob is only meaningful to its issuer. + +Doing this properly needs a per-provider decision about which opaque fields are +round-trippable, a scope key so a blob from provider A is never replayed to +provider B, and a cache lifetime. `src/responses/reasoning-replay-cache.ts` +already solves a narrower version of this inside one provider's session and is the +natural starting point. It is a design unit, not a line change. + +## R2 — real audio transport in the translated IR (from F5) + +Phase 4 preserves the *presence* of an audio attachment and explicitly does not add +audio support. `OcxContentPart` has no audio member, no adapter consumes one, and +per-provider audio capability is not recorded anywhere in the catalog — +`src/providers/registry.ts:1062` notes exactly this when it omits audio from the +Baseten hints. + +Adding it means a carrier type, capability data for 93 providers, and a wire mapping +per vendor. Guessing any one of those produces a request that fails at call time +instead of a modality that works. + +## R3 — Kiro remote images stay uninlined + +Phase 4 makes the loss visible. It does not make the image arrive. Kiro's wire takes +base64 bytes only, and fetching a remote reference server-side is explicitly out of +scope for this unit: it would add an outbound request on a request path, with the +SSRF surface and the credential-bearing-URL handling that implies. + +## R4 — Vertex `responseJsonSchema` support is not locally gated + +Phase 3 sends the field on AI Studio and Vertex and refuses on Cloud Code Assist. +There is no local capability table asserting which Vertex model versions accept it, +so a model that rejects it produces an upstream error rather than a local refusal. +Inventing that table without evidence would be a guess with a worse failure mode +than the upstream's own message. + +## R5 — findings owned elsewhere + +- **F10** (native describer ignores operator `modelCapabilities` text-only) is + `#4501` / PR `#4511`. Not duplicated here. +- **`#4505`** gateway modality metadata: the audit found a display/policy + inconsistency, which is not evidence about that gateway's native vision behavior. + Changing it needs real evidence first. +- **Cursor** native/external image path differences were not confirmed as a real + loss, so there is nothing to fix yet. diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index c65f7c378b..4ccd319a5e 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -289,7 +289,9 @@ "catalog-zero-credit-picker.test.ts": "codex-integration", "chat-completions-endpoint.test.ts": "responses", "chat-conversation-affinity.test.ts": "responses", + "chat-inbound-reasoning-none.test.ts": "server", "chat-json-sse-fallback.test.ts": "responses", + "chat-native-image-normalization.test.ts": "server", "chat-refusal.test.ts": "responses", "chat-refusal-scope.test.ts": "responses", "chatgpt-device-auth.test.ts": "oauth", diff --git a/src/chat/image-parts.ts b/src/chat/image-parts.ts new file mode 100644 index 0000000000..cabed6b957 --- /dev/null +++ b/src/chat/image-parts.ts @@ -0,0 +1,118 @@ +/** + * Inbound Chat Completions image parts, recognized once for every consumer. + * + * Two call sites used to answer "does this body carry an image?" independently and + * gave different answers: the translated path understood Pi/MCP and Anthropic-shaped + * parts, while the native fast path's route-eligibility predicate matched only + * `image_url`. A text-only routed model therefore kept an image-bearing body and + * forwarded a non-OpenAI part verbatim to an OpenAI-compatible upstream. + * + * Normalization runs before route selection so the diversion decision and the + * forwarded wire see the same parts. This module deliberately imports nothing: it is + * shared by `src/chat/` and `src/server/` and must not create an edge between them. + */ + +type Rec = Record; + +function isRec(v: unknown): v is Rec { + return !!v && typeof v === "object" && !Array.isArray(v); +} + +/** + * The image reference a Chat content part carries, as a URL or data URI. + * + * Accepts OpenAI `image_url` (string or `{url}`), Pi/MCP-style + * `{type:"image", data, mimeType}` (Aside read_file tool results), and + * Anthropic-shaped `{type:"image", source:{...}}` in both base64 and url form. + * Returns null for anything else — including a part with no usable reference, which + * must be left alone rather than turned into a claim of an attachment. + */ +export function chatImageUrlFromPart(part: Rec): string | null { + if (part.type === "image_url") { + const imageUrl = part.image_url; + if (typeof imageUrl === "string" && imageUrl.length > 0) return imageUrl; + if (isRec(imageUrl) && typeof imageUrl.url === "string" && imageUrl.url.length > 0) return imageUrl.url; + return null; + } + if (part.type === "image") { + const data = part.data; + if (typeof data === "string" && data.length > 0) { + if (data.startsWith("data:")) return data; + const media = typeof part.mimeType === "string" && part.mimeType.length > 0 ? part.mimeType + : typeof part.mediaType === "string" && part.mediaType.length > 0 ? part.mediaType + : "image/png"; + return "data:" + media + ";base64," + data; + } + const source = part.source; + if (isRec(source)) { + if (source.type === "base64" && typeof source.data === "string" && source.data.length > 0) { + const media = typeof source.media_type === "string" && source.media_type.length > 0 ? source.media_type : "image/png"; + return "data:" + media + ";base64," + source.data; + } + if (source.type === "url" && typeof source.url === "string" && source.url.length > 0) return source.url; + } + } + return null; +} + +/** The fidelity hint a recognized part carries, when it is one the wire accepts. */ +export function chatImageDetailFromPart(part: Rec): "auto" | "low" | "high" | undefined { + const raw = isRec(part.image_url) ? part.image_url.detail : part.detail; + return raw === "auto" || raw === "low" || raw === "high" ? raw : undefined; +} + +/** + * True when any `messages[].content[]` part carries a recognized image, in any of + * the accepted shapes. This is the predicate native-route eligibility depends on, so + * widening `chatImageUrlFromPart` widens the text-only diversion with it. + */ +export function chatBodyCarriesImage(rawBody: Rec): boolean { + const messages = rawBody.messages; + if (!Array.isArray(messages)) return false; + for (const message of messages) { + if (!isRec(message) || !Array.isArray(message.content)) continue; + for (const part of message.content) { + if (isRec(part) && chatImageUrlFromPart(part) !== null) return true; + } + } + return false; +} + +/** + * Rewrite every recognized non-OpenAI image part into `image_url` form. + * + * Returns the SAME object reference when nothing needed rewriting, so a body with no + * image — and a body whose images are already `image_url` — is passed through + * untouched. The native path is a whitelist passthrough, so an incidental deep clone + * would itself be a behavior change: only the `messages` array, the messages holding + * a rewritten part, and their `content` arrays are rebuilt. Every sibling part, + * every other message field and every top-level body field keep their exact value. + * + * Each rewritten Pi/Anthropic base64 part costs one copy of its payload string. On + * the translated path that copy already happened inside the old recognizer; on the + * native path it is new peak memory, bounded by the inbound body limit that + * `readChatBody` already enforces. + */ +export function normalizeChatImageParts(rawBody: Rec): Rec { + const messages = rawBody.messages; + if (!Array.isArray(messages)) return rawBody; + let bodyChanged = false; + const nextMessages = messages.map(message => { + if (!isRec(message) || !Array.isArray(message.content)) return message; + let messageChanged = false; + const nextContent = message.content.map(part => { + // Already-OpenAI parts are left byte-identical; only foreign shapes are rewritten. + if (!isRec(part) || part.type === "image_url") return part; + const url = chatImageUrlFromPart(part); + if (url === null) return part; + messageChanged = true; + const detail = chatImageDetailFromPart(part); + return { type: "image_url", image_url: { url, ...(detail ? { detail } : {}) } }; + }); + if (!messageChanged) return message; + bodyChanged = true; + return { ...message, content: nextContent }; + }); + if (!bodyChanged) return rawBody; + return { ...rawBody, messages: nextMessages }; +} diff --git a/src/chat/inbound.ts b/src/chat/inbound.ts index 6e4ebd833b..597724fb7a 100644 --- a/src/chat/inbound.ts +++ b/src/chat/inbound.ts @@ -5,6 +5,8 @@ * Same translate-and-replay pattern as Claude Messages: the produced body must pass * responsesRequestSchema so routing/OAuth/pool/sidecars are inherited unchanged. */ +import { chatImageUrlFromPart } from "./image-parts"; + export class ChatCompletionsRequestError extends Error {} type Rec = Record; @@ -25,7 +27,12 @@ export function assertChatCompletionsRoutingBody(raw: unknown): asserts raw is C } } -const OUTPUT_CONFIG_EFFORTS = new Set(["minimal", "low", "medium", "high", "xhigh", "max", "ultra"]); +// "none" is the runtime's disable sentinel, not an unknown value: src/reasoning-effort.ts +// accepts it and maps it to "omit the reasoning parameter", and the Pi client export maps +// Pi's "off" thinking level onto it (src/clients/config-export.ts). Dropping it here let a +// provider default re-enable thinking the caller had explicitly turned off — and for the +// Anthropic families that think by default, omission is not the same as disabled. +const OUTPUT_CONFIG_EFFORTS = new Set(["none", "minimal", "low", "medium", "high", "xhigh", "max", "ultra"]); const OUTPUT_CONFIG_SUMMARIES = new Set(["auto", "concise", "detailed", "none"]); function contentToText(content: unknown): string { @@ -45,38 +52,9 @@ function contentToText(content: unknown): string { return parts.join("\n"); } -function imageUrlFromPart(part: Rec): string | null { - if (part.type === "image_url") { - const imageUrl = part.image_url; - if (typeof imageUrl === "string" && imageUrl.length > 0) return imageUrl; - if (isRec(imageUrl) && typeof imageUrl.url === "string" && imageUrl.url.length > 0) return imageUrl.url; - return null; - } - // Agent clients whose native wire shape is not OpenAI's still send images over - // Chat Completions: Pi/MCP-style parts carry {type:"image", data, mimeType} - // (Aside read_file tool results), Anthropic-shaped clients carry a source - // object. Dropping either silently blinds a vision model, so normalize both - // to the URL/data-URI form the Responses pipeline already understands. - if (part.type === "image") { - const data = part.data; - if (typeof data === "string" && data.length > 0) { - if (data.startsWith("data:")) return data; - const media = typeof part.mimeType === "string" && part.mimeType.length > 0 ? part.mimeType - : typeof part.mediaType === "string" && part.mediaType.length > 0 ? part.mediaType - : "image/png"; - return "data:" + media + ";base64," + data; - } - const source = part.source; - if (isRec(source)) { - if (source.type === "base64" && typeof source.data === "string" && source.data.length > 0) { - const media = typeof source.media_type === "string" && source.media_type.length > 0 ? source.media_type : "image/png"; - return "data:" + media + ";base64," + source.data; - } - if (source.type === "url" && typeof source.url === "string" && source.url.length > 0) return source.url; - } - } - return null; -} +// Recognition moved to src/chat/image-parts.ts so the native fast path's +// route-eligibility predicate and this translator cannot drift apart again. +const imageUrlFromPart = chatImageUrlFromPart; function videoUrlFromPart(part: Rec): string | null { if (part.type !== "video_url") return null; diff --git a/src/server/chat-completions.ts b/src/server/chat-completions.ts index b50af22cb5..947f71f240 100644 --- a/src/server/chat-completions.ts +++ b/src/server/chat-completions.ts @@ -11,6 +11,7 @@ import { ChatCompletionsRequestError, chatCompletionsToResponsesBody, } from "../chat/inbound"; +import { normalizeChatImageParts } from "../chat/image-parts"; import { chatCompletionsErrorResponse, collectChatCompletion, @@ -111,7 +112,11 @@ async function handleChatCompletionsWithBudget( try { const rawBody = await readChatBody(req, translatorBudget, resolveInboundBodyLimitBytes(config.maxInboundBodyBytes)); assertChatCompletionsRoutingBody(rawBody); - chatBody = rawBody; + // Normalize foreign image shapes BEFORE routing. isNativeChatRouteEligible below + // decides the pipeline from the image parts it can see, and the native path then + // forwards this body as-is, so both must observe the same parts. A body with no + // foreign image part is returned by reference and stays byte-identical. + chatBody = normalizeChatImageParts(rawBody); } catch (err) { const overflow = isTranslatorBudgetExceededError(err); const status = overflow ? 413 : err instanceof ChatCompletionsRequestError ? 400 : 500; diff --git a/src/server/chat-native.ts b/src/server/chat-native.ts index 8cc633665c..c2614c8510 100644 --- a/src/server/chat-native.ts +++ b/src/server/chat-native.ts @@ -1,4 +1,5 @@ import { buildOpenAIChatPassthroughRequest, createOpenAIChatAdapter } from "../adapters/openai-chat"; +import { chatBodyCarriesImage } from "../chat/image-parts"; import type { AdapterRequest, ProviderAdapter } from "../adapters/base"; import { chatCompletionsErrorBody, @@ -164,19 +165,6 @@ export function isNativeChatRouteEligible(route: RouteResult, rawBody: Rec): boo return true; } -/** Any messages[].content[] part of type image_url. */ -function chatBodyCarriesImage(rawBody: Rec): boolean { - const messages = rawBody.messages; - if (!Array.isArray(messages)) return false; - for (const message of messages) { - if (!isRec(message) || !Array.isArray(message.content)) continue; - for (const part of message.content) { - if (isRec(part) && part.type === "image_url") return true; - } - } - return false; -} - function chatCompletionJson(value: unknown): Rec | null { if (!isRec(value) || !Array.isArray(value.choices) || value.choices.length === 0) return null; return value; diff --git a/structure/data-planes/inbound-compat.md b/structure/data-planes/inbound-compat.md index 8b27dd5688..49a24b39aa 100644 --- a/structure/data-planes/inbound-compat.md +++ b/structure/data-planes/inbound-compat.md @@ -247,3 +247,33 @@ Translated Chat request construction uses the [inline-image budget](../transport The [explicit model-capability contract](../config.md#explicit-per-model-capability-declarations) preserves operator declarations through provider storage and catalog capture; it does not infer upstream capability or change this surface's routing behavior. Provider-scoped approval reviewer settings are projected by the [catalog owner](../catalog.md#provider-scoped-approval-reviewer); this surface retains its existing routing, transport and account-selection behavior. + +## Shared inbound Chat image recognition + +`src/chat/image-parts.ts` owns which `messages[].content[]` shapes count as an image +on the Chat Completions ingress: OpenAI `image_url` in both spellings, Pi/MCP +`{type:"image", data, mimeType}`, and Anthropic-shaped `{type:"image", source}` in +base64 and url form. The translator and the native fast path both read it, because +they previously answered that question separately and disagreed: native +route-eligibility matched only `image_url`, so a text-only routed model kept a +Pi-shaped or Anthropic-shaped image body and the native whitelist passthrough +forwarded the foreign part verbatim. + +`normalizeChatImageParts` runs in `handleChatCompletionsWithBudget` immediately +after routing-body validation and before `routeModel`, so the text-only diversion in +`isNativeChatRouteEligible` and the forwarded native wire observe the same parts. It +rewrites only recognized foreign parts into `image_url` form and returns its input by +reference when nothing matched, so a body with no image — and one already in OpenAI +shape — stays byte-identical. Sibling parts, message fields and top-level body fields +are preserved; the native path is a whitelist passthrough, so an incidental deep clone +would itself be a behavior change. A remote reference is recognized and rewritten, +never fetched. + +## Explicit reasoning disable on the Chat ingress + +The Chat inbound effort allowlist accepts `none` alongside the ladder values. +`none` is the runtime's disable sentinel — `src/reasoning-effort.ts` maps it to +omitting the wire parameter, and the Pi client export maps Pi's `off` thinking level +onto it. Dropping it let a provider default re-enable reasoning the caller had +explicitly turned off, which is not neutral for the Anthropic families that think by +default and require an explicit `thinking:{type:"disabled"}` to stop. diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 145cbfc22b..9cbdbaf660 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -122,7 +122,9 @@ "catalog-zero-credit-picker.test.ts": "codex-integration", "chat-completions-endpoint.test.ts": "responses", "chat-conversation-affinity.test.ts": "responses", + "chat-inbound-reasoning-none.test.ts": "server", "chat-json-sse-fallback.test.ts": "responses", + "chat-native-image-normalization.test.ts": "server", "chat-refusal.test.ts": "responses", "chat-refusal-scope.test.ts": "responses", "chatgpt-device-auth.test.ts": "oauth", diff --git a/tests/server/chat-inbound-reasoning-none.test.ts b/tests/server/chat-inbound-reasoning-none.test.ts new file mode 100644 index 0000000000..4a453ccd55 --- /dev/null +++ b/tests/server/chat-inbound-reasoning-none.test.ts @@ -0,0 +1,56 @@ +/** + * Audit F7 (2026-09-14): the Chat inbound effort allowlist omitted "none", so an + * explicit request to disable reasoning was dropped as if nothing had been asked. + * + * "none" is the runtime's disable sentinel, not an unknown string: + * src/reasoning-effort.ts accepts it and maps it to "omit the reasoning parameter", + * and the Pi client export maps Pi's "off" level onto it + * (src/clients/config-export.ts). For Anthropic families that think by default, + * omitting the field is NOT equivalent to disabling — only an explicit + * thinking:{type:"disabled"} turns thinking off (src/adapters/anthropic.ts:960-966). + * So dropping "none" silently re-enabled thinking the caller had turned off. + */ +import { describe, expect, test } from "bun:test"; +import { chatCompletionsToResponsesBody } from "../../src/chat/inbound"; +import { responsesRequestSchema } from "../../src/responses/schema"; + +function chat(extra: Record): Record { + return { model: "m", messages: [{ role: "user", content: "hi" }], ...extra }; +} + +function reasoningOf(body: Record): Record | undefined { + return body.reasoning as Record | undefined; +} + +describe("F7 explicit reasoning disable survives the Chat boundary", () => { + test("preserves a flat reasoning_effort of none", () => { + const body = chatCompletionsToResponsesBody(chat({ reasoning_effort: "none" })); + expect(reasoningOf(body)?.effort).toBe("none"); + }); + + test("preserves the nested reasoning.effort spelling", () => { + const body = chatCompletionsToResponsesBody(chat({ reasoning: { effort: "none" } })); + expect(reasoningOf(body)?.effort).toBe("none"); + }); + + test("the produced body still validates against responsesRequestSchema", () => { + const body = chatCompletionsToResponsesBody(chat({ reasoning_effort: "none" })); + expect(responsesRequestSchema.safeParse(body).success).toBe(true); + }); + + test("every other ladder value is unchanged", () => { + for (const effort of ["minimal", "low", "medium", "high", "xhigh", "max", "ultra"]) { + expect(reasoningOf(chatCompletionsToResponsesBody(chat({ reasoning_effort: effort })))?.effort).toBe(effort); + } + }); + + test("an unknown effort is still ignored rather than forwarded", () => { + const body = chatCompletionsToResponsesBody(chat({ reasoning_effort: "turbo" })); + expect(reasoningOf(body)?.effort).toBeUndefined(); + }); + + test("omitting an effort entirely still produces no effort", () => { + const body = chatCompletionsToResponsesBody(chat({})); + expect(reasoningOf(body)?.effort).toBeUndefined(); + }); +}); diff --git a/tests/server/chat-native-image-normalization.test.ts b/tests/server/chat-native-image-normalization.test.ts new file mode 100644 index 0000000000..f4a5894bb3 --- /dev/null +++ b/tests/server/chat-native-image-normalization.test.ts @@ -0,0 +1,163 @@ +/** + * Audit F1 (2026-09-14): the native Chat fast path recognized only `image_url`, + * while the translated path also understood Pi/MCP `{type:"image", data, mimeType}` + * and Anthropic-shaped `{type:"image", source}` parts. + * + * Two failures followed from that one gap. A text-only routed model kept an + * image-bearing body, because `isNativeChatRouteEligible` could not see the image. + * And the native path is a whitelist passthrough, so the foreign part was forwarded + * verbatim to an OpenAI-compatible upstream that does not accept it. + * + * These assert the desired behavior: one shared recognizer, and normalization before + * route selection. No network is involved — a remote `source.type:"url"` is + * recognized and rewritten, never fetched. + */ +import { describe, expect, test } from "bun:test"; +import { + chatBodyCarriesImage, + chatImageUrlFromPart, + normalizeChatImageParts, +} from "../../src/chat/image-parts"; +import { isNativeChatRouteEligible } from "../../src/server/chat-native"; +import type { OcxProviderConfig } from "../../src/types"; +import type { RouteResult } from "../../src/router"; + +const PNG = "iVBORw0KGgoAAAANSUhEUg=="; + +function route(overrides: Partial = {}, modelId = "vision-model"): RouteResult { + return { + provider: { + adapter: "openai-chat", + baseUrl: "https://gateway.example/v1", + authMode: "key", + apiKey: "test-key", + ...overrides, + }, + providerName: "gateway", + modelId, + } as unknown as RouteResult; +} + +/** + * An operator-declared text-only model: the case that must be diverted. + * isModelVisionSidecarConsumer (src/vision/eligibility.ts:79-89) reads an explicit + * modelCapabilities.inputModalities declaration first, so ["text"] without "image" + * is the operator saying this model is blind. + */ +function textOnlyRoute(): RouteResult { + return route({ modelCapabilities: { "text-only-model": { inputModalities: ["text"] } } }, "text-only-model"); +} + +function userBody(parts: unknown[]): Record { + return { model: "m", messages: [{ role: "user", content: parts }] }; +} + +describe("F1 shared inbound image recognition", () => { + test("recognizes the OpenAI shape in both spellings", () => { + expect(chatImageUrlFromPart({ type: "image_url", image_url: { url: "https://x/i.png" } })).toBe("https://x/i.png"); + expect(chatImageUrlFromPart({ type: "image_url", image_url: "https://x/j.png" })).toBe("https://x/j.png"); + }); + + test("recognizes a Pi/MCP part and builds a data URI from mimeType", () => { + expect(chatImageUrlFromPart({ type: "image", data: PNG, mimeType: "image/png" })) + .toBe(`data:image/png;base64,${PNG}`); + }); + + test("recognizes both Anthropic source forms", () => { + expect(chatImageUrlFromPart({ type: "image", source: { type: "base64", media_type: "image/jpeg", data: PNG } })) + .toBe(`data:image/jpeg;base64,${PNG}`); + expect(chatImageUrlFromPart({ type: "image", source: { type: "url", url: "https://x/k.png" } })) + .toBe("https://x/k.png"); + }); + + test("returns null for a part carrying no usable reference", () => { + expect(chatImageUrlFromPart({ type: "image" })).toBeNull(); + expect(chatImageUrlFromPart({ type: "text", text: "hi" })).toBeNull(); + }); +}); + +describe("F1 normalization before route selection", () => { + test("rewrites a Pi part into image_url form", () => { + const body = userBody([{ type: "text", text: "look" }, { type: "image", data: PNG, mimeType: "image/png" }]); + const out = normalizeChatImageParts(body); + const content = (out.messages as Record[])[0]!.content as Record[]; + + expect(content[1]).toEqual({ type: "image_url", image_url: { url: `data:image/png;base64,${PNG}` } }); + // The sibling text part and its order are untouched. + expect(content[0]).toEqual({ type: "text", text: "look" }); + }); + + test("preserves a detail hint through the rewrite", () => { + const out = normalizeChatImageParts(userBody([{ type: "image", data: PNG, mimeType: "image/png", detail: "high" }])); + const content = (out.messages as Record[])[0]!.content as Record[]; + expect(content[0]).toEqual({ type: "image_url", image_url: { url: `data:image/png;base64,${PNG}`, detail: "high" } }); + }); + + test("normalizes an image-only message with no text part", () => { + const out = normalizeChatImageParts(userBody([{ type: "image", source: { type: "url", url: "https://x/o.png" } }])); + const content = (out.messages as Record[])[0]!.content as Record[]; + expect(content[0]).toEqual({ type: "image_url", image_url: { url: "https://x/o.png" } }); + }); + + test("normalizes a tool message's image part", () => { + const body = { + model: "m", + messages: [{ role: "tool", tool_call_id: "call1", content: [{ type: "image", data: PNG, mimeType: "image/png" }] }], + }; + const content = (normalizeChatImageParts(body).messages as Record[])[0]!.content as Record[]; + expect(content[0]).toMatchObject({ type: "image_url" }); + }); + + test("returns the identical reference when there is no image", () => { + const body = userBody([{ type: "text", text: "plain" }]); + expect(normalizeChatImageParts(body)).toBe(body); + }); + + test("returns the identical reference when images are already image_url", () => { + const body = userBody([{ type: "image_url", image_url: { url: "https://x/p.png" } }]); + expect(normalizeChatImageParts(body)).toBe(body); + }); + + test("leaves every other body field untouched", () => { + const body = { ...userBody([{ type: "image", data: PNG, mimeType: "image/png" }]), temperature: 0.5, stream: true }; + const out = normalizeChatImageParts(body); + expect(out.temperature).toBe(0.5); + expect(out.stream).toBe(true); + expect(out.model).toBe("m"); + }); +}); + +describe("F1 text-only diversion sees every image shape", () => { + test("diverts a Pi-shaped image away from the native fast path", () => { + expect(chatBodyCarriesImage(userBody([{ type: "image", data: PNG, mimeType: "image/png" }]))).toBe(true); + expect(isNativeChatRouteEligible(textOnlyRoute(), userBody([{ type: "image", data: PNG, mimeType: "image/png" }]))).toBe(false); + }); + + test("diverts an Anthropic base64 image", () => { + const body = userBody([{ type: "image", source: { type: "base64", media_type: "image/png", data: PNG } }]); + expect(isNativeChatRouteEligible(textOnlyRoute(), body)).toBe(false); + }); + + test("diverts an Anthropic remote-url image without fetching it", () => { + const body = userBody([{ type: "image", source: { type: "url", url: "https://x/q.png" } }]); + expect(isNativeChatRouteEligible(textOnlyRoute(), body)).toBe(false); + }); + + test("diverts an image carried by a tool message", () => { + const body = { + model: "m", + messages: [{ role: "tool", tool_call_id: "call1", content: [{ type: "image", data: PNG, mimeType: "image/png" }] }], + }; + expect(chatBodyCarriesImage(body)).toBe(true); + }); + + test("a text-only body still takes the native fast path", () => { + expect(chatBodyCarriesImage(userBody([{ type: "text", text: "plain" }]))).toBe(false); + expect(isNativeChatRouteEligible(textOnlyRoute(), userBody([{ type: "text", text: "plain" }]))).toBe(true); + }); + + test("a vision-capable route keeps an image-bearing body on the native path", () => { + const body = userBody([{ type: "image", data: PNG, mimeType: "image/png" }]); + expect(isNativeChatRouteEligible(route(), body)).toBe(true); + }); +}); From 6c20646a31ba991d59a63d7ae6b640208021b149 Mon Sep 17 00:00:00 2001 From: JUN Date: Mon, 14 Sep 2026 02:47:07 +0900 Subject: [PATCH 2/3] [agent] test: place the new Chat regressions in their seeded layout domain Hosted CI caught this: tests/test-layout-tooling.test.ts reported "chat-inbound-reasoning-none.test.ts: seed responses != server" for both new files. scripts/test-layout/layout.json seeds a `chat-` prefix to the `responses` domain, which is where the sibling Chat-translation tests already live, so registering them under `server` contradicted the seed. Moves both files to tests/responses/ and registers them there. Import depth is unchanged, so no import edits were needed. Local verification NOT RUN BY USER INSTRUCTION. --- scripts/test-layout/layout.json | 4 ++-- tests/fixtures/test-layout-expected.json | 4 ++-- .../{server => responses}/chat-inbound-reasoning-none.test.ts | 0 .../chat-native-image-normalization.test.ts | 0 4 files changed, 4 insertions(+), 4 deletions(-) rename tests/{server => responses}/chat-inbound-reasoning-none.test.ts (100%) rename tests/{server => responses}/chat-native-image-normalization.test.ts (100%) diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index 4ccd319a5e..2067ecf819 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -289,9 +289,9 @@ "catalog-zero-credit-picker.test.ts": "codex-integration", "chat-completions-endpoint.test.ts": "responses", "chat-conversation-affinity.test.ts": "responses", - "chat-inbound-reasoning-none.test.ts": "server", + "chat-inbound-reasoning-none.test.ts": "responses", "chat-json-sse-fallback.test.ts": "responses", - "chat-native-image-normalization.test.ts": "server", + "chat-native-image-normalization.test.ts": "responses", "chat-refusal.test.ts": "responses", "chat-refusal-scope.test.ts": "responses", "chatgpt-device-auth.test.ts": "oauth", diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 9cbdbaf660..2030949943 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -122,9 +122,9 @@ "catalog-zero-credit-picker.test.ts": "codex-integration", "chat-completions-endpoint.test.ts": "responses", "chat-conversation-affinity.test.ts": "responses", - "chat-inbound-reasoning-none.test.ts": "server", + "chat-inbound-reasoning-none.test.ts": "responses", "chat-json-sse-fallback.test.ts": "responses", - "chat-native-image-normalization.test.ts": "server", + "chat-native-image-normalization.test.ts": "responses", "chat-refusal.test.ts": "responses", "chat-refusal-scope.test.ts": "responses", "chatgpt-device-auth.test.ts": "oauth", diff --git a/tests/server/chat-inbound-reasoning-none.test.ts b/tests/responses/chat-inbound-reasoning-none.test.ts similarity index 100% rename from tests/server/chat-inbound-reasoning-none.test.ts rename to tests/responses/chat-inbound-reasoning-none.test.ts diff --git a/tests/server/chat-native-image-normalization.test.ts b/tests/responses/chat-native-image-normalization.test.ts similarity index 100% rename from tests/server/chat-native-image-normalization.test.ts rename to tests/responses/chat-native-image-normalization.test.ts From 279ba0ad78175cd405a2b41c03bfec8a1c27e8d1 Mon Sep 17 00:00:00 2001 From: JUN Date: Mon, 14 Sep 2026 03:29:00 +0900 Subject: [PATCH 3/3] [agent] docs: correct the stale reason for dropping the opencode none variant Independent review found this comment now states a false fact about this stack's own change. It said the chat ingress allowlist OUTPUT_CONFIG_EFFORTS "has no none", which stopped being true in 63fbe66907 when F7 added the disable sentinel to that allowlist. The filter itself is kept, narrowly and on a stated basis: emitting the variant would change what this exporter writes into a user's opencode config, and whether opencode's picker round-trips reasoningEffort "none" back to a wire this proxy reads has not been verified. Re-enabling it is a scoped follow-up needing that check, not a side effect of an ingress fix. MCode and ZCode filter none for their own separate reasons, which remain accurate at their call sites. Comment-only; no behavior change. Local verification NOT RUN BY USER INSTRUCTION. --- src/clients/config-export.ts | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/clients/config-export.ts b/src/clients/config-export.ts index 2aac798de4..21f0858179 100644 --- a/src/clients/config-export.ts +++ b/src/clients/config-export.ts @@ -627,10 +627,20 @@ function opencodeProviderConnection(baseURL: string, config: OcxConfig): Opencod * override a default the user controls in opencodex. Variants are opt-in per selection, * which is the same reason we never emit `defaultModel` for MCode. * - * `none` is dropped even when a ladder declares it. It is a valid *declared* effort, but the - * chat ingress filters wire efforts against `OUTPUT_CONFIG_EFFORTS`, which has no `none`, so - * selecting it would send no effort at all and silently fall back to the proxy default — a - * selectable value that cannot do what its label says. Same call MCode makes for its picker. + * `none` is dropped even when a ladder declares it. + * + * The original reason no longer holds and is recorded here so it is not repeated: the chat + * ingress `OUTPUT_CONFIG_EFFORTS` allowlist DID omit `none`, so selecting it sent no effort + * at all and fell back to the proxy default. That allowlist now accepts `none` (audit F7), + * because it is the runtime's disable sentinel and dropping it let a provider default + * re-enable thinking a caller had turned off. + * + * The variant stays filtered anyway, deliberately and narrowly: emitting it would change + * what this exporter writes into a user's opencode config, and whether opencode's own + * picker round-trips `reasoningEffort: "none"` to the wire this proxy reads has not been + * verified here. Re-enabling it is a scoped follow-up that needs that check first, not a + * side effect of an ingress fix. MCode and ZCode filter `none` for their own separate + * reasons, documented at their call sites. */ function opencodeEffortVariants(model: OpencodeCatalogModel): OpencodeModelVariant[] | undefined { if (model.reasoningEfforts === undefined) return undefined;