Repository navigation
feat(serve): support model and reasoning selection at session creation - #12308
Conversation
E2E verification reportResult: final candidate bundle passed controlled-provider E2E (exit 0), with 15 captured provider requests. This is an API-sequence and wire verification, not browser UI automation or a real external-provider test.
Every main-wire comparison requires a nonempty capture. Fixtures isolate settings/runtime/workspaces and the standalone output root. Auxiliary suggestion/memory traffic is excluded from main-turn comparisons. Build, typecheck, bundle and changed-file lint passed. Focused tests: bridge 13, SDK 628, standalone 136, REST/ACP creation and scope 22 — total 799 passed. The REST/ACP run filtered out 1663 unrelated tests; it was not a full-suite run. No browser UI, real external provider, Windows or Linux execution is claimed. Fallback, skill overrides, reload, cold restoration and disconnect paths were not exhaustively validated in E2E. CI and merge status are separate from these local results. 中文最终候选 bundle 的受控 provider E2E 退出码为 0，共捕获 15 个请求，覆盖普通/standalone 启动确认、high/default/none、工具后续请求、后续轮次、设置及兄弟会话隔离、SDK REST/ACP HTTP/ACP WS、第二个 workspace、输入框等效 API 请求比较和之后人工修改。旧 CLI 0.18.5-preview.0 的能力预检产生零 POST mutation。 所有主请求比较都要求非空捕获。Fixture 隔离设置、runtime、workspace 和 standalone 输出目录；辅助建议与记忆请求不计作主轮请求。Standalone 内部 commit → 模型 → 思考 → release → 首条 prompt 顺序，以及准备失败时不 release/不准入、清理失败保持结果不确定，由独立 service 单测验证。 build、typecheck、bundle、变更文件 lint 通过。聚焦测试：bridge 13、SDK 628、standalone 136、REST/ACP 创建和 scope 22，总计 799 通过。REST/ACP 测试过滤了 1663 项无关用例，并非运行全部测试。 未测试浏览器 UI、真实外部 provider、Windows 或 Linux。E2E 未穷尽 fallback、skill 覆盖、reload、冷恢复和断连路径。本地验证与 CI、合并状态分别报告。 |
Follow-up: optional reasoning selection
Validation passed: build, full typecheck, bundle, changed-file lint, 804 focused tests, and the capability-registry unit/integration cases that failed on the prior commit. That CI failure was expected-capability lists missing Updated controlled-provider E2E passed with 24 captured requests. Non-reasoning gpt-4.1 succeeds in ordinary/standalone model-only creation without a reasoning wire field; reasoning-capable gpt-5.5 with omitted effort keeps the existing shared none preference instead of forcing model default low. Explicit high on the non-reasoning model fails with zero provider calls. All prior explicit-selection, tool-loop, settings/sibling isolation, later-turn, secondary-workspace and manual-change checks remain passing. Ordinary SDK model-only creation and prompts were tested through REST, ACP HTTP and ACP WS. Standalone creation remains REST-backed even on clients configured for ACP; prompts use the owning REST path. The existing generic ACP-to-standalone prompt ownership mismatch was also reproduced without startupConfig and is outside this change. No browser UI automation or real external provider is claimed. 中文
build、完整 typecheck、bundle、变更文件 lint、804 项聚焦测试和此前失败的能力列表单元/集成用例通过。旧 CI 失败是预期能力列表漏加 新受控 provider E2E 捕获 24 个请求：非思考模型 gpt-4.1 可仅指定模型创建普通/standalone 会话，请求不含思考字段；gpt-5.5 省略档位时沿用现有 none 偏好，不强制变为模型默认 low。对非思考模型显式传 high 仍失败且零 provider 调用。此前显式选择、工具循环、设置/兄弟隔离、后续轮次、第二个 workspace、人工修改等回归仍通过。 普通 SDK model-only 创建和提问覆盖 REST、ACP HTTP、ACP WS。Standalone 创建在 ACP 配置下仍使用既有 REST 路径，提问使用其所属 REST 路径；通用 ACP 向 standalone 提问的既有归属问题在不带 startupConfig 时同样复现，不在本次修改范围。未测试浏览器 UI 或真实外部 provider。 |
CR round 1 follow-up verificationAll sixteen findings have corresponding fixes or committed regression coverage. The critical standalone rollback was reproduced before fixing it: the failed UUID remained listed/loadable and could not be retried. The final real-child probes now verify422, absence of the owned JSONL/catalog entry, GET404, and successful same-UUID retry/load. Real child exit remains a coded recoverable outcome-unknown result. The new real-daemon tests also caught two error-boundary mistakes that mocks missed: JSON-RPC rejects decoded error objects rather than RequestError instances, and split bundles contain distinct constructors for the same startup error class. Classification now uses only the narrow stable code/message contracts; auth and transport errors are not blanket-wrapped. Validation: build, complete typecheck, bundle and changed-file lint passed. Bridge28 + SDK645 + CLI service/docs/error mappings228 + focused startup/scope/registry routes30 + full real-daemon route integration41 = 972 passed. The full committed route integration file includes model-only confirmation/current model cache, shared none→high, ordinary rejected-selection live teardown, and standalone durable rollback/retry. Five isolated verification runs passed, including the existing 24-request controlled-provider E2E. The full CLI/provider-key integration suite was not run; the committed real-daemon route tests belong to the existing no-AK CI suite. Browser UI and real external-provider execution remain untested. Ordinary failed-creation persisted-history/UUID semantics remain unchanged; the durable rollback/retry guarantee here is explicitly standalone scoped. 中文16 条发现均已有修复或已提交的回归覆盖。Critical 在修复前已复现：失败 UUID 仍可列出和加载且不能重试。最终真实子进程探针验证422、删除本次 JSONL/目录索引记录、GET404，并能用同 UUID 成功重试和加载；真实子进程退出仍返回可恢复的结果不确定错误。 新增真实 daemon 测试还捕获到 mock 未暴露的两个边界：RPC reject 的是解码后的普通错误对象，且拆分 bundle 内存在同一错误类的不同构造器。现在仅按明确的稳定错误码/消息契约分类，不把认证和传输错误统一包装成配置拒绝。 build、完整 typecheck、bundle、变更文件 lint 通过。Bridge28 + SDK645 + CLI service/文档/错误映射228 + 启动/scope/registry 路由30 + 完整真实 daemon 路由集成41 = 972 通过。5 组隔离验证通过，包含既有 24 次请求的受控 provider E2E。 未运行完整需要 provider 凭据的 CLI 集成套件；已提交的真实 daemon 路由测试属于现有 no-AK CI。未测试浏览器 UI 或真实外部 provider。普通失败创建的持久历史/UUID 语义保持原状；本次持久回滚和同 ID 重试保证明确限于 standalone。 |
Superseded by f497b74. All 16 R1 findings have code/test/documentation fixes and per-thread replies; all threads are resolved. The blocking standalone rollback was reproduced and then verified with real child probes (record removed, GET404, same UUID retry/load succeeds), while transient failures retain outcome-unknown recovery. Build/typecheck/lint, 972 focused/unit/integration tests and the controlled-provider matrix passed. This dismisses the stale review of 4c9467a; it is not an approval of the new head and does not bypass its required checks/review. Evidence: #12308 (comment)
|
@qwen-code /takeover from 4 |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This window's round counter starts at 4 (the rounds this PR spent in review before takeover), so the Critical-only brake engages after 1 more change-producing round(s) instead of a full fresh 5. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。本窗口轮次计数从 4 起算（即本 PR 托管前已进行的评审轮数），因此再经过 1 个产生改动的轮次即进入 Critical-only，而非重新计满 5 轮。移除 |
Deep verification: PR #12308 —
|
| Cell | Oracle | base 39b22efb |
head f497b74 |
|---|---|---|---|
GET /capabilities |
session_startup_config feature |
absent (as predicted) | present |
POST /session + startupConfig {model, high} |
response confirmation | 200, no modelApplied/startupConfigApplied (silent ignore) |
200, modelApplied: true, canonical startupConfigApplied incl. effectiveReasoning {enabled, high} |
| Session context after create | model / reasoning_effort currentValue |
plain-default(openai) / none |
qwen-startup-test(openai) / high |
| Provider wire, turn 1 | model, reasoning_effort |
plain-default, absent |
qwen-startup-test, high |
| Provider wire, turn 2 (retention) | same | n/a (base never applied) | qwen-startup-test, high |
Shared settings.json |
bytes before/after | unchanged | unchanged |
| Sibling session (no startupConfig) | context | n/a | default model + shared none, first session unaffected |
| Model-only create on no-reasoning model | startupConfigApplied shape |
silently ignored | 200, only modelServiceId, no reasoning keys; wire has no reasoning_effort |
Reasoning on no-reasoning model / tier max outside efforts |
status + code | n/a | 422 startup_config_rejected ×2; rejected caller-supplied ID admits nothing (GET 404) |
Malformed matrix (6 shapes: unknown field, legacy modelServiceId mixed, sessionScope:'single', missing/invalid/oversized values) |
status + code | n/a | 400 invalid_startup_config ×6 |
Standalone {reasoningEffort:'max'} → same-id retry 'high' |
rollback semantics | 400 invalid_request (exact-body route rejects the unknown key) |
422 → GET 404 (recording removed) → retry 200 with confirmation |
Witness: 01-ab-head-vs-base.png (27/27 head, 9/9 base control, both green under per-arm encoded expectations).
Secondary claim: SDK capability preflight — zero mutations against old daemons
Head-built @qwen-code/sdk DaemonClient pointed at the base daemon through a logging loopback proxy (Host header rewritten, forwarding verified by positive control):
createOrAttachSession({startupConfig})→ throwsDaemonCapabilities.session_startup_config is missing; proxy counted 0POST /session(total 1 request: the capabilities GET).createStandaloneSession({startupConfig})→ same rejection; 0POST /standalone/sessions.- Positive control: same SDK through an identical proxy against the head daemon — legacy create succeeds (proxy saw the POST), model-only startupConfig succeeds with
startupConfigApplied {modelServiceId}(proxy saw exactly 1 POST each).
Witness: 02-sdk-preflight-zero-mutation.png (7/7).
Secondary claim: composer-equivalence on the wire
Same two-turn scenario run twice on one head daemon: (A) startupConfig creation; (B) legacy modelServiceId creation + setSessionConfigOption('reasoning_effort', 'high') (the REST-exposed composer setter). Sorted per-request wire signatures (category:model:effort) are identical:
A(startup) : turn:qwen-startup-test:high | memory:high | turn:high | memory:high | suggestion:none
B(composer) : turn:qwen-startup-test:high | memory:high | turn:high | memory:high | suggestion:none
Witness: 05-composer-equivalence.png (4/4). This is also what cleared up an artefact in my own first harness revision: the next-prompt suggestion-mode call embeds full conversation history and runs at the shared default effort (none here), so a naive "last request containing my probe text" oracle intermittently misattributed it as turn 2. With the oracle anchored on the last user message, the head arm is stable at 27/27 across 3 consecutive runs; the suggestion call's none is identical on the composer path, i.e. pre-existing ancillary behavior, not something this PR introduces.
Vacuity / mutation check
Reverting exactly one line in the built head (...(startupConfig ? { startupConfig } : {}) in dist/src/serve/routes/session.js) and re-running the full head suite flipped precisely the 10 central-claim assertions red (confirmation fields, context, both wire checks, retention, model-only applied shape, both 422s → 200) while the parse-layer (S5 malformed ×6) and standalone-path (S6, its own hunk) checks stayed green — no import/compile breakage, failures are the behavioral mismatches the harness exists to catch. The file was restored byte-identical (cmp verified, RESTORED-OK).
Witness: 03-vacuity-mutation.png.
Gates (PR head, Linux aarch64)
| Gate | Result |
|---|---|
packages/acp-bridge session-startup-config.test.ts |
28/28 |
packages/sdk-typescript DaemonClient + DaemonClientStandalone + acpRouteTable |
622/622 |
packages/cli 7 serve-surface files (routes/standalone, acp-http dispatch-error + transport, server, error-response, docs-contract, standalone-session-service) |
1914/1914 |
integration-tests qwen-serve-routes.test.ts -t startup (the PR's 3 new tests, real daemon + real bundle) |
3/3 |
Witness: 04-gates.png. The integration gate needed npm run bundle in the worktree (its globalSetup pins TEST_CLI_PATH to dist/cli.js); noted for anyone reproducing.
Merge-drift check
Base is 70 commits behind current main (7837c6c200), and main has touched several PR-touched files since. git merge-tree 39b22efb main f497b74 produces zero conflict markers; GitHub reports MERGEABLE. All PR CI lanes that run on the merge ref (Test, Lint & Static, Integration, Serve A/B) are green at this head, so the merged tree is covered; my local A/B deliberately compared the pure head against the pure base.
Findings
None blocking. Two informational observations:
- Suggestion-mode calls ignore per-session reasoning selection (they run at the shared default effort) — identical on the composer path, so pre-existing and out of scope; worth knowing only because it can confuse wire-level verification (see above).
- CI
review-prlane failed by timeout (ran to its 6h ceiling mid-review, log ends inside the automated review loop). Not a code failure, but the automated review verdict for this head does not exist; this round plus CI's other lanes are the available evidence.
Not covered
- ACP HTTP/WS transport end-to-end (
session/newwithstartupConfigover those transports) — covered by CI's serve A/B, the author's controlled-provider E2E (all three SDK transports), and the passingdispatch-error/transport/acpRouteTableunit tests; my harnesses drove REST + SDK. - Real external providers (all traffic here is a controlled loopback fake); Windows/macOS; fallback, reload, skill override, cold-restoration and transport-disconnect edges (the PR itself lists these as not exhaustively exercised).
- Per-commit attribution (verified the aggregate diff; the PR carries 4 commits).
- Direct child ACP and
create_sub_session(explicitly out of the PR's scope).
Methodology
Two throwaway git worktrees at the exact base/head OIDs, each with the repo node_modules hardlink-copied in (cp -al); the internal @qwen-code/* workspace symlinks resolve relative, and I asserted readlink -f node_modules/@qwen-code/qwen-code-core points into each own tree before trusting the control. The PR touches no dependency manifests (only an exports addition in packages/acp-bridge/package.json), so the shared install is a clean control. Both trees built with npm run build (exit 0) plus npm run bundle on head. Harnesses are self-contained .mjs scripts driving the real daemons over real loopback sockets; every PASS/FAIL line is a scripted comparison. Head arm was re-run 3× after the oracle fix (27/27 each). Assertion accounting: distinct scripted checks counted once — A/B harness 27 (head) + 9 (base control with encoded expected behavior) + SDK preflight 7 + composer-equivalence 4 + 1 adjudicated mutation check + 2567 vitest tests = 2615 pass, 0 fail. Raw per-run JSON (results-*.json, wire-*.json), harness sources, and build logs live in tmp/pr12308-verify-20260922-222800/ on the verifying machine; images were captured live with scripts/verify-capture.mjs.
Evidence
01-ab-head-vs-base.png— full A/B run, both arms02-sdk-preflight-zero-mutation.png— SDK preflight vs old daemon + positive control03-vacuity-mutation.png— mutation flips central assertions red, thenRESTORED-OK04-gates.png— focused unit + integration gates on PR head05-composer-equivalence.png— identical wire signatures for startup vs composer creation
|
🤖 AutoFix ran out of time before finishing (timeout (3600000ms)) (attempt 5/100) — it will retry on the next scan.
See the Qwen Autofix agent step logs for model/tool output. 中文说明🤖 AutoFix 在完成前耗尽了时间（timeout (3600000ms)）（第 5/100 次尝试）—— 将在下次扫描时重试。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/35739497141 🧠 Handled by Qwen Code · model/模型 |
- Map the ACP internalError wire shape ({code: -32603, data.details})
produced when switchModel refuses an unregistered model to
startup_config_rejected on the startup-config call path, so the
ordinary route answers 422 instead of 500 and the standalone path
takes its definite rollback branch.
- Roll back the persisted recording of a caller-supplied sessionId on
POST /session after a definite startup rejection, so a retry of the
same id is no longer answered with 409 session_id_conflict; uncertain
failures keep the recording.
- Publish auto in the OpenAPI approvalMode enum and pin it against
APPROVAL_MODES; narrow the 422/protocol/design cleanup wording to
match the implemented best-effort semantics; restore the sessionId in
the new rollback warning.
|
🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 6/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #12308Critical-only brake is active (prior rounds exhausted), so this round implements the four Critical findings plus three adjacent, low-cost Suggestions the Critical findings themselves couple in, and defers the remaining Suggestions to the next round via inline replies. The deferred non-Critical audit section was left untouched per the brake rules. Addressed
Already resolved before this round (verified)
Deferred to the next round (replied inline, threads left open)
Notes
Verification
中文说明本轮摘要 — PR #12308Critical-only 制动已生效（此前轮次已用尽），因此本轮实现了四条 Critical 发现，以及 Critical 发现本身明确耦合的三条低成本 Suggestion；其余 Suggestion 通过行内回复延后到下一轮。按制动规则，未触碰"已延后的非 Critical"审计区。 已处理
本轮前已解决（已核实）
延后到下一轮（已行内回复，线程保持开放）
说明
验证
Deferred non-Critical feedbackCritical-only mode is active: the round counter reached 5 (this window was seeded at round 4 by
中文说明已进入仅处理 Critical 的模式：轮次计数已达 5（本窗口由 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧵 Resolved all 8 selected review thread(s). · 已关闭全部选中的 8 条评审线程。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🔀 Base updated: red check(s) [ubuntu-latest / Java 21, Lint & Static (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [ubuntu-latest / Java 21, Lint & Static (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
… recordings (#12308) - session-startup-config: drop the -32603 arm so child-side internal errors (auth, credential, timeout, transport) stay uncertain instead of a false definite startup_config_rejected; the ACP child now maps only its two caller-caused switchModel refusals (unknown model for the auth type, media-only primary) to invalid params, so definite rejections ride -32602 alone. - dispatch: roll back the persisted recording of a caller-supplied sessionId on the ACP session/new transports after a definite startup rejection, mirroring the REST route; uncertain outcomes keep it. - routes/session: warn once when the recording rollback is refused or fails instead of silently discarding the boolean outcome. - standalone-session-service: extract the shared rollback sequence into one helper used by both the startup-config and model_selection_failed branches. - openapi: qualify the 422 close/rollback as best-effort and replace the unconfirmable status-view pointer with the self-certifying retry check.
|
🤖 Addressed the latest review feedback (round 7/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 7/100 轮）。改动内容与我反驳保留之处如下： Round 6 — address-review summaryCommit: All seven actionable findings are resolved in code. The non-Critical items the workflow deferred (R2-7–R2-11, R2-15 and the older reviews) are outside this round by the deterministic brake and were not touched. Findings[Critical] R3-1 (rc:4077430752) — ACP
|
|
Review convergence / follow-up disposition This PR has passed five review rounds. The remaining Suggestion threads below are explicitly deferred to separate follow-up work, not to another automatic fix round in this PR. They are not claimed fixed. The current PR remains responsible for all valid Critical findings; their current-head verification is tracked separately.
Closing these Suggestion discussions records the scope decision, not completion of the follow-up work. No production code is changed by this disposition. |
|
Independent verification of exact head 9379287 confirms all seven historical Critical findings are fixed. Verified: standalone recording rollback permits same-ID retry; the approval enum includes auto; cleanup documentation matches best-effort directory discard; REST and ACP definite startup rejections roll back persisted recordings; invalid model selections remain definite parameter errors; ordinary child -32603 auth/internal/transient errors retain their uncertain classification instead of being destructively rolled back. Isolated build and bundle pass. Focused validation passes: 29 bridge startup tests, 29 CLI tests across five files, and three integration cases against the real bundled daemon (61 total). Unselected tests were not run; this is not a full-suite claim. No source changes or pushes were made during this verification. All Critical threads are resolved. The 14 remaining Suggestions have been explicitly deferred to separate follow-up work and closed as deferred, not fixed: #12308 (comment). No live human objection was found. The historical bot blocking reviews are now stale; normal required approval and check protections still apply. |
Stale at 9379287: all seven historical Criticals independently verified fixed (standalone/REST/ACP recording rollback, auto enum, cleanup docs and precise rejection classification). Build/bundle and 61 focused/real-daemon tests pass; evidence comment5791850516. Suggestions explicitly deferred in comment5791801846; zero unresolved threads or live human objections. Normal required review remains.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round — no action requiredThis round's feedback contains no actionable items, so no code changes were made and no commit was created. What the feedback contained
Why no automatic action
State left behind
中文说明Autofix 本轮 —— 无需处理本轮反馈不包含任何可执行条目,因此未做任何代码修改,也未创建提交。 反馈内容
为何不采取自动行动
留下的状态
Deferred non-Critical feedbackCritical-only mode is active: the round counter reached 5 (this window was seeded at round 4 by 中文说明已进入仅处理 Critical 的模式：轮次计数已达 5（本窗口由 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed at 93792871 (base 99bf4ce8). Approving — no P0/P1 found.
Input validation is fail-closed at every layer (parseSessionStartupConfig rejects non-objects, unknown keys including __proto__, over-length ids, bad tiers, and conflicts with the legacy modelServiceId/sessionScope shapes), and the model id only ever reaches the child as a JSON string via setSessionConfigOption — same trust level as the pre-existing legacy path, so no new privilege or injection surface. Unknown models map to -32602 → startup_config_rejected → 422 while transport/credential faults stay 500, with a documented definite-vs-uncertain split (uncertain outcomes keep the session-id recording so a retry surfaces 409 instead of lying). The contract is coherent end-to-end: same field names and required/optional semantics across SDK, REST routes, ACP dispatch and bridge, and an omitted reasoningEffort issues exactly one setter call with no default substitution (test-pinned). Capability gating protects SDK callers against old daemons.
Two optional P3s:
- Model confirmation accepts any non-empty
currentValueecho and the reasoning path only checks echo₁===echo₂, never echo===request — a child that silently substitutes a different model on a successful set would produce a 200 on the wrong model (disclosed viastartupConfigApplied.modelServiceId, but SDK-side assertion only checks non-empty). A test pins this as intentional; consider an opt-in strict equality check when the id is already canonical. DAEMON_REASONING_SELECTIONSand the 256-char bound are duplicated in the SDK instead of single-sourced — a new reasoning tier would make the daemon accept what the SDK rejects client-side, and the docs contract test does not cover SDK↔daemon.
CI green at this head; 0 unresolved threads.
315a535
|
🤖 Addressed the latest review feedback (round 8/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 8/100 轮）。改动内容与我反驳保留之处如下： Round summaryMerged Merge conflict resolution (
|
|
🤖 AutoFix ran out of time before finishing (timeout (3600000ms)) (attempt 1/100) — it will retry on the next scan.
See the Qwen Autofix agent step logs for model/tool output. 中文说明🤖 AutoFix 在完成前耗尽了时间（timeout (3600000ms)）（第 1/100 次尝试）—— 将在下次扫描时重试。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/35921449611 🧠 Handled by Qwen Code · model/模型 |
A modelServiceId carrying a newline slipped past parseSessionStartupConfig and produced a core refusal message the single-line classifier regexes could not match, so a definite caller-caused rejection degraded to an internal error and skipped the recording rollback. Reject control characters at the parse boundary and make isCallerCausedModelRefusal newline-tolerant; align the SDK and integration-guide modelApplied presence rule with the spawn-scoped wording the bridge and OpenAPI already publish.
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #12308Round commit: Feedback addressed
Deliberately not done
Verification
Mutation probes (red before the fix, green after):
中文说明Autofix 本轮总结 — PR #12308本轮提交： 已处理的反馈
本轮刻意不做的事
验证
变异探针（修复前为红，修复后为绿）：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧵 Resolved all 2 selected review thread(s). · 已关闭全部选中的 2 条评审线程。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
Scope disposition for the two reopened Suggestion discussions R2-7 and R2-8: these remain deferred to separate follow-up work, as already recorded in #12308 (comment). The schema mutual-exclusion encoding and standalone schema/path assertion coverage are not claimed fixed. They should not be reintroduced into another automatic fix round of this already-reviewed PR. Closing these duplicate discussions records the same deferral; verification of the new Critical fixes is being performed separately against 133ad52. |
|
Independent verification at 133ad52: build and 78 focused tests pass (bridge33, CLI32, SDK13). Tests used the original configs with only server.host=127.0.0.1 in temporary wrappers because this host cannot resolve localhost; no source changes. This is not a fresh full-suite or bundled-daemon E2E claim. R4-1 is fixed: definite rejection carries the actual session ID, REST/ACP prefer it for rollback, and generated-ID recording removal is tested alongside caller-ID and uncertain controls. R4-2/R5-1 presence wording now consistently distinguishes fresh spawn, legacy attach omission and coalesced spawn-owner result in bridge, SDK, OpenAPI and integration documentation. R5-2's measured newline defect is fixed: parser rejects control characters before spawn, while the child classifier handles newline-containing refusal text; credential-fault identity tests remain green. The review explicitly labeled its second auth-refresh claim unproven. Finding the same message at a second throw site is not a reachable counterexample: the media-only precheck occurs before selection/handleModelChange, and refreshAuth synchronously checks the same selected model before its first await. A real built-core probe for image/voice/realtime-only attempts rejected each before the callback and retained the previous chat model. This rebuts that secondary claim as unsupported, not as impossible or a newly fixed defect. Message-classifier coupling remains a recorded nonblocking follow-up. Other actionable R4 fixes (refused-rollback logging, service parser guard, typed SDK rejection) are covered. Repeated schema Suggestions remain explicitly deferred, including comment5813834668; not claimed implemented. With no unresolved threads or live human blocking objection, the two historical bot CHANGES_REQUESTED reviews are stale. Normal required approval and checks remain in force. No code edits or pushes in this verification. |
133ad52: R4 generated-ID recording rollback and spawn/attach wording corrected; 78 current-head tests and build pass. R5 newline case fixed, speculative secondary claim rebutted with code and real-core probe. Evidence comment5813901376; Suggestions explicitly deferred, zero unresolved threads or live human objection.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round — no changeOutcome: no code change. The single unresolved finding in this round's feedback is already fixed at the PR head ( Feedback triage
Verification
中文说明Autofix 本轮 —— 无代码改动结论：无代码改动。 本轮反馈中唯一未决的发现已在 PR 头提交（ 反馈分诊
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧵 Resolved all 1 selected review thread(s). · 已关闭全部选中的 1 条评审线程。 🧠 Handled by Qwen Code · model/模型 |
Verification round 2: PR #12308 at head
|
| Check (×3 transports unless noted) | prev f497b74 |
head 133ad52 |
|---|---|---|
S1: {model, high} returns modelApplied: true and the canonical startupConfigApplied |
✅ | ✅ |
S1: provider wire has model=qwen-startup-test, reasoning_effort=high on turn 1 and turn 2 |
✅ | ✅ |
S2: model-only on the no-reasoning model gives applied keys [modelServiceId] and no reasoning_effort on the wire |
✅ | ✅ |
S3: 'max' without a caller ID is rejected with 422 startup_config_rejected |
✅ | ✅ |
| S3: no phantom session listed or recorded after that rejection | ❌ one new listed session + .jsonl per transport |
✅ none |
S4: modelServiceId: "qwen-startup-test\n(openai)" returns 400 invalid_startup_config |
❌ REST: 500 -32603, and the message echoes the raw newline; ACP: 500 internal |
✅ 400 on all three |
| S4: no session left behind | ✅ | ✅ |
S5 (ACP HTTP, raw JSON-RPC from a tee of the /acp stream): code for startup_config_rejected |
❌ -32603 |
✅ -32602 (invalid_startup_config is also -32602) |
S6 (SDK): local malformed rejection is a TypeError with code: 'invalid_startup_config' |
❌ code undefined |
✅ |
S7: shared settings.json byte-identical after all creations |
✅ | ✅ |
| Total | 19 pass / 8 fail | 27 pass / 0 fail |
The ACP HTTP/WS rows are the part the previous round did not cover end-to-end. Model/effort confirmation, wire retention across turns, model-only creation and rollback all behave the same on ACP as on REST. The 422 status survives the move to -32602 through data.httpStatus, as the new comment in dispatch.ts says.
Mutation check: are the new hunks load-bearing?
I edited the built head's dist, ran the full harness, then restored each file and confirmed the restore with cmp.
- M1+M2: in
routes/session.jsandacp-http/dispatch.js, the rollback ignoreserr.sessionId. Exactly the 3 "no phantom" checks (REST, ACP HTTP, ACP WS) turn red, and the other 24 stay green. The newsessionIdfield onSessionStartupConfigErroris the only thing that reaches the daemon-generated ID. - M3: in
acp-bridge/session-startup-config.js, drop the/\p{Cc}/ucheck. Exactly the 3 S4 checks turn red. They now return 422startup_config_rejected, not 400. The previous head, which has neither hunk, returned 500. So the[\s\S]fix inSession.tsmakes a newline-bearing ID a caller refusal (422), and the boundary guard turns it into 400 before any session exists. I confirmed both hunks independently.
Gates at head (macOS)
| Gate | Result |
|---|---|
acp-bridge session-startup-config.test.ts |
33/33 |
sdk-typescript DaemonClient + DaemonClientStandalone + acpRouteTable |
636/636 |
cli: 8 PR-touched serve/session test files |
3034/3038 in the parallel run. The 4 failures were timeouts, all in non-startup server.test.ts cases. Run on its own, server.test.ts passed 1332/1332 in 5 of 7 runs. The other 2 runs had 1–2 intermittent failures. The one I captured by name, isolates webhook configs across workspace sources, already exists on main and is not part of this PR's diff. The PR's startup tests in that file (-t startup, 8 tests) passed every time. |
integration-tests/cli/qwen-serve-routes.test.ts -t startup, real daemon on the bundled dist/cli.js |
3/3. CI's "Integration Tests (CLI, No Sandbox)" lane skips this PR, so this is the local substitute. |
Findings
No blockers.
One observation, not blocking (doc accuracy / transport parity): the new DaemonStartupConfigError JSDoc says:
one
code-based catch covers both this local rejection and the daemon's400 invalid_startup_configresponse
That holds on REST, where the body has body.code. Over ACP HTTP and ACP WS, the SDK's DaemonHttpError.body is { error, data: { errorKind, httpStatus } } and has no top-level code (measured, row "S3 error body shape" in image 01). A caller on a negotiated ACP transport needs body.data.errorKind. That body shape comes from synthesizeResponse in the ACP transports, which predates this PR and applies to every ACP error. Either of these would be enough:
- a doc note naming both locations, or
- a follow-up that mirrors
errorKindintocodein the ACP synthesized body.
Not covered
- Real external providers. All traffic went to a controlled loopback fake.
- Windows.
- Fallback, reload, skill-override, cold-restore and transport-disconnect edges. The PR lists these as not exhaustively exercised.
- Standalone creation over ACP. The SDK routes standalone through REST, and the previous round already covered standalone rollback and same-ID retry.
- SDK preflight against an old daemon and composer-equivalence on the wire. The previous round verified both, and the delta doesn't touch either.
Reproduce
The harness is two self-contained scripts, and every PASS/FAIL line comes from a scripted comparison:
Steps:
- Create worktrees at
133ad52andf497b74. - In each, run
pnpm install --frozen-lockfile && npm run build. - Run
node harness.mjs <worktree> <label>.
中文版
第 2 轮验证：PR #12308 在 head 133ad52 上的结果（macOS，REST / ACP HTTP / ACP WS）
结论：无阻塞问题，可以合并。 在当前 head 上，用真实 qwen serve daemon 加真实 SDK 跑三种 transport，27 项检查全部通过。同一套装置跑在上一轮验证过的 head f497b74 上，失败 8 项，每一项都对应此后新增的 4 个提交中的某一个。从构建产物里逐个删掉新增的 hunk，恰好只有它自己负责的检查变红。
- 验证的 head：
133ad5266a0871dbd64066957d1d3a145a5b9379，即发帖时的 PR head。与当前main（90232f0）做git merge-tree无冲突。57 个评审 thread 全部已解决，CI 没有失败的 lane。 - 环境：macOS arm64（Darwin 25.6），Node v24.18.1。PR 的 "Tested on" 表和上一轮（Linux aarch64）都没有覆盖 macOS 和 ACP HTTP/WS 端到端，本轮补上这两块。
自上次验证以来的变化
f497b74 → 133ad52 包含一次合并 main 和 3 个 PR 提交。不算合并，PR 自身的改动是 19 个文件，+370/−50，改了四处行为：
- daemon 生成 ID 时的回滚。 确定的
startup_config_rejected现在带上被拒会话的sessionId，所以调用方没有提供 ID 时，REST 路由和 ACP dispatcher 也会回滚持久记录。之前会残留一个能被列出、能被恢复的空会话。 - 模型 ID 中的控制字符。
modelServiceId里含/\p{Cc}/u时，现在在入口处直接返回400 invalid_startup_config。isCallerCausedModelRefusal的正则也从.改成了[\s\S]。 - ACP JSON-RPC 错误码。
startup_config_rejected从-32603（内部错误）改为-32602（参数错误），data.httpStatus仍然是 422。 - SDK 错误类型。 SDK 本地的格式错误拒绝改为
DaemonStartupConfigError extends TypeError，并带code: 'invalid_startup_config'。
同场景 A/B：当前 head 对上一轮验证的 head
两个臂的装置相同：
- 每个臂用各自 worktree 构建出的
packages/cli/dist和packages/sdk-typescript/dist。 - daemon 使用隔离的
HOME/QWEN_HOME/QWEN_RUNTIME_DIR和全新的工作区。 - 回环假 OpenAI provider 记录每一个请求体。
- 配置：一个支持思考的模型（
efforts: [low, high]），一个不支持思考的模型，共享默认reasoningEffort: 'none'。 - 每个场景分别通过 REST、
AcpHttpTransport、AcpWsTransport上的DaemonClient跑一遍。
| 检查（未注明的均为三种 transport 各一次） | 上一轮 f497b74 |
当前 133ad52 |
|---|---|---|
S1：{model, high} 返回 modelApplied: true 和规范的 startupConfigApplied |
✅ | ✅ |
S1：provider 实际收到的请求在第 1、2 轮都是 model=qwen-startup-test、reasoning_effort=high |
✅ | ✅ |
S2：无思考模型只选模型时，applied 只有 [modelServiceId]，请求里没有 reasoning_effort |
✅ | ✅ |
S3：不带调用方 ID 的 'max' 被拒，返回 422 startup_config_rejected |
✅ | ✅ |
| S3：被拒之后不残留幽灵会话（列表和持久记录中都没有） | ❌ 每种 transport 各多出一个可列出的会话和一个 .jsonl |
✅ 没有 |
S4：modelServiceId: "qwen-startup-test\n(openai)" 返回 400 invalid_startup_config |
❌ REST 返回 500 -32603，错误消息原样带出换行符；ACP 返回 500 internal |
✅ 三种 transport 都是 400 |
| S4：不残留会话 | ✅ | ✅ |
S5（ACP HTTP，对 /acp 流做 tee 抓到的原始 JSON-RPC）：startup_config_rejected 的错误码 |
❌ -32603 |
✅ -32602（invalid_startup_config 也是 -32602） |
S6（SDK）：本地格式错误拒绝是带 code: 'invalid_startup_config' 的 TypeError |
❌ code 为 undefined |
✅ |
S7：所有创建完成后，共享 settings.json 逐字节不变 |
✅ | ✅ |
| 合计 | 19 通过 / 8 失败 | 27 通过 / 0 失败 |
ACP HTTP/WS 这几行是上一轮没有端到端覆盖的部分。模型和思考档位的确认、跨轮次保持、只选模型、回滚，在 ACP 上的行为都和 REST 一致。错误码改成 -32602 之后，422 状态仍然通过 data.httpStatus 还原出来，和 dispatch.ts 新注释的说法一致。
变异检验：新增的 hunk 是否承重
我在当前 head 构建出的 dist 里做改动，跑完整装置，然后还原每个文件并用 cmp 确认还原成功。
- M1+M2：在
routes/session.js和acp-http/dispatch.js里，让回滚忽略err.sessionId。恰好 3 项「不残留幽灵会话」检查（REST、ACP HTTP、ACP WS）变红，其余 24 项保持绿色。能拿到 daemon 生成的 ID 的只有SessionStartupConfigError上新增的sessionId字段。 - M3：在
acp-bridge/session-startup-config.js里删掉/\p{Cc}/u检查。恰好 3 项 S4 检查变红，返回的是 422startup_config_rejected，而不是 400。上一轮的 head 两处改动都没有，返回的是 500。所以Session.ts里的[\s\S]修复让带换行的 ID 被识别为调用方拒绝（422），入口处的检查则在创建任何会话之前就把它拦成 400。两处改动我都分别验证过。
门禁（当前 head，macOS）
| 门禁 | 结果 |
|---|---|
acp-bridge 的 session-startup-config.test.ts |
33/33 |
sdk-typescript 的 DaemonClient、DaemonClientStandalone、acpRouteTable |
636/636 |
cli：PR 改动的 8 个 serve/session 测试文件 |
并行跑 3034/3038。4 个失败都是超时，全部在 server.test.ts 中与启动配置无关的用例里。单独跑 server.test.ts 共 7 次，其中 5 次是 1332/1332，另外 2 次有 1–2 个偶发失败。我抓到名字的那个是 isolates webhook configs across workspace sources，它在 main 上本来就有，不在这个 PR 的改动里。这个文件里 PR 的启动配置用例（-t startup，8 个）每次都通过。 |
integration-tests/cli/qwen-serve-routes.test.ts -t startup，真实 daemon 加打包后的 dist/cli.js |
3/3。CI 的 "Integration Tests (CLI, No Sandbox)" 在这个 PR 上是跳过的，本地这次运行作为替代。 |
发现
没有阻塞问题。
一个观察项，不阻塞（文档准确性 / transport 一致性）： 新增的 DaemonStartupConfigError JSDoc 写道：
one
code-based catch covers both this local rejection and the daemon's400 invalid_startup_configresponse
这在 REST 上成立，因为响应体里有 body.code。但在 ACP HTTP 和 ACP WS 上，SDK 的 DaemonHttpError.body 是 { error, data: { errorKind, httpStatus } }，没有顶层 code（实测结果见图 01 的 "S3 error body shape" 行）。走协商出来的 ACP transport 的调用方需要读 body.data.errorKind。这个响应体形状来自 ACP transport 里的 synthesizeResponse，在这个 PR 之前就是这样，对所有 ACP 错误都适用。以下任一做法即可：
- 在文档里写明两个位置；或
- 后续 PR 在 ACP 合成的响应体里把
errorKind同时写到code。
未覆盖
- 真实外部 provider。所有流量都走受控的回环假 provider。
- Windows。
- fallback、reload、skill 覆盖、冷恢复、transport 断连等边界。PR 自己也注明这些没有充分覆盖。
- 通过 ACP 创建 standalone 会话。SDK 的 standalone 固定走 REST，上一轮已验证过 standalone 回滚和同 ID 重试。
- SDK 对旧 daemon 的能力预检、与输入框行为在请求层面的一致性。上一轮已验证，本次增量也没有涉及。
复现
装置是两个独立脚本，每一行 PASS/FAIL 都是脚本里的比较结果：
步骤：
- 在
133ad52和f497b74各建一个 worktree。 - 在每个 worktree 里运行
pnpm install --frozen-lockfile && npm run build。 - 运行
node harness.mjs <worktree> <label>。
Evidence
01: head 133ad52, 27/27 across REST / ACP HTTP / ACP WS
02: control arm, previous head f497b74, 19/8 (the 8 failures are this round's fixes)
03: mutation check. Each new hunk removed turns exactly its checks red, and the dist is restored afterwards
04: focused unit and integration gates at head
…onfigError JSDoc (#12308) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Autofix round — PR #12308One feedback item was newer than the last evaluation: @wenshao's verification-round report ( Feedback dispositions
Changes
No conflicts with Verification
中文说明Autofix 本轮汇总 — PR #12308比上次评估更新的反馈只有一条：@wenshao 的验证轮报告（ 反馈处理结论
改动
未合并 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
Verification round 3: PR #12308 at head
|
| Check | Result |
|---|---|
Harness: real qwen serve + SDK over REST / ACP HTTP / ACP WS against a loopback fake provider. Covers model+high confirmation and wire retention over 2 turns, model-only creation, no phantom session after a rejection without a caller ID, control-character model ID → 400, raw ACP code -32602, the SDK DaemonStartupConfigError code, and settings.json staying unchanged. |
27/27 |
Control arm 133ad52 (round 2's verified head), same harness, same machine |
27/27; output identical to the new head's after replacing session IDs and ports |
M1+M2: in the built routes/session.js and acp-http/dispatch.js, the rollback ignores err.sessionId |
exactly the 3 "no phantom" checks turn red (24/3); files restored and confirmed with cmp |
M3: in the built acp-bridge/session-startup-config.js, drop /\p{Cc}/u |
exactly the 3 S4 checks turn red (422 instead of 400) (24/3); file restored and confirmed with cmp |
acp-bridge session-startup-config.test.ts |
33/33 |
sdk-typescript DaemonClient + DaemonClientStandalone + acpRouteTable |
637/637 |
cli: 7 PR-touched serve/session files |
1710/1710 |
cli server.test.ts, one run on its own |
1335/1335 |
integration-tests/cli/qwen-serve-routes.test.ts -t startup on the bundled CLI |
3/3 |
CI
All 57 review threads are resolved. At posting time, 24 checks pass, 2 are pending (web-shell E2E Smoke, review-pr), and 1 has failed: Integration Tests (no-AK, No Sandbox). That failure is infrastructure, not code. The job log shows runner ecs-qwen-hk4-18 got The runner has received a shutdown signal at 13:36:44, 71 seconds into the job, while npm run build was still running. No test ran. A rerun of that job should clear it. The PR's own integration tests passed locally above.
Not covered (unchanged from round 2)
Real external providers, Windows, fallback/reload/cold-restore/disconnect edges, standalone creation over ACP, and old-daemon preflight. The previous rounds covered the last two, and neither this round's PR commit nor anything the main merge brought in changes that behavior.
中文版
第 3 轮验证：PR #12308 在 head f4f5edd 上的结果
结论：无阻塞问题，可以合并。 自第 2 轮以来，PR 自身代码的唯一改动是针对第 2 轮观察项的 JSDoc 修正。因为合并 main 动到了 PR 的核心文件，我在新 head 上把第 2 轮的整套装置重跑了一遍。新 head 27/27 全过，输出和 133ad52 逐行一致（忽略会话 ID 和端口号）。变异检验的结果也和上一轮相同。
- 验证的 head：
f4f5eddeaae5368899d77fbff609cc6c3a382d75，即发帖时的 PR head。环境：macOS arm64，Node v24.18.1，全新 worktree，执行pnpm install --frozen-lockfile && npm run build && npm run bundle。
自 133ad52 以来的变化
7820525，PR 自身的改动：DaemonStartupConfigError的 JSDoc 增加 3 行注释，写明 daemon 返回的错误码在 REST 上位于body.code，在 ACP transport 上位于body.data.errorKind。这和本装置在新 head 上的实测一致：rest body.code=startup_config_rejected，acp-http/acp-ws body.data.errorKind=startup_config_rejected（见图 05 中 "S3 error body shape" 这几行）。第 2 轮的观察项已解决。 在synthesizeResponse里把errorKind同步写到code的方案被判为超出本 PR 范围，没有采纳。这个判断合理：这段 transport 代码在本 PR 之前就存在，并且作用于所有 ACP 错误。f4f5edd，合并main：带入了 74 个提交。合并后的 tree 与git merge-tree c9a9a8a 7820525自动生成的结果逐字节相同（4048f6d…），没有手工解决的冲突。但main改动了 PR 涉及的 12 个文件，包括Session.ts、routes/session.ts、capabilities.ts以及 SDK 的DaemonClient.ts/types.ts。所以我重跑了行为检查，而不是直接沿用第 2 轮的结论。
f4f5edd 上的结果
| 检查 | 结果 |
|---|---|
装置：真实 qwen serve 加 SDK，分别走 REST / ACP HTTP / ACP WS，provider 为回环假服务。覆盖：模型加 high 的确认及两轮请求中的保持、只选模型、调用方不带 ID 被拒后不残留幽灵会话、带控制字符的模型 ID 返回 400、ACP 原始错误码 -32602、SDK DaemonStartupConfigError 的错误码、settings.json 不被改动 |
27/27 |
对照臂 133ad52（第 2 轮验证过的 head），同一装置、同一台机器 |
27/27；把会话 ID 和端口号替换掉之后，输出与新 head 完全一致 |
M1+M2：在构建产物 routes/session.js 和 acp-http/dispatch.js 里，让回滚忽略 err.sessionId |
恰好 3 项「不残留幽灵会话」检查变红（24/3）；文件已还原并用 cmp 确认 |
M3：在构建产物 acp-bridge/session-startup-config.js 里删掉 /\p{Cc}/u |
恰好 3 项 S4 检查变红，返回 422 而不是 400（24/3）；文件已还原并用 cmp 确认 |
acp-bridge 的 session-startup-config.test.ts |
33/33 |
sdk-typescript 的 DaemonClient、DaemonClientStandalone、acpRouteTable |
637/637 |
cli：PR 改动的 7 个 serve/session 测试文件 |
1710/1710 |
cli 的 server.test.ts，单独跑一次 |
1335/1335 |
integration-tests/cli/qwen-serve-routes.test.ts -t startup，使用打包后的 CLI |
3/3 |
CI
57 个评审 thread 全部已解决。发帖时 24 项通过，2 项仍在运行（web-shell E2E Smoke、review-pr），1 项失败：Integration Tests (no-AK, No Sandbox)。这是基础设施问题，不是代码问题。job 日志显示，runner ecs-qwen-hk4-18 在 13:36:44（job 开始 71 秒后）收到 The runner has received a shutdown signal，当时 npm run build 还在执行，一个测试都没有跑。重跑这个 job 应该就能通过。PR 自己的集成测试已在上面本地跑过。
未覆盖（与第 2 轮相同）
真实外部 provider、Windows、fallback/reload/冷恢复/断连等边界、通过 ACP 创建 standalone 会话、旧 daemon 能力预检。后两项之前几轮已验证过，本轮的 PR 提交和合并进来的 main 改动都没有影响这部分行为。
Evidence
05: head f4f5edd, 27/27 across REST / ACP HTTP / ACP WS
06: control arm identical after the main merge; mutations M1+M2 and M3 each turn exactly their checks red
07: focused unit and integration gates at f4f5edd
The harness is unchanged from round 2: harness.mjs, fake-provider.mjs.
|
@qwen-code /triage |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Independent Critical-only review — head f4f5edde
Not approving, on budget alone. I want to be explicit that the recorded evidence here is as strong as any PR I have approved this week, and that the only thing missing is my own complete read of the diff.
What I verified myself
Nothing behavioural has changed since the last clean audit. Round 6 ran at 133ad526 (2026-09-24T13:48:40Z) with floor:"c" and returned total 0, CRITS 0, posted 0, fresh 0. Since then this PR has exactly one commit of its own, 78205258, which I checked file by file: one file, packages/sdk-typescript/src/daemon/session-startup-config.ts, +3/-0 — a JSDoc addition naming both transport error-code locations. No production logic changed. Head f4f5edde is a merge of main into the feature branch, and the 300-file delta between the two is upstream movement (76 merged PRs), not this PR's work.
The function that produced three separate Criticals is correct at head. session-startup-config.ts's rejectInvalidSelection was the anchor for R2-14 (only JSON-RPC -32602 mapped, an unregistered startup option falling elsewhere), R2-4 (a -32603 arm reclassifying every child-side internal error, including credential failures, as caller-caused) and R4-1 (a definite startup rejection unable to name the recording it should roll back). At this head:
if (error === null || typeof error !== 'object' || !('code' in error)
|| !('message' in error) || typeof error.message !== 'string') {
throw error;
}
// Only a deterministic parameter rejection is a definite, client-caused
// refusal … Internal errors (-32603) also carry auth, credential,
// timeout and transport failures — uncertain outcomes that stay unmapped.
if (error.code === -32602) {
throw new SessionStartupConfigError('startup_config_rejected', error.message, sessionId);
}
throw error;Only -32602 becomes the definite client-caused refusal. -32603 propagates unchanged, which is the correct answer for a code that also carries auth, credential, timeout and transport failures — mapping it would report an uncertain daemon-side outcome as the caller's fault. A malformed error (not an object, missing code or message, non-string message) is rethrown untouched rather than being coerced into a refusal. The function is typed never and every path throws, so no caller can mistake a rejection for success. Both setSessionConfigOption calls route their failures through it.
The record is clean. All 57 review threads are resolved with none outstanding. wenshao approved at this exact head f4f5edde at 2026-09-25T16:04:08Z. CI is green at head with only review-pr in progress, which is not a gate. Rounds 1-5 filed 1, 4, 2, 2 and 2 Criticals, and the ledger demonstrably carried standing blockers forward rather than dropping them — round 2 restated R1-1, round 3 carried R2-4, round 4 carried R4-2 into R5-1 — so round 6's zero is evidence those eleven closed, not evidence the round skipped them.
Why I am still not approving
My standard for an approval is that I have read the diff, and I have not. This is 3,084 additions across 42 files, and beyond rejectInvalidSelection I read only the commit-level metadata and the normalizer's modelServiceId.trim(). Unread by me at head: standalone-session-service.ts (R1-1's site — a failed startupConfig preparation reporting a definite creation failure), session-control-plane.ts around the POST /session path where R2-13 found a rejected startupConfig closing a live channel, acp-http/dispatch.ts where R3-1 found the session/new mapping never rolling back a persisted recording, bridgeTypes.ts and sdk-typescript/src/daemon/types.ts where R4-2/R5-1 turned on the modelApplied presence rule, Session.ts where R5-2 found the caller-caused-versus-daemon-fault split hand-rolled from core's human-readable text, the published approvalMode enum and protocol sentence behind R2-11/R2-12, and the control-character fail-closed validation that 133ad526 itself added.
Those are precisely the places this PR's own history concentrated, and several of them are the class where a wrong answer is silent — a presence rule that mis-states when a field appears, or a fault split that blames the caller for a credential failure. I am not willing to certify them on the strength of an audit round I did not run, even one I have good reason to trust, when I have two minutes left and a merge head.
I am applying the same standard I applied to #9661 earlier today, where I also declined to approve a diff whose latest audit round was clean at the head and whose threads were all resolved. The difference here is that I did verify the single highest-risk function myself, and that the post-audit delta is provably documentation — so this is a narrower gap than that one, and it should not be read as doubt about the code.
CI
Green at head with review-pr in progress; no failure attributable to this diff, and CI state is not the basis for this verdict either way.
Verdict: COMMENT — No Critical found, none recorded open, and the one function I read is correct. The outstanding gate is an independent Critical-only pass over the remaining ~3,000 lines, concentrated in the six files named above. Next step: re-request review at this head with budget for a full pass; on the recorded evidence — a zero-finding audit at a behaviourally identical commit, a maintainer approval at this head, and 57 of 57 threads resolved — this PR is close to the line and needs a reader, not a fix.
|
@qwen-code /triage |
…s-p0-p8 Brings in main through 53afcb6: QwenLM#12308 (model and reasoning selection at session creation) and QwenLM#12693 (durable Managed Session journal and failover), the latter split from this branch. QwenLM#12308 merges as is. Its startup configuration rides the branch's spawn path, including the deferred start used with paired execution engines. From QwenLM#12693, the review fixes are taken: - a harness-requested event of any kind needs an activation subject, while a trusted entry's message.committed does not; the commit digest rejects an empty or oversized event list before encoding; - sealing a Managed lease checks the commit proof before it reads the transcript; - title and source reads skip the Managed probe for a transcript already known to be legacy, and prefer the committed Managed record otherwise; - legacy rename, fork, recording and TUI resume refuse a Managed transcript up front; a restore reports an engine refusal as -32024 with the Session id, and the daemon answers it with 409. Where the branch already enforces the same rule, its implementation stays: the event and commit digests keep the Managed Tool canonical encoder (same bytes; main's pinned digest still matches), the actor check keeps its harness-subject helper, and loadCliConfig applies main's up-front legacy check only to a legacy host, since a Managed host restores through its own execution engine check.
6 participants
What this PR does
External callers can create ordinary and standalone sessions with
startupConfig: { modelServiceId, reasoningEffort }.modelServiceIdis required andreasoningEffortis optional. When omitted, creation selects only the model without calling the reasoning setter or substitutingdefault; this supports models without reasoning controls. Creation confirms the requested selection before returning or admitting an initial prompt, using composer behavior without saving shared defaults. The TypeScript SDK supports REST and daemon ACP HTTP/WS, checks the daemon capability, and rejects missing or inconsistent confirmation.Definite standalone startup rejection now removes its owned recording so the caller can retry a corrected request with the same ID. Timeouts and channel failures retain the existing outcome-unknown recovery. SDK confirmation mismatch after a readable successful response fails directly, without recovery/adoption.
Why it's needed
A successful legacy create does not jointly confirm model and reasoning selection; an old daemon can silently ignore the new input. Callers should be able to request a model with
high,default, ornoneand know that startup preparation succeeded before sending work. This is the narrowed PR 4 from #11944: subsequent prompts, manual changes, reload, fallback, skills, and restoration retain ordinary session behavior.Reviewer Test Plan
How to verify
high; expectmodelApplied: trueand a canonicalstartupConfigAppliedacknowledgment. The first model request and requests after tool results should use that pair, and a later turn should retain the selection.startupConfigApplied, without reasoning fields. An explicit unsupported reasoning selection must still fail. Repeat withdefaultandnone, including a saved shared preference ofnoneoverridden by requestedhigh. Compare the effective provider request with composer-equivalent model/reasoning selection.Evidence (Before & After)
Before: globally installed CLI
0.18.5-preview.0lackssession_startup_configand silently ignores the object. The candidate SDK rejects that daemon with zero POST mutations.After: build, typecheck, bundle, changed-file ESLint, and 972 focused unit/integration tests passed. Controlled-provider E2E captured 24 requests and passed model/reasoning confirmation, tool continuation, later turns, settings and sibling isolation, all three SDK transports, secondary-workspace ownership, composer-equivalent API/wire comparison, and manual changes. Two complete diff self-audits and a local medium review found no remaining actionable issues. A separate E2E report accompanies this PR.
Tested on
Environment
Node.js 22, local built daemon and SDK, isolated settings/runtime/workspaces, and a controlled local OpenAI-compatible provider. No browser UI automation or real external-provider calls were performed.
Risk & Scope
startupConfigkeep legacy behavior.Linked Issues
Refs #11944. This implements independent PR 4; it does not close the four-PR tracker.
中文说明
本 PR 的改动
外部调用方可以用
startupConfig: { modelServiceId, reasoningEffort }创建普通和 standalone 会话。modelServiceId必填，reasoningEffort可选。省略时只选择模型，不调用思考 setter，也不自动补default，支持没有思考控件的模型。创建返回或准入首条 prompt 前确认请求的选择，行为与输入框一致，但不保存共享默认值。TypeScript SDK 支持 REST 和 daemon ACP HTTP/WS，预检 daemon 能力，并拒绝缺失或不一致的确认。确定的 standalone 启动拒绝现在会删除本次持久记录，使调用方可以用同一 ID 修正后重试。超时和断连保留原有结果不确定恢复语义。SDK 收到可读成功响应但确认不匹配时直接报错，不触发恢复或采用该会话。
为什么需要
旧的创建成功不代表模型和思考强度共同确认生效；旧 daemon 还可能静默忽略新参数。调用方应能指定模型及
high、default或none，在发送工作前确认启动准备成功。这是 #11944 收敛后的 PR 4：后续提问、人工修改、reload、fallback、skill 和恢复保持普通会话行为。评审测试计划
如何验证
high创建普通会话，确认返回modelApplied: true及规范的startupConfigApplied。首个模型请求及工具结果之后的请求使用该组合，后续轮次保留选择。startupConfigApplied只含规范模型选择符、不含思考字段；显式传入不支持的思考选择仍应失败。重复测试default、none，以及共享设置为none、请求为high的情况；将实际 provider 请求与输入框等效的模型/思考选择流程比较。验证证据（改动前后）
改动前：全局 CLI
0.18.5-preview.0不支持session_startup_config，会静默忽略该对象。候选 SDK 对该 daemon 执行零 POST mutation 并返回能力缺失。改动后：build、typecheck、bundle、变更文件 ESLint 和 972 项聚焦单元/集成测试通过。受控 provider E2E 捕获 24 个请求，覆盖模型/思考确认、工具后续请求、后续轮次、设置和兄弟会话隔离、三种 SDK transport、第二个 workspace 归属、输入框等效 API/请求比较及人工修改。两轮完整 diff 自审和本地 medium review 未发现剩余可操作问题。本 PR 另附独立 E2E 报告。
测试平台
环境
Node.js 22、本地构建的 daemon 和 SDK、隔离设置/runtime/workspace，以及本地受控 OpenAI-compatible provider。未进行浏览器 UI 自动化或真实外部 provider 调用。
风险与范围
startupConfig的请求保持 legacy 行为。关联 Issue
Refs #11944。本次实现独立 PR 4，不关闭包含四个 PR 的跟踪 issue。