[codex] Preserve cache_control when merging system prompts#1946
[codex] Preserve cache_control when merging system prompts#1946
Conversation
Strict OpenAI-compatible chat backends still need fragmented Claude\nsystem prompts collapsed into one leading system message, but that\nnormalization should not silently drop stable cache hints. Preserve\nmessage-level cache_control when the merged system fragments agree,\nand fall back to omitting it when the fragments conflict.\n\nConstraint: Must keep single-system normalization for Nvidia/Qwen-style chat backends\nRejected: Always copy the first cache_control | could misrepresent conflicting cache boundaries\nConfidence: high\nScope-risk: narrow\nReversibility: clean\nDirective: If system prompt merging changes again, preserve cache_control whenever the merged metadata is unambiguous\nTested: cargo test proxy::providers::transform --manifest-path src-tauri/Cargo.toml\nNot-tested: End-to-end prompt caching behavior against cache-aware OpenAI-compatible upstreams\nRelated: #1881
|
@codex review |
![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: b11e36963f
ℹ️ 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".
| if let Some(cache_control) = message.get("cache_control") { | ||
| match &inherited_cache_control { | ||
| None => inherited_cache_control = Some(cache_control.clone()), | ||
| Some(existing) if existing == cache_control => {} | ||
| Some(_) => cache_control_conflict = true, |
There was a problem hiding this comment.
When system fragments are merged, this branch only marks a conflict if two present cache_control values differ, but it ignores fragments where cache_control is absent. That means one cached fragment plus one uncached fragment will still produce a merged message with cache_control on the entire combined prompt. In mixed-prefix scenarios (static cached prefix + dynamic uncached suffix), this changes cache scope and can cache content that was intentionally left uncached; treating present/absent mixes as ambiguous and dropping cache_control would preserve original intent more safely.
Useful? React with 👍 / 👎.
The follow-up cache hint fix still treated mixed present/absent\ncache_control across fragmented system prompts as inheritable, which\nexpanded the cache scope after prompt collapse. Treat that mix as\nambiguous and only preserve cache_control when every merged fragment\nexplicitly agrees on the same value.\n\nConstraint: Must preserve strict-backend system prompt normalization from #1942\nRejected: Inherit first present cache_control | widens cache scope when later fragments were intentionally uncached\nConfidence: high\nScope-risk: narrow\nReversibility: clean\nDirective: Any future merged-system cache hint logic should treat missing cache_control as semantically significant\nTested: cargo test proxy::providers::transform --manifest-path src-tauri/Cargo.toml\nNot-tested: End-to-end upstream caching behavior against cache-aware relays\nRelated: #1881\nRelated: #1946
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Reflow the two long cache-control regression assertions in transform.rs so the neighboring merge cases stay rustfmt-aligned and easier to scan. This keeps the preserved code change separate from the untracked Markdown design notes the user did not want committed. Constraint: Exclude Markdown design files from the commit while preserving the local code change Rejected: Include docs in the same commit | user explicitly asked to leave Markdown files out Confidence: high Scope-risk: narrow Reversibility: clean Directive: Treat this as a readability-only test change; do not infer runtime behavior changes from it Tested: cargo test --manifest-path src-tauri/Cargo.toml test_anthropic_to_openai_drops_ --lib Tested: cargo check --manifest-path src-tauri/Cargo.toml --tests Tested: pnpm format:check Tested: pnpm typecheck Not-tested: Full application integration and manual flows
yovinchen
force-pushed
the
codex/issue-1881-cache-control
branch
from
223f976 to
5bccb4f
Compare
1 participant
Summary
cache_controlwhen fragmentedsystemprompts are merged into a single leading OpenAI chatsystemmessagecache_controlonly when mergedsystemfragments carry conflicting cache hintsWhy
PR #1942 fixed strict OpenAI-compatible backends that reject fragmented
systemprompts, but the merge path rebuilt the combinedsystemmessage with onlyroleandcontent. That droppedcache_controlfrom the originalsystemblocks and could disable prompt-prefix caching for cache-aware upstreams.This follow-up keeps the strict-backend compatibility fix while preserving cache hints whenever they can be inherited unambiguously.
Validation
cargo test proxy::providers::transform --manifest-path src-tauri/Cargo.tomlCloses #1942
Closes #1945