[WRONG BRANCH] fix: stabilize cache inputs - #626
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. |
📝 WalkthroughWalkthroughChangesClaude cache stability and cold-key coordination
Tray proxy lifecycle coordination
Model routing diagnostics
Usage pricing overlays
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ClaudeMessages
participant ClaudeInbound
participant ColdCacheKeyLease
participant OpenAIResponses
ClaudeMessages->>ClaudeInbound: Translate request and derive prompt_cache_key
ClaudeMessages->>ColdCacheKeyLease: Acquire lease for prompt_cache_key
ColdCacheKeyLease-->>ClaudeMessages: Return leader or waitForLeader
ClaudeMessages->>OpenAIResponses: Dispatch upstream request
OpenAIResponses-->>ClaudeMessages: Produce first output
ClaudeMessages->>ColdCacheKeyLease: Release leader lease
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Please Update your PR description. |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/claude-messages.ts (1)
676-717: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound the cold-cache lease wait and keep the release tied to first output
src/server/claude-messages.ts:687-716leaves followers waiting forever if the leader stalls before first output, becauselease.waitForLeaderis neither timed out nor raced withreq.signal. Thefinallyfallback is also too early for streamed turns:handleResponsesreturns aResponsewhile the SSE pumps insrc/server/relay.ts:588-626/src/bridge.ts:431-444continue in the background, so the leader lease can be released before the first token lands. Race the wait against abort/timeout, and keep the lease release in the first-output path (withfinallyonly as the error/abort fallback).🤖 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 - 717, Bound the follower wait in the cold-cache lease block around acquireColdCacheKeyLease by racing lease.waitForLeader against req.signal and a timeout, while preserving cancellation and allowing followers to proceed when the bound expires. Keep coldKeyRelease tied to onFirstOutput so streamed responses retain the lease until the first token; use the finally block only to release it when handleResponses errors or the request aborts before output.Source: Path instructions
🤖 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 Native OpenAI/ChatGPT routing documentation to
state that prompt_cache_key is content-scoped when system content exists, but
falls back to hashing metadata.user_id when no system content is present. Apply
the same clarification to the ja, ko, ru, and zh-cn translations so they remain
consistent with the English guide.
In `@src/adapters/anthropic-sort-stabilize.ts`:
- Around line 1-12: Update anthropicNativePassthrough() to call
stabilizeSystemAndToolOrder() on the request body before it is serialized with
JSON.stringify. Ensure sk-ant-* native passthrough requests send the normalized
system and tools ordering while preserving the existing request flow and payload
contents.
In `@src/adapters/openai-responses.ts`:
- Around line 748-757: Add focused regression coverage near the existing tests
for the adapter behavior around the replay-repair condition `!unexpandedMiss`.
Verify the truth table by asserting oversized `call_id` values are repaired for
API-key requests without `previousResponseId`, forward requests without
`previousResponseId`, and expanded replays, while repair is intentionally
skipped for an unexpanded continuation with `previousResponseId`; also confirm
whether that forward-with-`previousResponseId` exclusion is intentional.
- Around line 663-670: Update the usage object returned by the Responses parser
to populate both cachedInputTokens and cacheReadInputTokens from
inputDetails.cached_tokens, preserving the existing numeric guard and all other
token mappings.
In `@src/cli/tray-proxy.ts`:
- Around line 17-24: Extend the TrayProxyLive/onStarted contract to pass the
live proxy metadata, including hostname, rather than only the port. Update
handleEnsure’s already-running and newly-started branches to invoke the callback
with the same live object, and update handleTrayProxyStart() to use the
callback’s live hostname when synchronizing configuration instead of
config.hostname.
In `@src/router.ts`:
- Around line 350-359: Update the diagnostic around activeProviderEntries and
MODEL_PROVIDER_PATTERNS in src/router.ts lines 350-359 to remove global
defaultModels suppression and resolve each bare model through the same
provider-selection precedence as routeModelInternal and
routeByKnownModelPattern, consistently excluding disabled providers; warn
whenever the resolved provider differs from the declaring provider. Update
tests/warn-unreachable-bare-model-aliases.test.ts lines 97-103 to expect the
cursor/claude-4-sonnet warning and add coverage where a disabled pattern
provider precedes an active matching provider.
In `@src/usage/expected-prices.ts`:
- Around line 148-154: Update the lookup around the exact and wildcard overlay
selection so wildcard prices are considered only when exact.length === 0; an
exact unverified entry must return no price rather than falling back to a
trusted wildcard. Add a regression test covering an unverified exact row
alongside a trusted wildcard row.
In `@tests/grok-lifecycle.test.ts`:
- Around line 90-111: Replace the source-text assertions in the lifecycle tests
with focused Bun runtime tests around handleTrayProxyStart, using fake I/O to
exercise both already-live and freshly-started proxy branches. Assert that
onStarted receives the expected metadata and is awaited to completion in each
branch, while preserving the existing model-sync coverage near the lifecycle
tests.
---
Outside diff comments:
In `@src/server/claude-messages.ts`:
- Around line 676-717: Bound the follower wait in the cold-cache lease block
around acquireColdCacheKeyLease by racing lease.waitForLeader against req.signal
and a timeout, while preserving cancellation and allowing followers to proceed
when the bound expires. Keep coldKeyRelease tied to onFirstOutput so streamed
responses retain the lease until the first token; use the finally block only to
release it when handleResponses errors or the request aborts before output.
🪄 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: 66cf2202-c9c3-4fcb-b8ae-047880d889ac
📒 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 | 🔵 Trivial | ⚡ Quick win
Doc omits the metadata.user_id fallback for prompt_cache_key.
This section states the key is unconditionally "content-scoped," but anthropicToResponsesTranslation (src/claude/inbound.ts) only does that when systemParts.length > 0; with no system content it falls back to hashing metadata.user_id (cacheKeySource = "metadata"). Worth a one-line caveat so operators reading this don't assume every request gets a content-scoped key.
Additionally, please confirm the ja/ko/ru/zh-cn translations of this guide are updated to match — they aren't included in this diff.
📝 Suggested doc tweak
**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.
+
+When a request has no system content to fingerprint, `prompt_cache_key` falls back to a
+`metadata.user_id`-derived key instead.As per path instructions, "translated locale pages (ja, ko, ru, zh-cn) are not left contradicting the English source."
📝 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.
| **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. | |
| **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. | |
| When a request has no system content to fingerprint, `prompt_cache_key` falls back to a | |
| `metadata.user_id`-derived key instead. |
🤖 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 Native OpenAI/ChatGPT routing documentation to state that
prompt_cache_key is content-scoped when system content exists, but falls back to
hashing metadata.user_id when no system content is present. Apply the same
clarification to the ja, ko, ru, and zh-cn translations so they remain
consistent with the English guide.
Source: Path instructions
| /** | ||
| * Deterministic ordering of Claude Code's own tool/skills/deferred-tools listings on | ||
| * the native Anthropic passthrough (`anthropicNativePassthrough`, `claude-messages.ts`). | ||
| * Claude Code enumerates MCP tools/skills in whatever order its own reconnect/discovery | ||
| * race resolves them, so byte-identical conversations can arrive with different array | ||
| * order turn to turn — busting Anthropic's prompt-cache prefix for no reason. Sorting is | ||
| * safe: `tool_choice` targets tools by name, not position, and the two system-reminder | ||
| * blocks below are pure listings with no inherent order the model depends on. | ||
| * | ||
| * Ported (algorithm only, re-implemented in TypeScript) from `sort-stabilization.mjs`, | ||
| * MIT licensed, github.com/cnighswonger/claude-code-cache-fix. | ||
| */ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether stabilizeSystemAndToolOrder is ever imported/called
rg -n "stabilizeSystemAndToolOrder" --type=tsRepository: lidge-jun/opencodex
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline src/adapters/anthropic-sort-stabilize.ts --view expanded || true
printf '\n== Search for related symbols and file imports ==\n'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' \
"anthropic-sort-stabilize|sort-stabilize|stabilizeSystemAndToolOrder|normalizeSystemReminderText|sortToolsByName|anthropicNativePassthrough|claude-messages" src
printf '\n== Key files that mention the native passthrough or inbound normalization ==\n'
for f in src/adapters/claude-messages.ts src/adapters/inbound.ts src/adapters/anthropic-sort-stabilize.ts; do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
cat -n "$f" | sed -n '1,220p'
fi
doneRepository: lidge-jun/opencodex
Length of output: 7115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Search for imports of the file by path ==\n'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' \
"from ['\"].*anthropic-sort-stabilize['\"]|require\\(['\"].*anthropic-sort-stabilize['\"]\\)" src
printf '\n== Search for any direct references to the function name in non-source files ==\n'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' \
"stabilizeSystemAndToolOrder" .Repository: lidge-jun/opencodex
Length of output: 465
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== Imports and passthrough body flow in src/server/claude-messages.ts =='
ast-grep outline src/server/claude-messages.ts --view expanded || true
echo
echo '== Relevant slices =='
cat -n src/server/claude-messages.ts | sed -n '1,140p'
echo
cat -n src/server/claude-messages.ts | sed -n '280,380p'
echo
cat -n src/server/claude-messages.ts | sed -n '540,620p'
echo
cat -n src/server/claude-messages.ts | sed -n '840,880p'Repository: lidge-jun/opencodex
Length of output: 21360
Call stabilizeSystemAndToolOrder() before the native passthrough request is serialized.
src/adapters/anthropic-sort-stabilize.ts:66-78 exports the normalizer, but src/server/claude-messages.ts:291-329 still forwards body to Anthropic unchanged. On sk-ant-* requests that leaves system/tools ordering dependent on Claude Code’s discovery race, so the prompt-cache prefix can still drift. Apply the helper in anthropicNativePassthrough() before JSON.stringify(body).
🤖 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 1 - 12, Update
anthropicNativePassthrough() to call stabilizeSystemAndToolOrder() on the
request body before it is serialized with JSON.stringify. Ensure sk-ant-* native
passthrough requests send the normalized system and tools ordering while
preserving the existing request flow and payload contents.
| 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.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve cacheReadInputTokens alongside cachedInputTokens.
This parser reads the same input_tokens_details.cached_tokens field as src/server/request-log.ts:440-492, but only sets cachedInputTokens. Since OcxUsage also exposes cacheReadInputTokens (src/types.ts:308-323), downstream usage or cost accounting can under-report cache reads for passthrough Responses.
Proposed fix
- ...(typeof inputDetails?.cached_tokens === "number" ? { cachedInputTokens: inputDetails.cached_tokens } : {}),
+ ...(typeof inputDetails?.cached_tokens === "number"
+ ? {
+ cachedInputTokens: inputDetails.cached_tokens,
+ cacheReadInputTokens: inputDetails.cached_tokens,
+ }
+ : {}),📝 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 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 } : {}), | |
| 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, | |
| cacheReadInputTokens: inputDetails.cached_tokens, | |
| } | |
| : {}), | |
| ...(typeof outputDetails?.reasoning_tokens === "number" ? { reasoningOutputTokens: outputDetails.reasoning_tokens } : {}), |
🤖 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, Update the usage
object returned by the Responses parser to populate both cachedInputTokens and
cacheReadInputTokens from inputDetails.cached_tokens, preserving the existing
numeric guard and all other token mappings.
| // Claude Code's Messages API never sets previousResponseId at all, so on a | ||
| // custom apiKey-auth provider (non-forward) the original forward-only guard | ||
| // never fired, and long tool-chains hit "Invalid 'input[N].call_id': string | ||
| // too long" (call_id replay can exceed the Responses API's 64-char limit). | ||
| // !unexpandedMiss is a safe superset of the old condition: it still covers | ||
| // forward mode and already-expanded replays, but also covers any request with | ||
| // no previousResponseId at all (every Claude-surfaced request) — while still | ||
| // excluding the one genuinely risky case, a raw unexpanded native-Codex | ||
| // continuation that might reference call ids stored upstream. | ||
| if (!unexpandedMiss) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add regression coverage for the replay-repair truth table.
!unexpandedMiss is not a superset of the previous forward || expanded condition: it now skips forward requests that have a previousResponseId and unexpanded input. Verify that exclusion is intentional, then test at least:
- API-key request without
previousResponseId: oversizedcall_idis repaired. - Forward request without
previousResponseId: oversizedcall_idis repaired. - Unexpanded continuation with
previousResponseId: repair is intentionally skipped. - Expanded replay: oversized
call_idis repaired.
No companion test file is included in the supplied review context. As per path instructions, adapter behavior changes should include a focused regression test near the existing tests for that subsystem.
🤖 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 748 - 757, Add focused
regression coverage near the existing tests for the adapter behavior around the
replay-repair condition `!unexpandedMiss`. Verify the truth table by asserting
oversized `call_id` values are repaired for API-key requests without
`previousResponseId`, forward requests without `previousResponseId`, and
expanded replays, while repair is intentionally skipped for an unexpanded
continuation with `previousResponseId`; also confirm whether that
forward-with-`previousResponseId` exclusion is intentional.
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 | 🏗️ Heavy lift
Preserve the live hostname in the post-start callback.
handleEnsure() already uses live.hostname because the running proxy’s bound hostname can differ from config.hostname. This new callback exposes only port, so handleTrayProxyStart() cannot preserve that metadata and may rewrite ~/.grok/config.toml with a stale or default hostname for an already-running proxy. Extend TrayProxyLive/onStarted to carry the live metadata and use it in both branches.
Also applies to: 32-32, 52-52
🤖 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, Extend the
TrayProxyLive/onStarted contract to pass the live proxy metadata, including
hostname, rather than only the port. Update handleEnsure’s already-running and
newly-started branches to invoke the callback with the same live object, and
update handleTrayProxyStart() to use the callback’s live hostname when
synchronizing configuration instead of config.hostname.
| 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 | 🟠 Major | ⚡ Quick win
Make the diagnostic follow actual routing precedence.
A models[] entry is still unreachable when another provider’s defaultModel claims it first. For example, anthropic.defaultModel = "claude-4-sonnet" causes cursor.models = ["claude-4-sonnet"] to route to anthropic, but Lines 351-355 suppress the warning. Also, the diagnostic searches active providers while routeByKnownModelPattern searches all configured providers, so disabled matching providers can make the reported target differ from the actual target.
Resolve the bare id with the same shared provider-selection logic used by routing (and consistently exclude disabled providers), then warn whenever the resolved provider differs from the declaring provider.
src/router.ts#L350-L359: remove the globaldefaultModelssuppression and align default/pattern target selection withrouteModelInternal/routeByKnownModelPattern.tests/warn-unreachable-bare-model-aliases.test.ts#L97-L103: expect a warning forcursor/claude-4-sonnet; add coverage for a disabled pattern-provider preceding an active matching provider.
As per path instructions, shared routing changes require focused regression coverage.
📍 Affects 2 files
src/router.ts#L350-L359(this comment)tests/warn-unreachable-bare-model-aliases.test.ts#L97-L103
🤖 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 350 - 359, Update the diagnostic around
activeProviderEntries and MODEL_PROVIDER_PATTERNS in src/router.ts lines 350-359
to remove global defaultModels suppression and resolve each bare model through
the same provider-selection precedence as routeModelInternal and
routeByKnownModelPattern, consistently excluding disabled providers; warn
whenever the resolved provider differs from the declaring provider. Update
tests/warn-unreachable-bare-model-aliases.test.ts lines 97-103 to expect the
cursor/claude-4-sonnet warning and add coverage where a disabled pattern
provider precedes an active matching provider.
Source: Path instructions
| 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 bypass an exact unverified entry with a wildcard price.
exact includes unverified rows, but found excludes them. When an exact row is unverified, lines 152-154 therefore return a trusted wildcard row instead of remaining unpriced. Only enter the wildcard branch when exact.length === 0; add a regression covering an unverified exact row plus a trusted wildcard row.
Proposed fix
- if (found) return found;
+ if (exact.length > 0) return found;📝 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 lookup
around the exact and wildcard overlay selection so wildcard prices are
considered only when exact.length === 0; an exact unverified entry must return
no price rather than falling back to a trusted wildcard. Add a regression test
covering an unverified exact row alongside a trusted wildcard row.
Source: Path instructions
| // 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()"); | ||
| }); | ||
|
|
||
| test("handleTrayProxyStart still syncs the Grok fence and Codex models", () => { | ||
| // Repointing restart at handleTrayProxyStart (above) fixed the codexAutoStart no-op but | ||
| // silently dropped the Grok-fence/model sync handleEnsure always performed — caught by | ||
| // this suite failing on the OLD assertion above once the swap landed. Guard both sides: | ||
| // the sync exists in the function, and it's wired through onStarted (fires whether the | ||
| // proxy was already live or this call just started it), not just after a fresh spawn. | ||
| const trayStartFn = sliceFn(CLI_SOURCE, "async function handleTrayProxyStart(", "async function handleTrayProxyRestart("); | ||
| expect(trayStartFn).toContain("syncModelsToCodex(port)"); | ||
| expect(trayStartFn).toContain('await import("../grok/sync")'); | ||
| expect(trayStartFn).toContain("onStarted:"); | ||
|
|
||
| const trayProxySource = readFileSync(join(import.meta.dir, "..", "src", "cli", "tray-proxy.ts"), "utf8"); | ||
| const onStartedCalls = trayProxySource.match(/await io\.onStarted\?\.\(/g); | ||
| // Once for the already-live early return, once after a fresh start succeeds. | ||
| expect(onStartedCalls).toHaveLength(2); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Exercise the lifecycle instead of matching source text.
These assertions only search source files and count two callback expressions; they never execute either tray-start branch. A broken implementation could still satisfy the strings while passing the wrong metadata, invoking the hook in the wrong branch, or returning before an async hook completes. Add focused Bun tests with fake I/O for already-live and freshly-started proxies, asserting callback arguments and completion order.
As per path instructions, this behavior change should have a focused runtime regression test near the existing lifecycle 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 `@tests/grok-lifecycle.test.ts` around lines 90 - 111, Replace the source-text
assertions in the lifecycle tests with focused Bun runtime tests around
handleTrayProxyStart, using fake I/O to exercise both already-live and
freshly-started proxy branches. Assert that onStarted receives the expected
metadata and is awaited to completion in each branch, while preserving the
existing model-sync coverage near the lifecycle tests.
Source: Path instructions
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc72a4a983
ℹ️ 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".
| .update(canonicalJson({ | ||
| version: 2, |
There was a problem hiding this comment.
When a Claude request with system content is routed to provider xai, this content-only key omits both metadata.user_id and user messages. The responses pipeline passes parsed.options.promptCacheKey into resolveProviderTransport, which derives x-grok-conv-id and x-grok-session-id from it (src/providers/xai-transport.ts:101-109), so two separate Claude Code sessions with the same system/tools but different histories now get sent as the same Grok conversation/session. Keep the old metadata-derived key for xAI transport affinity, or avoid feeding this shared cache key into those headers.
Useful? React with 👍 / 👎.
| } 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 serializeColdSubagentCache is enabled on normal Claude streaming turns, handleResponses resolves after constructing the SSE Response; onFirstOutput is invoked later while that body is consumed. This finally runs immediately after the response object is returned, so the leader releases before any prompt has been ingested/output and followers dispatch cold anyway. Move the fallback release to true error/non-stream paths, or wrap the returned stream so it only releases on first output/close.
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 documented as stabilizing the native Anthropic passthrough, but rg stabilizeSystemAndToolOrder only finds this definition and the native path still serializes body directly in anthropicNativePassthrough. As a result, subscription/native Claude requests continue to forward nondeterministic system reminder and tools ordering, so the cache-input fix only applies to routed translation. Call this in the native passthrough before JSON.stringify(body).
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.
In the already-running tray start path, waitForProxy/findLiveProxy knows the hostname that the live proxy actually answered on, but onStarted receives only the port and then writes the Grok fence with config.hostname. If the config was edited after the proxy started, this can overwrite ~/.grok/config.toml with a host the running process never bound; handleEnsure deliberately uses live.hostname in that branch. Pass the live hostname through TrayProxyLive/onStarted and use it here.
Useful? React with 👍 / 👎.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation