fix: preserve prompt_cache_key in Responses API, escape \n in tagContent (#517, #515) - #518
Conversation
…n in tagContent (#517, #515) fix(translator): preserve prompt_cache_key when translating Responses API requests (#517) — prompt_cache_key is an account-affinity signal used by Codex for prompt cache routing. Deleting it from the translated request prevented full cache effectiveness. Removed delete from openai-responses.ts and responsesApiHelper.ts cleanup blocks. fix(combo): escape \n in tagContent so injected JSON string is valid (#515) — omniModel tag content used template literal newlines (U+000A) which produce unescaped newline chars inside a JSON string value. Replaced with literal \n escape sequences for valid JSON injection in streaming SSE content chunks.
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 implements two crucial fixes to enhance system stability and functionality. It addresses an issue where the 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. Using 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. Footnotes
|
![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 introduces two fixes. The first preserves the prompt_cache_key in the Responses API to Chat Completions translation, which is a necessary fix for cache affinity with providers like Codex. This change is correct and well-documented. The second fix escapes newline characters in tagContent to prevent invalid JSON during stream injection. While this solves the issue for the streaming case, I've found a critical issue where this change breaks another part of the code that also uses tagContent. Please see my detailed comment.
| // that marker is silently dropped. | ||
| if (!res.body) return res; | ||
| const tagContent = `\n<omniModel>${modelStr}</omniModel>\n`; | ||
| const tagContent = `\\n<omniModel>${modelStr}</omniModel>\\n`; |
There was a problem hiding this comment.
This change fixes the JSON injection for streaming responses by escaping the newline characters. However, this breaks another usage of tagContent in the flush part of the TransformStream (line 514), where it's used in a JSON.stringify call.
With this change, tagContent is a string containing a literal backslash and 'n' (e.g., "\\n..."). When JSON.stringify({ content: tagContent }) is called, the backslash is also escaped, resulting in "\\\\n..." in the JSON output. The client will then parse this as a literal \n string, not a newline character.
To address this, tagContent should contain raw newlines when passed to JSON.stringify. A comprehensive fix would involve defining the raw content and escaping it only for the regex injection. Since that requires changing code outside this diff, a more localized fix would be to un-escape tagContent at line 514 before it's stringified. For example: delta: { content: tagContent.replace(/\\n/g, '\n') }.
Please adjust the implementation to ensure both use cases are handled correctly.
There was a problem hiding this comment.
Pull request overview
Fixes two translation/streaming edge cases: keep prompt_cache_key during OpenAI Responses → Chat Completions translation to preserve Codex prompt-cache affinity, and avoid invalid JSON in combo streaming tag injection by eliminating raw newline characters in the injected tagContent.
Changes:
- Preserve
prompt_cache_keyin Responses→Chat translation cleanup steps (fixes Codex cache-affinity regression, #517). - Change combo streaming
<omniModel>tag injection to use escaped newline sequences instead of raw newlines to keep injected JSON valid (#515).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| open-sse/translator/request/openai-responses.ts | Stops deleting prompt_cache_key during Responses→Chat translation cleanup. |
| open-sse/translator/helpers/responsesApiHelper.ts | Stops deleting prompt_cache_key during Responses→Chat conversion helper cleanup. |
| open-sse/services/combo.ts | Updates streamed <omniModel> tag injection to avoid raw newlines in JSON-string regex replacement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // that marker is silently dropped. | ||
| if (!res.body) return res; | ||
| const tagContent = `\n<omniModel>${modelStr}</omniModel>\n`; | ||
| const tagContent = `\\n<omniModel>${modelStr}</omniModel>\\n`; |
There was a problem hiding this comment.
tagContent is now pre-escaped with \\n, but later you embed it into JSON in two different ways: (1) via raw string replacement into an existing JSON line, and (2) via JSON.stringify(...) in flush(). With the new value, path (2) will double-escape backslashes so clients receive a literal "\n" sequence, while path (1) will be parsed as an actual newline escape, making behavior inconsistent. Consider keeping tagContent as real newlines and using proper JSON escaping for the replacement (e.g., derive an escaped form from JSON.stringify(tagContent)), or otherwise ensure both paths emit the same decoded content.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge OverviewThe PR correctly implements fixes for issues #517 and #515:
Note: There is a minor inconsistency between the two code paths that use Files Reviewed (3 files)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b556de03b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // that marker is silently dropped. | ||
| if (!res.body) return res; | ||
| const tagContent = `\n<omniModel>${modelStr}</omniModel>\n`; | ||
| const tagContent = `\\n<omniModel>${modelStr}</omniModel>\\n`; |
There was a problem hiding this comment.
When context_cache_protection is enabled and a streamed response never emits a delta.content chunk before flush runs (tool-call-only turns are a common case), this escaped tagContent is fed through JSON.stringify in the fallback chunk builder, so clients receive literal \\n text instead of real newlines. On the next request extractPinnedModel() still finds the tag, but stripModelTags() in open-sse/services/comboAgentMiddleware.ts removes only the tag and leaves "\\n\\n" behind, which then gets forwarded upstream as prompt text. The old raw-newline form trimmed away cleanly, so this change regresses context-cache protection for no-text/tool-only streams.
Useful? React with 👍 / 👎.
…15-prompt-cache-key-tagcontent fix: preserve prompt_cache_key in Responses API, escape \n in tagContent (diegosouzapw#517, diegosouzapw#515)
…15-prompt-cache-key-tagcontent fix: preserve prompt_cache_key in Responses API, escape \n in tagContent (diegosouzapw#517, diegosouzapw#515)
Fixes
Closes #517, #515
Changes
fix(translator): preserve
prompt_cache_key(#517)Codex uses
prompt_cache_keyas a cache-affinity signal — the same key routes the request to the same cached slot. OmniRoute was deleting it during Responses API → Chat Completions translation, which prevented caching from working.Files changed:
open-sse/translator/request/openai-responses.ts— removeddelete result.prompt_cache_keyopen-sse/translator/helpers/responsesApiHelper.ts— removeddelete result.prompt_cache_keyfix(combo): escape
\nintagContentJSON injection (#515)The
tagContentvariable used template literal newlines (U+000A character). When injected into a JSON string via regex replacement, these raw newlines produce invalid JSON (unescaped control characters). Replaced with literal\nescape sequences.File changed:
open-sse/services/combo.ts—tagContentnow uses\\ninstead of actual newlinesTests