Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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
agents/src/agents/AgentContext.ts
Lines 1050 to 1052 in e3fe253
Keep the dynamic tail in an instruction role
When explicit caching is enabled and both instruction strings are nonempty, this converts the entire dynamic system tail into a HumanMessage. That tail includes host-supplied additional_instructions and cross-run summary context; before this commit they were part of the SystemMessage, and AgentInputFields still documents additional_instructions as a system tail. On OpenAI and Azure this lowers its instruction priority to user level, allowing later user content to override constraints and changing agent behavior merely by enabling caching. Preserve a system/developer role for the tail and make the request serializer target the stable instruction message or block for its breakpoint.
ℹ️ 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".
|
Fixed in The tail now keeps a system role on the OpenAI explicit path. Anthropic and OpenRouter keep the The breakpoint had to move with it, which is the second half of the suggestion: Tests: the OpenAI and Azure tails assert a system role, and the breakpoint test asserts it lands on the stable prefix; all three fail on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77b12e10c9
ℹ️ 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 ( | ||
| (role === 'system' || role === 'developer') && | ||
| cacheable[index] && | ||
| instructionIndex === -1 | ||
| ) { | ||
| instructionIndex = index; |
There was a problem hiding this comment.
When the exported ChatOpenAI/Azure wrappers are invoked directly rather than through AgentContext, multiple system/developer messages do not follow the assumed stable-first/volatile-last layout. For example, a short system preamble followed by a large stable developer prompt now receives a breakpoint only on the short first message; explicit mode therefore cannot cache the useful combined instruction prefix and may fall below the provider's minimum cacheable prefix. The previous last-instruction selection supported these requests, so the stable-first behavior should be limited to messages explicitly identified as the relocated AgentContext tail rather than applied to every request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7d27987. Valid regression: taking only the first instruction message left a direct caller's multi-message stable prompt unmarked once history separated it from the current turn. selectCacheBreakpointIndexes now marks both the first instruction (the stable prefix AgentContext builds) and the end of the leading instruction run, which are the same message when there is one. With the history breakpoint that is at most three, inside the four an explicit request may write. Covered for Chat Completions and Responses in managedRequests.test.ts ("also marks the end of a leading multi-message instruction prompt", "marks the leading instruction run on Responses input too").
`promptCacheExplicit` had no effect on where the breakpoint landed, because `buildSystemMessage` collapsed the stable instructions and the dynamic tail into one string for OpenAI and Azure. The OpenAI client marks the last system/developer message, so the breakpoint sat behind per-turn content and the cached prefix was invalidated every turn. The dynamic tail now moves behind the stable prefix, reusing the relocation Anthropic and OpenRouter already use, while leaving `promptCacheProvider` undefined so no `cache_control` or `cachePoint` marker is stamped: the OpenAI breakpoints are attached to the serialized request, not the message content.
Moving the dynamic tail behind the stable prefix sent it as a `HumanMessage` on OpenAI and Azure. That tail is `additional_instructions` and the cross-run summary, declared a system tail in `graph.ts` and `run.ts` and built as one, so enabling explicit caching dropped host constraints to user rank where later user content outranks them: a caching switch changing how an agent behaves. The tail keeps a system role on that path now. It stays a `HumanMessage` for Anthropic and OpenRouter, which shipped it that way before this branch. The breakpoint had to move with it. `selectCacheBreakpointIndexes` marked the last system or developer message, which with a system-role tail is the volatile half; it takes the first now, which is the stable prefix, and is the same message whenever there is only one.
Taking only the first system or developer message dropped the breakpoint from a direct caller that splits one stable prompt across several instruction messages, once history separates it from the current turn. The end of the leading instruction run is marked as well. It is the same message whenever there is one instruction message, keeps the stable prefix AgentContext builds marked, and stays at three breakpoints at most with the history one, inside the four an explicit request may write.
77b12e1 to
7d27987
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d279878d4
ℹ️ 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".
| ? [new HumanMessage(dynamicInstructions)] | ||
| ? [ | ||
| keepsInstructionRole | ||
| ? new SystemMessage(dynamicInstructions) |
There was a problem hiding this comment.
When explicit caching is enabled with additional_instructions, this inserts the dynamic system message immediately before the latest human message. selectCacheBreakpointIndexes then scans backward from that human message and chooses this tail as the history breakpoint, rather than the preceding assistant/history message. On each new user turn the tail moves after newly completed history, so the prefix ending at that breakpoint cannot match the prior turn; ordinary one-call chat turns therefore reuse only the stable-instructions breakpoint and repeatedly miss the potentially large conversation history that the previous fixed-at-the-front system layout could cache. Add a separate breakpoint immediately before the relocated tail (while retaining the tail breakpoint if needed for tool-loop calls).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d913723. Valid: the history breakpoint landed on the relocated tail, which moves behind each new turn, so the history prefix never recurred. selectCacheBreakpointIndexes now also marks the last history message before an instruction run that directly precedes the current turn, keeping the tail breakpoint for tool-loop calls (four at most). The cross-run summary carrier had the same problem, so on the OpenAI explicit path it now stays ahead of history as it does without explicit caching. Tests: managedRequests "marks the history in front of a relocated tail, not only the tail" and AgentContext "keeps the summary ahead of history and only the tail behind it under explicit caching", both failing on 7d27987.
The relocated tail sits before the current turn and moves behind each new one, so a breakpoint ending at it never recurs and the conversation history in front of it was never read back. The client now also marks the last history message before a run of instructions that directly precedes the current turn, four breakpoints at most. The cross-run summary no longer rides that tail on the OpenAI explicit path. It stays ahead of history as it does without explicit caching, where it changes only on re-summarization.
This branch has not been deployed
1 participant
Summary
With
promptCacheExpliciton an OpenAI or Azure client, the GPT-5.6 instruction breakpoint landed on a system message that joined the stable instructions with the per-turnadditional_instructions, so it was written every turn and never read back.AgentContextnow keeps only the stable instructions in the system message and relocates the dynamic tail, still as a system message, behind stable history before the current turn. The cross-run summary stays ahead of history, and no Anthropiccache_controlmarkers are added on this path.On the client side,
prompt_cache_breakpointnow marks the first instruction message, the end of the leading instruction run (for callers that split one prompt across several), the last history message before the relocated tail, and the message before the current turn: four at most, the explicit-mode limit. Nothing changes unlesspromptCacheExplicitis true.Explicit-cache half of LibreChat-AI/LibreChat#15959, tracked at berry-13/LibreChat#243.
Change Type
Testing
npx jest src/llm/openai src/agents src/graphs src/summarization: 701 passed, 0 failednpx tsc --noEmit: clean;npx eslinton the touched files: cleanmanagedRequests.test.tscover breakpoint placement for Chat Completions and Responses; newAgentContext.test.tscases cover the stable system message, the system-role tail, dynamic-only instructions, summary placement, and unchanged Anthropic, OpenRouter, and Bedrock shapesChecklist