Skip to content

fix(core): retry requests when providers require thinking - #7534

Merged
wenshao merged 8 commits into
QwenLM:mainfrom
yiliang114:cx/fix-btw-thinking-capability-retry
Jul 23, 2026
Merged

fix(core): retry requests when providers require thinking#7534
wenshao merged 8 commits into
QwenLM:mainfrom
yiliang114:cx/fix-btw-thinking-capability-retry

Conversation

@yiliang114

Copy link
Copy Markdown
Collaborator

What this PR does

This change retries an OpenAI-compatible request once when the actual wire request sent enable_thinking: false and the provider returns HTTP 400 explicitly requiring it to be true. The retry rebuilds the request through the existing pipeline and remembers the model capability for the lifetime of that provider pipeline. Streaming and non-streaming requests share the same behavior; unrelated 400 responses and cancelled requests are unchanged.

Why it's needed

#7303 added a config-driven thinkingMandatory capability for the current qwen3.8 Token Plan preset. Existing and custom provider configurations may still contain extra_body.enable_thinking: true without that capability field, so /btw can override it with false and reproduce the original 400 on 0.20.1.

Learning from the provider's explicit capability response closes that gap without adding another Token Plan or model-name special case.

Reviewer Test Plan

How to verify

Use an existing or custom Token Plan model configuration that enables thinking but does not include thinkingMandatory. The first /btw request should recover from the required-thinking 400 and complete; the next /btw request for the same model should send thinking enabled immediately. Confirm that an unrelated 400 is not retried and cancelling before the provider response does not start a retry.

Evidence (Before & After)

Before, a real Token Plan request on 0.20.1 failed with:

400 The value of the enable_thinking parameter is restricted to True.

After, a real interactive TUI and OpenAI SDK protocol-boundary test produced:

  • First /btw: false → required-thinking 400 → true → BTW_RETRY_OK
  • Next /btw: true → BTW_CACHE_OK
  • Unrelated 400: one false request, no retry
  • Escape cancellation: one false request, no retry

Tested on

OS Status
🍏 macOS
🪟 Windows N/A
🐧 Linux N/A

Environment (optional)

Local TypeScript workspace using the real interactive TUI, OpenAI SDK, and a local HTTP/SSE protocol-boundary provider.

Risk & Scope

  • Main risk or tradeoff: One additional request is made only after the provider rejects the original request during parameter validation. The learned capability is process-local.
  • Not validated / out of scope: A post-fix live Token Plan request could not be completed because the current credentials return 401; the original live failure and the full post-fix protocol boundary were both verified.
  • Breaking changes / migration notes: None.

Linked Issues

Follow-up to #7284

Related to #7440 and #7303

中文说明

本 PR 做了什么

当实际发送的 OpenAI 兼容请求包含 enable_thinking: false,且 provider 返回 HTTP 400 并明确要求该参数必须为 true 时,本改动会重试一次。重试会通过现有 pipeline 重新构造请求,并在当前 provider pipeline 的生命周期内记住该模型能力。流式和非流式请求共用相同行为;无关的 400 响应和已取消请求保持不变。

为什么需要

#7303 为当前 qwen3.8 Token Plan preset 增加了配置驱动的 thinkingMandatory 能力。已有或自定义 provider 配置可能仍然只有 extra_body.enable_thinking: true,而没有这个能力字段,因此 /btw 仍会把它覆盖为 false,并在 0.20.1 上复现原始 400。

根据 provider 明确返回的能力约束进行学习,可以覆盖这个缺口,同时不需要继续增加 Token Plan 或模型名称特判。

Reviewer Test Plan

如何验证

使用一个开启 thinking、但没有 thinkingMandatory 的已有或自定义 Token Plan 模型配置。第一次 /btw 请求应从 required-thinking 400 中恢复并完成;同一模型的下一次 /btw 应直接开启 thinking。同时确认无关 400 不会触发重试,并且在 provider 响应前取消请求不会启动重试。

证据(修改前后)

修改前,0.20.1 上的真实 Token Plan 请求失败:

400 The value of the enable_thinking parameter is restricted to True.

修改后,真实交互式 TUI 与 OpenAI SDK 协议边界测试结果:

  • 第一次 /btwfalse → required-thinking 400 → true → BTW_RETRY_OK
  • 下一次 /btwtrue → BTW_CACHE_OK
  • 无关 400:仅一个 false 请求,不重试
  • Escape 取消:仅一个 false 请求,不重试

测试平台

OS Status
🍏 macOS
🪟 Windows N/A
🐧 Linux N/A

环境(可选)

本地 TypeScript 工作区,使用真实交互式 TUI、OpenAI SDK 和本地 HTTP/SSE 协议边界 provider。

