Skip to content

🗝️ feat: Cache Stable OpenAI Prompt Prefixes Across Conversations - #15959

Open
berry-13 wants to merge 72 commits into
devfrom
berry-13/enhancement-optimize-gpt-5.6-prompt-caching-for
Open

berry-13 wants to merge 72 commits into
devfrom
berry-13/enhancement-optimize-gpt-5.6-prompt-caching-for

Conversation

@berry-13

@berry-13 berry-13 commented Sep 15, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Every OpenAI and Azure OpenAI request LibreChat sends carries user: <userId> (packages/api/src/endpoints/openai/initialize.ts). On models before GPT-5.6 that field is what OpenAI routes prompt-cache lookups on, so one user's second conversation may not reach the machine holding the prefix it just wrote; on GPT-5.6 and later OpenAI routes automatically, and a key instead decides how cache usage is accounted and where the boundary between users sits.

Requests to first-party OpenAI and Azure OpenAI now carry a deterministic prompt_cache_key that names one agent's configured prompt prefix and the user it is accounted to. Nothing about the conversation enters it, so a new chat or a different user message reaches the same entry; a real change to the agent's instructions, tools, delegation targets, handoff edges, output schema or API mode produces a different digest and a new cache identity, with no invalidation step. Implicit caching stays on, so growing conversation history keeps being reused as it is today.

The key is scoped per user by default, which preserves the partitioning the user field already produces, including the property that a user cannot learn, from a cache hit, that someone else already sent a prompt they can guess. promptCacheScope: shared opts into the other trade: one cached prefix serves everyone running the same agent, so a deployment pays one cache write instead of one per user.

Two adjacent levers ship with it: promptCacheScope and promptCacheRetention (prompt_cache_retention). Both are librechat.yaml endpoint options, settable under openAI, azureOpenAI or endpoints.all, and the wire-level ones are now in knownOpenAIParams. That last part is a fix in its own right: as unknown params they were routed to modelKwargs, and @langchain/openai spreads modelKwargs before the explicit prompt_cache_key field on Chat Completions, so an admin setting it by hand had it overwritten with undefined and silently never sent.

Gateways are untouched. The gate is the existing firstPartyOpenAI || firstPartyAzure computation, which is false for OpenRouter, the Vercel gateway, every custom endpoint and any reverse-proxy base URL. Anthropic, Bedrock, Google and Vertex are not on this path at all.

Partially addresses #14949. GPT-5.6 explicit breakpoints (prompt_cache_options, prompt_cache_breakpoint) are not an endpoint option here: they wait on LibreChat-AI/agents#546, which anchors the breakpoint to the stable prefix, and on a model capability gate that is its own change. Agent authors still cannot set them, and dropParams still removes them.

How it works

Policy and value are resolved in different places, because no single place knows both:

initializeOpenAI                 reads librechat.yaml (endpoint first, then endpoints.all)
  getOpenAIConfig
    getOpenAILLMConfig           knows endpoint + base URL + model, not instructions
      └─ llmConfig.promptCacheKeyEnabled = true        # policy only
         llmConfig.promptCacheScope = 'shared'         # policy only, when set

createRun
  buildAgentInput
    └─ clientOptions.promptCacheScopeId = user.id      # partition identity
  finalizePromptCacheKey(input)
    └─ clientOptions.promptCacheKey = buildPromptCacheKey(input)

getOpenAILLMConfig runs long before the agent's instructions and discovered tools exist, so it can decide whether a key is allowed but not what it is. createRun consumes the markers at the one point where the input is final, then drops them so none reaches the wire.

The identity is derived by exclusion, not from a field list

buildPromptCacheKey takes the finished AgentInputs and walks it against a total map over keyof AgentInputs: every field is either hashed, or excluded with the reason it cannot change the prefix.

const agentInputDispositions: Record<keyof AgentInputs, PromptCacheDisposition> = {
  instructions: identity(),
  clientOptions: identity(clientOptionsIdentity),
  toolDefinitions: identity(toolsIdentity),
  graphTools: identity(toolsIdentity),
  subagentConfigs: identity(subagentConfigsIdentity),
  …
  discoveredTools: excluded(
    'Tool names this conversation already discovered through tool_search. …',
  ),
};

That shape is the point. A hand-picked list at the call site is why the delegation tool, the handoff edges, the subagent_type enum, the Responses output format and the API mode each had to be added after the fact: nothing failed when a model-facing surface was missing. A total map makes the next upstream field a build failure in promptCache.ts, and a field present at runtime but absent from the map is hashed rather than dropped, so a newer SDK than these types partitions the cache (a miss) instead of pointing two different prefixes at one entry (a wrong identity).

clientOptions is projected the same way: everything except a declared non-prefix set (credentials, transport, sampling, the cache levers themselves) participates, with model resolved to the modelKwargs override that Azure Astra actually addresses. useResponsesApi therefore enters the identity, because Chat Completions and the Responses API serialize one prefix into two wire shapes. The exclusions are deliberate and tested: per-request transport must stay out because resolveConfigHeaders resolves ${conversationId} into configuration.defaultHeaders, and reasoning effort and verbosity must stay out because a user moving those sliders does not change the prefix the model reads.

What the key does not name is one turn's exact wire tool list. Tools a conversation discovered through tool_search, and the dynamic system tail of memory and file context, are excluded with their reasons: a key that followed them would give every conversation its own entry and there would be nothing left to reuse.

The partition comes from the authenticated user

promptCacheScopeId is stamped in buildAgentInput from createRun's authenticated user. The user field on the request is not an identity: addParams.user pins it to a constant, dropParams: ['user'] removes it, and the gpt-4o*search models drop it unconditionally, each of which would merge every user of an agent onto one cache entry, which is exactly what the per-user default exists to prevent.

Each occurrence of a shared input is sealed on its own

Tools are still added and stripped after an input is first assembled, so the key is stamped where the input becomes final:

buildIsolatedAgentInputs(child)      draft: tools stripped, no descendants yet
  buildSubagentConfigs(child, …)     recurse for the child's own spawn targets
    sealSubagentInputs(…)            attach descendants, then seal the identity

ownSealableInputs gives one occurrence its own shell before sealing. Sealing writes the key and removes the marker that allowed it, so an object reaching two occurrences would keep the first one's identity: a saved-team member listed by two teams with different edges, and the self-spawn child of its parent, are both sealed independently now.

An isolated child's always-apply skill bodies are recorded on promptCacheStableInstructions where they are still distinguishable from the memory and file context they are joined to, so editing such a skill retires the child's key while the volatile tail stays out.

Adjacent fixes

  • resolveSummarizationProvider already neutralizes the agent's useResponsesApi, firstPartyEndpoint, modelKwargs and reasoning when a summarizer shares the agent's provider. The inherited promptCacheKey joins that list (it names a stable prefix a summarization request does not send), while a key the summarization config sets for itself survives.
  • The three prompt-cache levers read endpoints.all before the endpoint's own block, so a value written under openAI or azureOpenAI was ignored whenever a global default existed, the opposite of what the example config documents. endpoints.all is now the fallback.
  • handoffEdgeIdentity resolves the handoff parameter name rather than passing it through: the SDK falls back to instructions, so an edge that spelled the default out landed in a different partition than one that left it unset while advertising the same tool.
packages/api/src/endpoints/openai/
├── promptCache.ts      # the disposition map, buildPromptCacheKey
├── llm.ts              # knownOpenAIParams + first-party policy resolution
├── config.ts           # forwards the levers
└── initialize.ts       # resolves them from librechat.yaml
packages/api/src/agents/
└── run.ts              # scope identity, sealing, handoff edges
packages/api/src/utils/
└── canonicalize.ts     # lifted out of agents/compatibility.ts, now shared

Change Type

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Testing

