fix(openai): replay DeepSeek tool reasoning_content#71473
Conversation
![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: b8b1b6281a
ℹ️ 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".
| const providerOrFamilyIsDeepSeek = | ||
| isDefaultRouteProvider(input.provider, "deepseek") || knownProviderFamily === "deepseek"; | ||
| const isDeepSeek = endpointClass === "deepseek-native" || providerOrFamilyIsDeepSeek; |
There was a problem hiding this comment.
This change makes any model whose ID contains deepseek evaluate as isDeepSeek, which takes precedence over isOpenRouterLike in resolveOpenAICompletionsCompatDefaults. For OpenRouter DeepSeek models (for example openrouter/deepseek/... used in this repo), thinkingFormat now flips from openrouter to deepseek, so buildOpenAICompletionsParams stops sending OpenRouter's reasoning object and instead sends reasoning_effort plus injected reasoning_content; that can break reasoning controls or request compatibility on OpenRouter routes.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds DeepSeek thinking-mode compatibility for Ollama/custom-hosted
Confidence Score: 3/5Not safe to merge as-is — the model-ID heuristic placed before the provider switch breaks thinkingFormat for OpenRouter-hosted DeepSeek models. One P1 regression: OpenRouter DeepSeek models lose proper params.reasoning setup and get an incorrect thinkingFormat, silently disabling thinking on that provider. The core fix (Ollama DeepSeek tool-call injection) is correct, but the detection scope is too broad. src/agents/provider-attribution.ts — the early-return model-ID check in resolveKnownProviderFamily needs to be moved to the default branch to avoid overriding known non-DeepSeek providers like openrouter. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/provider-attribution.ts
Line: 344-348
Comment:
**Model-ID check shadows provider for OpenRouter-hosted DeepSeek models**
The early-return on `normalizedModelId?.includes("deepseek")` runs before the `provider` switch, so an OpenRouter model like `deepseek-r1` gets `knownProviderFamily = "deepseek"` instead of `"openrouter"`. Back in `openai-completions-compat.ts`, `thinkingFormat` resolves `isDeepSeek` before `isOpenRouterLike`, so those models end up with `thinkingFormat = "deepseek"` rather than `"openrouter"`. As a result, `params.reasoning = { effort: ... }` (OpenRouter's thinking field) is never populated, `params.reasoning_effort` is set instead (which OpenRouter ignores for reasoning), and the DeepSeek `reasoning_content` injection runs unnecessarily. Thinking stops working for any OpenRouter-hosted DeepSeek model.
A safer fix is to move the model-ID heuristic into the `default` branch so it only fires for unknown/generic providers:
```typescript
default:
if (normalizedModelId?.includes("deepseek")) return "deepseek";
return provider || "unknown";
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/openai-completions-compat.ts
Line: 75-77
Comment:
**`isDefaultRoute` guard silently dropped from `isDefaultRouteProvider` check**
The original condition was `isDefaultRoute && isDefaultRouteProvider(input.provider, "deepseek")`, so a user with `provider: "deepseek"` and a custom `baseUrl` (making `isDefaultRoute = false`) was not treated as `isDeepSeek`. The new line removes that guard, meaning any custom-baseUrl configuration that still carries `provider: "deepseek"` now gets `isDeepSeek = true`. This could apply DeepSeek-specific compat settings (e.g. `thinkingFormat: "deepseek"`, the new `reasoning_content` injection) to proxies or mirrors of the official DeepSeek API that may not support those DeepSeek extensions.
If the intent is to support the full `provider="deepseek"` + custom baseUrl combo, a comment explaining why the guard was intentionally removed would help future readers avoid restoring it accidentally.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(openai): replay deepseek tool reason..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function resolveKnownProviderFamily(provider: string | undefined, modelId?: string | null): string { | ||
| const normalizedModelId = normalizeOptionalLowercaseString(modelId); | ||
| if (normalizedModelId?.includes("deepseek")) { | ||
| return "deepseek"; | ||
| } |
There was a problem hiding this comment.
The early-return on normalizedModelId?.includes("deepseek") runs before the provider switch, so an OpenRouter model like deepseek-r1 gets knownProviderFamily = "deepseek" instead of "openrouter". Back in openai-completions-compat.ts, thinkingFormat resolves isDeepSeek before isOpenRouterLike, so those models end up with thinkingFormat = "deepseek" rather than "openrouter". As a result, params.reasoning = { effort: ... } (OpenRouter's thinking field) is never populated, params.reasoning_effort is set instead (which OpenRouter ignores for reasoning), and the DeepSeek reasoning_content injection runs unnecessarily. Thinking stops working for any OpenRouter-hosted DeepSeek model.
A safer fix is to move the model-ID heuristic into the default branch so it only fires for unknown/generic providers:
default:
if (normalizedModelId?.includes("deepseek")) return "deepseek";
return provider || "unknown";Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/provider-attribution.ts
Line: 344-348
Comment:
**Model-ID check shadows provider for OpenRouter-hosted DeepSeek models**
The early-return on `normalizedModelId?.includes("deepseek")` runs before the `provider` switch, so an OpenRouter model like `deepseek-r1` gets `knownProviderFamily = "deepseek"` instead of `"openrouter"`. Back in `openai-completions-compat.ts`, `thinkingFormat` resolves `isDeepSeek` before `isOpenRouterLike`, so those models end up with `thinkingFormat = "deepseek"` rather than `"openrouter"`. As a result, `params.reasoning = { effort: ... }` (OpenRouter's thinking field) is never populated, `params.reasoning_effort` is set instead (which OpenRouter ignores for reasoning), and the DeepSeek `reasoning_content` injection runs unnecessarily. Thinking stops working for any OpenRouter-hosted DeepSeek model.
A safer fix is to move the model-ID heuristic into the `default` branch so it only fires for unknown/generic providers:
```typescript
default:
if (normalizedModelId?.includes("deepseek")) return "deepseek";
return provider || "unknown";
```
How can I resolve this? If you propose a fix, please make it concise.| const providerOrFamilyIsDeepSeek = | ||
| isDefaultRouteProvider(input.provider, "deepseek") || knownProviderFamily === "deepseek"; | ||
| const isDeepSeek = endpointClass === "deepseek-native" || providerOrFamilyIsDeepSeek; |
There was a problem hiding this comment.
isDefaultRoute guard silently dropped from isDefaultRouteProvider check
The original condition was isDefaultRoute && isDefaultRouteProvider(input.provider, "deepseek"), so a user with provider: "deepseek" and a custom baseUrl (making isDefaultRoute = false) was not treated as isDeepSeek. The new line removes that guard, meaning any custom-baseUrl configuration that still carries provider: "deepseek" now gets isDeepSeek = true. This could apply DeepSeek-specific compat settings (e.g. thinkingFormat: "deepseek", the new reasoning_content injection) to proxies or mirrors of the official DeepSeek API that may not support those DeepSeek extensions.
If the intent is to support the full provider="deepseek" + custom baseUrl combo, a comment explaining why the guard was intentionally removed would help future readers avoid restoring it accidentally.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/openai-completions-compat.ts
Line: 75-77
Comment:
**`isDefaultRoute` guard silently dropped from `isDefaultRouteProvider` check**
The original condition was `isDefaultRoute && isDefaultRouteProvider(input.provider, "deepseek")`, so a user with `provider: "deepseek"` and a custom `baseUrl` (making `isDefaultRoute = false`) was not treated as `isDeepSeek`. The new line removes that guard, meaning any custom-baseUrl configuration that still carries `provider: "deepseek"` now gets `isDeepSeek = true`. This could apply DeepSeek-specific compat settings (e.g. `thinkingFormat: "deepseek"`, the new `reasoning_content` injection) to proxies or mirrors of the official DeepSeek API that may not support those DeepSeek extensions.
If the intent is to support the full `provider="deepseek"` + custom baseUrl combo, a comment explaining why the guard was intentionally removed would help future readers avoid restoring it accidentally.
How can I resolve this? If you propose a fix, please make it concise.
|
Addressed the review feedback in
Validation: |
|
Codex maintainer review: closing this as not ready / not proven against a current failing path. I tested the PR head with the advertised focused suite: pnpm test src/agents/openai-transport-stream.test.ts extensions/deepseek/index.test.tsResult: passed ( I also tested the available local Ollama paths:
So I cannot reproduce the linked #71455 failure on current available Ollama paths, and the Kimi/Ollama part of the claim does not match live behavior here. Implementation concern: this patch promotes any model id containing If this still fails for a specific Ollama-hosted DeepSeek V4 model, please reopen with the exact model id that is available through Ollama, the request/response error, and ideally a live repro we can run. A safer fix would target that provider/model path explicitly and include a regression that proves the failing replay shape. |
|
Follow-up Mac Studio live check after maintainer request:
Replay results through Ollama
So even on the beefy Mac Studio / Ollama DeepSeek path, I still cannot reproduce a current Ollama-side requirement for |
|
Detailed closeout / live-test matrix: Local machine (
Mac Studio (
Conclusion: I could not reproduce the claimed current Ollama failure on any available tool-capable Ollama route. The PR's production change promotes any model id containing Closing remains correct. If a specific Ollama-hosted DeepSeek model still fails, we need the exact Ollama model id, the failing request/response error, and a live repro against that route before accepting a core transport change. |
2 participants
Fixes #71455.
Summary
deepseek-*models get DeepSeek thinking compatreasoning_content: ""on assistant messages with replayedtool_callswhen DeepSeek thinking mode is activereasoning_contentTest plan
pnpm test src/agents/openai-transport-stream.test.ts extensions/deepseek/index.test.ts