风险与范围

  • 主要风险或权衡:只有 provider 在参数校验阶段拒绝原请求后,才会额外发送一次请求。学习到的能力仅在当前进程内有效。
  • 未验证或超出范围:由于当前凭据返回 401,无法完成修复后的真实 Token Plan 请求;原始线上失败和修复后的完整协议边界均已验证。
  • 破坏性变更或迁移说明:无。

关联 Issue

Follow-up to #7284

Related to #7440 and #7303

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: observed bug with solid evidence. Linked P1 issues #7284 and #7440 both document the enable_thinking: false → 400 failure on Token Plan endpoints. The PR description includes a concrete before/after reproduction (400 The value of the enable_thinking parameter is restricted to True. → retry → success). This is a real user-facing regression on 0.20.1 for existing/custom provider configs that lack the thinkingMandatory capability field.

Direction: aligned. The "learn from the provider's explicit error" approach is the right direction — it closes the gap for configs that predate #7303's thinkingMandatory without adding more model-name special cases or requiring users to update their configs. CHANGELOG reference: Claude Code recently fixed a similar class of issue (thinking.disabled.display: Extra inputs are not permitted 400 errors on subagent spawns), confirming that thinking-parameter negotiation with providers is an active pain point.

Size: ~59 production lines (pipeline.ts) + ~177 test lines (pipeline.test.ts). Well under thresholds — no maintainer escalation needed.

Approach: the scope feels right. Retry once on a specific 400 pattern, remember the capability per-pipeline, and let the existing buildRequest pipeline rebuild the request with thinking enabled. The abort-signal guard and the unrelated-400 passthrough are both necessary and correctly scoped. No drive-by changes or scope creep.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:已观测到的 bug,证据充分。关联的 P1 issue #7284#7440 都记录了 enable_thinking: false → 400 的 Token Plan 端点故障。PR 描述包含具体的 before/after 复现。这是 0.20.1 上针对缺少 thinkingMandatory 能力字段的已有/自定义 provider 配置的真实用户回归。

方向:对齐。"从 provider 的明确错误中学习"是正确方向——为 #7303 之前的配置弥补缺口,无需增加更多模型名特判或要求用户更新配置。

规模:约 59 行生产代码 + 约 177 行测试代码,远低于阈值。

方案:范围合理。对特定 400 模式重试一次,按 pipeline 记住能力,让现有 buildRequest 管线重建请求。abort 信号守卫和无关 400 透传都必要且范围正确。

进入代码审查 🔍

Qwen Code · qwen3.8-max-preview

Reviewed at 8b212176600279ff1947e517a156c3b10a534d73 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal: I'd catch the specific 400 about enable_thinking in executeWithErrorHandling, cache the model's thinking requirement per-pipeline, and retry once through the existing buildRequest so the rebuilt request picks up the learned capability. Guard against retrying unrelated 400s and cancelled requests.

Comparison: the PR does exactly this, and does it cleanly. A few observations:

  • isRequiredThinkingError reuses getErrorStatus, getErrorMessage, and getRateLimitErrorDetails (which despite its name extracts generic provider error fields). The detection — status 400 + message contains enable_thinking + matches /(?:restricted to|must be) true/i — is specific enough to avoid false positives on unrelated 400s.
  • The requiredThinkingModels Set is per-pipeline and process-local, which is the right scope: it survives across requests in a session but doesn't leak into config or persist across restarts.
  • The requiresThinking method cleanly replaces the old inline thinkingMandatory computation and adds the learned capability on top. The buildRequest call site is a one-liner now.
  • The retry guard checks three conditions: we actually sent enable_thinking: false, the request wasn't cancelled, and the error matches the pattern. All three are necessary.
  • The executeAttempt closure is a minimal restructuring — buildRequest + capture + diagnostics + executor, same as before, just extracted so the retry can call it again.
  • Tests cover the full matrix: retry + cache hit on second call, unrelated 400 passthrough, and streaming retry. The test assertions verify both the wire request shape (enable_thinking, tool_choice) and that errorHandler.handle is not called on successful retry.

No critical blockers. No convention violations.

Real-Scenario Testing

This fix targets a specific provider error path (Token Plan endpoints returning 400 The value of the enable_thinking parameter is restricted to True). Reproducing it requires a Token Plan configuration without thinkingMandatory — the PR author notes their live credentials return 401, so a full live before/after wasn't possible there either.

What I verified:

Unit tests — all 114 pipeline tests pass (including the 3 new ones):

 ✓ src/core/openaiContentGenerator/pipeline.test.ts (114 tests) 163ms

 Test Files  1 passed (1)
      Tests  114 passed (114)
   Duration  4.95s

Smoke test — CLI starts and processes a query normally with the PR changes (no regression):

$ npm run dev -- -p 'say hello in one word' --output-format text

> @qwen-code/qwen-code@0.20.1 dev
> node scripts/dev.js -p say hello in one word --output-format text

Hello

The retry behavior itself is thoroughly covered by the unit tests, which mock the exact provider error and verify the full retry → cache → subsequent-request flow.

中文说明

代码审查

独立方案:executeWithErrorHandling 中捕获 enable_thinking 相关的特定 400 错误,按 pipeline 缓存模型的 thinking 需求,通过现有 buildRequest 重试一次。防止重试无关 400 和已取消请求。

对比: PR 完全按此思路实现,且很干净。isRequiredThinkingError 复用现有工具函数,检测条件(400 + enable_thinking + restricted to/must be true)足够具体。requiredThinkingModels Set 作用域正确(per-pipeline、进程内)。重试守卫检查三个必要条件。测试覆盖完整矩阵。无关键阻塞项,无规范违反。

真实场景测试

此修复针对特定 provider 错误路径。复现需要没有 thinkingMandatory 的 Token Plan 配置——PR 作者也指出其线上凭据返回 401。已验证:全部 114 个管线测试通过(含 3 个新增),CLI 冒烟测试正常。重试行为由单元测试充分覆盖。

Qwen Code · qwen3.8-max-preview

Reviewed at 8b212176600279ff1947e517a156c3b10a534d73 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean, minimal fix for a real P1 bug; only reservation is the inability to do a live before/after (401 credentials), but the unit tests cover the protocol boundary comprehensively.

This is a well-scoped follow-up to #7284. The problem is real — two P1 issues document users hitting 400 The value of the enable_thinking parameter is restricted to True on Token Plan endpoints when /btw sends enable_thinking: false. The solution learns from the provider's explicit error instead of adding more model-name special cases, which is the right direction.

The implementation matches my independent proposal almost exactly: detect the specific 400, cache the capability per-pipeline, retry once through the existing buildRequest. Every line in the diff serves the stated goal — no drive-by changes, no scope creep. The three new tests cover the full matrix (retry + cache, unrelated 400, streaming), and all 114 pipeline tests pass.

If I had to maintain this in six months, I'd thank the author — the error detection is specific, the cache scope is right, and the tests document the behavior clearly.

中文说明

置信度:4/5 — 干净、最小化的修复,针对真实 P1 bug;唯一保留是无法进行线上 before/after 测试(401 凭据),但单元测试全面覆盖了协议边界。

这是 #7284 的良好后续。问题真实——两个 P1 issue 记录了用户在 Token Plan 端点遇到 enable_thinking 400 错误。方案从 provider 的明确错误中学习,而非增加更多模型名特判,方向正确。实现与我的独立方案几乎完全一致,diff 中每一行都服务于目标,无顺手改动。三个新测试覆盖完整矩阵,全部 114 个管线测试通过。

Qwen Code · qwen3.8-max-preview

Reviewed at 8b212176600279ff1947e517a156c3b10a534d73 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/core/openaiContentGenerator/pipeline.ts
Comment thread packages/core/src/core/openaiContentGenerator/pipeline.ts
@yiliang114

Copy link
Copy Markdown
Collaborator Author

Handled the required-thinking retry-failure test in ae5046e. I did not add a retried marker to OpenAI wire diagnostics because that changes the diagnostics record shape and consumer contract, which is outside this focused bug-fix closeout. Verification: npx vitest run src/core/openaiContentGenerator/pipeline.test.ts; npm run lint -- src/core/openaiContentGenerator/pipeline.test.ts.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/core/openaiContentGenerator/pipeline.ts
Comment thread packages/core/src/core/openaiContentGenerator/pipeline.ts Outdated

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Approve to Comment: CI failing: Test (ubuntu-latest, Node 22.x). Reviewed.

— qwen3.7-max via Qwen Code /review

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/core/openaiContentGenerator/pipeline.ts
@gwinthis

Copy link
Copy Markdown
Collaborator

Review & Local Verification Report

代码审查

设计评价:精巧实用。 本 PR 解决了 provider 要求 enable_thinking: true/btw 将其覆盖为 false 导致 400 的问题。核心策略是"从 provider 错误中学习"——检测到 required-thinking 400 后自动重试一次,并在 pipeline 生命周期内记住该模型能力。

关键变更(仅 2 个文件,+331/-9):

  1. isRequiredThinkingError():精准匹配 400 + 错误消息包含 enable_thinking + 正则 /(?:restricted to|must be) true\b/i。避免误匹配无关 400。
  2. requiredThinkingModels Set:进程级缓存,学习到的能力在 pipeline 生命周期内有效。后续请求直接走 requiresThinking() 判断,不再触发 400。
  3. requiresThinking() 方法:统一了配置驱动的 thinkingMandatory 和运行时学习到的能力,消除了原先 configModel 大小写比较的重复逻辑。
  4. executeWithRetry 重构:将 buildRequest + executor 提取为 executeAttempt() 闭包,retry 时重新构建请求(此时 requiresThinking() 返回 true,thinking 不会被禁用)。
  5. 流式/非流式共用:retry 逻辑在 executeWithRetry 层,stream 和 non-stream 共享同一路径。

亮点:

  • 检查 abortSignal?.aborted !== true,取消的请求不触发 retry
  • 同时处理 enable_thinking(DashScope)和 chat_template_kwargs.enable_thinking(非 DashScope)两种 wire 格式
  • retry 失败时走正常 handleError 路径,不吞错误
  • 测试覆盖 5 个场景:正常 retry、非 DashScope 格式、retry 失败、无关 400 不 retry、stream retry

建议(非阻塞):

  • isRequiredThinkingError 的正则 /(?:restricted to|must be) true\b/i 目前覆盖了已知 provider 消息格式,若未来有新 provider 使用不同措辞(如 "should be true"),可能需要扩展。当前范围合理,无需过度设计。

本地测试验证

测试项 结果
pipeline.test.ts (116 tests) ✅ 全部通过
tsc --noEmit (core) ✅ 无类型错误

tmux E2E 验证

在 tmux 会话中以 npm run dev -- --prompt 'say hi' --output-format text 启动 CLI:

  • ✅ CLI 正常启动并响应 "Hi! How can I help you today?"
  • ✅ 无运行时错误或异常日志

结论

LGTM。 改动精准(仅 2 文件),策略优雅(从 provider 错误中学习而非硬编码模型名),测试覆盖完整。解决了 #7284 的后续问题,不引入新的特判。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

— qwen3.7-max via Qwen Code /review

doudouOUC
doudouOUC previously approved these changes Jul 23, 2026

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestions are inline.

— qwen3.8-max-preview via Qwen Code /review

Comment thread packages/core/src/core/openaiContentGenerator/pipeline.ts
Comment thread packages/core/src/core/openaiContentGenerator/pipeline.ts
Comment thread packages/core/src/core/openaiContentGenerator/pipeline.ts Outdated

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Review — reviewed at c74792c

Verdict: LGTM with follow-ups. The mechanism is correct and I verified it end-to-end at the real protocol boundary, not just at the SDK mock. Everything below is Suggestion-level; nothing blocks the merge.

Verification

I built a throwaway protocol-boundary suite (real openai SDK, real converter, real DashScope/Default provider, real local HTTP/SSE server — no vi.mock) and A/B'd it against the merge-base 32c491f:

Scenario base 32c491f this PR
DashScope 400 → recover, and remember for the next call
Streaming: 400 at stream creation → recover
Non-DashScope chat_template_kwargs opt-out rejected → recover
Unrelated real 400 → no retry

So the real SDK's APIError shape does flow through getErrorStatus/getErrorMessage correctly, and the retried wire body really does carry thinking enabled. Also clean: prettier --check, tsc --noEmit on packages/core, and 681/681 in src/core/openaiContentGenerator.

Two structural properties I checked and can confirm are safe:

  • The retry cannot loop. After learning, buildRequest never re-emits enable_thinking: false, so the wire-shape guard can't fire a second time.
  • Reusing one context across both attempts is safe. toolCallParser / textDeltaState / reasoningDeltaState are only touched once chunks arrive, and the streaming failure happens at creation time — before the generator is returned. provider.buildRequest and the converter are pure, so rebuilding is side-effect free, and openaiRequestCaptureContext is single-slot, so the OpenAI logger records the request that actually succeeded.

Worth calling out separately: dropping the isDashScope gate on the if (thinkingMandatory) block is a real latent fix on its own. On base, a config-driven thinkingMandatory model pointed at a non-DashScope endpoint kept extra_body.enable_thinking: false on the wire, because the !thinkingMandatory guard in the reasoning-disabled branch skipped the delete.

Suggestions

1. The learned capability is never rolled back when the retry doesn't prove the hypothesis.

requiredThinkingModels.add(model) runs before the retry and is never removed if the retry fails. Confirmed at the protocol boundary: required-thinking 400 → learn → retry fails with an unrelated 400 → the next healthy request for that model still ships enable_thinking: true. Because thinkingMandatory deliberately overrides both the per-request thinkingConfig.includeThoughts: false and the config-level reasoning: false, one mis-parsed 400 silently turns thinking on for the rest of that pipeline's life — with no way for the user to turn it back off. Cheap fix: this.requiredThinkingModels.delete(model) in the retry's catch, unless the retry error is itself a required-thinking error.

2. "Process-local" is optimistic — it's per content generator, and a session has several.

ContentGenerationPipeline is constructed once per OpenAIContentGenerator, and generators are created in at least three places: the session's own (config.ts), baseLlmClient.perModelGeneratorCache (one per side-query model), and each subagent via createRuntimeContentGeneratorView. So the wasted 400 is paid once per generator, not once per process. If you want the PR-body claim to hold, hoisting the Set to a module-level Map keyed by `${baseUrl}::${model}` would do it — though per-pipeline is a defensible conservative choice if you'd rather not share learned state across auth contexts.

3. Test gaps — 5 of 13 mutants I planted survive the suite.

Killed (good): the abort guard, requiredThinkingModels.add, the chat_template_kwargs strip, the regex, restoring && isDashScope on the outer block, the retry call itself, the cache key, and the config-driven thinkingMandatory fallback.

Survived — all 119 tests still pass with each of these applied individually:

# Mutation Result
M2 delete the wireRequest['enable_thinking'] === false || chatTemplateKwargs?.[...] === false guard 119/119 pass
M3 delete message.includes('enable_thinking') 119/119 pass
M4 delete the getErrorStatus(error) !== 400 check 119/119 pass
M7 delete isDashScope && on the tool_choice strip 119/119 pass
M11 drop getRateLimitErrorDetails(error).providerMessage from the matched string 119/119 pass

M11 is the one I'd most want covered: the provider-payload half of the matched string is the branch that exists precisely for providers whose error.message is generic, and no test ever exercises it. Two cheap tests close most of the table — a 500 carrying the required-thinking text (must not retry, kills M4), and an error whose text lives only in error.error.message (must retry, kills M11).

4. The match is narrow enough to miss common phrasings.

/(?:restricted to|must be) true\b/ does not match "must be set to true", "only supports true", or a Chinese-locale message — all plausible from the same family of gateways. Since the whole feature keys off provider prose, consider widening to must be (?:set to )?true and/or also matching on the structured code / param fields (DashScope populates param).

Notes (no action needed)

  • The DashScope retry silently drops tool_choice: 'required'. Verified on the wire: attempt 1 carries it, the retry doesn't. For structured side-queries (the AUTO-mode classifier's respond_in_schema) the retry therefore runs with thinking forced on and the tool call no longer forced — exactly the combination the comment in buildRequest warns about. It's not a regression (on base that request just 400s), and it's the same tradeoff fix(core): support qwen3.8 side queries on DashScope #7303 already accepted; but learning makes it reachable for arbitrary models, so it's worth a line of comment at the strip site.
  • SSE-embedded errors bypass the retry. isRequiredThinkingError goes through getRateLimitErrorDetails, which understands the event:error / HTTP_STATUS/ transports — but an error raised inside processStreamWithLogging (HTTP 200 + error frame) never reaches the new catch, since the executor has already returned the generator. Fine for DashScope, which returns a real HTTP 400; just means that half of the message extraction is unreachable on the streaming path.
  • debugLogger.warn is a no-op unless a debug log session is active, so silently re-enabling thinking — which changes latency and token cost for every /btw and classifier call afterwards — leaves no trace a user or oncall would see. A telemetry counter would be more useful than the warn.
中文版

评审 —— 基于 c74792c

结论:LGTM,附带若干后续项。 机制正确,我在真实协议边界(而非 SDK mock 层)做了端到端验证。以下全部是 Suggestion 级别,不阻塞合入。

验证

我写了一套一次性的协议边界测试(真实 openai SDK、真实 converter、真实 DashScope/Default provider、真实本地 HTTP/SSE server,不使用 vi.mock),并与 merge-base 32c491f 做 A/B:

场景 base 32c491f 本 PR
DashScope 400 → 恢复,且下一次直接命中缓存
流式:建流阶段 400 → 恢复
非 DashScope:chat_template_kwargs 关闭项被拒 → 恢复
无关的真实 400 → 不重试

也就是说真实 SDK 的 APIError 形状确实能被 getErrorStatus/getErrorMessage 正确识别,重试的 wire body 也确实带上了 thinking。另外 prettier --checkpackages/coretsc --noEmit、以及 src/core/openaiContentGenerator 的 681/681 测试均通过。

两个我确认安全的结构性性质:

  • 重试不会成环。 学习之后 buildRequest 不再发出 enable_thinking: false,wire-shape 守卫无法第二次命中。
  • 两次尝试复用同一个 context 是安全的。 toolCallParser / textDeltaState / reasoningDeltaState 只在收到 chunk 之后才被写入,而流式失败发生在建流阶段——即 generator 返回之前。provider.buildRequest 与 converter 均为纯函数,重建无副作用;openaiRequestCaptureContext 是单槽覆盖,因此 OpenAI logger 记录的是真正成功的那次请求。

单独值得一提:把 if (thinkingMandatory) 上的 isDashScope 门去掉,本身就修了一个潜伏问题。在 base 上,配置了 thinkingMandatory 但指向非 DashScope 端点的模型,extra_body.enable_thinking: false 仍会上到 wire,因为 reasoning-disabled 分支里的 !thinkingMandatory 守卫跳过了那个 delete

建议

1. 重试没有证实假设时,学习到的能力不会回滚。

requiredThinkingModels.add(model) 在重试之前执行,且重试失败时不会被移除。协议边界已确认:required-thinking 400 → 学习 → 重试因无关的 400 失败 → 该模型下一次正常请求仍然携带 enable_thinking: true。由于 thinkingMandatory 会刻意覆盖 per-request 的 thinkingConfig.includeThoughts: false 与 config 级的 reasoning: false,一次误判的 400 就会在该 pipeline 的整个生命周期里静默打开 thinking,用户无法关闭。低成本修法:在重试的 catchthis.requiredThinkingModels.delete(model),除非重试错误本身也是 required-thinking 错误。

2. “进程内”这个说法偏乐观——实际是 per content generator,而一次会话有多个。

ContentGenerationPipelineOpenAIContentGenerator 构造一次,而 generator 至少在三处创建:会话自身(config.ts)、baseLlmClient.perModelGeneratorCache(每个 side-query 模型一个)、以及每个 subagent 的 createRuntimeContentGeneratorView。因此那次浪费的 400 是每个 generator 付一次,而不是每进程一次。若希望 PR 描述里的说法成立,把 Set 提升为模块级、以 `${baseUrl}::${model}` 为键的 Map 即可——当然,如果不希望学习状态跨鉴权上下文共享,保持 per-pipeline 也是合理的保守选择。

3. 测试缺口——我植入的 13 个变异里有 5 个存活。

被杀掉(覆盖良好):abort 守卫、requiredThinkingModels.addchat_template_kwargs 剥离、正则、给外层块恢复 && isDashScope、重试调用本身、缓存键、以及配置驱动的 thinkingMandatory 兜底。

存活——以下每一项单独施加后,119 个测试仍全绿:

# 变异 结果
M2 删除 wireRequest['enable_thinking'] === false || chatTemplateKwargs?.[...] === false 守卫 119/119 通过
M3 删除 message.includes('enable_thinking') 119/119 通过
M4 删除 getErrorStatus(error) !== 400 判断 119/119 通过
M7 删除 tool_choice 剥离上的 isDashScope && 119/119 通过
M11 从匹配串中去掉 getRateLimitErrorDetails(error).providerMessage 119/119 通过

其中我最希望补上的是 M11:匹配串里来自 provider payload 的那一半,正是为“error.message 很笼统”的 provider 准备的分支,而没有任何测试走到它。两个廉价用例能覆盖表中大部分——一个携带 required-thinking 文案的 500(必须重试,杀 M4),以及一个文案只存在于 error.error.message 的错误(必须重试,杀 M11)。

4. 匹配过窄,会漏掉常见措辞。

/(?:restricted to|must be) true\b/ 匹配不到 “must be set to true”、“only supports true”,也匹配不到中文 locale 的报错——这些都可能来自同一族网关。既然整个特性依赖 provider 的自然语言文案,建议放宽为 must be (?:set to )?true,并/或同时匹配结构化的 code / param 字段(DashScope 会填 param)。

备注(无需处理)

  • DashScope 重试会静默丢掉 tool_choice: 'required' wire 上已验证:第一次带、重试不带。对结构化 side-query(AUTO 模式分类器的 respond_in_schema)而言,重试就变成了 thinking 强制开启 + 工具调用不再强制——恰好是 buildRequest 注释里警告的组合。这不是回归(base 上该请求直接 400),且与 fix(core): support qwen3.8 side queries on DashScope #7303 已接受的权衡一致;但“学习”让它对任意模型都可达,因此在剥离处补一行注释是值得的。
  • SSE 内嵌错误绕过了重试。 isRequiredThinkingErrorgetRateLimitErrorDetails,后者能识别 event:error / HTTP_STATUS/ 传输形态——但 processStreamWithLogging 内抛出的错误(HTTP 200 + error 帧)永远到不了新的 catch,因为 executor 早已返回了 generator。对返回真实 HTTP 400 的 DashScope 无影响,只是意味着流式路径上那半段消息提取不可达。
  • debugLogger.warn 在没有 debug log session 时是 no-op,因此静默重新打开 thinking(此后每次 /btw 和分类器调用的延迟与 token 成本都会变)不会留下用户或 oncall 能看到的痕迹。相比 warn,一个 telemetry 计数器更有用。

@wenshao

wenshao commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

🔬 Maintainer E2E Verification Report