packages/api/src/endpoints/openai/promptCache.spec.ts covers the identity contract from both sides: what retires it (instructions, wire model including the Azure Astra deployment override, tool schemas and their classification fields, the delegation tool's name/description/subagent_type, handoff edges, both output-schema shapes, the API mode, the scope) and what deliberately does not (the volatile tail, tools discovered in this conversation, the tool-search corpus, sampling parameters, reasoning effort and verbosity, per-request transport headers, credentials). It also pins that an unknown input field partitions rather than collides.

packages/api/src/agents/__tests__/run-promptCache.test.ts drives the real createRun and reads the request the SDK receives: two authenticated users never share a key while one user reuses theirs across conversations; addParams.user pinned to a constant and dropParams: ['user'] both leave the partition intact; a delegated child's partition follows the authenticated user; one saved-team member listed by two teams with different edges gets one key per team; no marker reaches the wire.

packages/api/src/endpoints/openai/requests.spec.ts drives the real initializeModel against a fake fetch and asserts the actual request body on all four surfaces (OpenAI Chat Completions, OpenAI Responses, Azure Chat Completions, Azure Responses), with two turns per surface sharing one prompt_cache_key and prompt_cache_retention on the wire, and no camelCase leakage. llm.spec.ts covers the policy boundary, and initialize.spec.ts the config precedence.

Seven scenarios run through the app in the mock harness, on desktop light and desktop dark:

Scenario What it observes
stable-prefix-shares-one-cache-key-across-conversations one key across two conversations of one agent
changed-agent-instructions-retire-the-cache-key editing instructions produces a new key
gateway-endpoint-sends-no-cache-key a custom endpoint sends none
responses-api-switch-retires-the-cache-key switching to the Responses API retires it
two-users-of-one-agent-get-separate-cache-keys two users, identical agents, different keys
delegated-child-keys-its-own-stable-prefix a child sends its own identity, not its parent's
always-apply-skill-edit-retires-the-child-key editing a skill the child always applies retires its key

Test Configuration:

cd packages/api && npx jest src/endpoints/openai src/agents   # 160 suites, 4436 passed
cd packages/data-provider && npx jest                         # 12 suites, 547 passed
cd packages/api && npx tsc --noEmit                           # clean
cd packages/data-provider && npx tsc --noEmit                 # clean
npx playwright test --config=e2e/playwright.config.mock.ts \
  e2e/specs/mock/scenarios/prompt-cache-key.spec.ts \
  e2e/specs/mock/scenarios/prompt-cache-subagent.spec.ts      # 7 scenarios, desktop light + dark
E2E_CHROMIUM_CHANNEL=chrome npm run lighthouse                # passed

Lighthouse medians under the 250 ms/query serial-latency hook: LCP 3,644 ms (budget 4,500), CLS 0.017 (0.1), TBT 54 ms (500).

Checklist

  • My code adheres to this project's style guidelines
  • I have performed a self-review of my own code
  • I have commented in any complex areas of my code
  • I have made pertinent documentation changes
  • My changes do not introduce new warnings
  • I have written tests demonstrating that my changes are effective or that my feature works
  • Local unit tests pass with my changes

Copilot AI lite review requested due to automatic review settings September 15, 2026 09:13
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 15, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-19T20:25:59.526500Z e1942a5 New commits
🔒 Security Review ✅ Completed 2026-09-15T09:29:10.462650Z a58c157 PR opened
ℹ️ 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" or "@codex security review".

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a58c15725d

ℹ️ 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".

Comment thread packages/api/src/endpoints/openai/llm.ts Outdated
Comment thread packages/api/src/agents/run.ts Outdated
*/
const cacheOptions = llmConfig as Partial<t.OAIClientOptions> & { response_format?: unknown };
if (cacheOptions.promptCacheKeyEnabled === true) {
cacheOptions.promptCacheKey = buildPromptCacheKey({

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve an administrator-supplied cache key

When addParams supplies promptCacheKey, getOpenAILLMConfig retains that value but also enables promptCacheKeyEnabled; this assignment then silently replaces the configured key during every agent run. Only synthesize a key when no explicit promptCacheKey is already present, otherwise the advertised fix for administrator-provided keys does not survive the real createRun path.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4e457f09ce2f0f06ffc5b8e87b25596d3c2d6dc7. Confirmed exactly as described: addParams.promptCacheKey set both the client field and the marker, and createRun then overwrote the pinned value on every run.

Root cause was that the marker carried no record of the administrator having already settled the key. getOpenAILLMConfig now withholds it when a key is pinned, and the createRun assignment additionally guards on promptCacheKey == null so the invariant holds locally for every caller of that seam rather than depending on one producer.

Your finding also exposed a bad test: llm.spec.ts asserted only on getOpenAILLMConfig output, which is why it passed while the composed path was broken. It now asserts the marker is absent, which is the assertion that would have caught this.

Verified on bc230e1d351a6f3781bcd876c1a6a06565a5720a by the stable-prefix-shares-one-cache-key-across-conversations and changed-agent-instructions-retire-the-cache-key scenarios, which drive the key through the running app.

Comment thread packages/api/src/agents/run.ts Outdated
* prefix reach one cache entry instead of each writing their own.
*/
const cacheOptions = llmConfig as Partial<t.OAIClientOptions> & { response_format?: unknown };
if (cacheOptions.promptCacheKeyEnabled === true) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Honor dropParams when synthesizing the cache key

When an endpoint lists promptCacheKey in dropParams, deleteConfigParam removes the key but leaves the separate promptCacheKeyEnabled marker intact. This condition later observes that marker and recreates the supposedly dropped key, so dropParams does not provide the final removal asserted by the new test; dropping the key must also clear or suppress the marker.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4e457f09ce2f0f06ffc5b8e87b25596d3c2d6dc7, same root cause as #discussion_r4014063933 and correct as reported: deleteConfigParam removes promptCacheKey but never the separately named marker, so createRun recreated a key the operator had dropped.

The marker now reads dropParams directly, because the drop cascade runs below that point and cannot see it. Grouped with the pinned-key case rather than patched separately, since both are the same defect — the marker not carrying the administrator's decision about the key.

You are also right that the existing test asserted something weaker than it claimed; it now asserts the marker is absent as well as the key.

Verified on bc230e1d351a6f3781bcd876c1a6a06565a5720a.

if (
firstPartyEndpoint &&
promptCacheExplicit === true &&
supportsExplicitPromptCache(llmConfig.model)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Allow explicit caching for aliased Azure models

For Azure groups whose visible model name is an alias such as production-chat for a GPT-5.6 deployment, this capability check runs against that alias before Azure replaces it with the deployment name. Consequently promptCacheExplicit: true is silently ignored for a supported model unless the administrator happens to include gpt-5.6 in the visible alias; the Azure path needs capability metadata or must trust the explicit administrator opt-in.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4e457f09ce2f0f06ffc5b8e87b25596d3c2d6dc7. Confirmed: the capability check ran before Azure replaces the visible model with the deployment, so an alias such as production-chat hid a supported deployment and promptCacheExplicit: true was silently ignored.

It now also reads azure.azureOpenAIApiDeploymentName, and the capability pattern accepts the dash form (gpt-5-6) because Azure deployment names cannot contain a dot — which is why testing the alias alone could never have worked there.

Of your two suggested directions I kept the gate rather than trusting the opt-in outright: one Azure endpoint can host mixed deployments, and OpenAI rejects unknown body parameters rather than ignoring them, so a blanket opt-in would 400 the older ones. A fully opaque alias mapping to an opaque deployment name is still undecidable from names; that limitation is recorded in the closeout rather than papered over.

Covered by two new llm.spec.ts cases: an alias whose deployment is supported, and one where neither is.

Comment thread packages/api/src/agents/run.ts Outdated
cacheOptions.promptCacheKey = buildPromptCacheKey({
model: cacheOptions.model,
instructions: systemContent,
toolDefinitions,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Include direct tool schemas in the cache identity

For ordinary built-in, action, and provider tools, ToolService.js places the model-bound instances in agent.tools while toolDefinitions contains only the classification/event-driven definitions. Hashing only this array therefore gives agents that differ solely in those direct tool schemas the same cache bucket even though their wire prefixes differ, reducing or defeating the intended reuse; derive the identity from the schemas of both model-bound tool surfaces.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4e457f09ce2f0f06ffc5b8e87b25596d3c2d6dc7, though the mechanism you named does not hold on this head, and the distinction matters for anyone reading this later.

ToolService.js does not place built-in, action or MCP instances in agent.tools here: every production loader is definitions-only (Endpoints/agents/initialize.js:638-647 and :1274-1283 pass true; controllers/agents/openai.js:119 and responses.js:171 default it to true), so ToolService.js:1603-1605 returns loadToolDefinitionsWrapper, whose result has no tools key at all. Those schemas were already in toolDefinitions and already hashed. The branch you described is real code but unreachable from any caller.

Your conclusion was right anyway, through two sources you did not name: provider-native tools ({ type: 'web_search' }, carried as AgentInputs.tools) and graph tools (ask_user_question, stripped from toolDefinitions, plus the run-file tools appended after the old hash site). AgentContext.getEventDrivenToolsForBinding binds all three together.

The key now hashes [...toolDefinitions, ...graphTools, ...tools], built below the run-file append so nothing lands after it, with runtime instances projected to name and description because a Zod schema is neither stable JSON nor safe for canonicalize to walk. PROMPT_CACHE_KEY_VERSION bumped to 2.

Verified on bc230e1d351a6f3781bcd876c1a6a06565a5720a.

clientOverrides.firstPartyEndpoint ??= false;
clientOverrides.modelKwargs ??= {};
clientOverrides.reasoning ??= undefined;
clientOverrides.promptCacheKey ??= undefined;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear cache keys for same-endpoint summaries

For ordinary self-summarization, initializeAgent sets agent.endpoint to the provider, so shapeSummarizationConfig takes its isSameEndpointAsAgent branch and never executes this cleanup. The SDK then reuses the agent client options containing the synthesized stable-prefix promptCacheKey for a summarization request that sends a different prefix, mixing unrelated prompts in one cache bucket; the same-endpoint branch also needs an explicit key-clearing override.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4e457f09ce2f0f06ffc5b8e87b25596d3c2d6dc7. Confirmed: ordinary self-summarization takes the isSameEndpointAsAgent branch, which returns clientOverrides: undefined, so the cleanup in resolveSummarizationProvider never ran and the summarizer inherited a key naming a prefix it does not send.

promptCacheKey is now cleared on the resolved summarization parameters when the summarizer shares the agent's provider and the agent carries a synthesized key. Scoped to the synthesized key on purpose: one an administrator pinned through addParams is an endpoint-wide choice and stays.

Worth recording that the obvious fix was wrong. Adding the override unconditionally at the branch turned parameters from undefined into an object for every self-summary and broke three existing expectations; the condition keeps the plain path untouched, and run-summarization.test.ts now pins the single override that a first-party agent does carry.

Verified on bc230e1d351a6f3781bcd876c1a6a06565a5720a.

Comment thread librechat.example.yaml Outdated
# # claude-3.5-haiku:
# # deploymentName: claude-3-5-haiku@20241022 # Override for this model

# openAI:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Consolidate the duplicate OpenAI example block

The example already defines an endpoints.openAI block for custom headers at line 751, and this second block encourages operators to uncomment both when configuring headers and prompt caching. That produces duplicate YAML mapping keys, which js-yaml rejects rather than merging, so a configuration assembled from the two documented examples cannot load; add these fields to the existing OpenAI example instead.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4e457f09ce2f0f06ffc5b8e87b25596d3c2d6dc7. Correct, including the consequence: endpoints.openAI already existed at line 751 for the headers example, and js-yaml rejects the duplicate mapping key rather than merging, so a config assembled from both documented blocks could not load.

The prompt-cache fields are merged into that existing block; there is now exactly one endpoints.openAI example under endpoints:.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4e457f09ce

ℹ️ 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".

Comment thread packages/api/src/agents/run.ts Outdated
model: cacheOptions.model,
instructions: systemContent,
boundTools: [...toolDefinitions, ...(graphTools ?? []), ...(tools ?? [])],
responseSchema: cacheOptions.response_format,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Hash Responses API output formats

When a Responses-based endpoint supplies structured output through the supported text.format option (for example via addParams.text), changing that schema leaves this digest unchanged because only the Chat Completions-style response_format is hashed. This assigns different wire output schemas the same cache identity, contrary to the key contract; include the active Responses text.format value as well.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fdb6e27f83cfd28ba43b1d08a9aad8312a6d785a. Correct, and it broke the key's own stated contract: a Responses request carries its structured output schema as text.format (llmConfig.text, reachable through addParams since text is a known param), so changing it left the digest unchanged.

Both are now hashed under their own name rather than collapsed behind a precedence rule, so neither API's schema can borrow the other's identity. modelKwargs.text is deliberately excluded: applyResponsesVerbosity puts only verbosity there, which is not part of the cached prefix and would split the cache whenever a user toggles it.

Covered by a new promptCache.spec.ts row and verified on bc230e1d351a6f3781bcd876c1a6a06565a5720a.

Comment thread packages/api/src/endpoints/openai/promptCache.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bc230e1d35

ℹ️ 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".

Comment thread packages/api/src/agents/run.ts Outdated
cacheOptions.promptCacheKey = buildPromptCacheKey({
model: cacheOptions.model,
instructions: systemContent,
boundTools: [...toolDefinitions, ...(graphTools ?? []), ...(tools ?? [])],

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Build the cache key after finalizing tools

When background tasks or isolated subagents are enabled, this hashes the pre-transformation tool list: registerBackgroundTaskTool adds definitions later at line 2677, while buildIsolatedAgentInputs and the self-spawn path strip background/intent definitions after inheriting the computed key. Requests with different final wire tool prefixes can therefore share a cache bucket, and changes to those late-added or removed tools do not retire the key; compute the digest from each finalized AgentInputs after all tool transformations.

AGENTS.md reference: AGENTS.md:L22-L27

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a3e3fa771da0040c4d9e39d00b5909ebd70247ab. Confirmed on both paths you name: registerBackgroundTaskTool reassigns agentInput.toolDefinitions at run.ts:2688, after buildAgentInput had already hashed, and buildIsolatedAgentInputs strips inherited background and intent definitions after calling toInput.

Rather than move the call site again, the computation is now extracted as finalizePromptCacheKey(input) and stamped onto each finished AgentInputs exactly once — after the background-task registration for a top-level agent, and at the end of buildIsolatedAgentInputs, which is the single funnel every isolated child input passes through (the self-spawn, eager-child, graph-member and prebuilt paths all call it).

This is the same cause as #discussion_r4015112637 and, in the previous round, #discussion_r4014063965 and #discussion_r4014909063: the key was being decided from a mid-flight snapshot instead of the finished request. Treating it structurally rather than patching a fourth site is the point of the change.

Verified on a3e3fa771da0040c4d9e39d00b5909ebd70247ab.

Comment on lines +986 to +992
if (
firstPartyEndpoint &&
promptCacheExplicit === true &&
(supportsExplicitPromptCache(llmConfig.model) ||
supportsExplicitPromptCache(azure ? azure.azureOpenAIApiDeploymentName : undefined))
) {
llmConfig.promptCacheExplicit = true;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Gate addParams explicit caching by model

When addParams.promptCacheExplicit is true for an unsupported first-party model such as gpt-4o, the earlier known-parameter loop has already copied that value into llmConfig; this condition merely declines to assign a new value and never removes the existing one. The SDK consequently emits the explicit cache parameters that this capability check is intended to withhold, causing the provider to reject the request, so all configuration sources must pass through the model gate.

AGENTS.md reference: AGENTS.md:L22-L27

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a3e3fa771da0040c4d9e39d00b5909ebd70247ab. This was the more serious of the two, because it produces a provider rejection rather than a cache miss, and you have the mechanism exactly right: promptCacheExplicit is in knownOpenAIParams, so the addParams and defaultParams loops assign it onto llmConfig before the capability check ever runs, and that check only declined to set a value it never removed.

On a first-party surface an unsupported model now has the field removed rather than merely not added. A gateway still keeps whatever it is configured with, which is this change's stated non-goal and the reason the removal is scoped rather than global.

The existing test passed because it drove the function parameter, not the parameter sources. A new case covers both addParams and defaultParams against gpt-4o.

Verified on a3e3fa771da0040c4d9e39d00b5909ebd70247ab.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a3e3fa771d

ℹ️ 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".

Comment thread packages/api/src/agents/run.ts Outdated
}
if (options.promptCacheKeyEnabled === true && options.promptCacheKey == null) {
const { graphTools } = input as AgentInputs & { graphTools?: GenericTool[] };
options.promptCacheKey = buildPromptCacheKey({

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Give sanitized self-spawns their own cache key

When self-spawn is enabled with background or intent tools, the sanitized agentInputs copy at lines 1953-1969 retains the parent's clientOptions object and is never passed to finalizePromptCacheKey; this assignment later mutates that shared object using the parent's unsanitized tools, so the isolated child still inherits a key for a different wire tool prefix. Fresh evidence after the prior finalization finding is that moving synthesis later exposed this shared-reference path rather than giving the sanitized copy an independently finalized options object.

AGENTS.md reference: AGENTS.md:L22-L27

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 189c62f7245c42d10cf25baa230f4e491f091724. Confirmed: the sanitized self-spawn entry was a shallow spread of the parent input, so clientOptions was literally the parent's object, and finalizing the parent stamped this child with a key naming the tools the child strips. The child now takes its own clientOptions copy and is finalized against the tools it actually sends.

Your framing is right that the previous round's change is what exposed this — moving synthesis later turned a shared reference from harmless into a defect. Same cause as #discussion_r4015112635 and #discussion_r4015112637.

Verified on 189c62f7245c42d10cf25baa230f4e491f091724.

Comment thread packages/api/src/agents/run.ts Outdated
options.promptCacheKey = buildPromptCacheKey({
model: options.model,
instructions: input.instructions,
boundTools: [...(input.toolDefinitions ?? []), ...(graphTools ?? []), ...(input.tools ?? [])],

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Include generated subagent tools in the cache identity

When an agent enables, disables, or changes its subagent configurations, the SDK injects or reshapes the model-facing subagent tool (packages/data-provider/src/schemas.ts:405 explicitly notes that disabling the feature prevents that injection), but this finalized projection hashes only toolDefinitions, graphTools, and tools. Because agentInput.subagentConfigs is assigned before finalization yet omitted here, materially different subagent tool prefixes retain the same cache key instead of retiring it.

AGENTS.md reference: AGENTS.md:L22-L27

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 189c62f7245c42d10cf25baa230f4e491f091724. Verified in the SDK rather than inferred: Graph.ts:5164-5170 turns a non-empty subagentConfigs into executable configs and :2841-2844 adds the delegation tool to the model-facing set, so it is generated from those entries and never appears in toolDefinitions, graphTools or tools. Enabling, disabling or retargeting subagents therefore changed the wire prefix while the key stood still.

The finalized projection now includes each entry's name and description. Only that identity is hashed, not the child inputs: a child's prefix is keyed on the child's own request, and pulling child state into the parent's key would over-partition it.

Verified on 189c62f7245c42d10cf25baa230f4e491f091724.

Comment on lines +986 to +988
const explicitCacheSupported =
supportsExplicitPromptCache(llmConfig.model) ||
supportsExplicitPromptCache(azure ? azure.azureOpenAIApiDeploymentName : undefined);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Gate explicit caching on the finalized Azure deployment

When AZURE_USE_MODEL_AS_DEPLOYMENT_NAME=true, the actual Azure deployment is replaced later with the sanitized visible model, but this capability check still accepts the originally configured deployment name. For example, visible model gpt-4o plus configured deployment gpt-5-6 enables promptCacheExplicit, after which lines 1107-1109 route the request to the unsupported gpt-4o deployment; the explicit parameters can then make the provider reject the request. Compute the gate from the same finalized deployment selection used for the wire request.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 189c62f7245c42d10cf25baa230f4e491f091724. Confirmed, including the exact route: AZURE_USE_MODEL_AS_DEPLOYMENT_NAME=true replaces the configured deployment with the sanitized visible model at llm.ts:1112-1113, after the gate had already accepted the configured name — so gpt-4o behind a gpt-5-6 deployment enabled explicit caching and then addressed the unsupported deployment.

Rather than widen the check again, the gate is now a closure invoked once the wire identity is final, at both the non-Azure and the Azure return path. That is the same structural correction as the other two findings in this round: decide from the finished request, not a mid-flight snapshot.

One consequence worth recording: running after the drop cascade, the gate also had to re-honor dropParams: ['promptCacheExplicit'], otherwise it reinstated what the cascade had removed. An existing test caught that before this was pushed.

Verified on 189c62f7245c42d10cf25baa230f4e491f091724.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 189c62f724

ℹ️ 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".

Comment thread packages/api/src/agents/run.ts Outdated
Comment thread packages/api/src/agents/run.ts Outdated
Comment thread packages/api/src/agents/run.ts Outdated
Comment thread packages/api/src/agents/run.ts Outdated
Comment thread packages/api/src/endpoints/openai/promptCache.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 22d33ddb61

ℹ️ 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".

Comment thread packages/api/src/agents/run.ts Outdated
@berry-13
berry-13 force-pushed the berry-13/enhancement-optimize-gpt-5.6-prompt-caching-for branch from b519c83 to 4c78aab Compare September 16, 2026 08:35

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4c78aab79a

ℹ️ 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".

Comment thread packages/api/src/agents/run.ts Outdated
Comment thread packages/api/src/agents/run.ts
@berry-13
berry-13 force-pushed the berry-13/enhancement-optimize-gpt-5.6-prompt-caching-for branch from 9d3479a to cb6cd88 Compare September 16, 2026 21:14

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cb6cd8833d

ℹ️ 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".

Comment on lines +991 to +992
if (typeof llmConfig.user === 'string') {
llmConfig.promptCacheScopeId = llmConfig.user;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Derive user cache scope from the authenticated user

When a first-party endpoint configures a constant addParams.user, the earlier parameter loop overwrites the authenticated user ID before these lines capture it, so every user receives the same supposedly user-scoped cache key and the documented accounting/probing boundary collapses. Fresh evidence after the resolved dropped-user thread is that promptCacheScopeId is still copied from the mutable wire parameter after addParams processing; derive it from the authenticated createRun user instead.

AGENTS.md reference: AGENTS.md:L22-L27

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e932a86e18024c97e08f9347f0663dd23c7c7b69 (the derivation landed in 9d7d6243b1). Confirmed exactly as reported: promptCacheScopeId was copied from llmConfig.user after the addParams loop, so a constant addParams.user collapsed every user onto one entry — the second way that field could be rewritten after the previous round closed the dropParams one.

Reading the field back was the mistake, so it is no longer read at all. getOpenAILLMConfig stops producing a partition identity, and createRun stamps it in buildAgentInput from the run's authenticated user:

const cacheOptions = llmConfig as Partial<t.OAIClientOptions>;
if (cacheOptions.promptCacheKeyEnabled === true && typeof user?.id === 'string') {
  cacheOptions.promptCacheScopeId = user.id;
}

That closure is the toInput every subagent input is built through, so a delegated child carries the same partition rather than inheriting a blank one.

Verified on the published head: run-promptCache.test.ts drives the real createRun and pins addParams: { user: 'tenant-fixed' } (asserting the pinned value really reached clientOptions.user), dropParams: ['user'], and a delegated child — all three keep two users on different keys. Through the app, @scenario:two-users-of-one-agent-get-separate-cache-keys gives two authenticated users byte-identical agents and asserts the keys differ.

Comment thread packages/api/src/agents/run.ts Outdated
*/
agents: memberConfigs.map((member) =>
sealSubagentInputs(
prebuiltGraphInputs?.get(member.id) ?? buildIsolatedAgentInputs(member, toInput),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clone shared graph inputs before sealing each team

When the same saved agent is a member of two graph subagent definitions with different outgoing edges, this map returns the same AgentInputs object for both. The first sealSubagentInputs call computes a key and deletes its enable marker, so the second graph cannot recompute the key and retains an identity for the first graph's transfer tools. Fresh evidence after the resolved saved-team thread is the direct reuse of the cached mutable input here; each graph occurrence needs an independently sealable copy.

AGENTS.md reference: AGENTS.md:L22-L27

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e932a86e18024c97e08f9347f0663dd23c7c7b69, and the mechanism turned out to be two things rather than one — the second of which you found again a round later as #discussion_r4041186982.

You are right that the shared object was sealed once: sealSubagentInputs writes the key and deletes the marker that allows it, so the second occurrence could not recompute. ownSealableInputs now gives each occurrence its own shell (the built tool arrays and registry stay shared, so the build is still done once):

agents: memberConfigs.map((member) =>
  sealSubagentInputs(
    ownSealableInputs(prebuiltGraphInputs?.get(member.id) ?? buildIsolatedAgentInputs(member, toInput)),
    [],
    outgoingHandoffEdges(definition.edges, member.id),
  ),
),

The part worth recording is that the divergence you predicted should not have existed in the first place. A saved team's edges are edgeType: 'direct' by schema (GraphSubagentEdge), and a direct edge creates automatic routing rather than an lc_transfer_to_* tool — packages/api/src/agents/tools.ts skips exactly those when it builds the model-facing tool allowlist. Hashing them was the real defect: it split a member across teams that differ only in routing, which is the reuse this feature exists for. outgoingHandoffEdges now filters them, so both occurrences of a member describe one prefix and share one entry.

Verified on the published head: run-promptCache.test.ts builds one member into two teams with different direct edges and asserts both sealed keys exist and agree, plus a pair of cases pinning that a handoff edge retires the key while a direct one does not.

Comment thread packages/api/src/agents/run.ts Outdated
typeof options.modelKwargs?.model === 'string' ? options.modelKwargs.model : options.model;
options.promptCacheKey = buildPromptCacheKey({
model: wireModel,
instructions: input.instructions,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Include stable additional instructions in the cache identity

When a saved agent's additional_instructions or an isolated child's always-apply skill body changes while its base instructions and tools remain unchanged, the model receives a different system prefix but this digest remains identical because it hashes only input.instructions. buildIsolatedAgentInputs explicitly appends always-apply skill bodies to additional_instructions, so a skill edit is not covered by the deployment-level key version; preserve the exclusion of volatile memory/file context while separately hashing the stable additional-instruction sources.

AGENTS.md reference: AGENTS.md:L22-L27

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e932a86e18024c97e08f9347f0663dd23c7c7b69 (derivation in 9d7d6243b1), for the always-apply half, with the volatile half deliberately still out.

You separated the two correctly and that separation is the whole fix. input.additional_instructions is the dynamic system tail: prepareRuntimeAgent appends shared run context, memory and file context to it every turn, so hashing it would give every conversation its own entry and leave nothing to reuse. It stays excluded, with that reason recorded in the disposition map.

The stable source folded into it is captured where it is still separable — the one place that knows which half is configuration:

childInputs.additional_instructions = [childInputs.additional_instructions, skillInstructions]…
const childOptions = childInputs.clientOptions as Partial<t.OAIClientOptions> | undefined;
if (childOptions != null) {
  childOptions.promptCacheStableInstructions = skillInstructions;
}

The marker is hashed and then deleted with the others, so it never reaches the wire. A primary agent needs no equivalent: its always-apply skills are injected as conversation messages by injectSkillPrimes, not into its instruction prefix.

Verified through the app on the published head: @scenario:always-apply-skill-edit-retires-the-child-key creates an always-apply skill, attaches it to a delegated child, reads the child's key, PATCHes the skill body, and asserts the child's key retired.

Comment thread packages/api/src/agents/run.ts Outdated
Comment on lines +1870 to +1872
options.promptCacheKey = buildPromptCacheKey({
model: wireModel,
instructions: input.instructions,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Separate Chat and Responses cache identities

When the same model and agent switch between Chat Completions and the Responses API—for example by changing addParams.useResponsesApi—this call produces the same key whenever no structured-output format distinguishes the requests. Those APIs serialize the instruction and tool prefix through different wire shapes, so retaining one identity can route or account two different prefixes together; include the finalized options.useResponsesApi mode in the hashed payload.

AGENTS.md reference: AGENTS.md:L22-L27

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 9d7d6243b1485f39e6056d42c4fdf3f9d4e47dba, and not by adding a sixth field to the list.

You have it right that the two APIs serialize one instruction and tool prefix into different wire shapes, so one identity must not cover both. That was the fifth model-facing surface this key had to be told about after the fact, which is the actual defect: nothing failed when a field was missing. The derivation is now exhaustive by construction — buildPromptCacheKey takes the finished AgentInputs and walks it against a total Record<keyof AgentInputs, …>, where every field is either hashed or excluded with the reason it cannot change the prefix, and an SDK upgrade that adds a field fails the typecheck in promptCache.ts instead of quietly leaving the digest.

clientOptions is projected the same way — everything except a declared non-prefix set (credentials, transport, sampling, the cache levers) participates — so useResponsesApi, response_format and text.format all enter the identity, with model resolved to the modelKwargs override Azure Astra actually addresses. A field present at runtime but absent from the map is hashed rather than dropped, so a newer SDK than these types partitions the cache (a miss) instead of pointing two prefixes at one entry.

The exclusions are tested too, because each one is a claim: per-request transport stays out (resolveConfigHeaders resolves ${conversationId} into configuration.defaultHeaders), and reasoning effort and verbosity stay out because moving those sliders does not change what the model reads.

Verified on the published head by @scenario:responses-api-switch-retires-the-cache-key, which flips the agent's useResponsesApi through PATCH /api/agents/:id and asserts a different key across the switch.

@berry-13
berry-13 force-pushed the berry-13/enhancement-optimize-gpt-5.6-prompt-caching-for branch from cb6cd88 to 1dba5be Compare September 17, 2026 20:19

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1dba5be8a9

ℹ️ 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".

Comment on lines +1895 to +1897
return (edges ?? [])
.filter((edge) => (Array.isArray(edge.from) ? edge.from : [edge.from]).includes(agentId))
.map(handoffEdgeIdentity);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude direct edges from the prompt-cache identity

For saved subagent teams, whose schema restricts edges to edgeType: 'direct', this helper still adds every outgoing edge to the digest. Fresh evidence beyond the earlier edge comments is that the production tool allowlist explicitly skips direct edges in packages/api/src/agents/tools.ts:364-367, matching the GraphEdge contract that only handoff edges create lc_transfer_to_* tools. Consequently, the same member used in teams that differ only in automatic routing gets separate cache keys despite an identical model-facing prefix, reducing the reuse this feature is intended to provide; filter out direct edges before hashing.

AGENTS.md reference: AGENTS.md:L22-L27

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.filter((edge) => (Array.isArray(edge.from) ? edge.from : [edge.from]).includes(agentId))
.map(handoffEdgeIdentity);

}
handoffEdgeIdentityinstructionsrun-promptCache.test.tshandoffdirect` edge does not, a spelled-out default name agrees with an omitted one, and one member in two teams with different direct edges keeps one identity.
EOF
)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e932a86e18024c97e08f9347f0663dd23c7c7b69. Correct, with the evidence you cite: only a handoff edge becomes an lc_transfer_to_* tool, packages/api/src/agents/tools.ts skips edgeType: 'direct' when it builds the model-facing allowlist, and a saved team's edges are direct by schema (GraphSubagentEdge).

function outgoingHandoffEdges(edges: readonly GraphEdge[] | undefined, agentId: string): unknown[] {
  return (edges ?? [])
    .filter((edge) => edge.edgeType !== 'direct')
    .filter((edge) => (Array.isArray(edge.from) ? edge.from : [edge.from]).includes(agentId))
    .map(handoffEdgeIdentity);
}

This is the same cause as #discussion_r4030918204, in the opposite direction: that finding read the split as a missing identity, and it was really a surplus one. Hashing automatic routing partitioned exactly the case the feature exists to serve — one agent reused across teams — so filtering it dissolves the team-occurrence divergence rather than papering over it. The per-occurrence copy introduced for that finding stays, because sealing still has to be occurrence-local.

Folded in from the same reading: handoffEdgeIdentity now resolves the handoff parameter name rather than passing it through, since the SDK falls back to instructions and an edge that spelled the default out was partitioning away from one that left it unset while advertising the same tool.

Verified on the published head: run-promptCache.test.ts pins that a handoff edge retires the key, a direct edge does not, a spelled-out default name agrees with an omitted one, and one member listed by two teams with different direct edges keeps a single identity.

(The earlier reply on this thread was posted truncated by a shell quoting error on my side — this is the complete answer.)

Comment thread packages/api/src/agents/run.ts Outdated
Comment on lines +1466 to +1470
provider === fallbackProvider &&
agentParameters?.promptCacheKeyEnabled === true &&
parameters?.promptCacheKey == null
) {
parameters = { ...parameters, promptCacheKey: undefined };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Re-gate explicit caching for same-endpoint summaries

When a GPT-5.6 agent has endpoint-level promptCacheExplicit: true but its same-endpoint summarizer selects an unsupported model such as gpt-4o, the summarization path reuses the agent's client options and this cleanup clears only promptCacheKey. The inherited promptCacheExplicit: true therefore survives onto the summary client, bypassing the model gate in getOpenAILLMConfig and causing the provider to reject the explicit cache parameters once summarization triggers; recompute or explicitly clear this flag for the summary model unless the summary parameters intentionally supply their own value.

AGENTS.md reference: AGENTS.md:L22-L27

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e932a86e18024c97e08f9347f0663dd23c7c7b69. This was the more serious of the two findings in this round, because it ends in a provider rejection rather than a cache miss, and you have the route exactly right: the capability gate in getOpenAILLMConfig ran against the agent's model, the same-endpoint branch supplies no client overrides, and the summary client therefore inherited promptCacheExplicit: true past its own gate.

The cleanup now covers the flag as well as the key, scoped the same way — an inherited value is withheld, a value the summarization config chose for itself survives:

if (
  provider === fallbackProvider &&
  agentParameters?.promptCacheExplicit === true &&
  userParameters?.promptCacheExplicit == null &&
  !supportsExplicitPromptCache(model)
) {
  parameters = { ...parameters, promptCacheExplicit: undefined };
}

Gated on the summary model rather than cleared unconditionally on purpose: a same-provider summarizer that runs the agent's own GPT-5.6 model sends a prefix that does accept the explicit controls, and there is no reason to withhold them there.

Verified on the published head by a new run-summarization.test.ts case: a first-party GPT-5.6 agent with endpoint-level promptCacheExplicit: true summarizing on gpt-4o ends with both promptCacheExplicit and promptCacheKey cleared on the summary parameters.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8a6581cd2b

ℹ️ 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".

clientOptions: identity(clientOptionsIdentity),
/** Every surface the model is offered a tool from. */
tools: identity(toolsIdentity),
toolDefinitions: identity(toolsIdentity),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude discovered tool definitions from the cache key

When two conversations have discovered different deferred tools, buildAgentInput promotes those per-conversation definitions into toolDefinitions at run.ts:2618-2626, and this identity projection hashes them even though discoveredTools itself is deliberately excluded. The resulting key changes with each conversation's discovery state, contradicting the documented cross-conversation identity and preventing later turns from routing to the stable prefix cached by another conversation; retain or project the configured definition set separately from definitions promoted from discovery.

AGENTS.md reference: AGENTS.md:L22-L27

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8bb378fea5f4c08a8a23ff934dc3d1d4af602e21. Correct, and it contradicted the key's central claim rather than a detail: buildAgentInput promotes a deferred tool's definition onto the request once a conversation has discovered it, so hashing toolDefinitions wholesale made the identity follow each conversation's discovery state — exactly what excluding discoveredTools was supposed to prevent.

Promoted names are recorded where the promotion happens and excluded by name, rather than by filtering on discoveredTools:

const toolDef = agent.toolRegistry.get(toolName);
if (toolDef) {
  toolDefinitions = [...toolDefinitions, toolDef];
  promotedDiscoveredToolNames.push(toolName);
}

By name from the promotion list on purpose. A tool can be both configured and previously discovered, and filtering on discoveredTools would then drop a configured definition from the identity — turning an over-partition into a collision, which is the worse direction. The marker travels with the other cache markers and is deleted before the request is sent.

This also removes the defer_loading question hiding behind it: overrideDeferLoadingForDiscoveredTools mutates the registry entries that get promoted, so those objects no longer reach the digest at all, while a configured definition's own classification still does.

Verified on the published head by two promptCache.spec.ts cases: a promoted definition leaves the key unchanged, and a configured tool that happens to share a discovered name still keys.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e932a86e18

ℹ️ 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".

Comment thread packages/api/src/agents/run.ts Outdated
provider === fallbackProvider &&
agentParameters?.promptCacheExplicit === true &&
userParameters?.promptCacheExplicit == null &&
!supportsExplicitPromptCache(model)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Gate Azure summary caching on the resolved deployment

When an Azure agent's visible model is an alias such as production-chat mapped to a supported gpt-5-6-prod deployment, a same-model self-summary inherits promptCacheExplicit: true but this check evaluates only the alias and clears the flag, silently disabling the configured explicit caching behavior. Fresh evidence beyond the resolved same-endpoint-summary thread is that resolveAzureSummarization exposes the finalized deployment through clientOverrides.modelKwargs.model, yet this re-gate never considers it; use the resolved wire deployment as well as the visible model.

AGENTS.md reference: AGENTS.md:L22-L27

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8bb378fea5f4c08a8a23ff934dc3d1d4af602e21. Correct: the re-gate I added a round earlier read only the visible model, so an Azure alias such as production-chat fronting a supported deployment lost the explicit caching an administrator had configured — the same alias-versus-deployment split that getOpenAILLMConfig already had to learn.

It now reads the deployment the summary request actually addresses, which resolveAzureSummarization puts on the modelKwargs override:

const summaryWireModel = isPlainObject(parameters?.modelKwargs)
  ? parameters.modelKwargs.model
  : undefined;
const summarySupportsExplicitCache =
  supportsExplicitPromptCache(model) ||
  (typeof summaryWireModel === 'string' && supportsExplicitPromptCache(summaryWireModel));

Both directions are now gated on the request that is sent: an unsupported summary model still has the inherited flag withheld (run-summarization.test.ts pins a GPT-5.6 agent summarizing on gpt-4o), and a supported deployment behind an opaque alias keeps it.

The residual limit is the same one recorded for the main path: a fully opaque alias fronting an opaque deployment name is undecidable from names alone, and an administrator who needs it there can set promptCacheExplicit on the summarization config, which this cleanup deliberately leaves alone.

Comment on lines +984 to +987
llmConfig.promptCacheKeyEnabled = true;
if (promptCacheScope != null) {
llmConfig.promptCacheScope = promptCacheScope;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject user-supplied cache-scope controls

When an agent author puts promptCacheScope: 'shared' in model_parameters, the public agent schema accepts it through z.record(z.unknown()), and getOpenAILLMConfig copies it into llmConfig before this block. With the endpoint's documented default scope omitted, this conditional leaves that user-supplied value intact, so buildPromptCacheKey drops the authenticated user from the key and an ordinary agent author can opt all consumers of a shared agent into cross-user cache accounting/probing without administrator approval. Fresh evidence beyond the administrator-pinned-key thread is that these controls also arrive through user-editable stored model parameters; strip them from modelOptions and source the scope only from trusted endpoint configuration.

AGENTS.md reference: AGENTS.md:L31-L34

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8bb378fea5f4c08a8a23ff934dc3d1d4af602e21. Correct, and the more important half of your framing is the mechanism rather than the field: model_parameters is author-owned, the public agent schema accepts it as a free-form record, and getOpenAILLMConfig builds llmConfig by spreading modelOptions — so every prompt-cache control could arrive from an agent author, not just the scope.

So the fix is the rule rather than one conditional. All seven are stripped before policy resolution, and only endpoint configuration puts them back:

for (const field of [
  'promptCacheKey',
  'promptCacheKeyEnabled',
  'promptCacheScope',
  'promptCacheScopeId',
  'promptCacheStableInstructions',
  'promptCacheRetention',
  'promptCacheExplicit',
] as const) {
  delete (llmConfig as Record<string, unknown>)[field];
}

What each one bought an author before that: promptCacheScope: 'shared' dropped the user from the key, so a cache hit on a shared agent reveals that someone else already sent a guessable prompt — the boundary this feature's default exists to keep; a pinned promptCacheKey did the same thing by another route; promptCacheRetention: '24h' is billed differently; promptCacheScopeId would have forged the partition identity createRun stamps; promptCacheKeyEnabled would have synthesized a key on an endpoint the policy excludes.

addParams is unaffected and remains the administrator's route to a pinned key, because it is applied after this point from librechat.yaml.

Verified on the published head by two llm.spec.ts cases: an agent whose model parameters carry all five author-visible controls ends with none of them on the config and the ordinary per-user policy intact, while the same controls set through endpoint configuration still apply.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8bb378fea5

ℹ️ 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".

Comment thread packages/api/src/agents/run.ts Outdated
const toolDef = agent.toolRegistry.get(toolName);
if (toolDef) {
toolDefinitions = [...toolDefinitions, toolDef];
promotedDiscoveredToolNames.push(toolName);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve keys when prelisted deferred tools are discovered

When a deferred MCP tool is discovered in prior history, buildToolClassification has already copied every registry value into toolDefinitions, preserving the same object reference. Consequently existingToolNames takes the continue branch, this newly added recording never occurs, and overrideDeferLoadingForDiscoveredTools has still mutated the hashed definition from defer_loading: true to false. The cache key therefore changes with conversation discovery state instead of remaining reusable. Fresh evidence after the earlier discovered-tool comment is that the attempted marker only covers newly appended definitions, not these prelisted shared definitions; project their configured state consistently on both sides of discovery.

AGENTS.md reference: AGENTS.md:L22-L27

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already fixed — true on 8bb378f, corrected on a2e45333f95ae8c16e0a8bbc910bdeffaf01d078, and your reading of the mechanism is what the fix rests on: toolDefinitions is built from the registry's own values, so overrideDeferLoadingForDiscoveredTools reshapes definitions that are already bound and the continue branch meant they were never recorded.

Every discovered definition is recorded now, appended or not, and the digest restores the configured state captured before the override — so a conversation that discovered a prelisted deferred tool and one that has not hash the same agent. The recorded value can be false or absent, which is the case #discussion_r4041917494 then named.

Comment on lines +701 to +703
'promptCacheStableInstructions',
'promptCacheRetention',
'promptCacheExplicit',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Strip the discovered-tool marker from agent parameters

When an agent author submits model_parameters.promptCacheDiscoveredToolNames, the public agent schema accepts it through its unknown-valued parameter record, but this administrator-boundary cleanup omits that internal marker. If the conversation has no genuinely promoted tools, buildAgentInput leaves the supplied array intact and buildPromptCacheKey trusts it, removing any named model-facing definitions from the identity. An author can therefore make materially different tool prefixes share a key. Fresh evidence after the prior user-supplied cache-controls thread is that the newly introduced discovered-tool marker is the only cache marker absent from this stripping list; remove it alongside the other internal fields.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate of #discussion_r4041917479, which names both discovery markers; fixed there in a2e45333f95ae8c16e0a8bbc910bdeffaf01d078.

The shared cause is worth recording here: the markers lived in three hand-kept lists and a newly added one reached only two. promptCache.ts now declares PROMPT_CACHE_MARKER_FIELDS and PROMPT_CACHE_ADMIN_FIELDS once, and the strip in getOpenAILLMConfig, the delete in finalizePromptCacheKey and the digest's exclusion set all read from them.

Comment thread packages/api/src/agents/run.ts Outdated
return {
to: edge.to,
...(typeof edge.description === 'string' ? { description: edge.description } : {}),
...(edge.edgeType != null ? { edgeType: edge.edgeType } : {}),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Normalize the default handoff edge type

When one persisted handoff edge omits edgeType and an otherwise identical edge spells out edgeType: 'handoff', both create the same model-facing transfer tool: outgoingHandoffEdges treats every non-direct edge as a handoff, and the repository's edge identity helper likewise resolves an omitted type to handoff. This projection includes the field only in the explicit case, however, so the two equivalent configurations receive different cache keys and cannot reuse the same cached prefix. Hash the resolved type (edge.edgeType ?? 'handoff') instead.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already fixed — true on 3a21668, resolved on a2e45333f95ae8c16e0a8bbc910bdeffaf01d078:

/** Absent means `handoff`, so both spellings must hash alike. */
edgeType: edge.edgeType ?? 'handoff',

run-promptCache.test.ts pins that an edge omitting edgeType and one spelling out handoff produce the same key, and that a direct edge changes nothing because it creates no transfer tool.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3a21668977

ℹ️ 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".

Comment on lines +279 to +281
.filter((tool) => {
const name = (tool as { name?: unknown } | null)?.name;
return typeof name !== 'string' || !context.promotedToolNames.has(name);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve configured deferred definitions in the cache identity

When restored history shows that an already-listed deferred tool was discovered, the undiscovered request hashes its complete defer_loading: true definition, while the discovered request mutates that flag to false and this filter then removes the entire definition, so the two conversations still receive different keys. Fresh evidence on the current head is that the new test compares two inputs that both carry the discovery marker, causing both definitions to be filtered; it never compares against the unmarked pre-discovery identity. Preserve or reconstruct the configured pre-mutation definition instead of dropping it.

AGENTS.md reference: AGENTS.md:L22-L27

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a2e45333f95ae8c16e0a8bbc910bdeffaf01d078, with #discussion_r4041917494 — same cause, and your note about the test is the more useful half: both inputs it compared carried the discovery marker, so it never compared against the unmarked pre-discovery identity and could not see the split.

The identity reconstructs the recorded pre-discovery definition instead of dropping it, and the spec now compares exactly that pair — a discovered definition against the same agent before discovery. Only a definition the configured request never carried is dropped, because an undiscovered conversation does not send it at all.

Comment on lines +155 to +156
'max_tokens',
'max_completion_tokens',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude Responses output limits from the cache identity

When a GPT-5+ Responses agent changes its maximum output-token setting, getOpenAILLMConfig moves that value into modelKwargs.max_output_tokens, but this exclusion list omits that spelling while excluding the equivalent Chat Completions limits. The output budget does not change the instruction or tool prefix, yet every such slider/configuration change retires the cache key and forfeits otherwise reusable cached prefixes; exclude max_output_tokens alongside the other token-limit fields.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a2e45333f95ae8c16e0a8bbc910bdeffaf01d078. Correct: max_output_tokens is the Responses spelling of an output budget, it cannot change the instruction or tool prefix, and it was the one token limit missing beside max_tokens and max_completion_tokens. Both spellings are excluded now, and promptCache.spec.ts pins that changing the Responses output budget leaves the key alone.

Comment thread packages/api/src/agents/run.ts Outdated
* out and one that leaves it unset advertise the same parameter and must
* not land in different cache partitions.
*/
promptKey: typeof edge.promptKey === 'string' ? edge.promptKey : 'instructions',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Ignore promptKey when the handoff has no prompt

For a handoff edge without prompt, the GraphEdge contract says promptKey does not apply because no handoff input parameter is created, but this projection hashes a configured value anyway. Thus adding or changing an inert promptKey gives the source agent a different cache key despite an identical model-facing transfer tool, unnecessarily fragmenting reuse; only include the resolved parameter name when edge.prompt is present.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a2e45333f95ae8c16e0a8bbc910bdeffaf01d078. Correct: promptKey names the handoff input parameter, GraphEdge only creates that parameter when the edge has a prompt, and hashing an inert value fragmented reuse over something no tool carries. It is included only alongside a prompt now, with the default resolution kept for the case where it does apply.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 17239bc06b

ℹ️ 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".

Comment thread packages/api/src/endpoints/openai/llm.ts
Comment thread api/server/controllers/agents/client.js Outdated
* author's text apart from this run's memory and file context, and the
* prompt cache identity has to follow the configured half.
*/
agent.configuredAdditionalInstructions = agent.additional_instructions;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Capture stable additional instructions on every run path

When a run enters through agents/openai.js, agents/responses.js, the API service, or the resume-only preparation branch, this local normalizeInstructions helper never executes before createRun; those paths initialize or apply context and then call createRun directly. Because the key projection excludes additional_instructions and hashes only configuredAdditionalInstructions, editing a saved agent's additional instructions leaves the same cache identity on those surfaces. Fresh evidence after the resolved stable-instructions thread is these additional createRun callers that bypass the new capture; move the capture to a shared initialization/context boundary.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already fixed — true on 3a21668, corrected on a2e45333f95ae8c16e0a8bbc910bdeffaf01d078, along the line you recommend.

The capture happens at two shared boundaries now: initializeAgent, before anything appends to the tail, and applyContextToAgent, before run context is joined. It is idempotent, so the first writer wins. agents/openai.js, agents/responses.js and the service all call initializeAgent, and the resume paths pass through applyContextToAgent, so every createRun caller carries it — the chat controller's local helper no longer does anything cache-related, and /api has no diff against dev in this change.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3b1bdd25c1

ℹ️ 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".

Comment on lines +703 to +704
'promptCacheExplicit',
] as const) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Strip both discovery markers from model parameters

When an agent author submits both promptCacheDiscoveredToolNames and promptCacheAppendedToolNames through the public free-form model_parameters, this cleanup leaves both intact. With no genuine discovery, buildAgentInput does not overwrite them, and buildPromptCacheKey consequently omits every matching model-facing tool definition, allowing different tool prefixes to share a synthesized key. Fresh evidence after the existing discovered-marker finding is the current head's additional promptCacheAppendedToolNames marker; strip both internal markers here.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a2e45333f95ae8c16e0a8bbc910bdeffaf01d078. Correct, and it is the earlier author-parameter defect arriving through a list that fell behind: the markers were kept in three hand-maintained places — stripped from model parameters, deleted before the request, excluded from the digest — and the two newest ones reached only two of them.

The lists are gone. promptCache.ts declares the fields once and everything else consumes them:

export const PROMPT_CACHE_MARKER_FIELDS = [
  'promptCacheKeyEnabled',
  'promptCacheScope',
  'promptCacheScopeId',
  'promptCacheStableInstructions',
  'promptCacheConfiguredToolState',
] as const;

export const PROMPT_CACHE_ADMIN_FIELDS = [
  'promptCacheKey',
  'promptCacheRetention',
  'promptCacheExplicit',
  ...PROMPT_CACHE_MARKER_FIELDS,
] as const;

getOpenAILLMConfig strips the admin set, finalizePromptCacheKey deletes the marker set, and the digest's non-prefix set spreads the admin set, so a field added to the protocol reaches all three by construction. The two discovery markers also collapse into one structured field, leaving one fewer thing to keep in step, and llm.spec.ts asserts that an author submitting these through model_parameters gets none of them.

Comment on lines +83 to +87
const nonPrefixClientOptionKeys: ReadonlySet<string> = new Set([
/** Credentials and transport. */
'apiKey',
'organization',
'configuration',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude Azure credentials from the cache identity

When Azure Chat Completions uses API-key authentication, getOpenAILLMConfig copies updatedAzure.azureOpenAIApiKey into the client options, and only the Responses path removes it. This exclusion set handles the generic apiKey but not azureOpenAIApiKey, so rotating an Azure resource key changes the deterministic digest despite identical instructions, tools, model, and deployment, discarding otherwise reusable cache entries; exclude the Azure authentication fields as credentials too.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a2e45333f95ae8c16e0a8bbc910bdeffaf01d078. Correct: the Chat Completions path keeps azureOpenAIApiKey on the client options and the generic apiKey exclusion did not cover it, so rotating an Azure resource key churned the key for an otherwise identical agent — a pure loss, since the deployment is what identifies the request.

The Azure credential and transport fields join the credential exclusions (azureOpenAIApiKey, azureOpenAIBasePath, azureOpenAIEndpoint, azureADTokenProvider), while the deployment stays in the identity through the modelKwargs override. promptCache.spec.ts pins that a rotated Azure key leaves the key unchanged.

* back, so a change to this tool's schema or classification still retires
* the key while a conversation discovering it does not.
*/
identities.push(safeIdentity({ ...(tool as Record<string, unknown>), defer_loading: true }));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the configured deferred-tool state

When restored history records discovery of a tool that an administrator has since changed from deferred to eager, the discovery marker still names it and this line forces its cache identity back to defer_loading: true even though the current configured value is false or absent. New conversations then hash the eager definition while restored conversations using the same current configuration retain the old deferred identity. Fresh evidence beyond the earlier discovery-marker threads is that the new reconstruction hard-codes true; capture the pre-mutation configured state instead.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a2e45333f95ae8c16e0a8bbc910bdeffaf01d078. Correct, and it is a defect my previous round introduced: reconstructing defer_loading: true assumed the configured value, which is wrong in exactly the case you name — an administrator who has since made the tool eager, where a restored conversation that recorded the discovery hashes true while a fresh one hashes the current configuration.

The marker now records what discovery overwrote, read before the override runs, and the digest puts that value back including its absence:

if (configured.deferLoading === undefined) {
  delete restored.defer_loading;
} else {
  restored.defer_loading = configured.deferLoading;
}

Three promptCache.spec.ts cases pin it: a discovering conversation agrees with one that has not discovered the tool, an administrator making it eager retires the key, and the tool's own schema still keys.

Comment thread packages/api/src/endpoints/openai/promptCache.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 604f5c8b92

ℹ️ 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".

Comment thread packages/api/src/agents/initialize.ts Outdated
Comment on lines +2254 to +2259
/**
* Before the temporal branch below moves a resolved instruction block into
* `additional_instructions`: that text carries today's date, so it belongs
* with the volatile tail rather than in the prompt cache identity.
*/
captureConfiguredAdditionalInstructions(agent);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Include generated artifact instructions in the cache identity

When an OpenAI/Azure agent enables artifacts, generateArtifactsPrompt appends the mode-specific prompt at lines 2295-2300 after this capture. Because captureConfiguredAdditionalInstructions is idempotent and buildPromptCacheKey excludes additional_instructions, changing the artifact mode—such as switching to SHADCNUI—changes the model-facing system text without changing the synthesized key. Capture the generated artifact prompt as stable instructions or perform the capture after all stable additions are applied.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a2e45333f95ae8c16e0a8bbc910bdeffaf01d078, and the fix took your second suggestion rather than the first, because the same class had already produced two findings.

Appending to the instruction tail now records the contribution as part of the cache identity unless the caller marks it request-scoped:

export function appendAgentInstructionTail(agent, text, options: { stable?: boolean } = {}): void

So the artifact prompt is covered, and so is the skill catalog that Devin found in the same place. Only three writers opt out — the temporally resolved instruction block (today's date), this run's context, and the per-run dynamic tool instructions. Forgetting the flag now over-partitions, which costs a cache miss instead of reusing a key for a different prefix.

Capturing strictly after all stable additions does not work on its own: the temporal block is appended between the author's text and the artifact prompt, so position cannot separate configuration from run data — the writers have to say which they are.

Comment on lines +156 to +158
const nonPrefixModelKwargsKeys: ReadonlySet<string> = new Set([
'verbosity',
/** `applyResponsesVerbosity` moves verbosity here; the output schema lives on the top-level `text`. */

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve Responses formats nested under modelKwargs

When a Responses agent supplies model_parameters.modelKwargs.text.format, the free-form model parameters preserve that object and LangChain sends it as the Responses output schema. Excluding the entire text value here therefore makes agents with materially different JSON schemas share a cache key; only the relocated text.verbosity field is non-prefix output configuration. Project text while removing verbosity rather than dropping the whole object.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a2e45333f95ae8c16e0a8bbc910bdeffaf01d078. Correct, and it is the mirror of that exclusion's own justification: applyResponsesVerbosity writes modelKwargs.text = { verbosity }, which is why the field was dropped — but a Responses agent can carry an output schema on the same object through model parameters, and LangChain sends it as the output format.

Only the verbosity leaves now; the rest of modelKwargs.text is hashed, and an empty projection is treated as absent so an agent that sets only verbosity still hashes like one that sets nothing. promptCache.spec.ts pins both directions.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a2e45333f9

ℹ️ 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".

Comment thread packages/api/src/agents/initialize.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4ae7494296

ℹ️ 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".

Comment on lines +696 to +698
for (const field of PROMPT_CACHE_ADMIN_FIELDS) {
delete (llmConfig as Record<string, unknown>)[field];
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Strip cache controls from fallback client options

When an agent author supplies model_parameters.fallbacks[].clientOptions, the free-form agent schema preserves the nested object and withModelCallbacks confirms those options are used to construct fallback clients. This loop removes administrator-only fields only from the top-level llmConfig, so a fallback can still carry an author-chosen promptCacheKey, billed promptCacheRetention: '24h', or unsupported promptCacheExplicit; once the primary fails, those values bypass the policy enforced here and reach the provider. Recursively strip PROMPT_CACHE_ADMIN_FIELDS from fallback client options before they are retained.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed; carried on the pushed head 99bfb24106. Correct, and the administrator boundary's last hole: withModelCallbacks builds a fallback client from its own clientOptions, so an author-supplied key, billed 24h retention or unsupported explicit caching there would reach the provider as soon as the primary client failed. The strip recurses into fallback client options now, and llm.spec.ts pins that a fallback loses all three while keeping its model.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cc295f17f7

ℹ️ 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".

Comment on lines +1027 to +1028
const supported =
supportsExplicitPromptCache(llmConfig.model) || supportsExplicitPromptCache(deploymentName);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Gate explicit caching on the modelKwargs wire model

When a first-party OpenAI agent has a supported visible model such as gpt-5.6 but supplies an unsupported wire override such as model_parameters.modelKwargs.model: 'gpt-4o', this check enables promptCacheExplicit based only on the visible model. The request and the cache identity both treat modelKwargs.model as the effective wire model, so GPT-4o receives the GPT-5.6-only explicit cache parameters and rejects the request. Include the finalized llmConfig.modelKwargs.model override in this capability gate.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed; carried on the pushed head 99bfb24106. Correct: modelKwargs.model overrides the visible model on the wire — the digest already keys on it — so a supported visible name fronting gpt-4o would have had the request rejected. The gate now requires every name the request can address to accept the controls, pinned by llm.spec.ts.

const identities: unknown[] = [];
for (const tool of value) {
const name = (tool as { name?: unknown } | null)?.name;
const configured = typeof name === 'string' ? state[name] : undefined;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restrict configured-tool lookups to own properties

When a configured tool is named toString or constructor and any other tool has been discovered in the conversation, state[name] resolves an inherited Object.prototype member even though no state was recorded for that tool. The projection consequently treats the tool as discovery-mutated and deletes its defer_loading classification, allowing deferred and eager versions of the model-visible tool surface to hash to the same key. Use an own-property check or a null-prototype record before restoring configured state.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed; carried on the pushed head 99bfb24106. Correct: a tool named after an inherited object property resolved a prototype member in the configured-state lookup, so its own defer_loading classification left the identity. The lookup is own-property now, with a promptCache.spec.ts case for a tool named toString.

* token placeholders into `configuration.defaultHeaders`, so hashing the
* transport would partition the cache per conversation and per turn.
*/
const nonPrefixClientOptionKeys: ReadonlySet<string> = new Set([

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude service tier from the prompt identity

When an administrator changes service_tier between values such as default, flex, or priority, clientOptionsIdentity hashes the setting because it is absent from this exclusion set. The tier changes request scheduling and cost, not the instructions, tool schemas, or output schema that form the cached prefix, so this produces a new deterministic key and forfeits an otherwise reusable cache entry. Exclude service_tier alongside the other non-prefix routing and sampling options.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed; carried on the pushed head 99bfb24106. Correct: the service tier changes scheduling and cost, not the prefix, so hashing it forfeited a reusable entry whenever an administrator moved between default, flex and priority. Both spellings are excluded, pinned by promptCache.spec.ts.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6379cf5486

ℹ️ 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".

for (const key of Object.keys(record).sort()) {
const item = canonicalize(record[key], seen);
if (item !== undefined) {
normalized[key] = item;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve __proto__ in canonicalized objects

When a model-facing JSON schema contains an own property named __proto__—for example, an OpenAPI action parameter with that name—assigning it onto this ordinary object invokes the legacy prototype setter instead of creating an own property. The field then disappears from JSON.stringify(canonicalize(payload)), so schemas with and without that parameter can receive the same prompt-cache key despite exposing different tool definitions; construct normalized with a null prototype or define keys as data properties.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed; carried on the pushed head 99bfb24106. Correct: assigning a key named __proto__ onto an ordinary object invokes the legacy setter instead of creating an own property, so a tool schema carrying that field vanished from the digest and hashed like a schema without it. canonicalize and every record the projections build have a null prototype now, with a promptCache.spec.ts case built from parsed JSON so the property is genuinely own.

Comment on lines +2702 to +2704
configuredToolState[toolName] = existingToolNames.has(toolName)
? { deferLoading: toolDef.defer_loading }
: { appended: true };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Record __proto__ tool discovery state as an own property

When a deferred tool named __proto__ is discovered, this assignment changes the prototype of configuredToolState rather than recording an enumerable entry. Consequently the later Object.keys check can omit the marker, and the definition mutated by overrideDeferLoadingForDiscoveredTools hashes differently before and after discovery, defeating cross-conversation key reuse. Fresh evidence after the own-property lookup fix is that the write still targets a normal {}; use a null-prototype record or another own-property-safe representation.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed; carried on the pushed head 99bfb24106. Correct and fixed with the canonicalize half: a deferred tool named __proto__ set a prototype instead of recording its configured state, so the definition hashed differently before and after discovery. The record is null-prototype, and the lookup beside it is own-property.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d3b01f0a42

ℹ️ 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".

Comment on lines +628 to +631
function stripPromptCacheControls(options: Record<string, unknown>): void {
for (const field of PROMPT_CACHE_ADMIN_FIELDS) {
delete options[field];
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Strip raw cache controls from modelKwargs

When a Responses agent author places wire-level fields such as prompt_cache_key, prompt_cache_retention, or prompt_cache_options under the free-form model_parameters.modelKwargs, this sanitizer only removes the camel-case controls from the outer options object and fallback clients. Those nested kwargs are forwarded as request parameters, allowing an author to pin a shared cache key, request billed retention, or bypass the explicit-cache capability gate despite these being endpoint-admin controls; sanitize the raw wire spellings inside every modelKwargs object as well.

AGENTS.md reference: AGENTS.md:L66-L68

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed; carried on the pushed head 99bfb24106. Correct: modelKwargs is forwarded to the request body verbatim, so an author who wrote prompt_cache_key, prompt_cache_retention or prompt_cache_options there reached the provider past every check that governs the camelCase fields. Both alphabets are declared in promptCache.ts now and stripped at the top level and inside each fallback's kwargs, with an llm.spec.ts case for the raw spellings.

Comment on lines +277 to +278
const projectedValue =
key === 'modelKwargs' ? modelKwargsIdentity(options[key]) : safeIdentity(options[key]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude top-level Responses verbosity from the cache identity

When a Responses agent supplies the native shape text: { verbosity: 'low', format: ... }, changing only text.verbosity changes the digest here even though verbosity affects output detail rather than the cached instruction/tool prefix. The equivalent modelKwargs.text path already removes this field in modelKwargsTextIdentity; apply the same projection to top-level text so moving the verbosity setting does not unnecessarily retire an otherwise reusable cache entry while still hashing text.format.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed; carried on the pushed head 99bfb24106. Correct: a Responses agent can supply the native text: { verbosity, format } shape, and only the modelKwargs.text twin had the projection. The top-level text gets it too — the format keys, the verbosity does not — pinned by promptCache.spec.ts in both directions.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4555ab21ed

ℹ️ 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".

Comment on lines +1039 to +1040
if (firstPartyEndpoint && promptCacheKeyEnabled === false) {
delete llmConfig.promptCacheKey;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Remove raw pinned keys when caching is disabled

When a first-party Responses endpoint has a legacy/admin addParams.prompt_cache_key and the operator sets promptCacheKey: false, addParams has already placed the raw field in the local modelKwargs, while this branch deletes only the camel-case property. The kwargs are attached to llmConfig later and Responses forwards the raw field, so the documented kill switch still sends the pinned cache key; remove prompt_cache_key from modelKwargs here as well.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed; carried on the pushed head 99bfb24106. Correct: addParams is applied after the administrator sanitizer, so an operator's raw prompt_cache_key sat in the request kwargs by the time the opt-out ran, and the documented “send no key at all” still sent one. The opt-out removes the raw spelling too — and only the key, because retention and the explicit controls are separate levers with their own policy, which a later review round caught in the first version of this fix.

The relocation sends the temporal prompt and its repository block at the front
of the dynamic tail, and the identity recorded them at the back. The digest is
a join of that recorded text, so an agent with `{{current_date}}` and a tail of
`X` and one whose prompt is `X` followed by `{{current_date}}` recorded the
same string while the model read two different prefixes: one key, two prompts.

`prependAgentInstructionTail` now writes both halves, the wire text and its
configured form, to the same end. One writer owns the order so the two cannot
drift again, and `recordStableInstructionText` goes with its last caller.
Clearing the agent's inherited `prompt_cache_key` compared against the yaml
`summarization.parameters` alone. A custom endpoint's own key arrives through
`clientOverrides`, merged into the same parameters a few lines above, and it
stays in the request kwargs rather than moving onto the constructor field,
because that promotion is first-party only. Summarizing from one custom
endpoint through another therefore read the target's own pinned key as the
agent's inherited one and overwrote it with undefined.

The comparison reads both layers that can carry a key, so only the source
agent's survives being cleared.
The cleanup that withholds `prompt_cache_options` and `prompt_cache_breakpoint`
from a summary model that rejects them also required the summarizer to share
the agent's provider. Only the inherited half of it needs that: an Anthropic or
Google agent selecting a built-in OpenAI summarizer skipped the check entirely,
so `promptCacheExplicit` set in `summarization.parameters` reached `gpt-4o` and
the request was rejected outright, failing the compaction.

The gate now asks only what the summary model accepts. The inherited half is
naturally inert when the providers differ, because `inheritedKwargs` is
undefined there.
A regular expression carries its meaning on `source` and `flags`, neither of
them enumerable, so the whole-definition walk read no own keys from one and
filed every pattern under the same empty identity. A runtime action whose
schema the walker has to fall back on could change `^A-\d+$` to `^B-\d+$` and
keep its cache key while the model was shown the new pattern.

Regular expressions are recorded as their source and flags.
Below twelve levels every subtree became the same `[depth]` marker, so two
runtime schemas that agreed that far and differed underneath were one
identity: a leaf changing from `customerId: string` to `orderId: number` kept
the cache key while the model was shown the new schema. Two reviewers reached
the same collision from different examples, which is what makes it the cause
rather than a case.

Cycles were already stopped by the `seen` set, so the depth limit was bounding
work a second time and erasing structure to do it. A node budget bounds the
same work without a fixed horizon, and a schema large enough to exhaust it
partitions rather than collides. `PROMPT_CACHE_KEY_VERSION` goes to 4 for the
changed payload shape.
Replacing the depth cutoff with a node budget removed a collision and left a
worse failure in its place: the walk is recursive, so a schema nested deeply
enough exhausts the call stack and throws `RangeError` while the key is being
built, failing the request rather than missing a cache.

Depth is bounded again, separately and for that reason alone, at a limit no
schema reaches and far short of the stack. The node budget keeps bounding
total work.
`toolChoice` and `parallelToolCalls` are excluded from the identity because
they pick among schemas the prefix already carries rather than changing them.
`function_call` is the legacy spelling of that same decision, `knownOpenAIParams`
still accepts it, and it was hashed: changing it retired the key and lost the
reuse for nothing.

It joins its successors in the exclusion set, and the spec now pins all three
selectors together so the next one is not a separate omission.
The comment beside this gate described precedence — the wire override first,
then the Azure deployment, then the visible model — while the code asked
whether the visible model *or* the deployment looked supported. A `gpt-5.6`
alias over a `gpt-4o-prod` deployment therefore passed, and the deployment
rejects those body parameters outright, failing every request that carried
them.

Each name decides alone when it is present. An opaque deployment name now
costs the explicit controls rather than risking the request, which is the
trade this gate already makes everywhere else.
@codegraph-librechat codegraph-librechat Bot added the 🗺️ Backend Platform codegraph: the taxonomy area this belongs to (classifier, confidence ≥ 0.9) label Oct 1, 2026
Dev now initializes a shallow copy of the agent when project context is on,
so the stored definition never gains project guidance. The cache identity
capture ran on the destructured input before that copy existed. It now runs
on the copy, before project guidance is appended, so the guidance is part of
the identity and the definition stays untouched.
…ts by Model

A model group that set prompt_cache_options or prompt_cache_breakpoint in
addParams left the raw value in modelKwargs. It reached the wire without the
breakpoints the SDK adds only for the constructor field, and an endpoint-wide
promptCacheExplicit replaced it. Both spellings are now promoted onto
promptCacheExplicit like the key and retention already are; presence opts in
unless its mode names something other than explicit.

An Azure deployment name decided explicit-cache support even when it was an
administrator label such as production-chat, so a deployment serving gpt-5.6
lost the controls it was configured with. The deployment now vetoes only when
it names a model family, which keeps a gpt-4o-prod deployment from receiving
parameters it rejects, and an opaque label falls back to the visible model.
Zod stores a default behind a thunk, which the runtime schema walk recorded
as bare presence, so two tools differing only in default('a') and
default('b') shared one cache key while the model was shown each default.
The walk now evaluates the defaultValue thunk and hashes its result, the
same value the JSON schema conversion emits.
@berry-13
berry-13 force-pushed the berry-13/enhancement-optimize-gpt-5.6-prompt-caching-for branch from e1942a5 to 0298eea Compare October 2, 2026 08:48
…e Specs

A new agent chat's URL now carries ?agent_id=, which the id pattern captured, so the message lookup asked for a conversation that does not exist.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0298eeae0b

ℹ️ 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".

Comment thread packages/api/src/endpoints/openai/promptCache.ts Outdated
Comment thread packages/api/src/agents/run.ts Outdated
Comment thread packages/api/src/agents/run.ts Outdated
Comment thread packages/api/src/endpoints/openai/initialize.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 160f6e9a43

ℹ️ 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".

Comment thread packages/api/src/endpoints/openai/promptCache.ts Outdated
Comment thread packages/api/src/endpoints/openai/llm.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 160f6e9a43

ℹ️ 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".

Comment thread packages/api/src/endpoints/openai/llm.ts
Comment on lines +501 to +503
if (typeof value === 'function') {
return '[fn]';
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Evaluate Zod lazy getters in runtime tool identities

When a runtime tool contains z.lazy(...) and reaches this fallback, Zod stores the provider-visible schema behind _def.getter, but this branch reduces every getter to the same "[fn]" marker. The provider's schema conversion invokes that getter, so lazy string and lazy object schemas, for example, are sent as different tool definitions while receiving the same synthesized cache key; evaluate this known schema thunk within the existing cycle/work guards or fingerprint the converted JSON schema.

AGENTS.md reference: AGENTS.md:L44-L47

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0544a13: the z.lazy getter is now evaluated under the same cycle and work guards as a default thunk, so a lazy string and a lazy object hash differently; promptCache.spec.ts covers it (jest-packages-api green).

The promptCacheExplicit endpoint option and its model capability gate move out of this change. Explicit breakpoints stay off by default until the agents SDK anchors them to the stable prefix, and deciding which models and Azure deployments accept them is a separate question from the cache key. Author-owned explicit controls are still stripped, dropParams still removes them, and the identity still ignores them.
canonicalize drops a Zod object's shape thunk, so an acyclic runtime schema canonicalized without throwing and without its fields: two tools differing only in their parameters shared one key. A tool whose schema is a Zod type now always goes through the schema walk, which the self-referential case already used.
z.lazy keeps its target behind _def.getter, which the walk recorded as bare presence, so a lazy string and a lazy object shared one key while the model was shown each. The getter is evaluated like a default's thunk, under the same cycle and work guards.
@berry-13

berry-13 commented Oct 3, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

Please review the current PR head 0544a13. State the exact reviewed commit and ignore findings that apply only to earlier heads.

Purpose supplied by the requester: 3 commits since reviewed head 160f6e9: lazy Zod schema hashing, Zod shape walk for runtime tools, explicit-cache gate removed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0544a13ff7

ℹ️ 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".

Comment thread packages/api/src/endpoints/openai/promptCache.ts
@berry-13

berry-13 commented Oct 4, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

Please review the current PR head 0544a13. State the exact reviewed commit and ignore findings that apply only to earlier heads.

Purpose supplied by the requester: Owner-approved same-head round on 0544a13: prior round's P2 (Zod v4 _def) disproved; precheck and all 8 scenarios now pass on this head

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0544a13ff7

ℹ️ 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".

Comment thread packages/api/src/endpoints/openai/promptCache.ts
Comment thread packages/api/src/agents/run.ts
Comment thread packages/api/src/agents/run.ts Outdated
Comment on lines +2152 to +2153
if (selfChildInputs != null) {
finalizePromptCacheKey(selfChildInputs);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Re-key ordinary self-spawn inputs after tool stripping

When self-spawn is enabled on an agent without background or injected-intent tools, selfChildInputs is undefined here, so the SDK later creates the child by copying the already-finalized parent inputs and retaining their promptCacheKey. Its child-input preparation then removes subagentConfigs and graphTools, meaning the child sends a prefix without the parent's delegation/direct tools under the key that was computed with them; always provide and seal a self-child input, or clear and recompute the inherited key after those fields are stripped.

AGENTS.md reference: AGENTS.md:L44-L47

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0818cc3. Reproduced on 0544a13: a plain self-spawn entry had no agentInputs, so the SDK spread the parent's finished inputs and sent the parent's key without the delegation tool and graphTools. Every self entry now carries its own sealed copy without graphTools; run-promptCache.test.ts 'keys a self-spawn child by the prefix it sends' fails on 0544a13 and passes here, and precheck plus all eight prompt-cache scenarios pass on 0818cc3.

A self-spawn entry without background or intent tools carried no inputs of its own, so the SDK spread the parent's finished inputs and sent the parent's cache key with a prefix that lacks the delegation tool and graphTools. Every self entry now gets its own copy, without graphTools, sealed under its own key.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0818cc3a0a

ℹ️ 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".

Comment thread packages/api/src/endpoints/openai/promptCache.ts

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🗺️ Backend Platform codegraph: the taxonomy area this belongs to (classifier, confidence ≥ 0.9) 🗺️ LLM Provider Config codegraph: the taxonomy area this belongs to (classifier, confidence ≥ 0.9)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants