Repository navigation
fix(ai): identify offending tool message part in OpenAI Chat errors - #55620
akushonkamen wants to merge 2 commits into
Conversation
The lowerToolMessages guard in the OpenAI Chat protocol aborted request lowering with "OpenAI Chat tool messages only support tool-result content for now", without saying which message or content part violated the guard, which made plugin-produced bad history hard to locate (anomalyco#55271). ProviderShared.unsupportedContent gains an optional trailing detail parameter appended in parentheses; existing call sites are unaffected. The OpenAI Chat tool guard now reports the request message index, part index, part type and, when present, the tool_call_id of the offending part, e.g. "...for now (message anomalyco#2 part #0 type=text)". Behavior is unchanged: the request is still rejected before it reaches the provider.
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sound, small change. The OpenAI Chat tool-content error now says which message and part failed, e.g. (message #2 part #0 type=text). I checked it by running the new test against the base, where it fails with the bare message, and against this branch, where the whole openai-chat test file passes (79/79). packages/ai typecheck passes.
Public API is unchanged: ProviderShared.unsupportedContent is internal and only gains an optional trailing detail parameter. Existing callers and their error text stay the same.
Two things worth addressing, neither blocking:
"id" in partalso matches compaction parts, so a provider checkpoint id would be labelledtool_call_id(see inline).- Anthropic Messages, Gemini and Bedrock Converse have the identical tool-message check and still give the bare error. In #55271 the same plugin problem also hit other routes. Building the location text once in
shared.tsand passing the indexes from each route would cover them all, instead of special-casing OpenAI Chat.
| return yield* ProviderShared.unsupportedContent("OpenAI Chat", "tool", ["tool-result"]) | ||
| for (const [partIndex, part] of message.content.entries()) { | ||
| if (!ProviderShared.supportsContent(part, ["tool-result"])) { | ||
| const id = "id" in part ? part.id : undefined |
There was a problem hiding this comment.
"id" in part also matches CompactionPart, whose optional id is a provider checkpoint id, not a tool call id. That would be reported as tool_call_id=.... Only tool-call and tool-result parts carry a real tool call id, and tool-result parts never get here, so part.type === "tool-call" ? part.id : undefined would be accurate.
This location text is built only for OpenAI Chat. Anthropic Messages (anthropic-messages.ts:982), Gemini (gemini.ts:402) and Bedrock Converse (bedrock-converse.ts:420) have the same tool / ["tool-result"] check and will still throw the bare message. In #55271 the same plugin issue also showed up on other routes. A small shared helper in shared.ts (taking message index, part index and part) used by each route would keep this consistent and avoid copying the formatting into every protocol.
Review feedback on anomalyco#55620: - Only tool-call parts carry a tool call id; other parts with an id field (compaction checkpoints) are no longer mislabeled as tool_call_id. - The location text (message/part/type/tool_call_id) is built once in ProviderShared.contentLocation and passed by every route with the tool/["tool-result"] check: OpenAI Chat, Anthropic Messages, Gemini, and Bedrock Converse. New regression tests cover each route's locator plus the compaction and tool-call id cases. bun test test/: 1816 pass / 28 skip / 0 fail.
|
Thanks for the careful review — both points addressed in 72a476a. 1. 2. Same bare error on Anthropic Messages / Gemini / Bedrock Converse — done.
Verification (all run locally on this machine against 72a476a):
One disclosure: I could not run the pre-push hook itself — it hard-requires |
Issue for this PR
Closes #55271
Type of change
What does this PR do?
When
lowerToolMessagesin the OpenAI Chat protocol hits a tool-message part that isn't atool-result, it aborts the whole request with "OpenAI Chat tool messages only support tool-result content for now" — no message index, no part, no type. Since a plugin can inject non-tool-result parts through the sessioncontexthook, the resulting failed drain is nearly undebuggable (that is the report in #55271). The cause is structural:ProviderShared.unsupportedContent(packages/ai/src/protocols/shared.ts:359-363ondev) only accepts route/role/types, so the message could not contain the location.Two small changes:
unsupportedContentgains a strictly-optional trailingdetailstring, appended in parentheses. The 20+ existing call sites are untouched.lowerToolMessages(packages/ai/src/protocols/openai-chat.ts:507-509ondev) now takes the message index and walks parts withentries(), passing a detail likemessage #2 part #0 type=text(plus the part'stool_call_id, read through"id" in part, when present). Thetoolbranch oflowerMessagespasses the real request-message index.Behavior is unchanged: the request is still rejected before it reaches the provider; only the error string gained a locator, e.g.
...for now (message #2 part #0 type=text).The triage on the issue (opencode-agent[bot], 2026-10-14) treats exactly this as the remaining improvement: "the error should give the message index, part index and part type ... this needs a change in core" — comment.
How did you verify your code works?
identifies the offending message and part when tool content is unsupportedinpackages/ai/test/provider/openai-chat.test.ts: ondevit fails withExpected to contain: "message #2"(received string has no locator); with the fix it passes.cd packages/ai && bun test test/→ 1811 pass / 28 skip / 0 fail across 135 files, 7852 expect() calls.cd packages/ai && bun run typecheck(tsgo --noEmit) → exit 0.bunx oxlinton the three changed files → 0 warnings, 0 errors.bun run check: lint + typecheck across the monorepo) passes: 36/36 tasks.Screenshots / recordings
Not a UI change.
Checklist