fix(openai): responses function_call mapping #8935
Conversation
* to handle function_call messages loaded from session file * use call_id as item id if response id is not available * ensure function_call item id is prefixed with fc
|
Keep this PR in a mergeable state → Learn moreAll Green is an AI agent that automatically: ✅ Addresses code review comments ✅ Fixes failing CI checks ✅ Resolves merge conflicts |
dosubot
bot
added
the
size:M
|
All contributors have signed the CLA ✍️ ✅ |
|
Reviewed the changes - no documentation updates needed. This PR fixes internal OpenAI Responses API type conversion logic for function_call messages loaded from session files. The changes are purely implementation details that don't affect any user-facing features, configuration options, or documented behavior. |
|
I have read the CLA Document and I hereby sign the CLA |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
* synthesize a single orchestrator function_call when multiple tool calls follow a reasoning (thinking) message to avoid protocol violations
dosubot
bot
added
size:L
RomneyDa
left a comment
RomneyDa
left a comment
There was a problem hiding this comment.
@Quazistax appreciate the contribution. This seems like a good change, I added some nitpick comments. Let's add some unit tests for these conversions
| const wrapperItemId = | ||
| rawWrapperId && rawWrapperId.startsWith("fc") | ||
| ? rawWrapperId | ||
| : `fc_${rawWrapperId}`; |
There was a problem hiding this comment.
I think this tool call ID modification would mess things up in GUI where we use tool call ID for streaming tool call ouput and a variety of things. If it doesn't match the original many GUI things will fail
| const shouldSynthesizeOrchestrator = | ||
| toolCalls.length > 1 && !!prevReasoningId; | ||
|
|
||
| if (shouldSynthesizeOrchestrator) { |
There was a problem hiding this comment.
consider restructuring so that synthesizeOrchestratorCall has pure inputs and outputs rather than directly modifying
| const rawArgs = tc?.function?.arguments as string | undefined; | ||
| let parsedArgs: any = {}; | ||
| try { | ||
| parsedArgs = rawArgs && rawArgs.length ? JSON.parse(rawArgs) : {}; |
There was a problem hiding this comment.
can use existing safeParseArgs utils for this
| } | ||
|
|
||
| // Emit function_call items directly from toolCalls | ||
| emitFunctionCallsFor(toolCalls, input, respId, prevReasoningId); |
There was a problem hiding this comment.
consider restructuring so that emitFunctionCallsFor has pure inputs and outputs rather than directly modifying
| const respId = msg.metadata?.responsesOutputItemId as | ||
| | string | ||
| | undefined; | ||
| const prevMsgForThis = messages[i - 1] as ChatMessage | undefined; |
There was a problem hiding this comment.
this type casting seems unnecessary
| } | ||
|
|
||
| // Emit function_call items directly from toolCalls | ||
| emitFunctionCallsFor(toolCalls, input, respId, prevReasoningId); |
There was a problem hiding this comment.
Should this be if else behavior? I.e. if should snythesize, do that, else emit function calls directly? Or I might be misunderstanding
2 participants
Maybe fixes #8773 , with 2nd commit might help with #8933 too (I tried it with VS Code, not with JetBrains)
Description
AI Code Review
@continue-reviewChecklist
Screen recording or screenshot
[ When applicable, please include a short screen recording or screenshot - this makes it much easier for us as contributors to review and understand your changes. See this PR as a good example. ]
Tests
[ What tests were added or updated to ensure the changes work as expected? ]
Summary by cubic
Fixes function_call mapping in OpenAI responses to emit valid items from session-loaded messages with stable IDs. When multiple tool calls follow a reasoning message, synthesizes a single orchestrator function_call to avoid protocol violations. Addresses #8773.
Written for commit 7ba2d46. Summary will update automatically on new commits.