Conversation
A tool_result whose content is a document block translated to an empty tool output, so routed models treated the attachment as a tool that returned nothing. Surface the same "[document: title]" marker the user-message path already emits.
Contributor
📝 WalkthroughWalkthroughChangesTool-result document handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This was referenced Aug 3, 2026
Owner
|
Carried onto the review stack as #953 (stack 3/3), unmodified. Your commits were taken with Verified on the stack: This PR stays open until #953 lands. If a maintainer prefers to take yours directly instead, that path is unaffected — the stack commits get dropped and this one merges. Once #953 merges I'll close this as carried, with the credit already in the commit history rather than in a comment. Stack: #951 (plan, base Thanks for the fix. |
This was referenced Aug 3, 2026
Wibias
marked this pull request as draft
August 3, 2026 20:50
DevMello
marked this pull request as ready for review
August 3, 2026 22:19
Wibias
marked this pull request as draft
August 3, 2026 22:38
Contributor
|
Leave it as draft. Thanks. It doesnt need any more changes as you can see in juns comment on which you didn't respond to. |
Owner
|
Closing this as carried into #953, not as rejected. Your fix is shipping — this is bookkeeping so the same change isn't sitting in the review queue twice. Your commit(s) are on the stack branch as $ git show <yours> | git patch-id --stable
$ git show <carried> | git patch-id --stable
IDENTICALNo content was changed and no conflict resolution was needed. One thing I want to be straight about: my earlier comment said I'd close this after #953 merged, and I'm closing it before. #953 is open and reviewable now, so closing early is my call as maintainer to keep the queue honest — not a signal that the review finished. If the stack doesn't land, reopening this is one click, and the branch is untouched. Verified on the stack: Stack: #951 (plan, base Thanks @DevMello — this was a clean fix and it's in. |
3 tasks
3 tasks
3 tasks
lidge-jun
added a commit
that referenced
this pull request
Sep 20, 2026
…5212, #5213) (#5271) * fix(chat): carry allowed_tools and a caller's parallel_tool_calls to the wire Two ways a Chat Completions caller restricts tool use reached the parser and were then dropped on the way out, both under a normal HTTP 200. A tool_choice of type allowed_tools is a record, is not type "function", and carries no "function" member, so it fell past every branch of the Chat inbound translator and body.tool_choice was never assigned. The upstream received the full catalogue and no choice at all. Chat nests the subset under allowed_tools and names each entry under a member keyed by its own type, while the Responses shape mapToolChoice reads carries mode and tools on the choice itself with a flat name, so neither level lined up. Flatten both. An entry nobody can name is refused rather than skipped, because dropping one widens the very subset the field was sent to narrow. parallel_tool_calls had three provider states and two branches, in two places. When a provider expresses no preference, which is the default for every provider that never configured the knob, neither branch ran and an explicit request-level false was lost — on the translated path and, from its own copy of the same branch, on the native Chat passthrough. That state now forwards the caller's false. An explicit true still omits the key, matching the configured opt-out, so strict OpenAI-compatible hosts never see a knob they did not have to accept before. The NVIDIA and pinParallelToolCallsFalse pins are unchanged. The decision moved into openai-chat/parallel-tool-calls.ts, which both builders now read, so the three states cannot drift between them again. openai-chat.ts is 811 lines against its 822-line cap. Regressions assert on the serialized outbound request body for both builders, since a successful tool call and a 200 response look identical with or without either constraint. Closes #5211 * fix(adapters): preserve tool declaration strict and allowed_callers Three fields a caller sets on a tool declaration were parsed, carried internally, and then dropped by the outbound adapter, so the request was dispatched as though the constraint were in force and answered normally. Messages to Messages rebuilt every tool from name, description and input_schema alone. Anthropic is the target that defines strict, the Messages inbound already kept the source intent deliberately, and the OpenAI Chat adapter already forwarded it, so Anthropic was the one destination losing it. It now emits an explicit strict: true. An unstated strict stays absent: the inbound records it as false, so a false on the wire cannot be told apart from silence and must not become an opt-out nobody asked for. allowed_callers had no carrier at all. The identifier existed once in the tree, raising a caller_mode diagnostic that only becomes a refusal when the operator has set claudeCode.compatibility. The field now rides OcxTool.allowedCallers from the Messages inbound through the Responses schema — where an undeclared key is stripped, which is why it never reached buildTools — to the Anthropic wire. The OpenAI Chat and Gemini builders have no counterpart for it, so they refuse with a 400 rather than rebuild the declaration without the fence, in the shape ollama-native and kiro already use for a tool_choice they cannot enforce. The unrestricted ["direct"] default is not treated as a restriction. Gemini expresses schema-enforced calling as functionCallingConfig.mode VALIDATED. The mode was plumbed to the wire compiler but only reachable by matching a model name, so a strict declaration arrived as an ordinary AUTO turn. It now replaces the absent-choice default. NONE, ANY and a forced-name choice are stronger constraints the caller asked for and are never overwritten. Native passthrough is unaffected on every route. Closes #5210 * fix(openai-chat): keep developer messages in their conversation position A developer message kept its slot only when the provider base URL host was exactly api.openai.com. On every other OpenAI-compatible Chat endpoint its text was appended to the system prompt and the message itself was skipped, so an instruction written to apply from the second turn onward arrived ahead of the first one and the caller got an ordinary completion either way. The two halves of a Claude Code route were working against each other because of it: #4161 established that folding in-conversation instructions into the prompt preamble is harmful and made the Claude inbound mint chronological developer items specifically to preserve timeline order, and this adapter then folded them again on every host but one. One destination already had the chronological behaviour, keyed to a model id and a registry entry, because hoisting a newly appended reminder rewrites the reusable prompt prefix. That is a property of prompt-prefix caching rather than of that destination, so it is now what every destination gets, and the model/registry test is gone. A reminder that arrives while a tool call is open is still deferred past the result, which is what keeps tool-call adjacency intact; it lands in its own slot immediately after, never at the front. This commit changes placement only. The wire role is still developer on api.openai.com and system elsewhere, and is addressed separately. Co-authored-by: Yum-wu <118118663+Yum-wu@users.noreply.github.com> * fix(openai-chat): forward the developer role instead of inferring it from the host A developer message reached the upstream as developer only when the provider base URL host was exactly api.openai.com. Everywhere else it was rewritten to system, so every OpenAI-compatible gateway was assumed not to support a standard Chat Completions role until proven otherwise — including gateways that proxy OpenAI itself — and the instruction silently lost the precedence the caller chose. The role is now forwarded as sent. A destination that genuinely rejects it sets foldDeveloperRoleToSystem, which converts the role where the message already is and never moves it, so the placement contract from the previous commit holds on both paths. That makes the conversion a recorded decision about one destination rather than an inference from its hostname, which is what the hostname test could never express. The flag is registered in the provider config schema and in the exhaustive provider field policy, which is keyed on keyof OcxProviderConfig and fails typecheck until a new key is classified. Closes #5213 * fix(inbound): carry inline document bytes through to the wires that hold them Both inbound parsers reduced an attached document to its name before any adapter ran, so no adapter could forward one even to a target that has a representation for it. The Messages inbound replaced a base64 document block with a "[document: title]" marker, and the Chat file part matched no branch of the content loop at all. The request succeeded either way, so the caller could not tell "the model read the document" from "the model was told a document existed". OcxContentPart gains a document member carrying the media type and the base64 payload. The Anthropic wire emits it as the document block the caller sent, the OpenAI Chat wire as the file part that is its direct counterpart, and Gemini as the inline_data part it already uses for images and video. Widening the union is the hazard here, so the part also carries the marker every text-only consumer already falls back to. That keeps a wire with no document representation emitting exactly what it emitted before instead of undefined or a mislabelled [video]. Six consumers needed more than the fallback and are fixed explicitly: ollama-native and the Cursor tool-result decoder would have read a nonexistent imageUrl, and Kiro, Devin, Cursor and coding-agent text serializers would have produced an empty turn. Token admission counts the encoded payload rather than the marker. The untranslated-media refusal is narrowed to match, and only where a converter actually builds the part: user content on the Chat projection, user and developer messages on the Responses one. A file in a tool output, a system message or an assistant message is still refused, because those converters flatten their content to a string and exempting them would restore the silent drop the scanner exists to prevent. The scanner and the decoder share one predicate, so a request cannot be exempted in one and reduced to a marker in the other; a ";notbase64," parameter is not a payload. A reference with no bytes — a file_id, a remote source — is unchanged in every position. Tool-result documents keep the #939 marker: the Responses tool-output vocabulary has no file block and every adapter's tool-result path flattens to text, so carrying bytes there needs a separate change. Closes #5212 * fix(adapters): refuse unrepresentable declarations by default, not per adapter Adversarial review of the whole branch found the same shape of hole in two of its fixes: a constraint the normalized request now carries still reached wires that rebuild the declaration or the message without it, and answered normally. tools[].allowed_callers was refused by the OpenAI Chat and Gemini builders because those are the two the report named. Cursor, Devin, Kiro, Command Code, Ollama and the coding-agent wires rebuild tools from name, description and schema, so a caller-restricted tool reached those upstreams unrestricted. Inline document bytes had the same problem from the other direction: admission exempted every user-content document without knowing the destination, and a wire with no carrier replaced the bytes with the marker and continued. Both are now default-deny allowlists in adapters/declaration-carrier.ts, enforced at the single guard in adapters/input-media-guard.ts that every registered adapter passes through. allowed_callers reaches the anthropic wire; document bytes reach anthropic, openai-chat and google. Adding an AdapterWire member makes the omission visible in those lists rather than at a customer's upstream, which a per-adapter opt-in could never do. The Responses passthrough stays exempt from the whole guard because it forwards the original body. The refusal no longer names the tool, which is caller-controlled and put client metadata into an error body. An allowed_tools entry whose selector kind is neither function, custom, nor a hosted type is refused rather than flattened to a bare name. Token admission counts a document's payload arithmetically instead of rebuilding a request-sized data URL to measure it. * test(document): derive the attachment marker from its source constant A restated literal is the union-defect class AGENTS.md records: the next change to the marker breaks a test for the wording rather than for the contract. Every assertion about it now reads inlineDocumentMarker, and the data URL spelling comes from inlineDocumentDataUrl. * fix(adapters): type the document scan against OcxMessage and keep a developer document's role Two defects from a final adversarial pass over the branch. The document scan took content shaped as OcxContentPart[], but context.messages is OcxMessage[] and an assistant turn carries OcxAssistantContentPart[], which is not assignable to the user-content union. It now takes OcxMessage and reads the discriminant structurally, which is all it ever needed. A developer message carrying a document reached the structured-content branch and was emitted as role user, undoing the role preservation the same adapter had just established. A developer message with images keeps the user-compatible shape it has always had on this wire; a document has no such precedent and keeps its role. * docs(devlog): record lane A meaning preservation What each of the four contracts restores, the review findings that changed the shape of the fix, the union-defect check run before push, and the one gap left open. * docs(structure): repoint the instruction-ordering links at the renamed heading Renaming the section from the OpenCode Go exception to the universal contract left five documents linking a heading anchor that no longer exists, which is what the SSOT gate is for. The link text now describes the contract rather than the destination it used to be scoped to. * test(anthropic): await the registered adapter build createRegisteredAdapter wraps openai-chat in withClinePassDeepSeekV4ToolReplayCompatibility, whose buildRequest is async, so reading .body off the returned promise parsed undefined. The refusal cases in the same file already tolerated both shapes. --------- Co-authored-by: Yum-wu <118118663+Yum-wu@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When a tool result contains a
documentblock, the inbound translation dropped it and routed providers received an empty tool output, so the model concluded the tool returned nothing. A document in a plain user message already gets a[document: title]marker; the tool-result path now emits the same marker.Verification
tests/claude-inbound.test.ts: titled, untitled, and mixed text plus document results.""before the fix and[document]after.bun run test,typecheck,lint:gui,privacy:scan.Checklist
Summary by CodeRabbit
Bug Fixes
Tests