[WRONG BRANCH] fix: stabilize cache inputs - #625
Conversation
…d routes too Claude Code's Messages API never sets previousResponseId, so the forward-only guard around repairOversizedReplayCallIds never fired on custom apiKey-auth providers. Long tool-chains hit a 400 'Invalid input[N].call_id: string too long' once replayed call_id values exceed the Responses API's 64-char limit. Broaden the guard to !unexpandedMiss: still covers forward mode and already-expanded replays, plus any request with no previousResponseId at all (every Claude-surfaced request), while still excluding the one risky case — a raw unexpanded native-Codex continuation that might reference call ids stored upstream.
…on-scoped OpenAI's Responses API caching is routed by prompt_cache_key, not pure byte-prefix matching. The session-scoped key (hash of Claude Code's metadata.user_id) meant every NEW Claude Code session got a fresh key, so a cache warmed by a prior session was unreachable even for byte-identical system+tools content — a guaranteed cold rewrite on the first call of every session, compounding heavily with subagent-heavy workloads that spawn many short-lived sessions. Flip the priority: try the content-based fallback key first (hash of resolved model + translated system + translated tools in wire order), falling back to the session-scoped key only when there's no system content to fingerprint. Any session can now hit a cache any other session already warmed. Session-scoped keying was originally chosen to avoid herding many different sessions/models/toolsets onto one content-hash key and burning OpenAI's ~15 RPM per-key routing budget at multi-tenant scale (audit R1#4/R2#5/R1#10). That collision risk is much smaller for a single-operator deployment, where the dominant cost is inter-session cache misses instead.
…s disabled restart was: stop, then handleEnsure(). handleEnsure() early-returns without relaunching whenever codexAutoStart is false — that flag only governs whether opencodex registers itself with the Codex CLI on boot, an unrelated concern, but it silently swallowed restart too: the proxy stopped cleanly and simply never came back, with no error. Swap to handleTrayProxyStart(), already used by the tray's own restart action and explicitly documented as relaunching unconditionally, independent of codexAutoStart.
…he_key Content-based prompt_cache_key (session-boundary fix) makes same-persona subagents agree on one cache cohort, but two dispatched in the same message can both miss if neither's cache write has landed before the other's request goes out. Opt-in leader/follower lease: first request for an unseen key proceeds immediately: any request sharing that key while the leader is in flight awaits the leader's release (fired on first output token, with a finally-block fallback) before dispatching. Each follower still gets its own real upstream response, only dispatch timing is delayed, not response sharing (unlike single-flight/ request-coalescing). Gated behind serializeColdSubagentCache, default false.
Empirically re-confirmed on the real phantomOpenai route (specter-debugger persona, 30 sequential + 4 concurrent probes): steady-state hit rate is effectively 100% and survives a 100s idle gap, but 1/4 truly concurrent requests on a fresh key still missed - validates this fix's premise.
…nreachable isBareOpenAiFamilyModel claims any bare gpt-/o1-/o3-/o4- prefixed model id for the native openai (Codex) provider before any other provider's defaultModel/models[] match is ever consulted. A provider that reuses one of those bare names (e.g. our phantomOpenai config reusing the upstream DEFAULT_SUBAGENT_MODELS names) silently loses to native Codex with no error - just the wrong provider and no prompt-cache benefit. Model is still reachable via the qualified <provider>/<model> form; this only surfaces the silent case at startup so it's not discovered by losing cache hits. Diagnostic only, never changes routing.
…llisions Same bug class, different table: a bare id whose prefix matches the hardcoded claude-/llama-/mixtral-/gemma- pattern table routes to whichever active provider is literally named after that pattern first, before any provider's own models[] array is ever consulted in routeModelInternal. A provider that explicitly lists such an id in models[] loses to the pattern-table provider silently. Fixed a bug in the extension itself: the pattern-table check was nested inside the openai-provider early-return guard, so it never ran when no native openai provider was configured. Caught by adding the test before assuming it worked.
Repointing restart at handleTrayProxyStart() (this session's earlier codexAutoStart-no-op fix) traded one silent bug for another: that action never synced the Grok Build config fence or Codex model list at all, unlike handleEnsure()'s two branches which both did. Caught by tests/grok-lifecycle.test.ts asserting the old handleEnsure() wiring — running the full suite (not just manual ocx restart smoke tests) is what surfaced it. runTrayProxyStart() gains an optional onStarted(port) hook, fired once a live proxy is confirmed whether it was already running or this call just started it (matching handleEnsure's own live+fresh parity). handleTrayProxyStart wires it to the same syncModelsToCodex + syncGrokConfig pair handleEnsure performs, fixing both the CLI restart and the tray's own restart button (both route through this action).
…ache-key priority The metadata.user_id-wins test encoded the old priority order that this session's cache-key-priority fix intentionally inverted: system/content now wins over metadata.user_id when both are present, with metadata as the fallback for requests with no system content to fingerprint. Split the old test into two cases matching the new contract and updated a stale comment in an already-passing test nearby.
…cing claude-opus-5 is absent from the jawcode bundle entirely, so pricing relies on explicit per-provider overlay rows (WP7). Only anthropic/ cursor/kiro had one, so any custom anthropic-adapter gateway (private proxies, company gateways) showed an em dash for claude-opus-5 in Logs. Naming each such gateway individually in this table would leak private provider names into the repo. Instead, findExpectedPriceOverlay now falls back to a "*" (any-provider) row for the same modelId when no exact-provider row exists, and one wildcard row covers claude-opus-5 for every unenumerated gateway. Exact-provider rows still take priority over the wildcard at the same status tier.
usageFromResponsesPayload only read input_tokens/output_tokens/total_tokens, silently dropping input_tokens_details.cached_tokens and output_tokens_details.reasoning_tokens. Cache-hit visibility for openai-responses traffic was always reported as zero regardless of actual upstream caching. Mirrors the existing openai-chat.ts extraction.
warn-unreachable-bare-model-aliases.test.ts used a fixture provider key that echoed an internal codename. Renamed to a neutral placeholder; behavior/assertions unchanged.
|
This pull request currently targets Its title has been prefixed with @jgautheron Please retarget this PR to This pull request is being kept as a draft automatically. Once the target branch is corrected, it will be marked ready for review again. |
📝 WalkthroughWalkthroughThe PR canonicalizes Anthropic tools and system reminders, changes prompt-cache and session-ID derivation, adds optional cold-cache serialization, updates tray-proxy restart hooks, introduces routing collision warnings, extends Responses usage parsing, and adds wildcard pricing fallback. ChangesPrompt cache stabilization and serialization
Tray-proxy lifecycle
Routing collision diagnostics
Usage and replay handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ClaudeMessages
participant ColdCacheLease
participant HandleResponses
ClaudeMessages->>ColdCacheLease: acquire prompt_cache_key lease
ColdCacheLease-->>ClaudeMessages: leader or follower wait promise
ClaudeMessages->>HandleResponses: dispatch request
HandleResponses-->>ClaudeMessages: first output or failure
ClaudeMessages->>ColdCacheLease: release leader lease
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/guides/claude-code.md`:
- Around line 386-389: Update the translated caching sections in the Japanese,
Korean, Russian, and Simplified Chinese Claude Code guides to match the current
behavior described in the native caching section: derive content-scoped
prompt_cache_key values from resolved model, normalized system content, and full
tool schemas, and derive session_id separately only when metadata.user_id is
present for backend affinity. Replace the outdated session-scoped
prompt_cache_key/session_id description while preserving the localized
documentation structure.
In `@src/adapters/anthropic-sort-stabilize.ts`:
- Around line 66-78: Invoke stabilizeSystemAndToolOrder on the native Anthropic
request body inside anthropicNativePassthrough before the body is serialized or
forwarded, ensuring system reminders and tools are normalized for cache
stability. Keep the existing passthrough behavior otherwise unchanged.
In `@src/adapters/openai-responses.ts`:
- Around line 663-670: Add a focused regression test in
openai-responses-passthrough.test.ts that exercises usageFromResponsesPayload
through the adapter and verifies the emitted done.usage includes
cachedInputTokens from input_tokens_details.cached_tokens and
reasoningOutputTokens from output_tokens_details.reasoning_tokens. Use a payload
containing both nested fields and assert both mapped values to guard against
parser field-name or event-shape regressions.
In `@src/cli/tray-proxy.ts`:
- Around line 17-24: Update the onStarted callback contract in
src/cli/tray-proxy.ts:17-24 to carry the live proxy hostname or TrayProxyLive
object, pass that value from both the already-running path at
src/cli/tray-proxy.ts:29-32 and fresh-start path at src/cli/tray-proxy.ts:46-52,
then use the propagated hostname instead of config.hostname when invoking
syncGrokConfig in src/cli/index.ts:409-421.
In `@src/router.ts`:
- Around line 349-359: The collision check in warnUnreachablePatternTableModels
currently excludes every active provider’s defaultModel; instead, exclude only
the inspected provider’s own defaultModel while evaluating each prov.models
entry. In tests/warn-unreachable-bare-model-aliases.test.ts lines 97-102, update
the existing expectation to require a warning for cursor and add a separate
regression test confirming the inspected provider’s own default model is
excluded.
In `@src/server/claude-messages.ts`:
- Around line 676-689: Scope the cold-key lease acquisition in the current
request flow to nativeRoute in addition to serializeColdSubagentCache and a
string prompt_cache_key. Update the condition around acquireColdCacheKeyLease so
followers wait only for native routes, while preserving the existing leader
release and wait behavior.
- Around line 701-717: Update the cold-key lease handling around onFirstOutput
and the handleResponses try/finally flow: track whether onFirstOutput has
executed, set that guard before releasing the lease in the callback, and only
perform the finally fallback release when no first output occurred. Preserve
release behavior for error or abort paths while keeping the lease until the
first output callback runs.
In `@src/usage/expected-prices.ts`:
- Around line 148-154: Update the overlay lookup so any existing exact
provider/model entry takes precedence: after computing exact and its
verified/verified-derived match, return the exact result even when it is
undefined, preventing wildcard fallback when exact rows are only unverified.
Only evaluate wildcard overlays when exact.length is zero, and add a regression
test for an unverified exact row alongside a verified wildcard row.
In `@tests/grok-lifecycle.test.ts`:
- Around line 90-95: Replace the CLI source-text assertions around the restart
case with focused Bun behavioral tests for runTrayProxyStart. Use fake
TrayProxyStartIo implementations to cover already-live and fresh-start paths,
asserting that onStarted is invoked in the correct path with the expected
arguments and metadata, and that restart produces the expected result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 884bfb97-e49e-4bdb-8c22-4738be32e93f
📒 Files selected for processing (18)
docs-site/src/content/docs/guides/claude-code.mdsrc/adapters/anthropic-sort-stabilize.tssrc/adapters/openai-responses.tssrc/claude/inbound.tssrc/cli/index.tssrc/cli/tray-proxy.tssrc/lib/cold-cache-key-lease.tssrc/router.tssrc/server/claude-messages.tssrc/server/index.tssrc/types.tssrc/usage/expected-prices.tstests/claude-inbound.test.tstests/claude-messages-endpoint.test.tstests/cold-cache-key-lease.test.tstests/grok-lifecycle.test.tstests/usage-cost.test.tstests/warn-unreachable-bare-model-aliases.test.ts
| **Native OpenAI/ChatGPT routing:** derives a content-scoped `prompt_cache_key` from the | ||
| resolved model, normalized system content, and full tool schemas. When `metadata.user_id` is | ||
| present, it separately derives a per-session `session_id` header for backend affinity. Tool and | ||
| system-reminder listings are canonicalized before either cache input is built. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -i claude-code.md docs-site/src/content/docs | grep -E '/(ja|ko|ru|zh-cn)/'Repository: lidge-jun/opencodex
Length of output: 368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
docs-site/src/content/docs/ja/guides/claude-code.md \
docs-site/src/content/docs/ko/guides/claude-code.md \
docs-site/src/content/docs/ru/guides/claude-code.md \
docs-site/src/content/docs/zh-cn/guides/claude-code.md \
docs-site/src/content/docs/guides/claude-code.md
do
echo "===== $f ====="
# show the caching section area if present
rg -n -C 3 'prompt_cache_key|session_id|session-scoped|content-scoped|cache' "$f" || true
echo
doneRepository: lidge-jun/opencodex
Length of output: 8058
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
files = [
Path('docs-site/src/content/docs/ja/guides/claude-code.md'),
Path('docs-site/src/content/docs/ko/guides/claude-code.md'),
Path('docs-site/src/content/docs/ru/guides/claude-code.md'),
Path('docs-site/src/content/docs/zh-cn/guides/claude-code.md'),
]
for f in files:
txt = f.read_text(encoding='utf-8')
print(f"\n===== {f} =====")
for needle in ['session-scoped', 'content-scoped', 'prompt_cache_key', 'session_id', 'normalized system content', 'tool schemas']:
if needle in txt:
print(f"contains: {needle}")
# print the surrounding paragraph for prompt_cache_key if present
idx = txt.find('prompt_cache_key')
if idx != -1:
start = max(0, txt.rfind('\n\n', 0, idx))
end = txt.find('\n\n', idx)
if end == -1:
end = len(txt)
print(txt[start:end].strip())
PYRepository: lidge-jun/opencodex
Length of output: 1353
Update the translated caching section in the locale docs. docs-site/src/content/docs/ja/guides/claude-code.md:300-302, docs-site/src/content/docs/ko/guides/claude-code.md:337-339, docs-site/src/content/docs/ru/guides/claude-code.md:317-319, and docs-site/src/content/docs/zh-cn/guides/claude-code.md:295-297 still describe the old session-scoped prompt_cache_key / session_id model. Please align them with docs-site/src/content/docs/guides/claude-code.md:386-389, which now uses a content-scoped prompt_cache_key and only derives session_id separately for backend affinity when metadata.user_id is present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-site/src/content/docs/guides/claude-code.md` around lines 386 - 389,
Update the translated caching sections in the Japanese, Korean, Russian, and
Simplified Chinese Claude Code guides to match the current behavior described in
the native caching section: derive content-scoped prompt_cache_key values from
resolved model, normalized system content, and full tool schemas, and derive
session_id separately only when metadata.user_id is present for backend
affinity. Replace the outdated session-scoped prompt_cache_key/session_id
description while preserving the localized documentation structure.
Source: Path instructions
| export function stabilizeSystemAndToolOrder(body: Rec): void { | ||
| if (Array.isArray(body.system)) { | ||
| const system = body.system as unknown[]; | ||
| for (let i = 0; i < system.length; i++) { | ||
| const block = system[i]; | ||
| if (!isRec(block) || block.type !== "text" || typeof block.text !== "string") continue; | ||
| const normalized = normalizeSystemReminderText(block.text); | ||
| if (normalized !== block.text) system[i] = { ...block, text: normalized }; | ||
| } | ||
| } | ||
|
|
||
| if (Array.isArray(body.tools)) sortToolsByName(body.tools as unknown[]); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "stabilizeSystemAndToolOrder" -C3Repository: lidge-jun/opencodex
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Files mentioning anthropic-sort-stabilize or related helpers\n'
rg -n "stabilizeSystemAndToolOrder|normalizeSystemReminderText|sortToolsByName|anthropicNativePassthrough|wantsNativePassthrough" src -C 2 || true
printf '\n## Candidate file outline\n'
ast-grep outline src/adapters/anthropic-sort-stabilize.ts || true
printf '\n## Relevant slices\n'
sed -n '1,220p' src/adapters/anthropic-sort-stabilize.tsRepository: lidge-jun/opencodex
Length of output: 11231
Call stabilizeSystemAndToolOrder from the native passthrough path. src/adapters/anthropic-sort-stabilize.ts:66-78 exports the cache-stabilizing helper, but there is no call site anywhere in the repo, and src/server/claude-messages.ts:291, 573-574, 856-857 forwards the raw body into anthropicNativePassthrough(...). That means the system/tool ordering normalization never runs for native Anthropic requests, so byte-identical conversations can still miss the prompt cache. Invoke it before serialization in anthropicNativePassthrough, or drop the export/docstring if the passthrough is meant to stay unchanged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/adapters/anthropic-sort-stabilize.ts` around lines 66 - 78, Invoke
stabilizeSystemAndToolOrder on the native Anthropic request body inside
anthropicNativePassthrough before the body is serialized or forwarded, ensuring
system reminders and tools are normalized for cache stability. Keep the existing
passthrough behavior otherwise unchanged.
| const inputDetails = isPlainObject(usage.input_tokens_details) ? usage.input_tokens_details : undefined; | ||
| const outputDetails = isPlainObject(usage.output_tokens_details) ? usage.output_tokens_details : undefined; | ||
| return { | ||
| inputTokens, | ||
| outputTokens, | ||
| ...(typeof usage.total_tokens === "number" ? { totalTokens: usage.total_tokens } : {}), | ||
| ...(typeof inputDetails?.cached_tokens === "number" ? { cachedInputTokens: inputDetails.cached_tokens } : {}), | ||
| ...(typeof outputDetails?.reasoning_tokens === "number" ? { reasoningOutputTokens: outputDetails.reasoning_tokens } : {}), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a parser-level regression test for the new token details.
usageFromResponsesPayload now maps input_tokens_details.cached_tokens and output_tokens_details.reasoning_tokens, but the supplied test changes do not exercise this adapter parser. A field-name or event-shape regression could silently drop cached and reasoning usage before aggregation. Add a focused test in tests/openai-responses-passthrough.test.ts asserting both fields on the emitted done.usage.
As per path instructions, adapter behavior changes under src/** should have a focused regression test under tests/.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/adapters/openai-responses.ts` around lines 663 - 670, Add a focused
regression test in openai-responses-passthrough.test.ts that exercises
usageFromResponsesPayload through the adapter and verifies the emitted
done.usage includes cachedInputTokens from input_tokens_details.cached_tokens
and reasoningOutputTokens from output_tokens_details.reasoning_tokens. Use a
payload containing both nested fields and assert both mapped values to guard
against parser field-name or event-shape regressions.
Source: Path instructions
| /** | ||
| * Called once a live proxy is confirmed on `port` — whether it was already running or | ||
| * this call just started it. `handleEnsure` performs the Grok-fence and Codex-model | ||
| * sync inline in both of its branches; this action skipped both entirely, so `ocx restart` | ||
| * silently lost them once restart was repointed at this action. Optional so tests that | ||
| * don't care about the sync can omit it. | ||
| */ | ||
| onStarted?: (port: number) => void | Promise<void>; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate the actual proxy hostname through startup callbacks.
The callback contract discards TrayProxyLive.hostname, causing Grok synchronization to use potentially stale config.hostname.
src/cli/tray-proxy.ts#L17-L24: extendonStartedto receive the hostname or fullTrayProxyLive.src/cli/tray-proxy.ts#L29-L32: pass the live proxy’s hostname on the already-running path.src/cli/tray-proxy.ts#L46-L52: pass the started proxy’s hostname after a fresh start.src/cli/index.ts#L409-L421: use the propagated hostname when callingsyncGrokConfig.
📍 Affects 2 files
src/cli/tray-proxy.ts#L17-L24(this comment)src/cli/tray-proxy.ts#L29-L32src/cli/tray-proxy.ts#L46-L52src/cli/index.ts#L409-L421
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/tray-proxy.ts` around lines 17 - 24, Update the onStarted callback
contract in src/cli/tray-proxy.ts:17-24 to carry the live proxy hostname or
TrayProxyLive object, pass that value from both the already-running path at
src/cli/tray-proxy.ts:29-32 and fresh-start path at src/cli/tray-proxy.ts:46-52,
then use the propagated hostname instead of config.hostname when invoking
syncGrokConfig in src/cli/index.ts:409-421.
| function warnUnreachablePatternTableModels(config: OcxConfig): void { | ||
| const active = activeProviderEntries(config); | ||
| const defaultModels = new Set(active.map(([, prov]) => prov.defaultModel).filter((m): m is string => typeof m === "string")); | ||
| for (const [name, prov] of active) { | ||
| if (!Array.isArray(prov.models)) continue; | ||
| for (const modelId of prov.models) { | ||
| if (typeof modelId !== "string" || modelId.includes("/") || defaultModels.has(modelId)) continue; | ||
| for (const { providerNames, prefixes } of MODEL_PROVIDER_PATTERNS) { | ||
| if (!prefixes.some(prefix => modelId.startsWith(prefix))) continue; | ||
| const patternProvider = active.find(([n]) => providerNames.some(pn => n === pn || n.startsWith(`${pn}-`))); | ||
| if (patternProvider && patternProvider[0] !== name) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep provider-specific collision detection consistent.
A bare model being reachable through another provider does not make it reachable through the provider whose models[] lists it. Default-model precedence therefore requires both the router diagnostic and its regression test to treat this as a collision.
src/router.ts#L349-L359: exclude only the inspected provider’s owndefaultModel, not every active provider’s default model.tests/warn-unreachable-bare-model-aliases.test.ts#L97-L102: expect a warning forcursorand add a separate test covering the provider’s own default model.
📍 Affects 2 files
src/router.ts#L349-L359(this comment)tests/warn-unreachable-bare-model-aliases.test.ts#L97-L102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/router.ts` around lines 349 - 359, The collision check in
warnUnreachablePatternTableModels currently excludes every active provider’s
defaultModel; instead, exclude only the inspected provider’s own defaultModel
while evaluating each prov.models entry. In
tests/warn-unreachable-bare-model-aliases.test.ts lines 97-102, update the
existing expectation to require a warning for cursor and add a separate
regression test confirming the inspected provider’s own default model is
excluded.
| // Cold-key lease (opt-in): a parallel Task-tool fan-out that dispatches several | ||
| // requests sharing the SAME content-based | ||
| // prompt_cache_key races all of them before any has written its cache entry, so | ||
| // every one misses even though they'd agree on the same key. The leader (first | ||
| // arrival for this key) proceeds immediately; followers wait for the leader's | ||
| // first output token before dispatching, so they land on a warm cache instead of | ||
| // racing a cold one. See src/lib/cold-cache-key-lease.ts. | ||
| let coldKeyRelease: (() => void) | undefined; | ||
| if (config.claudeCode?.serializeColdSubagentCache && typeof internalBody.prompt_cache_key === "string") { | ||
| const { acquireColdCacheKeyLease } = await import("../lib/cold-cache-key-lease"); | ||
| const lease = acquireColdCacheKeyLease(internalBody.prompt_cache_key); | ||
| if (lease.waitForLeader) await lease.waitForLeader; | ||
| else coldKeyRelease = lease.release; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Cold-key lease should be scoped to nativeRoute, not applied to every provider.
The lease acquisition at Lines 684-689 only checks config.claudeCode?.serializeColdSubagentCache and internalBody.prompt_cache_key. prompt_cache_key is set on internalBody whenever systemParts.length > 0 (essentially always), regardless of which adapter the request eventually routes to — it's never stripped for non-native routes (unlike max_output_tokens/temperature/top_p/stop/user, which are deleted only for route.provider.adapter === "openai-responses" at Lines 600-607). Since the whole point of this feature (per src/lib/cold-cache-key-lease.ts's docstring) is coordinating OpenAI's prompt_cache_key cold-cache race, gating on serializeColdSubagentCache alone means a user who opts in gets follower-wait latency injected on every routed (non-OpenAI) Task-tool fan-out too, with no cache benefit to show for it. nativeRoute is already computed and in scope by this point (Lines 594-630).
🔧 Proposed fix
let coldKeyRelease: (() => void) | undefined;
- if (config.claudeCode?.serializeColdSubagentCache && typeof internalBody.prompt_cache_key === "string") {
+ if (nativeRoute && config.claudeCode?.serializeColdSubagentCache && typeof internalBody.prompt_cache_key === "string") {
const { acquireColdCacheKeyLease } = await import("../lib/cold-cache-key-lease");
const lease = acquireColdCacheKeyLease(internalBody.prompt_cache_key);
if (lease.waitForLeader) await lease.waitForLeader;
else coldKeyRelease = lease.release;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Cold-key lease (opt-in): a parallel Task-tool fan-out that dispatches several | |
| // requests sharing the SAME content-based | |
| // prompt_cache_key races all of them before any has written its cache entry, so | |
| // every one misses even though they'd agree on the same key. The leader (first | |
| // arrival for this key) proceeds immediately; followers wait for the leader's | |
| // first output token before dispatching, so they land on a warm cache instead of | |
| // racing a cold one. See src/lib/cold-cache-key-lease.ts. | |
| let coldKeyRelease: (() => void) | undefined; | |
| if (config.claudeCode?.serializeColdSubagentCache && typeof internalBody.prompt_cache_key === "string") { | |
| const { acquireColdCacheKeyLease } = await import("../lib/cold-cache-key-lease"); | |
| const lease = acquireColdCacheKeyLease(internalBody.prompt_cache_key); | |
| if (lease.waitForLeader) await lease.waitForLeader; | |
| else coldKeyRelease = lease.release; | |
| } | |
| // Cold-key lease (opt-in): a parallel Task-tool fan-out that dispatches several | |
| // requests sharing the SAME content-based | |
| // prompt_cache_key races all of them before any has written its cache entry, so | |
| // every one misses even though they'd agree on the same key. The leader (first | |
| // arrival for this key) proceeds immediately; followers wait for the leader's | |
| // first output token before dispatching, so they land on a warm cache instead of | |
| // racing a cold one. See src/lib/cold-cache-key-lease.ts. | |
| let coldKeyRelease: (() => void) | undefined; | |
| if (nativeRoute && config.claudeCode?.serializeColdSubagentCache && typeof internalBody.prompt_cache_key === "string") { | |
| const { acquireColdCacheKeyLease } = await import("../lib/cold-cache-key-lease"); | |
| const lease = acquireColdCacheKeyLease(internalBody.prompt_cache_key); | |
| if (lease.waitForLeader) await lease.waitForLeader; | |
| else coldKeyRelease = lease.release; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/claude-messages.ts` around lines 676 - 689, Scope the cold-key
lease acquisition in the current request flow to nativeRoute in addition to
serializeColdSubagentCache and a string prompt_cache_key. Update the condition
around acquireColdCacheKeyLease so followers wait only for native routes, while
preserving the existing leader release and wait behavior.
| const onFirstOutput = () => { | ||
| if (logIds) recordFirstOutput(logCtx, logIds.start); | ||
| coldKeyRelease?.(); | ||
| }; | ||
| let upstream: Response; | ||
| try { | ||
| upstream = await handleResponses(internalReq, buildClaudeReplayConfig(config), logCtx, { | ||
| abortSignal: req.signal, | ||
| onFirstOutput, | ||
| onNativePassthroughTerminal: status => finalizeNativeLog(httpStatusForTerminalStatus(status), { terminalStatus: status, closeReason: "terminal" }), | ||
| onNativePassthroughCancel: () => finalizeNativeLog(499, { closeReason: "client_cancel" }), | ||
| }); | ||
| } finally { | ||
| // Fallback release: the leader errored/aborted before any output token, so | ||
| // onFirstOutput never fired — followers must not wait forever. | ||
| coldKeyRelease?.(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "onFirstOutput" -C6 src/server/responses/core.tsRepository: lidge-jun/opencodex
Length of output: 5587
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== claude-messages around lines 680-725 =="
sed -n '680,725p' src/server/claude-messages.ts
echo
echo "== core.ts around onFirstOutput plumbing and response creation =="
sed -n '1380,1515p' src/server/responses/core.ts
echo
echo "== core.ts around consumeForResponseLogMetadata =="
sed -n '1515,1605p' src/server/responses/core.tsRepository: lidge-jun/opencodex
Length of output: 12775
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate helper definitions =="
rg -n "function relaySseEagerBounded|const relaySseEagerBounded|function consumeForResponseLogMetadata|const consumeForResponseLogMetadata|function createSseInspector|const createSseInspector" src/server/responses/core.ts
echo
echo "== inspect the helper implementation boundaries =="
ast-grep outline src/server/responses/core.ts --view expandedRepository: lidge-jun/opencodex
Length of output: 189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== handleResponses branch around eager relay =="
sed -n '1400,1495p' src/server/responses/core.ts
echo
echo "== helper definitions for streaming inspection =="
sed -n '220,360p' src/server/responses/core.ts
echo
echo "== search for any awaited consumption in handleResponses =="
rg -n "await .*relaySseEagerBounded|await .*consumeForResponseLogMetadata|return new Response|new Response\\(" src/server/responses/core.tsRepository: lidge-jun/opencodex
Length of output: 10776
Keep the cold-key lease until the first output callback runs
src/server/claude-messages.ts:701-717 releases coldKeyRelease in finally, but src/server/responses/core.ts:1412-1500 returns a Response immediately after wiring onFirstOutput into the streaming inspector/relay. That means the finally can fire before any body chunk is emitted, so followers stop waiting before the cache is warm.
Keep the fallback release only for the no-output error/abort path, e.g. with a didFirstOutput guard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/claude-messages.ts` around lines 701 - 717, Update the cold-key
lease handling around onFirstOutput and the handleResponses try/finally flow:
track whether onFirstOutput has executed, set that guard before releasing the
lease in the callback, and only perform the finally fallback release when no
first output occurred. Preserve release behavior for error or abort paths while
keeping the lease until the first output callback runs.
| const exact = overlays.filter(row => row.provider === provider && row.modelId === modelId); | ||
| return exact.find(row => row.status === "verified") | ||
| const found = exact.find(row => row.status === "verified") | ||
| ?? exact.find(row => row.status === "verified-derived"); | ||
| if (found) return found; | ||
| const wildcard = overlays.filter(row => row.provider === "*" && row.modelId === modelId); | ||
| return wildcard.find(row => row.status === "verified") | ||
| ?? wildcard.find(row => row.status === "verified-derived"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not let a wildcard bypass an exact unverified overlay.
When an exact {provider, modelId} row exists but is only unverified, found is undefined and the function falls through to the wildcard row. That can silently apply a verified-derived price despite an explicit provider-specific fail-closed entry, contradicting the documented “when no exact provider match exists” behavior.
Return found whenever exact.length > 0; add a regression test covering an unverified exact row plus a verified wildcard row.
Proposed fix
const found = exact.find(row => row.status === "verified")
?? exact.find(row => row.status === "verified-derived");
- if (found) return found;
+ if (exact.length > 0) return found;
const wildcard = overlays.filter(row => row.provider === "*" && row.modelId === modelId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const exact = overlays.filter(row => row.provider === provider && row.modelId === modelId); | |
| return exact.find(row => row.status === "verified") | |
| const found = exact.find(row => row.status === "verified") | |
| ?? exact.find(row => row.status === "verified-derived"); | |
| if (found) return found; | |
| const wildcard = overlays.filter(row => row.provider === "*" && row.modelId === modelId); | |
| return wildcard.find(row => row.status === "verified") | |
| ?? wildcard.find(row => row.status === "verified-derived"); | |
| const exact = overlays.filter(row => row.provider === provider && row.modelId === modelId); | |
| const found = exact.find(row => row.status === "verified") | |
| ?? exact.find(row => row.status === "verified-derived"); | |
| if (exact.length > 0) return found; | |
| const wildcard = overlays.filter(row => row.provider === "*" && row.modelId === modelId); | |
| return wildcard.find(row => row.status === "verified") | |
| ?? wildcard.find(row => row.status === "verified-derived"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/usage/expected-prices.ts` around lines 148 - 154, Update the overlay
lookup so any existing exact provider/model entry takes precedence: after
computing exact and its verified/verified-derived match, return the exact result
even when it is undefined, preventing wildcard fallback when exact rows are only
unverified. Only evaluate wildcard overlays when exact.length is zero, and add a
regression test for an unverified exact row alongside a verified wildcard row.
| // handleEnsure() early-returns without relaunching when codexAutoStart is disabled, | ||
| // which silently ate `restart` too — handleTrayProxyStart() relaunches | ||
| // unconditionally instead. | ||
| const restartCase = sliceFn(CLI_SOURCE, 'case "restart"', 'case "health"'); | ||
| expect(restartCase).toContain("if (await handleStop()) await handleEnsure()"); | ||
| expect(restartCase).toContain("if (await handleStop()) await handleTrayProxyStart()"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Exercise the startup behavior instead of inspecting source text.
These assertions can pass while runTrayProxyStart never invokes onStarted, invokes it in the wrong branch, or passes incorrect metadata. Add focused Bun tests with fake TrayProxyStartIo implementations covering both the already-live and fresh-start paths, asserting the callback arguments and restart result.
As per path instructions, tests/** behavior changes should have focused behavioral regression coverage near the subsystem.
Also applies to: 97-111
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/grok-lifecycle.test.ts` around lines 90 - 95, Replace the CLI
source-text assertions around the restart case with focused Bun behavioral tests
for runTrayProxyStart. Use fake TrayProxyStartIo implementations to cover
already-live and fresh-start paths, asserting that onStarted is invoked in the
correct path with the expected arguments and metadata, and that restart produces
the expected result.
Source: Path instructions
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e89c411dc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } finally { | ||
| // Fallback release: the leader errored/aborted before any output token, so | ||
| // onFirstOutput never fired — followers must not wait forever. | ||
| coldKeyRelease?.(); |
There was a problem hiding this comment.
When claudeCode.serializeColdSubagentCache is enabled, this finally releases the leader as soon as handleResponses returns. For Claude Messages, internalBody.stream is forced true and handleResponses constructs an SSE Response before the upstream stream has produced a first output event, so followers arriving in that window stop waiting and dispatch while the cache is still cold. Move the fallback release to an actual stream terminal/error/cancel path, and keep the normal release in onFirstOutput.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| /** Sort `body.system` skills/deferred-tools listings and `body.tools` by name, in place. */ | ||
| export function stabilizeSystemAndToolOrder(body: Rec): void { |
There was a problem hiding this comment.
This helper is exported for native Anthropic passthrough, but it has no callers in this commit, so anthropicNativePassthrough still serializes the original body unchanged. In native Anthropic mode, Claude Code tool/system-reminder discovery can still arrive in different orders and continue busting the prompt-cache prefix; call this before JSON.stringify(body) on the passthrough path (including count_tokens if counts must match sends).
Useful? React with 👍 / 👎.
| const wildcard = overlays.filter(row => row.provider === "*" && row.modelId === modelId); | ||
| return wildcard.find(row => row.status === "verified") | ||
| ?? wildcard.find(row => row.status === "verified-derived"); |
There was a problem hiding this comment.
With this fallback, any unpriced provider that happens to log native model id claude-opus-5 now receives the Anthropic Opus price, even if it is not one of the private Anthropic-compatible gateways described in the comment (for example an OpenRouter/local/company provider with different or zero billing). That turns the estimator from exact-provider/fail-closed into a cross-provider fuzzy price for this model; keep the wildcard scoped by provider/adapter metadata or enumerate only the intended gateways.
Useful? React with 👍 / 👎.
| }); | ||
| try { | ||
| const { syncGrokConfig } = await import("../grok/sync"); | ||
| const g = await syncGrokConfig(port, config, config.hostname ? { hostname: config.hostname } : {}); |
There was a problem hiding this comment.
When runTrayProxyStart finds an already-running proxy (or a newly started fallback runtime), findLiveProxy()/waitForProxy() can return the hostname that actually answered, but this callback only receives port and then falls back to config.hostname. If the running proxy was discovered from the runtime record or the config hostname has drifted, this rewrites ~/.grok/config.toml to the wrong host; pass the live hostname through the tray start callback and use it like handleEnsure does.
Useful? React with 👍 / 👎.
No description provided.