Repository navigation
fix: omit synthesized reasoning items in Chat→Responses translation (#224) - #231
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical compatibility issue in OmniRoute's translation layer, specifically when converting Chat Completions to the OpenAI Responses API format. By ceasing the client-side generation of reasoning items, which previously led to 400 Invalid Request errors due to incorrect ID formats, the system now correctly aligns with the Responses API's requirement for server-generated IDs. This change ensures robust and error-free communication with Responses-compatible upstreams. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
ActivityUsing Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)The issue at line 294 is in unchanged code that exists after the diff hunk. The diff covers lines 275-285, but the stale comment is at line 294 (outside the diff range). This code was not modified in this PR but became incorrect after the reasoning logic was removed. Files Reviewed (1 file)
Review Notes: The PR correctly addresses issue #224 by removing client-side reasoning synthesis that was causing 400 errors from the OpenAI Responses API. The changes are logically sound - the reasoning content is now properly skipped rather than incorrectly synthesized with invalid IDs. The only issue is a stale comment that references code that no longer exists. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a bug causing 400 Invalid Request errors by removing the client-side synthesis of reasoning items when translating Chat Completions to the OpenAI Responses API format. The change is correct and effectively resolves the issue. As a minor point of improvement for maintainability, the removal of this logic has made a comment on line 294 (// Reasoning already moved above) outdated. While this line is outside the current diff, I recommend updating it to reflect that thinking blocks are now skipped, not moved.
There was a problem hiding this comment.
Pull request overview
This PR fixes an interoperability bug in OmniRoute’s Chat Completions → OpenAI Responses request translation by stopping the synthesis of client-generated “reasoning” items, which can produce invalid IDs and trigger upstream 400 Invalid Request errors.
Changes:
- Removed synthesis of
type: "reasoning"items fromreasoning_contentandthinking/redacted_thinkingblocks during Chat→Responses translation. - Added inline comments explaining why reasoning/thinking content is omitted for Responses-compatible upstreams.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // Skip reasoning_content — OpenAI Responses API requires server-generated | ||
| // rs_* IDs for reasoning items. Synthesizing client-side IDs (e.g. reasoning_N) | ||
| // causes 400 errors from Responses-compatible upstreams. (#224) | ||
|
|
||
| // Handle thinking blocks in array content | ||
| if (Array.isArray(msg.content)) { | ||
| for (const blockValue of msg.content) { | ||
| const block = toRecord(blockValue); | ||
| if (block.type === "thinking" || block.type === "redacted_thinking") { | ||
| input.push({ | ||
| type: "reasoning", | ||
| id: `reasoning_${input.length}`, | ||
| summary: [ | ||
| { type: "summary_text", text: toString(block.thinking || block.data, "...") }, | ||
| ], | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| // Skip thinking blocks in array content — same rs_* ID constraint applies | ||
|
|
There was a problem hiding this comment.
The new behavior omits reasoning items entirely, but there’s still an in-function comment later in the assistant content loop that says thinking blocks were “moved above”. After this change that’s no longer true (they’re omitted), so please update/remove that stale comment to match the new behavior and avoid future confusion.
| // Skip reasoning_content — OpenAI Responses API requires server-generated | ||
| // rs_* IDs for reasoning items. Synthesizing client-side IDs (e.g. reasoning_N) | ||
| // causes 400 errors from Responses-compatible upstreams. (#224) | ||
|
|
||
| // Handle thinking blocks in array content | ||
| if (Array.isArray(msg.content)) { | ||
| for (const blockValue of msg.content) { | ||
| const block = toRecord(blockValue); | ||
| if (block.type === "thinking" || block.type === "redacted_thinking") { | ||
| input.push({ | ||
| type: "reasoning", | ||
| id: `reasoning_${input.length}`, | ||
| summary: [ | ||
| { type: "summary_text", text: toString(block.thinking || block.data, "...") }, | ||
| ], | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| // Skip thinking blocks in array content — same rs_* ID constraint applies |
There was a problem hiding this comment.
This change fixes a specific interoperability bug (synthetic reasoning_* IDs causing 400s). Please add a unit test that exercises Chat→Responses translation with (a) assistant reasoning_content and (b) assistant array content containing thinking/redacted_thinking blocks, and asserts that no reasoning items are emitted and no synthetic IDs appear in result.input.
fix: omit synthesized reasoning items in Chat→Responses translation (#224)
…precedent CodeQL js/stack-trace-exposure does not recognize sanitizers reached through a custom helper indirection — flagging callsites like open-sse/utils/error.ts::errorResponse and open-sse/executors/cursor.ts::buildErrorResponse even though both route through sanitizeErrorMessage(). Record the dismissal precedent (alerts #224 and #231, May 2026): - Add a "Known CodeQL limitation" section in docs/security/ERROR_SANITIZATION.md documenting how to handle future occurrences (verify callchain → verify test coverage → dismiss with reference, do NOT duplicate the pattern inline). - Extend CLAUDE.md Hard Rule #14 with the precedent so the next engineer doesn't try to "fix" the false positive by weakening the shared sanitizer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…precedent CodeQL js/stack-trace-exposure does not recognize sanitizers reached through a custom helper indirection — flagging callsites like open-sse/utils/error.ts::errorResponse and open-sse/executors/cursor.ts::buildErrorResponse even though both route through sanitizeErrorMessage(). Record the dismissal precedent (alerts #224 and #231, May 2026): - Add a "Known CodeQL limitation" section in docs/security/ERROR_SANITIZATION.md - Extend CLAUDE.md Hard Rule #14 with the precedent Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…precedent CodeQL js/stack-trace-exposure does not recognize sanitizers reached through a custom helper indirection — flagging callsites like open-sse/utils/error.ts::errorResponse and open-sse/executors/cursor.ts::buildErrorResponse even though both route through sanitizeErrorMessage(). Record the dismissal precedent (alerts cheaper-inference#224 and cheaper-inference#231, May 2026): - Add a "Known CodeQL limitation" section in docs/security/ERROR_SANITIZATION.md - Extend CLAUDE.md Hard Rule cheaper-inference#14 with the precedent Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…precedent CodeQL js/stack-trace-exposure does not recognize sanitizers reached through a custom helper indirection — flagging callsites like open-sse/utils/error.ts::errorResponse and open-sse/executors/cursor.ts::buildErrorResponse even though both route through sanitizeErrorMessage(). Record the dismissal precedent (alerts cheaper-inference#224 and cheaper-inference#231, May 2026): - Add a "Known CodeQL limitation" section in docs/security/ERROR_SANITIZATION.md documenting how to handle future occurrences (verify callchain → verify test coverage → dismiss with reference, do NOT duplicate the pattern inline). - Extend CLAUDE.md Hard Rule cheaper-inference#14 with the precedent so the next engineer doesn't try to "fix" the false positive by weakening the shared sanitizer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…precedent CodeQL js/stack-trace-exposure does not recognize sanitizers reached through a custom helper indirection — flagging callsites like open-sse/utils/error.ts::errorResponse and open-sse/executors/cursor.ts::buildErrorResponse even though both route through sanitizeErrorMessage(). Record the dismissal precedent (alerts cheaper-inference#224 and cheaper-inference#231, May 2026): - Add a "Known CodeQL limitation" section in docs/security/ERROR_SANITIZATION.md - Extend CLAUDE.md Hard Rule cheaper-inference#14 with the precedent Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…precedent CodeQL js/stack-trace-exposure does not recognize sanitizers reached through a custom helper indirection — flagging callsites like open-sse/utils/error.ts::errorResponse and open-sse/executors/cursor.ts::buildErrorResponse even though both route through sanitizeErrorMessage(). Record the dismissal precedent (alerts cheaper-inference#224 and cheaper-inference#231, May 2026): - Add a "Known CodeQL limitation" section in docs/security/ERROR_SANITIZATION.md documenting how to handle future occurrences (verify callchain → verify test coverage → dismiss with reference, do NOT duplicate the pattern inline). - Extend CLAUDE.md Hard Rule cheaper-inference#14 with the precedent so the next engineer doesn't try to "fix" the false positive by weakening the shared sanitizer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…224-reasoning-ids fix: omit synthesized reasoning items in Chat→Responses translation (cheaper-inference#224)
…precedent CodeQL js/stack-trace-exposure does not recognize sanitizers reached through a custom helper indirection — flagging callsites like open-sse/utils/error.ts::errorResponse and open-sse/executors/cursor.ts::buildErrorResponse even though both route through sanitizeErrorMessage(). Record the dismissal precedent (alerts cheaper-inference#224 and cheaper-inference#231, May 2026): - Add a "Known CodeQL limitation" section in docs/security/ERROR_SANITIZATION.md - Extend CLAUDE.md Hard Rule cheaper-inference#14 with the precedent Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…precedent CodeQL js/stack-trace-exposure does not recognize sanitizers reached through a custom helper indirection — flagging callsites like open-sse/utils/error.ts::errorResponse and open-sse/executors/cursor.ts::buildErrorResponse even though both route through sanitizeErrorMessage(). Record the dismissal precedent (alerts cheaper-inference#224 and cheaper-inference#231, May 2026): - Add a "Known CodeQL limitation" section in docs/security/ERROR_SANITIZATION.md documenting how to handle future occurrences (verify callchain → verify test coverage → dismiss with reference, do NOT duplicate the pattern inline). - Extend CLAUDE.md Hard Rule cheaper-inference#14 with the precedent so the next engineer doesn't try to "fix" the false positive by weakening the shared sanitizer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
When OmniRoute translates Chat Completions history to OpenAI Responses API input, it was generating synthetic reasoning items with IDs like
reasoning_15. The Responses API requires server-generatedrs_*IDs, causing400 Invalid Requesterrors.Fix
Removed reasoning item synthesis entirely from
openaiToOpenAIResponsesRequest(). Since validrs_*IDs can only be generated server-side, these items are now omitted during translation. Reasoning content is non-essential for the request flow.Changes
open-sse/translator/request/openai-responses.ts: Removedreasoning_contentandthinkingblock synthesis in Chat→Responses translationCloses #224