Tested by: maintainer (local macOS, real interactive TUI)
Branch: cx/fix-btw-thinking-capability-retry @ 2980ad2
Date: 2026-07-23


1. Unit Tests

✓ src/core/openaiContentGenerator/pipeline.test.ts (119 tests) 58ms

Test Files  1 passed (1)
     Tests  119 passed (119)

All 119 tests pass, including the 7 new test cases covering:

  • Required-thinking retry (non-streaming + streaming)
  • Capability caching across requests
  • chat_template_kwargs cleanup on retry
  • Retry error propagation
  • Abort signal preventing retry
  • Non-required-thinking 400 errors not retried

2. Build / TypeCheck / Lint

Check Result
npm run build ✅ passed
npm run typecheck ✅ passed
eslint (changed files) ✅ 0 errors

3. E2E Test — Real Interactive TUI + Mock Provider

Set up a local mock OpenAI-compatible provider that rejects enable_thinking: false with HTTP 400 (simulating DashScope/Token Plan behavior), then ran the real interactive TUI against it.

Setup:

  • Mock provider: http://localhost:18977/v1 (returns 400 when enable_thinking is explicitly false)
  • CLI flags: --auth-type openai --model qwen3-test-model --openai-base-url http://localhost:18977/v1
  • Trigger: /btw command (sets thinkingConfig.includeThoughts = false)

Test A: First /btw — retry recovery ✅

The first /btw What is 2+2? triggered the full retry flow:

Request chat_template_kwargs Response
#1 (initial) { enable_thinking: false } 400 — restricted to True
#2 (auto-retry) (removed) 200 SSE stream ✅

The TUI displayed the successful response: "Hello! Thinking retry works."

/btw result

Test B: Second /btw — capability cached ✅

The second /btw What is 3+3? sent only 1 request (no retry):

Request chat_template_kwargs Response
#3 (none — cached) 200 SSE stream ✅

The model was remembered in requiredThinkingModels from the first retry.

Mock Provider Request Log

Server log

4. Test Summary

Summary

Test Result
Unit tests (119) ✅ all pass
TypeScript typecheck ✅ pass
ESLint ✅ 0 errors
E2E: /btw #1 retry recovery false → 400 → retry → 200
E2E: /btw #2 capability cache ✅ direct 200, no retry
E2E: non-DashScope chat_template_kwargs path ✅ verified

5. Conclusion

The retry mechanism works correctly end-to-end in the real interactive TUI. The implementation is clean: one additional request only when the provider explicitly rejects enable_thinking: false, with process-local capability caching for subsequent requests. No regressions observed in the full test suite.

Verdict: ✅ Ready to merge (from a testing perspective)


中文版本

🔬 维护者 E2E 验证报告

测试人: 维护者(本地 macOS,真实交互式 TUI)
分支: cx/fix-btw-thinking-capability-retry @ 2980ad2
日期: 2026-07-23


1. 单元测试

✓ src/core/openaiContentGenerator/pipeline.test.ts (119 tests) 58ms

Test Files  1 passed (1)
     Tests  119 passed (119)

全部 119 个测试通过,包含 7 个新增测试用例,覆盖:

  • Required-thinking 重试(非流式 + 流式)
  • 跨请求的能力缓存
  • 重试时 chat_template_kwargs 清理
  • 重试错误传播
  • Abort signal 阻止重试
  • 非 required-thinking 的 400 错误不触发重试

2. 构建 / 类型检查 / Lint

检查项 结果
npm run build ✅ 通过
npm run typecheck ✅ 通过
eslint(变更文件) ✅ 0 错误

3. E2E 测试 — 真实交互式 TUI + Mock Provider

搭建本地 mock OpenAI 兼容 provider,拒绝 enable_thinking: false 并返回 HTTP 400(模拟 DashScope/Token Plan 行为),然后运行真实交互式 TUI 进行测试。

配置:

  • Mock provider:http://localhost:18977/v1(当 enable_thinking 显式为 false 时返回 400)
  • CLI 参数:--auth-type openai --model qwen3-test-model --openai-base-url http://localhost:18977/v1
  • 触发方式:/btw 命令(设置 thinkingConfig.includeThoughts = false

测试 A:第一次 /btw — 重试恢复 ✅

第一次 /btw What is 2+2? 触发了完整的重试流程:

请求 chat_template_kwargs 响应
#1(初始) { enable_thinking: false } 400 — restricted to True
#2(自动重试) (已移除) 200 SSE stream ✅

TUI 显示了成功响应:"Hello! Thinking retry works."

测试 B:第二次 /btw — 能力缓存 ✅

第二次 /btw What is 3+3? 仅发送 1 个请求(无需重试):

请求 chat_template_kwargs 响应
#3 (无 — 已缓存) 200 SSE stream ✅

模型在第一次重试后已被记录到 requiredThinkingModels 中。

4. 测试总结

测试项 结果
单元测试 (119) ✅ 全部通过
TypeScript 类型检查 ✅ 通过
ESLint ✅ 0 错误
E2E:/btw #1 重试恢复 false → 400 → retry → 200
E2E:/btw #2 能力缓存 ✅ 直接 200,无需重试
E2E:非 DashScope chat_template_kwargs 路径 ✅ 已验证

5. 结论

重试机制在真实交互式 TUI 中端到端工作正常。实现简洁:仅在 provider 明确拒绝 enable_thinking: false 时额外发送一次请求,并在进程内缓存模型能力供后续请求使用。完整测试套件未观察到回归。

结论:✅ 可以合入(从测试角度)

@wenshao
wenshao added this pull request to the merge queue Jul 23, 2026
Merged via the queue into QwenLM:main with commit 819cd4a Jul 23, 2026
39 checks passed
@yiliang114

Copy link
Copy Markdown
Collaborator Author

Verified the current head end-to-end with the local Qwen Code TUI in tmux and an OpenAI-compatible mock endpoint.

  • First turn: the required-thinking 400 triggers one retry. Both enable_thinking: false values are removed while apply_chat_template: true is preserved.
  • Second turn in the same session: exactly one request, with both opt-outs still absent.
  • Unrelated 400 in a fresh session: one request, no automatic retry.
  • Escape cancellation in a fresh session: the client disconnects, with no retry after the delayed error window.

pipeline.test.ts also passes 119/119.

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full review at c74792c. No issues found. LGTM! ✅

Verification

  • Unit tests (local, at HEAD): packages/core pipeline suite — 119/119 passed
  • Typecheck (tsc --noEmit, core): 0 errors
  • CI: all checks green; mergeable
  • All 8 prior review threads: resolved, and each fix verified present in HEAD code (7 fixed, 1 reasonably declined as out of scope — the retried diagnostics marker)

Key points checked (all evidence-based against HEAD)

  1. Non-DashScope thinkingMandatory block expansion — no regression: the new block only strips an existing enable_thinking: false (top-level or chat_template_kwargs), and tool_choice deletion stays DashScope-gated. Semantics match "a mandatory-thinking model must never see a disabled shape on the wire".
  2. Model-key normalization — the learned Set writes context.model.toLowerCase() and reads via the same source and normalization in buildRequestrequiresThinking; no case-mismatch dead path.
  3. Streaming error timing — the SDK rejects HTTP 400 at withResponse() before iteration, inside the executor, so the retry catches it. Mid-stream SSE error chunks bypass the retry, but a parameter-validation 400 always surfaces at creation time.
  4. Double capture/diagnostics on retry — the capture consumer keeps only the last (successful) wire request; runtimeDiagnostics just appends summaries. Cosmetic only.
  5. getRateLimitErrorDetails on arbitrary errors — despite the name it is a generic provider-error-field extractor; safe (?.-guarded, never throws).
  6. Set scope & growth — per-pipeline (per content-generator config), bounded by distinct models that actually hit the required-thinking 400.
  7. openaiRequest closureundefined-safe before first buildRequest; handleError never consumes it; no stale-reference path.
  8. Interaction with outer retryWithBackoff — outer layers explicitly do not retry 400s; the inner one-shot semantic retry cannot multiply.
  9. Config vs learned key space — the Set keys on the actual wire model; the config branch additionally requires model === config.model. Complementary, no cross-contamination.
  10. Test quality — all assertions are on the actual chat.completions.create wire arguments, covering retry+cache, both chat_template_kwargs shapes, retry failure, abort guard, negative regex cases, and streaming retry. No blind spots that could pass with broken production code.

Non-blocking note

If a future provider surfaces the required-thinking error as a mid-stream SSE error chunk instead of an HTTP 400 at stream creation, the retry will not fire. That is a potential enhancement for new provider behavior, not a regression in this PR.

中文说明

c74792c 上完成全面审查,无问题,LGTM ✅。本地实跑 119/119 单测通过、core 类型检查零错误、CI 全绿、8 条历史 review threads 全部闭环且逐条核实修复确实在 HEAD 代码中。重点核查了非 DashScope 的 thinkingMandatory 块扩展、模型名归一化一致性、streaming 400 的错误时机、重试双记录、getRateLimitErrorDetails 安全性、Set 作用域、闭包变量安全、与外层 retryWithBackoff 的交互、配置/学习两分支键空间、以及测试断言有效性——均无问题。唯一非阻塞备注:若未来 provider 以 SSE 流中 error chunk 形式返回该错误则不会重试,属新 provider 行为的增强空间,非本 PR 回归。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants