fix(cli): Defer provisional standalone reasoning options - #10706
Merged
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
|
⏳ Approval still deferred — 1 PR CI workflow run(s) still in progress for ⏳ 审批仍在延迟中 —— |
Collaborator
Author
E2E verificationEnvironment: macOS, Node.js 22.22.3, a temporary
|
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 19 passed · 0 failed · 19 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：19 通过 · 0 失败 · 19 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 10706 Deep Verification — fix(cli): Defer provisional standalone reasoning optionsVerdict: 中文摘要
Central claim and A/B proofCentral claim (from PR body, treated as hypothesis): while a daemon-owned standalone session is still provisional ( Mechanism confirmed by code read before running: A/B design. Control = scratch worktree at the HEAD merge commit with only the production hunk reverted ( Scenario = the reviewer test plan's step 3 command (run without the author's macOS-specific
Witnesses: Secondary claims, also held by the head-cell test itself (green at head, unreachable at control because the request crashes first):
Mutation matrix (clause attribution + vacuity)One guard, two clauses; cells re-run the same 3 targeted tests, differing only in the mutated source of the control worktree:
FindingsNone blocking. Two informational observations, in severity order:
Sibling sweep (same root cause, adjacent doors): the four other Bridge tradeoff claim verified statically: the daemon HTTP bridge filters Reviewer Test Plan nit: step 3's command embeds a machine-local Not covered
MethodologyEnvironment: CI verify container, merge-ref checkout (HEAD Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
ytahdn
approved these changes
Sep 1, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
APPROVE —— 纯静态审查，head d2fe35e。
What this PR does / PR 主旨
EN: A targeted fix for the daemon-owned standalone flow (follow-up to PR 10514 under issue 8908). When a standalone create/load/resume runs with a reasoning-capable model before the content-generator config is initialized, buildConfigOptions dereferenced the missing generation config while assembling reasoning_effort, throwing a thinkingMandatory TypeError and returning HTTP 500. The fix returns only the safe mode and model options during the provisional phase; the ordinary post-activation session-context refresh republishes the complete reasoning set.
中文：针对 daemon 管理的 standalone 流程的定点修复（PR 10514 在 issue 8908 下的后续）。当 standalone create/load/resume 在 content-generator 配置初始化前选用支持 reasoning 的模型时，buildConfigOptions 在拼装 reasoning_effort 时解引用了尚不存在的 generation 配置，抛 thinkingMandatory TypeError 并返回 HTTP 500。修复让 provisional 阶段仅返回安全的 mode 与 model 选项，激活后的常规 session-context 刷新补发完整 reasoning 选项。
Findings / 核对情况
None blocking. The guard is placed at the exact first dereference of generation (acpAgent.ts:13918-13922), so every downstream read — thinkingMandatory, reasoning, the reasoningOverride[source] index — is skipped when the config is absent; the step-2 filter already handles non-reasoning models, active runtime snapshots and ACP-route ids, so the new branch fires precisely for the provisional standalone case that used to crash. Removing the old unguarded call removes no invariant, because the dereference still happens behind the null check. The optional-chaining getContentGeneratorConfig?.() matches the surrounding house style (getAuthType?.(), getReasoningEffort?.()) rather than adding noise, and the guard is a net-safety win even off the standalone path: a transiently-absent config now yields [mode, model] instead of throwing. No shared literal is borrowed, so there is no cross-PR coupling to track.
无阻塞项。守卫恰放在 generation 的第一次解引用处（acpAgent.ts:13918-13922），因此其下所有读取（thinkingMandatory、reasoning、reasoningOverride 的索引）在配置缺失时全部跳过；第二道过滤已处理非 reasoning 模型、活跃 runtime snapshot 与 ACP-route id，故新分支精确命中此前会崩溃的 provisional standalone 场景。删除旧的无守卫调用没有破坏任何不变量——解引用改到空值判断之后仍会执行。可选链 getContentGeneratorConfig?.() 与邻近的 getAuthType?.()、getReasoningEffort?.() 属同一仓库惯例，并非多余；且此守卫在 standalone 路径之外也是净安全收益：偶发无配置时返回 [mode, model] 而非抛错。未借用任何共享字面量，无跨 PR 耦合需追踪。
The three added regression tests are mutation-pinned and genuinely assert the contract: the standalone case checks provisional configOptions ids are exactly [mode, model] with refreshAuth not called, then flips getContentGeneratorConfig to {thinkingMandatory: true} and verifies the session-context refresh republishes a reasoning_effort option that omits the 'none' choice — proving the deferral is temporary and mandatory-thinking still holds. The load/resume case additionally pins that activateProvisionalWorkspace and the filesystem-state install do not fire early. Deleting the guard would make these tests throw; keeping reasoning in provisional would fail the [mode, model] assertion.
新增三项回归测试均为变异钉住、真实断言契约：standalone 用例先校验 provisional configOptions 恰为 [mode, model] 且 refreshAuth 未被调用，再把 getContentGeneratorConfig 置为 {thinkingMandatory: true}，验证 session-context 刷新会补发 reasoning_effort 选项且不含 none——证明 deferral 是临时的、且强制思考仍成立。load/resume 用例额外钉住 activateProvisionalWorkspace 与 filesystem 状态安装不会提前触发。删掉守卫这些测试即抛错；把 reasoning 留在 provisional 则 [mode, model] 断言失败。
CI / 持续集成
EN: At publish time on d2fe35e: Security Checks green, Qwen Code CI in_progress, SDK Java queued, the review-bot pass waiting. Stated as observed; I did not poll or run the suite (static review only). Approving on maintainer judgment.
中文：发布时刻 Security Checks 绿，Qwen Code CI 运行中，SDK Java 排队，机器人审查等待中。如实记录，未轮询、未运行测试（纯静态）。凭 maintainer 判断放行。
Verdict / 结论
🔵 APPROVE. Correct root-cause fix for the standalone provisional path, minimal and well-placed, backed by strong tests; no Critical or Important finding. / 针对 standalone provisional 路径的正确根因修复，改动最小、位置精准、测试充分，无 Critical 或 Important 发现。建议 Qwen Code CI 在本 head 转绿后合并。
Collaborator
|
Thanks for the quick follow-up!
Moving on to code review. 🔍 中文说明感谢及时的后续修复！
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewMy independent take before reading the diff: guard the reasoning-option construction when the generation config is missing, return the safe mode/model subset, and let the post-activation session-context refresh publish the full options. That is exactly what this does — the production change is 4 added lines, and I did not find a simpler path.
Test evidenceWhat this comment carries: the PR's own CI check-run results for the reviewed commit, fetched via the API — per gate policy the PR's code is never built or run here. No failures as of this snapshot; the two suites that matter most are still running, and the macOS/Windows unit matrices report skipped on this commit. The author's real-daemon E2E results (macOS, posted in this thread) are their own report, attributed as such — not independently re-run here. CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Not verified: real-daemon standalone create/load/resume behavior on this commit. The unit tests pin the deferral logic, but the end-to-end claim (HTTP 200 with only mode/model pre-activation, no early auth or workspace side effects against a live daemon) currently rests on the author's macOS run. Sandboxed verification would settle it: 中文说明代码审查读 diff 前的独立思路：在 generation 配置缺失时守卫 reasoning 选项构造，
测试证据本评论携带的证据：被审提交上 PR 自身 CI check-run 结果（经 API 获取—— CI 明细见上方表格（finalize 工作流会在 CI 结束后原地更新该区域）。 未验证：该提交上的真实 daemon standalone create/load/resume 行为。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — minimal, correct fix for a crash that shipped this morning with #10514, pinned by focused regression tests; only the still-running unit suite keeps this from a same-run approval. Stepping back: the approach matches my independent proposal — one guard deferring the reasoning options until the generation config exists, instead of papering over every The one reservation is evidential, not about the code: the end-to-end claim (provisional standalone create/load/resume returning HTTP 200 against a live daemon) currently rests on the author's macOS report while the ubuntu unit suite and the Real daemon E2E check are still in flight on this commit. Hence the deferred approval and the Verdict: approve — deferred until CI lands green on 中文说明置信度：4/5——对今天上午随 #10514 引入的崩溃的最小且正确的修复， 整体看：方案与我的独立思路一致——用一处守卫把 reasoning 选项暂缓到 唯一的保留在证据而非代码：端到端结论（provisional standalone 结论：批准——推迟到上述提交上 CI 全绿；finalize 任务会在所有检查绿色 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
doudouOUC
enabled auto-merge
September 1, 2026 13:30
chiga0
approved these changes
Sep 1, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
No blocking findings.
Approval blockers: none.
What was checked:
The fix is in buildConfigOptions (acpAgent.ts ~13921). Old code: const generation = config.getContentGeneratorConfig() then direct access to generation.thinkingMandatory, generation.reasoning, etc. with no null guard. When a daemon-owned standalone session uses its provisional configuration, getContentGeneratorConfig returns undefined, so every downstream field access throws TypeError.
Fix: config.getContentGeneratorConfig?.() + if (!generation) { return [modeConfigOption, modelConfigOption]; } — returns only the safe options (mode, model) until the content-generator config is available. After activateProvisionalWorkspace completes, the next sessionContext refresh publishes the full reasoning options including reasoning_effort. Confirmed correct by reading the code path through lines 13925–14021.
Test efficacy:
- New test at line 6635 mocks
getContentGeneratorConfigto returnundefined, then assertsconfigOptions.map(o => o.id)equals[mode, model]. This would fail (TypeError ongeneration.thinkingMandatory) without the guard. After updating the mock to return{ thinkingMandatory: true }, the test verifiesreasoning_effortappears insessionContextwithoutnoneas an option. Both checks are efficacious. - Updated load/resume test similarly asserts provisional response contains only
[mode, model]— catches any regression that exposes reasoning options prematurely.
Minor observation (not a blocker):
Two other call sites — case 'reasoning_effort': (line 5794) and getAccountInfo (line 12172) — still call config.getContentGeneratorConfig() without ?.. Both are functionally safe:
- Line 5794 is guarded by
!optionfrombuildConfigOptions, which also returns[mode, model]only when config is absent — soreasoning_effortoption is not found and the code throwsRequestError.invalidParamsbefore ever reachinggeneration.thinkingMandatory. - Line 12172 uses
cfg?.authType ?? config.getAuthType()and similar null-coalescing — safe ifcfgis undefined.
The ?. inconsistency is cosmetic; a follow-up to align all five call sites would remove any doubt.
Not covered: macOS/Windows filesystem behaviour — no platform-specific paths touched.
Reviewed with AI assistance.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
|
Released in v0.23.0. |
38 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Defers reasoning configuration options while a daemon-owned standalone session is still using its provisional configuration. Create, load, and resume responses expose the safe mode and model options until the content-generator configuration is available; after managed activation, the ordinary session-context refresh publishes the complete reasoning options.
Adds regression coverage for standalone create, load, and resume to verify that provisional responses do not initialize authentication or workspace state early, and that mandatory reasoning choices are restored once the live generation configuration exists.
Why it's needed
The standalone WebShell flow can select a reasoning-capable model before authentication and LLM initialization intentionally occur during managed activation. The provisional ACP response previously dereferenced the missing generation configuration while building
reasoning_effort, causing exact standalone create, load, or resume requests to fail with athinkingMandatoryTypeError and an HTTP 500 response.Reviewer Test Plan
How to verify
reasoning_effortis present and a mandatory-thinking model does not expose thenonechoice.PATH=/Users/jinye.djy/.nvm/versions/node/v22.22.3/bin:$PATH npx vitest run src/acp-integration/acpAgent.test.ts -t 'defers standalone reasoning options|defers standalone restore side effects'frompackages/cli; expect all three selected tests to pass.Evidence (Before & After)
Tested on
Environment (optional)
macOS with Node.js 22.22.3, Vitest, and a temporary real daemon environment. Repository build, typecheck, lint, changed-file formatting, the complete ACP agent unit suite, focused Node 22 regressions, and real-daemon standalone create/context checks passed.
Risk & Scope
reasoning_effort; the daemon bridge does not consume provisional configuration options, and the post-activation session-context refresh supplies the complete option set.Linked Issues
Follow-up to #10514
Refs #8908
中文说明
本 PR 内容
当 daemon 管理的 standalone 会话仍在使用 provisional 配置时，暂缓提供 reasoning 配置选项。create、load 和 resume 响应会在 content-generator 配置可用前仅返回安全的 mode 和 model 选项；managed activation 完成后，常规 session-context 刷新会发布完整的 reasoning 选项。
新增 standalone create、load 和 resume 回归覆盖，验证 provisional 响应不会提前初始化认证或 workspace 状态，并验证 live generation 配置可用后会恢复 mandatory reasoning 选项。
背景与动机
Standalone WebShell 流程可以在 managed activation 执行认证和 LLM 初始化前选择支持 reasoning 的模型。此前 provisional ACP 响应在构造
reasoning_effort时会解引用尚不存在的 generation 配置，导致精确 standalone create、load 或 resume 请求因thinkingMandatoryTypeError 而返回 HTTP 500。评审验证方式
如何验证
reasoning_effort，且 mandatory-thinking 模型不会暴露none选项。packages/cli中运行PATH=/Users/jinye.djy/.nvm/versions/node/v22.22.3/bin:$PATH npx vitest run src/acp-integration/acpAgent.test.ts -t 'defers standalone reasoning options|defers standalone restore side effects'，预期选中的 3 项测试全部通过。证据（改动前后）
测试平台
环境（可选）
macOS、Node.js 22.22.3、Vitest，以及临时真实 daemon 环境。仓库构建、类型检查、全量 lint、改动文件格式检查、完整 ACP agent 单元测试、Node 22 定向回归和真实 daemon standalone create/context 检查均已通过。
风险与范围
reasoning_effort；daemon bridge 不消费 provisional 配置选项，激活后的 session-context 刷新会提供完整选项集。关联问题
#10514 的后续修复
Refs #8908