fix: inject custom provider params at fetch layer#16102
Conversation
Signed-off-by: raymond <13162938362@163.com>
DeJeune
left a comment
There was a problem hiding this comment.
Review: fix: inject custom provider params at fetch layer
Thanks for the fix — the fetch-layer re-injection approach is sound and the core mechanics (low-precedence merge, non-JSON body passthrough, signature-fetch ordering) are correct and well-tested. I found one blocking reliability issue plus a couple of lower-severity notes.
Decision: 🔴 Request changes (the high-severity issue below should be resolved before merge).
🔴 Blocking — provider-instance cache can serve a stale / incorrect wrapped fetch
(See the inline comment on AiProvider.ts for the anchored code.)
Built provider instances are cached in a module-level LRU keyed by computeHash(mergedSettings) in packages/aiCore/src/core/providers/core/ProviderExtension.ts, and stableStringify collapses every function to the constant string "[function]" (~line 166). The fetch wrapped here now behaves differently depending on customProviderParams, but customProviderParams is not part of the hashed settings.
Consequence: two requests with the same provider + model + apiKey but different custom params hash to the same cache key and reuse the earlier cached instance, which closed over the earlier request's fetch. So custom params can be silently dropped or cross-contaminated between assistants/requests.
- Standard providers: collides between two requests that both carry (different) custom params.
- CherryIN / openai-compatible / other proxy providers that always carry a signature
fetch: collide even between an empty-params and a non-empty-params request — i.e. exactly the proxy providers this PR is meant to help.
Verified end-to-end: AiProvider.modernCompletions → createExecutor → ExtensionRegistry.createProvider → ProviderExtension.createProvider (LRU get(hash)); the provider closes over settings.fetch at construction and RuntimeExecutor does not re-resolve it per request; the registry is a module-level singleton, so the cache persists for the session.
Suggested directions (any one):
- Fold a stable hash of
customProviderParamsinto the hashed settings so the cache key differs. - Bypass the instance cache when
customProviderParamsis non-empty. - Inject via a per-request middleware/plugin instead of the cached provider's
fetch.
🟡 Test coverage
- No integration test exercises
AiProviderreadingmiddlewareConfig.customProviderParams, the empty-params no-op branch, or chaining onto an existingproviderSettings.fetch. A test here would also have surfaced the cache issue above. (The producer/forwarder/wrapper —buildProviderOptions,buildStreamTextParams,createCustomParamsFetch— are each covered in isolation.) parameterBuilder.test.tsmocks../../utils/optionsand then asserts the passthrough, so it mainly verifies plumbing. The real extraction is covered by the newoptions.test.tscases, so this one is minor.
ℹ️ Minor
isProviderScopedParam only excludes a provider-namespace key when its value is an object, so malformed input like openai: "foo" would leak as a stray top-level body field. Low impact (requires nonsensical input); optional hardening.
✅ Verified correct (no change needed)
- Precedence:
{ ...customParams, ...body }correctly lets SDK-computed body values win on key conflict. - Security:
model/messages/streametc. cannot be overridden; non-JSON bodies (FormData / Blob / stream) and invalid JSON are passed through untouched; the wrapper is scoped per-completion (no globalfetchpollution). - Wrapper ordering: custom-params fetch is the outer layer, CherryAI signature fetch the inner — so the signature is computed over the final injected body. ✔
reasoning_effortsnapshot (taken before the snake→camel conversion) is not a double-send: the openai-compatible adapter emitsreasoning_effortitself and wins the low-precedence merge.
Note for other reviewers
An earlier review pass flagged the summary/topic-naming path (ApiService.ts fetchMessagesSummary) for dropping customProviderParams. That is a non-issue: the summary assistant is built from getDefaultAssistant(), whose DEFAULT_ASSISTANT_SETTINGS.customParameters is always [], so customProviderParams is empty on that path and omitting it has no behavioral effect.
| if (middlewareConfig.customProviderParams && Object.keys(middlewareConfig.customProviderParams).length > 0) { | ||
| const existingFetch = (providerSettings as Record<string, any>).fetch ?? globalThis.fetch | ||
| const wrappedFetch = createCustomParamsFetch(existingFetch, middlewareConfig.customProviderParams) | ||
| providerSettings = { ...providerSettings, fetch: wrappedFetch } |
There was a problem hiding this comment.
🔴 Blocking — this cached provider can be served with a stale wrapped fetch.
Built providers are cached in a module-level LRU keyed by computeHash(mergedSettings) (packages/aiCore/src/core/providers/core/ProviderExtension.ts), and stableStringify serializes every function as the constant "[function]" (~line 166). The fetch wrapped here now behaves differently depending on customProviderParams, but that value is not part of the hashed settings.
So two requests with the same provider + model + apiKey but different custom params (and, for providers that always carry a signature fetch like CherryIN / openai-compatible, even empty-vs-non-empty) hash to the same key and reuse the earlier instance — which closed over the earlier request's fetch. Custom params can be silently dropped or cross-contaminated between assistants.
Options: fold a stable hash of customProviderParams into the hashed settings, bypass the instance cache when it is non-empty, or inject via a per-request middleware/plugin rather than the cached provider's fetch.
What this PR does
Before this PR:
Assistant custom parameters were merged into AI SDK
providerOptions, but many AI SDK provider adapters stripped unrecognized provider options before serializing the HTTP request body. As a result, user-defined params such asstore: false,n, or JSON params could silently disappear from chat requests.After this PR:
Cherry Studio carries filtered custom provider params through the chat parameter pipeline and wraps the provider fetch function to inject them into the serialized JSON request body at low precedence. SDK-generated body fields still win, and provider-scoped option objects are not injected as top-level HTTP body fields.
Fixes #16041
Why we need it and why it was done in this way
The following tradeoffs were made:
The fix operates at the fetch layer so it works across providers whose AI SDK adapters strip unknown provider options. It only injects flat body passthrough params and leaves provider-scoped option objects in
providerOptions.The following alternatives were considered:
Passing unknown fields through
providerOptionswas not sufficient because adapters strip them. Provider-specific adapter patches were avoided because they would repeat the same bug for every provider and parameter.Links to places where the discussion took place: #16041
Breaking changes
None.
Special notes for your reviewer
The wrapper merges custom params with low precedence (
{ ...customParams, ...body }) so SDK-computed body fields are preserved. Tests cover raw snake_case passthrough, provider-scoped object exclusion, invalid JSON/non-string body passthrough, and existing fetch wrapper chaining.Verification run locally:
pnpm formatpnpm lintpnpm testChecklist
Release note