Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…s for OpenRouter entries OpenAI-compatible entries pointing at OpenRouter now behave like a built-in OpenRouter provider, with nothing to configure (detected by the base URL, as CPA already does for reasoning signatures): - Cache breakpoints: chat-completions requests without cache_control get Anthropic-style breakpoints on the last system message and the latest user/tool message. OpenRouter caches Claude only when asked and ignores the markers for models that cache automatically; a Claude client's ttl "1h" is kept. - Session affinity: x-session-id from CPA's session, so a conversation stays on one upstream provider and keeps hitting its cache. - Model limits: OpenRouter's public catalog (/api/v1/models) fills context length and completion limit for configured models, fetched in the background and refreshed with the other catalogs; configured values win.
041fb38 to
cda8deb
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 041fb38b54
ℹ️ 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".
| // covers the whole conversation prefix. Requests that already carry cache_control are left alone. A Claude-format | ||
| // client asking for the 1-hour lifetime keeps it. | ||
| func applyOpenAICompatCacheControlDoc(compat *config.OpenAICompatibility, from sdktranslator.Format, req cliproxyexecutor.Request, opts cliproxyexecutor.Options, doc *ojson.Doc) { | ||
| if doc == nil || compat == nil || !compat.SupportCacheControl { |
There was a problem hiding this comment.
For an existing OpenRouter entry that only has the documented base URL, SupportCacheControl is zero-valued false and this guard returns before adding any breakpoints. The existing isOpenRouterUpstreamURL detector (internal/runtime/executor/openai_responses_signature.go:228-238) is used only for signature sanitation and is not consulted at either new call site, so the advertised no-configuration behavior does nothing unless operators discover and enable the new flag. Pass the base-URL detection into this path or default it for OpenRouter while leaving other providers unchanged.
Useful? React with 👍 / 👎.
| if err = e.applyPromptCacheKeyWithCompatDoc(ctx, auth, compat, from, baseModel, req, opts, doc); err != nil { | ||
| return resp, err | ||
| } | ||
| applyOpenAICompatCacheControlDoc(compat, from, req, opts, doc) |
There was a problem hiding this comment.
For an OpenRouter request whose downstream client did not supply x-session-id, both new execution paths proceed to construct httpReq without deriving helps.ProviderSessionUUID or setting that header; a target-tree search of the OpenAI-compatible executor found no outbound forwarding elsewhere. Repeated turns can therefore still land on different OpenRouter backends and miss the cache. Set a stable header for detected OpenRouter requests while preserving a client-supplied value.
Useful? React with 👍 / 👎.
| part, err := ojson.SetBytes([]byte(`{"type":"text","text":""}`), "text", text) | ||
| if err != nil { | ||
| return | ||
| } | ||
| part, err = ojson.SetRawBytes(part, "cache_control", []byte(cacheControl)) |
There was a problem hiding this comment.
When converting string content, these calls perform JSON writes against raw []byte values before attaching them to doc. That bypasses the required single-*ojson.Doc mutation pipeline; initialize and mutate the replacement content directly through the destination document so serialization still happens once at the final request exit.
AGENTS.md reference: AGENTS.md:L53-L53
Useful? React with 👍 / 👎.
| // Placement follows CPA's own Claude breakpoints: the last system message and the latest user or tool message, which | ||
| // covers the whole conversation prefix. Requests that already carry cache_control are left alone. A Claude-format | ||
| // client asking for the 1-hour lifetime keeps it. | ||
| func applyOpenAICompatCacheControlDoc(compat *config.OpenAICompatibility, from sdktranslator.Format, req cliproxyexecutor.Request, opts cliproxyexecutor.Options, doc *ojson.Doc) { |
There was a problem hiding this comment.
The newly added production file consists entirely of free cache-control support functions, but it is placed directly in internal/runtime/executor, which is reserved for executors and unit tests. Move this support implementation under internal/runtime/executor/helps and expose the small API needed by OpenAICompatExecutor.
AGENTS.md reference: AGENTS.md:L52-L52
Useful? React with 👍 / 👎.
| # prefix: "test" # optional: require calls like "test/kimi-k2" to target this provider's credentials | ||
| # base-url: "https://openrouter.ai/api/v1" # The base URL of the provider. | ||
| # support-prompt-cache-key: false # optional: derive prompt_cache_key for requests from all input protocols | ||
| # support-cache-control: false # optional: add Anthropic-style cache_control breakpoints to chat-completions requests (e.g. OpenRouter caches Claude only when asked); keeps a Claude client's ttl "1h" |
There was a problem hiding this comment.
For a configured OpenRouter model without a manual max-context-length, model registration still goes through buildOpenAICompatibilityConfigModels, whose buildConfiguredModelInfo path (sdk/cliproxy/service_models.go:844-848) only copies a configured context length and never sets MaxCompletionTokens. A target-tree path/content search also finds no registry/openrouter_models.go, catalog fetch, or refresh hook, so clients continue receiving generic fallback limits rather than OpenRouter's values. Add the advertised catalog/updater and merge its limits during registration.
Useful? React with 👍 / 👎.
| func openAIMessagesHaveCacheControl(messages ojson.Result) bool { | ||
| for _, msg := range messages.Array() { |
There was a problem hiding this comment.
When an opted-in OpenAI-format request already puts cache_control on tools[i] or tools[i].function (a shape this repository explicitly preserves in ConvertOpenAIRequestToClaude), this check scans only messages, so the helper adds two synthetic message markers instead of leaving the request alone. That changes the caller's explicit cache layout and can push a valid request over the provider's breakpoint limit; scan the supported tool locations before injecting.
Useful? React with 👍 / 👎.
…list
OpenAI-compatible entries get the generic thinking levels low/medium/high and
owned_by = the entry's name. For OpenRouter that hid xhigh and max, which
OpenRouter accepts (reasoning.supported_efforts in /api/v1/models), and a
request asking for max was sent upstream as high. Pools sharing a model id
with an OpenRouter member also listed only low/medium/high (the Codex client
list intersects members' levels), and their owned_by read "OpenRouter".
For entries pointing at OpenRouter, and models without configured thinking:
- the model list uses OpenRouter's published efforts as levels ("none" means
thinking can be turned off), sorted low to high;
- request-time capabilities apply the same levels, read from the catalog per
request, so they hold as soon as the catalog has loaded;
- owned_by is the vendor of the OpenRouter id (anthropic/... -> anthropic,
~openai/... -> openai), as for Vertex publisher/model names.
Configured thinking still wins; models OpenRouter doesn't list keep the
default. The catalog type becomes OpenRouterModel (limits + efforts).
|
Added a second commit (0d37de3): OpenRouter entries now take their reasoning levels from OpenRouter's model list ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d37de3ad2
ℹ️ 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".
| func applyOpenRouterSessionHeader(r *http.Request, baseURL, provider string, req cliproxyexecutor.Request, opts cliproxyexecutor.Options) { | ||
| if r == nil || !isOpenRouterUpstreamURL(baseURL) || strings.TrimSpace(r.Header.Get("x-session-id")) != "" { | ||
| return | ||
| } |
There was a problem hiding this comment.
With a normal OpenRouter entry, a downstream x-session-id (or another explicit session header) exists in opts.Headers, not in the freshly constructed httpReq, and ApplyCustomHeadersFromAttrs does not copy arbitrary client headers. session.Enrich also replaces derived-session metadata with canonical-session metadata for such requests, which ProviderSessionUUID does not read, so this guard and the following call leave the outbound header unset instead of preserving it. The fresh evidence beyond the earlier session-header finding is that the completed helper checks only r.Header and derived/execution metadata; consult opts.Headers or canonical metadata here.
Useful? React with 👍 / 👎.
| retryAllowed := c.failedAt.IsZero() || now.Sub(c.failedAt) > time.Minute | ||
| if stale && !c.fetching && retryAllowed { | ||
| c.fetching = true | ||
| go c.refresh() |
There was a problem hiding this comment.
When --local-model is set and an OpenRouter-compatible entry is registered, the first lookup still reaches this unconditional goroutine and contacts the hard-coded OpenRouter URL; SetLocalModelCatalogs only configures the existing catalog updaters, and this standalone catalog never consults that policy. This defeats the flag's remote-update opt-out in offline or network-restricted deployments, so gate this fetch through the shared catalog policy.
AGENTS.md reference: AGENTS.md:L32-L32
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| func (c *openRouterCatalog) refresh() { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) |
There was a problem hiding this comment.
On a slow proxy or link where /api/v1/models connects but the response body takes more than 30 seconds, this request-scoped deadline remains active through io.ReadAll, cancels the transfer, and leaves every configured model on generic fallback metadata. This catalog lookup is not credential acquisition, so use lifecycle cancellation without a deadline that can terminate established network I/O.
AGENTS.md reference: AGENTS.md:L61-L61
Useful? React with 👍 / 👎.
| }} | ||
| catalog.lookup("openrouter", "x") | ||
| <-calls | ||
| time.Sleep(50 * time.Millisecond) |
There was a problem hiding this comment.
Under scheduler delay, receiving <-calls only proves that the fetch function sent on the channel; the refresh goroutine may not yet have returned and recorded failedAt. This fixed sleep does not establish that state, so the second lookup can be suppressed merely by fetching == true and the test can pass without exercising the one-minute backoff; synchronize refresh completion and use a controllable clock instead.
AGENTS.md reference: AGENTS.md:L62-L62
Useful? React with 👍 / 👎.
…og policy, helpers in helps - Session affinity: clients that name their own session (explicit headers or body ids) have only a canonical session, so no X-Session-Id was sent. Now a client X-Session-Id is forwarded, then CPA's provider session, then a stable id from the canonical session. - The OpenRouter catalog follows the shared catalog policy: no fetch under --local-model or CLIProxyAPIHome, and the fetch runs under the service lifetime without a request deadline. - Cache breakpoints are not added when the caller already marks a tool (tools[i].cache_control or tools[i].function.cache_control). - String content becomes a text part through *ojson.Doc writes only. - The cache-control and session helpers move to internal/runtime/executor/helps (ApplyOpenRouterCacheControlDoc, ClaudeRequestAsksOneHourCache, ApplyOpenRouterSessionHeader). - Catalog tests use a controllable clock and a refresh hook instead of sleeps.
|
Pushed 8143329 for the review findings:
|
…ort (P1, P2) Claude Haiku 5.5 via an openai-compatibility OpenRouter entry on CPA 8.0.53 + router-for-me/CLIProxyAPI#7284: 5 - 7.5 min by default (was no cache), 30 - 65 min with ttl 1h. P3 (GPT-6.1 Sol, prompt_cache_options.ttl 12h) shows the 12h request is dropped in CPA's Responses -> Chat Completions translation.
Problem
OpenRouter is usually configured in CPA as an OpenAI-compatible entry. Three things go wrong:
cache_control, and Codex/OpenAI-format clients never send it, so every request re-bills the whole prompt.max_tokens).owned_by= the entry's name. OpenRouter acceptsxhighandmaxtoo, but a request asking formaxwas sent upstream ashigh. Aliases that share an id with an OpenRouter model (e.g. one model served by a Claude key, Devin and OpenRouter) list only low/medium/high in the Codex client list, which intersects the members' levels, and reportowned_by: "OpenRouter".Change
OpenAI-compatible entries whose base URL is OpenRouter now get built-in OpenRouter behaviour. There's nothing to configure: detection uses the base URL, as CPA already does for reasoning signatures (
isOpenRouterUpstreamURL, nowutil.IsOpenRouterURL).Cache breakpoints (
internal/runtime/executor/helps/openrouter.go):cache_controlget Anthropic-style breakpoints on the last system message and the latest user or tool message, mirroring CPA's Claude breakpoints;ttl: "1h"is kept;tools[i]ortools[i].function) are left alone;Session affinity: an
X-Session-Idheader (OpenRouter's session id). Order: a client-suppliedX-Session-Id, then CPA's provider session (execution or derived), then a stable id from the canonical session, which is all there is for clients that name their own session (e.g. pi).Model limits (
registry/openrouter_models.go):/api/v1/modelsfillsContextLengthandMaxCompletionTokensfor configured models;ModelsRefreshInterval, and backs off for a minute after failures;--local-modelor CLIProxyAPIHome, and the fetch runs under the service lifetime without a request deadline;max-context-lengthstill wins.Reasoning levels (second commit). The same catalog's
reasoning.supported_efforts(e.g.["max","xhigh","high","medium","low","none"]) becomes the model's thinking support: levels sorted low to high, and"none"means thinking can be turned off (ZeroAllowed). It's applied in two places:buildOpenAICompatibilityConfigModels);attachResolvedAPIKeyModelInfo, viawithOpenRouterThinking). These are read from the catalog per request, so they hold as soon as it has loaded, without rebuilding the routing snapshot.Configured
thinkingstill wins (now flagged withExplicitThinking). Models OpenRouter doesn't list keep low/medium/high.Owner:
owned_byis the vendor of the OpenRouter id (anthropic/claude-haiku-5.5→anthropic,~openai/…→openai), as CPA already does for Vertexpublisher/modelnames.Other OpenAI-compatible providers are unchanged.
Verification
Unit tests: breakpoint placement and no-op cases; executor end to end through an injected transport (OpenRouter URL → two 1h breakpoints and a stable
x-session-id; another URL → body and headers unchanged); catalog parsing, background fetch with refresh notification and no refetch while fresh, failure back-off;util.IsOpenRouterURL.Live, real OpenRouter key, no config flags:
/v1/models?client_version=pianthropic/claude-haiku-5.51,000,000 / 128,000;openai/gpt-6.1-sol1,050,000 / 128,000cache_creation_input_tokens: 12620, thencache_read_input_tokens: 12620cached_tokens: 0, thencached_tokens: 12620Before this change, the same Claude requests through the entry billed 12,624 input tokens every time.
Live, reasoning levels (second commit,
anthropic/claude-haiku-5.5through the entry, request log of the upstream body):owned_byOpenRouteranthropicoutput_config.effort: "max"→ upstreamreasoning_effort: "high"reasoning_effort: "max"reasoning.effort: "max"→ upstream"high""max"reasoning_effort: "max"→ upstream"high""max"OpenRouter honours it: the same question used 48–50 reasoning tokens at
lowand 229–282 atmax.Unit tests (third commit, review fixes): session id from a client header, execution, derived and canonical sessions (stable per conversation, distinct across conversations); tool markers leave the request alone; catalog back-off and refresh with a controllable clock and a refresh hook (no sleeps); no fetch under
--local-model; executor end to end with a canonical-only session.Unit tests (second commit): efforts parsing and copy-on-lookup; efforts → levels (order,
none, unknown efforts, nothing published); model list (levels, owner, configured thinking wins, unlisted model, non-OpenRouter entry unchanged); request-time capabilities (levels, thinking suffix, configured thinking wins, unlisted model, stored capability unchanged, non-OpenRouter entry unchanged).go build -o test-output ./cmd/server; gofmt clean on changed files. Executor, registry, config, translator and SDK tests are green, apart from the pre-existinginternal/utilTestInPlaceByteWritesAreReviewedfailure ondev(internal/kvstore/bolt_store.go).Supersedes #7283 (same breakpoints behind a config flag).