Skip to content

Fix MiniMax split reasoning in Codex#431

Draft
H-H-E wants to merge 1 commit into
lidge-jun:devfrom
H-H-E:agent/minimax-split-reasoning
Draft

Fix MiniMax split reasoning in Codex#431
H-H-E wants to merge 1 commit into
lidge-jun:devfrom
H-H-E:agent/minimax-split-reasoning

Conversation

@H-H-E

@H-H-E H-H-E commented Jul 25, 2026

Copy link
Copy Markdown

What changed

  • rebuilt this one-commit PR on current dev
  • added the generic reasoningSplitModels capability, which sends reasoning_split: true for supported OpenAI-compatible chat models
  • enabled split reasoning plus reasoning-history preservation for MiniMax M-series models
  • mapped MiniMax-M3 low effort to thinking.type=disabled and medium/higher effort to thinking.type=adaptive
  • fixed both streaming and nonstream parsing so reasoning_raw_delta is emitted before final text when one response carries both fields
  • preserved explicit user model-effort mappings and capability fields while registry defaults fill missing configuration

Scope and security

The OAuth, key-login, and safe-config DTO edits only propagate reasoningSplitModels through existing provider-field whitelists. They do not change OAuth behavior, authentication policy, CORS policy, credential handling, or logging.

Regression coverage

  • nonstream and same-frame stream responses now assert reasoning-before-answer ordering
  • empty deltas followed by a finish-only frame remain a clean no-op completion
  • EOF without [DONE], finish_reason, or usage remains fail-closed
  • MiniMax request shape, history replay, both MiniMax registry entries, and explicit user overrides are covered

Validation

  • focused adapter, EOF, MiniMax, reasoning-effort, and provider-registry suites: 106 passed
  • bun run typecheck
  • full bun run test
  • bun run privacy:scan
  • bun run lint:gui

The PR remains a draft pending CI and maintainer review.

@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 44db9026-41ed-4428-9f22-d9e965d9db32

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown

Target branch corrected

This pull request now targets dev.

The [WRONG BRANCH] title prefix has been removed. Its existing draft status has been preserved.

@github-actions github-actions Bot changed the title Fix MiniMax split reasoning in Codex [WRONG BRANCH] Fix MiniMax split reasoning in Codex Jul 25, 2026
@H-H-E
H-H-E force-pushed the agent/minimax-split-reasoning branch from d0275b9 to d25eba5 Compare July 25, 2026 01:54
@H-H-E H-H-E changed the title [WRONG BRANCH] Fix MiniMax split reasoning in Codex Fix MiniMax split reasoning in Codex Jul 25, 2026
@H-H-E
H-H-E changed the base branch from main to dev July 25, 2026 01:55
@Wibias Wibias added the bug Something isn't working label Jul 25, 2026
@lidge-jun

Copy link
Copy Markdown
Owner

🔒 Under maintainer review — detailed feedback incoming

@lidge-jun (maintainer) has this PR in an active review pass. Please do not merge or close
it
until the detailed review lands; a full comment with specific file:line findings,
failure modes, and suggested fixes is being prepared.

This PR is not part of the current integration batch. It is held back deliberately —
either because it touches a security boundary that MAINTAINERS.md requires an explicit
security review for, because it needs to be split into reviewable units, or because it needs
a rebase onto changes landing in dev right now.

Heads-up on rebasing: dev is receiving a coordinated integration batch today. If this PR
touches the same files, expect to rebase after that batch lands. The upcoming review comment
will name the exact overlapping paths so you only have to do it once.

No action needed from you until then. Thanks for the contribution and for your patience.

Review tracker: devlog/_plan/260725_pr_issue_rework · marker posted by the maintainer review pass

@lidge-jun

Copy link
Copy Markdown
Owner

Thank you for keeping this as a draft while adding MiniMax's split-reasoning request capability. Before marking it ready for review, please fix the response-ordering regression and tighten the scope/test contract.

src/adapters/openai-chat.ts:784-790 emits text_delta before reasoning_raw_delta when a non-streaming MiniMax response contains both content and reasoning_content, which is exactly the split response shape described in this PR. The bridge closes the text item when the later reasoning event arrives (src/bridge.ts:470-542), so the final answer is materialized before the reasoning item instead of reasoning preceding the answer. The streaming path has the same ordering when both fields share one delta at src/adapters/openai-chat.ts:683-690. Please emit non-empty reasoning before final content for both paths and add parser-level regressions for (1) a non-stream response containing both fields, (2) a stream frame containing both fields, (3) empty deltas followed by a finish-only frame, and (4) EOF without [DONE], finish_reason, or usage. The last two should preserve the existing no-op and fail-closed behavior rather than treating an interrupted stream as complete.

The current tests/minimax-reasoning-split.test.ts:38-95 only calls buildRequest; it proves reasoning_split, effort mapping, and history serialization, but never executes parseStream or parseResponse, so it cannot catch the response-ordering failure promised by the title. Please make the regression assert emitted adapter-event order, not only request JSON.

I traced the one-line additions in src/oauth/index.ts:492-499, src/oauth/login-cli.ts:86-93, and src/server/auth-cors.ts:313-323. They carry reasoningSplitModels through existing provider-config reconciliation, key-login construction, and the safe dashboard DTO; I do not see a new OAuth flow or CORS-origin policy in those hunks. Please explain that propagation in the PR scope and keep it to those whitelist paths, because OAuth/config serialization remains a security-sensitive boundary even when the value itself is only a model-id list.

Although this pinned patch currently passes git apply --check, it must be rebased before it becomes ready: src/providers/registry.ts overlaps the planned #385/WP7 integration, src/router.ts overlaps #389/WP5, and src/types.ts overlaps #439/WP3. After rebasing, verify that registry enrichment still preserves explicit user provider fields, run the focused MiniMax parser/request tests, and run the full typecheck and test suite. Thanks again for addressing the visible <think> leakage; the draft needs response-side proof and the integrated provider contracts before full review.

Current dev has now integrated #385 in src/providers/registry.ts, #389 in src/router.ts, and #439 in src/types.ts. Please rebase the draft against those completed contracts and preserve explicit user provider fields.


Reviewed as part of a maintainer integration pass. dev moved to ebc62d1f today (7 PRs integrated); Cross-platform CI is green on ubuntu, macOS, and Windows.

@H-H-E
H-H-E force-pushed the agent/minimax-split-reasoning branch from d25eba5 to 9568cb8 Compare July 25, 2026 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants