fix(openai-chat): inject reasoning placeholder when replay cache misses (#1193) - #1205
fix(openai-chat): inject reasoning placeholder when replay cache misses (#1193)#1205Yuxin-Qiao wants to merge 3 commits into
Conversation
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. Hygiene
|
|
📝 WalkthroughWalkthroughThe provider configuration now carries model-specific reasoning placeholder settings through registry, OAuth, routing, and serialized configuration paths. The OpenAI chat adapter uses a single-space ChangesReasoning Replay Fallback
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/adapters/openai-chat.ts`:
- Around line 424-434: Update the orphan repair around orphanReasoning and the
reasoning_content spread so cached empty strings are treated consistently: when
the cache returns "", still emit reasoning_content with an appropriate fallback
value for thinking-mode providers instead of omitting the field. Preserve the
existing behavior for non-empty cached reasoning and non-thinking providers, and
use the existing peekReasoningForCall or fallback symbols.
🪄 Autofix
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: f98d979e-83b5-4e66-964b-5030e9dedd15
📒 Files selected for processing (2)
src/adapters/openai-chat.tstests/deepseek-reasoning-replay-gaps.test.ts
![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: fa9fc63a2a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
I am replying to the latest comment only. Earlier comments were not available in the supplied context.
|
Address review findings on lidge-jun#1205: - chatgpt-codex-connector P2: preserveReasoningContentModels only opts models into replaying reasoning that exists; MiniMax-M3 low effort maps to thinking disabled, so a fabricated placeholder could reach non-thinking histories. Add requiresReasoningPlaceholderModels (registry/derive/router/oauth/auth-cors plumbing, docs-site table) defaulting to the preserve list; minimax/minimax-cn seed [] to opt out. Custom preserve-only provider configs keep the lidge-jun#1193 fix via fallback. - CodeRabbit minor: treat a falsy cache hit as a miss in the orphan-repair path (defense-in-depth; the write path already rejects empty strings). Refs lidge-jun#1193
github-actions
Bot
added
intake: hygiene-blocked
Address review findings on lidge-jun#1205: - chatgpt-codex-connector P2: preserveReasoningContentModels only opts models into replaying reasoning that exists; MiniMax-M3 low effort maps to thinking disabled, so a fabricated placeholder could reach non-thinking histories. Add requiresReasoningPlaceholderModels (registry/derive/router/oauth/auth-cors plumbing, docs-site table) defaulting to the preserve list; minimax/minimax-cn seed [] to opt out. Custom preserve-only provider configs keep the lidge-jun#1193 fix via fallback. - CodeRabbit minor: treat a falsy cache hit as a miss in the orphan-repair path (defense-in-depth; the write path already rejects empty strings). Refs lidge-jun#1193
e7db84b to
5e01d03
Compare
|
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== changed files and diff ==='
git diff --find-renames origin/dev...HEAD -- src/adapters/openai-chat.ts tests/deepseek-reasoning-replay-gaps.test.ts
echo '=== adapter outline ==='
ast-grep outline src/adapters/openai-chat.ts --items all --view expanded
echo '=== test outline ==='
ast-grep outline tests/deepseek-reasoning-replay-gaps.test.ts --items all --view expandedLength of output: 791 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== local refs ==='
git remote -v || true
git branch -a || true
echo '=== PR metadata ==='
gh pr view 1205 --repo lidge-jun/opencodex --json baseRefName,headRefName,headRefOid,url
echo '=== changed file patches ==='
gh api --paginate \
-H 'Accept: application/vnd.github+json' \
'/repos/lidge-jun/opencodex/pulls/1205/files?per_page=100' \
--jq '.[] | select(.filename == "src/adapters/openai-chat.ts" or .filename == "tests/deepseek-reasoning-replay-gaps.test.ts") | {filename, status, patch}'
echo '=== adapter outline ==='
ast-grep outline src/adapters/openai-chat.ts --items all --view expanded
echo '=== relevant adapter excerpts ==='
rg -n -C 8 'preserveReasoningContentModels|reasoning_content|replay|orphan|tool_calls|tool_call' src/adapters/openai-chat.ts
echo '=== test outline ==='
ast-grep outline tests/deepseek-reasoning-replay-gaps.test.ts --items all --view expanded
echo '=== complete focused test file ==='
cat -n tests/deepseek-reasoning-replay-gaps.test.tsLength of output: 50942 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== complete PR file list ==='
gh api --paginate \
-H 'Accept: application/vnd.github+json' \
'/repos/lidge-jun/opencodex/pulls/1205/files?per_page=100' \
--jq '.[] | {filename, status, additions, deletions}'
echo '=== requiresReasoningPlaceholderModels declarations and assignments ==='
rg -n -C 6 '\brequiresReasoningPlaceholderModels\b' .
echo '=== preserveReasoningContentModels declarations and built-in configuration ==='
rg -n -C 6 '\bpreserveReasoningContentModels\b' src tests
echo '=== provider configuration type context ==='
rg -n -C 12 'interface OcxProviderConfig|type OcxProviderConfig' src
echo '=== exact adapter region ==='
sed -n '315,452p' src/adapters/openai-chat.tsLength of output: 145941
You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ko/reference/configuration/providers.md`:
- Line 94: In the configuration table entry for
requiresReasoningPlaceholderModels, correct the Korean spelling by replacing
“따륩며” with “따르며”; leave the rest of the description unchanged.
🪄 Autofix
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: aa34fcdf-1755-48ae-b93b-1ec090af2439
📒 Files selected for processing (14)
docs-site/src/content/docs/ja/reference/configuration/providers.mddocs-site/src/content/docs/ko/reference/configuration/providers.mddocs-site/src/content/docs/reference/configuration/providers.mddocs-site/src/content/docs/ru/reference/configuration/providers.mddocs-site/src/content/docs/zh-cn/reference/configuration/providers.mdsrc/adapters/openai-chat.tssrc/oauth/index.tssrc/oauth/login-cli.tssrc/providers/derive.tssrc/providers/registry.tssrc/router.tssrc/server/auth-cors.tssrc/types.tstests/deepseek-reasoning-replay-gaps.test.ts
There was a problem hiding this comment.
💡 Codex Review
opencodex/src/adapters/openai-chat.ts
Line 340 in 5e01d03
Honor placeholder-only model lists on assistant turns
When a custom or future registry provider sets only requiresReasoningPlaceholderModels for a model that rejects missing reasoning_content, this outer guard still requires the same model to also be listed in preserveReasoningContentModels. In that configuration the main assistant-history path skips both the cache lookup and placeholder injection, so a compacted assistant tool_calls turn is emitted bare and can still 400 upstream; the orphan-repair path below already consults the new field directly, so broaden this guard to enter when either replay preservation or placeholder injection is required.
ℹ️ 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".
Address review findings on lidge-jun#1205: - chatgpt-codex-connector P2: preserveReasoningContentModels only opts models into replaying reasoning that exists; MiniMax-M3 low effort maps to thinking disabled, so a fabricated placeholder could reach non-thinking histories. Add requiresReasoningPlaceholderModels (registry/derive/router/oauth/auth-cors plumbing, docs-site table) defaulting to the preserve list; minimax/minimax-cn seed [] to opt out. Custom preserve-only provider configs keep the lidge-jun#1193 fix via fallback. - CodeRabbit minor: treat a falsy cache hit as a miss in the orphan-repair path (defense-in-depth; the write path already rejects empty strings). Refs lidge-jun#1193
…outs durable Address the remaining review findings on lidge-jun#1205: - chatgpt-codex-connector P2: the orphan-repair fallback checked only requiresReasoningPlaceholderModels, so a requires-only custom entry could fabricate reasoning_content on a path the main assistant history would never emit it on. Gate the orphan placeholder on the preserve list too. - chatgpt-codex-connector P2: Zhipu BigModel GLM ids are thinking-toggle models (low maps to disabled) AND preserve-listed, so the placeholder default could fabricate reasoning for non-thinking histories. Seed requiresReasoningPlaceholderModels: [] for zhipu-bigmodel, matching the MiniMax opt-out. - chatgpt-codex-connector P2: OAuth reconcile deleted an explicit requiresReasoningPlaceholderModels: [] opt-out on every startup because no OAuth preset seeds the field. Keep the field out of OAUTH_RECONCILE_FIELDS; registry seeds still reach existing rows via enrichProviderFromRegistry. - CodeRabbit minor: fix Korean spelling in the providers table. Refs lidge-jun#1193
5e01d03 to
61283a4
Compare
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== current PR head ==='
git rev-parse HEAD
echo '=== changed adapter and test diff ==='
git diff --find-renames origin/dev...HEAD -- \
src/adapters/openai-chat.ts \
tests/deepseek-reasoning-replay-gaps.test.ts
echo '=== adapter: placeholder and preserve-model conditions ==='
rg -n -C 10 \
'requiresReasoningPlaceholderModels|preserveReasoningContentModels|reasoning_content' \
src/adapters/openai-chat.ts
echo '=== focused tests: custom placeholder-only model coverage ==='
rg -n -C 8 \
'requiresReasoningPlaceholderModels|preserveReasoningContentModels|orphan|reasoning_content' \
tests/deepseek-reasoning-replay-gaps.test.tsLength of output: 1133 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== current commit and parent patch for relevant files ==='
git rev-parse HEAD
git diff --find-renames HEAD^ HEAD -- \
src/adapters/openai-chat.ts \
tests/deepseek-reasoning-replay-gaps.test.ts || true
echo '=== adapter outline ==='
ast-grep outline src/adapters/openai-chat.ts --items all --view expanded
echo '=== adapter: replay and orphan placeholder conditions ==='
rg -n -C 12 \
'modelInList\(provider\.(preserveReasoningContentModels|requiresReasoningPlaceholderModels)|reasoning_content|orphan' \
src/adapters/openai-chat.ts
echo '=== test outline ==='
ast-grep outline tests/deepseek-reasoning-replay-gaps.test.ts --items all --view expanded
echo '=== test: placeholder-only model and orphan assertions ==='
rg -n -C 12 \
'placeholder|preserveReasoningContentModels|requiresReasoningPlaceholderModels|orphan|reasoning_content' \
tests/deepseek-reasoning-replay-gaps.test.tsLength of output: 33953
The previous P2 finding is addressed. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
1 participant
Summary
src/responses/reasoning-replay-cache.ts) misses on a long session, the openai-chat adapter used to emit an assistanttool_callcontinuation withoutreasoning_content, which DeepSeek thinking mode rejects with HTTP 400." ") instead of a bare continuation for models inpreserveReasoningContentModels: the main assistant-history path and the orphan-repair path (lost assistant turn). Verified A/B upstream: the same conversation shape with the placeholder returns 200.requiresReasoningPlaceholderModels, which defaults to the preserve list so custom preserve-only configs (the exact opencode-zen setup from [Bug] preserveReasoningContentModels still emits bare tool_call continuations when the replay cache misses (long sessions) — DeepSeek thinking mode 400 #1193) keep the fix. Built-in toggleable-thinking providers opt out with[](minimax,minimax-cn,zhipu-bigmodel) because low effort maps to thinking disabled. The orphan-repair path is gated on the preserve list too, so a requires-only custom entry can never fabricatereasoning_content. Explicit[]opt-outs on OAuth rows survive startup reconcile.Verification
bun run typecheck— green.bun run privacy:scan— green.bun test tests/deepseek-reasoning-replay-gaps.test.ts tests/oauth-provider-reconcile.test.ts tests/provider-registry-parity.test.ts— 57 pass / 0 fail.bun run test): 9079 tests, 12 failures, all in provider-management / post-approval management-seam tests; the same files fail standalone on cleanorigin/devHEAD, so they are pre-existing and unrelated to this change.[]opt-outs, requires-only custom model on the orphan path, OAuth reconcile preserving an explicit[]opt-out, and the encrypted-reasoning test updated to expect the placeholder instead of a bare continuation.Checklist
Scope stays focused and avoids unrelated cleanup.
Docs or release notes were updated when needed (docs-site providers table: new
requiresReasoningPlaceholderModelsrow, en/zh-cn/ja/ko/ru; Korean spelling fixed).Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. NOTE for maintainers: commit 2 adds one-line allowlist entries for the new
requiresReasoningPlaceholderModelsfield insrc/oauth/index.ts,src/oauth/login-cli.ts, andsrc/server/auth-cors.ts(field pass-through only, no auth logic change) — this tripsunsponsored_surfacein the hygiene gate and needs themaintainer-sponsoredlabel after review. Commit 3 removes the OAuth reconcile entry (opt-out durability) and adds no new restricted surfaces.Local CI green (typecheck + focused suite green; the 12 full-suite failures reproduce on
devHEAD and are documented above).Branch is on the latest
devcommit (rebased onto latestorigin/dev).All correct Codex and CodeRabbit findings fixed (CodeRabbit minor + three chatgpt-codex-connector P2s addressed in commits 2-3; all bot review threads resolved).
Ready for review.
Closes #1193
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.