fix(daemon): Sanitize logs and type MCP restarts - #5006
Merged
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
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. |
wenshao
reviewed
Jun 12, 2026
doudouOUC
marked this pull request as ready for review
June 12, 2026 03:38
Collaborator
|
Thanks for the PR! Template looks good ✓ — all required sections present, bilingual, test plan included. On direction: this is a tight set of daemon-mode follow-ups — log sanitization, SDK type exposure, web shell compatibility, and a comment fix. All squarely within the daemon/SDK maintenance scope. No direction concerns. On approach: the scope is right for follow-ups. Four independent, small fixes bundled together — that's the correct granularity. The 中文说明感谢贡献！ 模板完整 ✓ — 所有必要部分齐全，双语，包含测试计划。 方向：这是一组紧凑的 daemon-mode follow-up —— 日志清洗、SDK 类型暴露、web shell 兼容性和注释修正，全部属于 daemon/SDK 维护范畴，无方向性问题。 方案：对于 follow-up 来说范围恰当。四个独立的小修复合并在一起，粒度正确。 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewThe diff is focused and consistent. The log sanitization in The The SDK The Unit TestsBuild & TypecheckCI (upstream)Real-Scenario Smoke Test (tmux)These changes are daemon-internal (ACP transport, SDK API, web shell types) — not directly visible in a standard CLI session. Smoke test confirms the CLI starts and responds normally under PR code: logSafe Primitive VerificationVerified the hardened regex against attack payloads: All control chars in the expanded Unicode ranges are correctly stripped; clean strings pass through unchanged. 中文说明代码审查diff 聚焦且一致。
SDK
单元测试构建与类型检查上游 CI真实场景冒烟测试 (tmux)这些变更是 daemon 内部的（ACP 传输层、SDK API、web shell 类型），在常规 CLI 会话中不直接可见。冒烟测试确认 CLI 在 PR 代码下仍能正常启动和响应： logSafe 原语验证用攻击载荷验证了加固后的正则： 扩展 Unicode 范围内的所有控制字符均被正确剥离；干净字符串原样通过。 — Qwen Code · qwen3.7-max |
Collaborator
|
This is a clean, well-scoped set of daemon follow-ups. The log sanitization fix addresses a real defense-in-depth concern — client-controlled session IDs and error messages were reaching stderr unsanitized beyond the C0 range. The hardened The SDK changes are purely additive and correctly typed. The web shell handles both the legacy single-server restart and the new pooled entries response with proper type narrowing. The All 233 unit tests pass, build and typecheck are clean across all packages, upstream CI is green on all three platforms, and the CLI smoke test shows no regressions. The three new commits since the last triage (hardened sanitization, lint fix, merge conflict resolution) are all incremental improvements that strengthen the PR. Approving. ✅ 中文说明这是一组干净、范围合理的 daemon follow-up。日志清洗修复了一个真实的纵深防御问题 —— 来自客户端可控的 session ID 和错误消息在 C0 范围之外未经清洗就写入 stderr。加固后的 SDK 变更是纯 additive 的，类型正确。Web shell 通过正确的类型缩窄同时处理传统的单 server 重启和新的 pooled entries 响应。 233 个单元测试全部通过，构建和类型检查在所有包上均通过，上游 CI 三个平台全绿，CLI 冒烟测试无回归。自上次 triage 以来新增的三个 commit（加固清洗、lint 修复、合并冲突解决）都是加强 PR 的增量改进。 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jun 12, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
previously approved these changes
Jun 12, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Local Verification Report — real-run (PR #5006)Verified the PR head merged into latest Environment: macOS Darwin 25.5.0, Node v22.22.2. Real-run tests drive a live Build / static / unit
Real-run 1 — SDK
|
| Call | URL the server actually received |
|---|---|
restartMcpServer('docs', { entryIndex: 3 }) |
✅ /workspace/mcp/docs/restart?entryIndex=3 |
restartMcpServer('docs', { entryIndex: '*' }) |
✅ /workspace/mcp/docs/restart?entryIndex=* |
restartMcpServer('docs') (omitted) |
✅ /workspace/mcp/docs/restart (no query — back-compat) |
restartMcpServer('a/b docs', { entryIndex: 7 }) |
✅ /workspace/mcp/a%2Fb%20docs/restart?entryIndex=7 (name encoded + query appended) |
The pooled { serverName, entries: [...] } response is returned and correctly narrowed by 'entries' in result (the new union arm): entries[0] = {entryIndex:3, restarted:true, durationMs:42}, entries[1] = {entryIndex:4, restarted:false, reason:'in_flight'}. All assertions pass.
Real-run 2 — log-injection reachability on a live daemon
Started a real qwen serve, drove _qwen/sessions/delete over real HTTP with a control-char session id "sess\nFAKE\r\x1b[31m", and inspected the daemon's real stderr:
| PR binary | main control |
|
|---|---|---|
| JSON-RPC response | notFound: ["sess\nFAKE\r�[31m"] (JSON-escaped, structured — not a log vector) |
identical |
sessions/delete stderr lines |
0 | 0 |
raw FAKE / control chars in stderr |
0 | 0 |
Finding (worth knowing before merge): with a bogus id, neither sanitized log line is reached on either build — closeSession(unknownId) throws SessionNotFoundError (→ treated as closed, not logged) and removeSessions rejects the id via SESSION_FILE_PATTERN = /^[0-9a-fA-F-]{32,36}\.jsonl$/ (→ notFound, not logged) before any writeStderrLine. So a control-char id is filtered to notFound ahead of the log sites. The sanitization changes what gets logged if a line is reached, not whether it's reached — it's correct defense-in-depth, not a fix for a wire-reachable injection via bogus ids.
Real-run 3 — happy path + Path-A/B reachability (live daemon)
- Happy path: created a real session, deleted it by its real UUID while the
qwen --acpchild was alive →closeSessionsucceeded (cleanclosing session "<uuid>" (reason: client_close)line, injection-safe viaJSON.stringify),removeSessions→notFound(freshly-created session isn't persisted to.jsonl). Zero error/failedlines, no injection. Endpoint works end-to-end; no regression. - Path A (closeSession-failed line): tried to force it by killing the live
qwen --acpchild, then deleting by real id. The bridge proactively tears the session down on channel exit (channel exited (signal=SIGKILL, 1 session(s) torn down)), so the subsequent close hitsSessionNotFoundError→notFound. I could not reach thecloseSession ... failedline this way; it needs a genuine close/teardown race (theHAZARDwindow the bridge code already calls out). - Path B (removeSessions-failed line): unreachable with a control-char id (pattern gate →
notFound); reaching it needs a persisted-but-unremovable session file, which requires a real model turn to first write the.jsonl.
Both sanitized lines are error-only paths. They are not wire-reachable with bogus/control-char input, but the PR's own unit tests cover both by mocking the throws (bridge.closeError for Path A; vi.spyOn(SessionService.prototype, 'removeSessions') returning an errors[] for Path B) — both pass on the merge tree.
Real-run 4 — logSafe primitive on the exact attack payloads (real built code)
Imported the built logSafe and ran the PR's test payloads through it:
sess\nFAKE\r\x1b[31m -> "sess FAK" (after slice(0,8)) / "sess FAKE [31m" (full)
close\nFAILED\r\x1b[31m -> "close FAILED [31m"
a\x85b\x7fc\x9fd -> "a b c d" (C1 controls + DEL stripped)
https://api.example.com/v1 -> unchanged
CR/LF/ESC/C0/C1/DEL all collapse to spaces; clean strings pass through. So if a sanitized line is reached, log-line splitting/forging is prevented — the security primitive is correct.
mcp-pool-key.ts comment audit
The corrected "Hashed fields" comment now lists exactly the 14 fields the fingerprint() canonical object actually hashes (transport, command, args, cwd, env, url, httpUrl, tcp, headers, timeout, oauth, authProviderType, targetAudience, targetServiceAccount). The PR adds the four that were missing from the old comment (tcp, authProviderType, targetAudience, targetServiceAccount) — verified line-by-line against the code. Comment now matches implementation.
Verdict
✅ Verified ready to merge. Real SDK wire behavior, real daemon happy-path, unit + typecheck, and the logSafe primitive all check out; the comment fix is accurate. The one nuance worth recording: the log-sanitization is sound defense-in-depth, but the specific "malicious session id forges log lines" vector is not reachable from the wire with a bogus id (the id is filtered to notFound before the log sites) — the genuinely reachable concern is a control-char error message surfacing on a real close/remove failure, which the fix does cover. No blockers.
Repro
git fetch origin main +refs/pull/5006/head:pr-5006
git worktree add /tmp/qwen-pr5006 origin/main --detach
cd /tmp/qwen-pr5006 && git merge --no-edit pr-5006 && npm ci && npm run build
# unit + typecheck
(cd packages/cli && npx vitest run src/serve/acpHttp/transport.test.ts)
(cd packages/sdk-typescript && npx vitest run test/unit/DaemonClient.test.ts)
# real SDK wire test: built DaemonClient -> real http.createServer, assert ?entryIndex=N
# real daemon: qwen serve --port 0 in tmux (isolated HOME, openai auth seeded so
# session/new can spawn the acp child), drive _qwen/sessions/delete over HTTP/SSE,
# inspect daemon stderr for control-char leakage.中文摘要
在 PR head 合入最新 main 的合并树上做了真实验证(tmux 跑真实 qwen serve daemon,真实 HTTP/SSE 驱动;真实构建的 SDK 走真实 TCP)。
- 构建、cli/core/sdk-typescript 类型检查、transport 85/85、DaemonClient 144/144 全绿;web-shell 仅 2 个预存错误,在 PR 未触碰的测试文件里,PR 改的
McpDialog.tsx/i18n.tsx干净。 - SDK entryIndex 真实线测试:真实
DaemonClient实际发出?entryIndex=3、?entryIndex=*,省略时无 query,server 名正确百分号编码,pooledentries结果正确返回并定型。 - 真实 daemon 日志注入可达性:带控制字符的伪造 session id →
notFound,daemon stderr 0 条sessions/delete日志、0 处FAKE/控制字符泄漏;PR 与 main 一致。原因:closeSession落空抛SessionNotFoundError(不记日志),removeSessions被SESSION_FILE_PATTERN挡成notFound(不记日志),在写日志之前就被过滤掉。 - Path A/B 实测:两条被 sanitize 的日志都是“仅错误”路径。杀 acp 子进程时 bridge 主动拆除 session(→
SessionNotFound),无法借此触发 Path A;控制字符 id 也到不了 Path B 的 pattern 门。两条路径不可从 wire 用伪造输入触达,但 PR 自带单测用 mock 抛错覆盖了它们,合并树上均通过。 - logSafe 原语(真实构建代码):CR/LF/ESC/C1/DEL 全部塌成空格,干净串不变——日志行确实被触达时可防伪造/拆行。
- mcp-pool-key 注释:修正后的 14 字段与
fingerprint()实际 hash 字段逐字一致,补上了旧注释漏的tcp/authProviderType/targetAudience/targetServiceAccount。
结论:✅ 可以合并。 日志清洗是稳妥的纵深防御;唯一需要记录的细节是“伪造 session id 伪造日志行”这一向量用伪造 id 从 wire 不可达(id 在写日志前已被过滤为 notFound),真正可达的是真实 close/remove 失败时错误信息里的控制字符——而这正是本 PR 覆盖到的。无阻塞项。
Collaborator
Follow-up Real-Run Verification — live daemon with a real MCP pool (PR #5006)This complements the verification report above: that run covered the merge tree, the SDK's URL construction against a stub HTTP server, and log-injection reachability. This run drives the actual Environment: PR head Baseline (agrees with the earlier report)Full build ✅ · repo 1. SDK
|
| Call (built SDK) | Live daemon result |
|---|---|
restartMcpServer('docs') |
✅ {serverName:'docs', restarted:true, durationMs:284} |
restartMcpServer('docs', {entryIndex: 0}) |
✅ restarted (legacy path allows index 0) |
restartMcpServer('docs', {entryIndex: '*'}) |
✅ restarted (route maps * → all) |
restartMcpServer('docs', {entryIndex: 3}) |
✅ typed error from the agent child: Invalid params: entryIndex=3 requested but pool not active for "docs" — legacy single-entry path only supports entryIndex=0 or undefined |
restartMcpServer('nosuch') |
✅ typed 404 MCP server not configured: "nosuch" |
The entryIndex=3 rejection is the strongest forwarding proof available: the error text originates in acpAgent.ts, so the query parameter demonstrably traveled SDK → HTTP route → workspace command → bridge → agent child and back.
Pool path (after createOrAttachSession spawned a real session; GET /workspace/mcp showed docs with entryCount:1, entrySummary:[{entryIndex:0, refs:1, status:'connected'}]):
| Call (built SDK) | Live daemon result |
|---|---|
restartMcpServer('docs') |
✅ pooled shape: {serverName:'docs', entries:[{entryIndex:0, restarted:true, durationMs:299}]} |
restartMcpServer('docs', {entryIndex: 0}) |
✅ entries:[{entryIndex:0, restarted:true, durationMs:290}] |
restartMcpServer('docs', {entryIndex: 7}) |
✅ soft signal entries: [] (documented no-match behavior) |
restartMcpServer('docs', {entryIndex: '*'}) |
✅ all entries restarted |
Supporting evidence:
- Real process rotation: the
docsMCP child PID changed12485 → 13631across a pooled restart — the OS process genuinely restarts. - Route validation (raw curl):
entryIndexof-1,abc,1.5,0x2all →400 {"code":"invalid_entry_index"}; the typed SDK can't even express the non-numeric ones. - Capability preflight: live
/capabilitiesadvertisesworkspace_mcp_restart,mcp_workspace_pool, andmcp_pool_restart(notefeaturesis an array of tags — preflight withcaps.features.includes(...)). - Operational note (pre-existing, not this PR):
POST /workspace/mcp/:server/restartis a strict-mutate route — it refuses to run until the daemon is configured with a bearer token, even on loopback. SDK callers neednew DaemonClient({baseUrl, token}).
2. removeSessions stderr path triggered live (upgrade over unit-mock coverage)
The earlier report noted this line wasn't reachable in its setup. It is reachable without a model turn: I planted a pattern-valid session file (bbbb….jsonl, 32 hex chars, first record's cwd matching the bound workspace) and made the chats directory read-only, so fs.unlinkSync fails with a real EACCES. Deleting that id together with a hostile control-char id in the same batch produced, on the live daemon:
-
JSON-RPC response unchanged in shape: the EACCES surfaced per-id in
errors:[{sessionId, error:"EACCES: permission denied, unlink '…'"}]; the hostile id ("sess\nFAKE-LOG injected\r\x1b[31mRED") landed JSON-escaped innotFound. -
Daemon stderr (raw bytes via
cat -v): exactly one clean line through the rewritten call site, no CR/LF/ESC anywhere:qwen serve: /acp sessions/delete removeSessions(bbbbbbbb) failed: EACCES: permission denied, unlink '/…/chats/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb.jsonl' -
Zero stderr output attributable to the hostile id.
For the closeSession line I reproduced the earlier report's finding: SIGKILL-ing the live agent child makes the bridge tear the session down first (channel exited (signal=SIGKILL, 1 session(s) torn down)), so the close lands in SessionNotFoundError → notFound. That path stays covered by the PR's unit test (FakeBridge throwing control-char errors), and logSafe itself is the same shared primitive already used by ~20 other call sites.
3. Web shell pooled message — real compiled bundle, real payloads
Extracted the compiled mcp.restartEntries EN/ZH message functions from the built bundle (web-shell/dist/assets/index-*.js) and executed them through McpDialog's exact mapping with the live daemon responses captured above:
| Input | EN | ZH |
|---|---|---|
| real pooled response (1 entry) | Restarted 1/1 docs entries |
已重启 docs 的 1/1 个条目 |
real entries: [] (entryIndex=7) |
Restarted 0/0 docs entries |
已重启 docs 的 0/0 个条目 |
| mixed fixture (SDK test shape) | Restarted 1/2 docs entries (failed: #4: in_flight) |
已重启 docs 的 1/2 个条目（失败：#4: in_flight） |
Type-chain check: the web shell's restartServer is typed Promise<DaemonMcpRestartResult> via @qwen-code/webui → imported from @qwen-code/sdk/daemon — i.e. the dialog's 'entries' in result narrows the exact union this PR extends (no any laundering). Branch order is right (entries → restarted → skipped); before this fix a pooled result would have fallen into the skipped branch and rendered Skipped docs: with an empty reason. The 0/0 rendering can only occur if the pool drains mid-call (web shell never sends entryIndex) — cosmetic and consistent with the PR's stated tradeoff.
4. mcp-pool-key.ts comment
Verified line-by-line against fingerprint(): the canonical object hashes exactly the 14 fields the new comment lists; the 4 added ones (tcp, authProviderType, targetAudience, targetServiceAccount) were the missing ones. Comment-only, now accurate.
Verdict
✅ Ready to merge. Everything the PR claims now has live-daemon evidence: entryIndex forwards end-to-end and the pooled entries union matches what the real daemon returns; the rewritten removeSessions stderr line fired in production wiring with a real fs error, stayed one clean line, and left the wire response intact; the web shell renders all three result shapes correctly in both languages; the comment fix matches the code. The only path not live-triggerable is the closeSession failure line (the bridge's teardown-on-exit is simply too robust to race from outside), which the PR's unit test covers.
Repro
gh pr checkout 5006 && npm install && npm run build
# scratch workspace with a real stdio MCP server
mkdir -p /tmp/qwen5006/ws/.qwen
cat > /tmp/qwen5006/ws/.qwen/settings.json <<'EOF'
{"mcpServers":{"docs":{"command":"node","args":["/tmp/qwen5006/mcp-docs-server.mjs"]}}}
EOF
# mcp-docs-server.mjs: McpServer + StdioServerTransport, one "lookup" tool
tmux new-session -d -s pr5006 -c /tmp/qwen5006/ws \
"node $PWD/packages/cli/dist/index.js serve --port 4271 --token T 2>/tmp/qwen5006/serve.stderr.log"
# SDK matrix: built dist DaemonClient -> restartMcpServer with no opts / 0 / 3 / '*' / unknown,
# then createOrAttachSession({}) and repeat to get the pooled {entries:[...]} shape.
# curl matrix: ?entryIndex=-1|abc|1.5|0x2 -> 400 invalid_entry_index
# removeSessions live trigger: plant <32-hex>.jsonl with matching cwd record in
# ~/.qwen/projects/<sanitized-ws>/chats, chmod 555 the chats dir,
# then _qwen/sessions/delete over ACP HTTP; inspect stderr with cat -v.中文版（完整翻译）
补充真实运行验证 —— 真实 daemon + 真实 MCP pool（PR #5006）
本报告补充上面那份验证报告：前一份覆盖了合并树、SDK 对 stub HTTP server 的 URL 构造、以及日志注入可达性分析；本次则端到端驱动真实的 qwen serve daemon —— 真实 stdio MCP server、真实创建 session 使 transport pool 持有 entry、真实的 pooled restart 响应，以及真实触发改写后的 removeSessions stderr 日志行（前一份只能靠单测 mock 覆盖该行）。
环境： PR head e35d782d3，macOS（Darwin 25.5.0），Node v22.22.2。PR 分支落后 main 9 个 commit，但均未触碰 PR 涉及的路径，head 即可代表合并后形态。daemon 在 tmux 中启动（qwen serve --port 4271 --token …），绑定的 scratch workspace 在 .qwen/settings.json 中配置了真实 stdio MCP server（docs，基于 @modelcontextprotocol/sdk）。所有 SDK 调用均使用构建产物 @qwen-code/sdk dist。
基线（与前一份报告一致）
全量构建 ✅ · 全仓 tsc --noEmit ✅ · 8 个改动文件 eslint 零问题 ✅ · transport.test.ts 85/85 ✅（含 2 个新清洗测试）· DaemonClient.test.ts 144/144 ✅（含 2 个新 entryIndex 测试）· 上游 CI 三平台全绿（含 Windows）✅
1. SDK entryIndex 对真实 daemon（非 stub）
Legacy 路径（尚无 session、pool 无 entry 时）：无参 → {restarted:true, durationMs:284}；entryIndex:0 → 正常重启；'*' → 正常重启；entryIndex:3 → 收到来自 agent 子进程的类型化报错 entryIndex=3 requested but pool not active for "docs" … —— 这是最强的转发证据：报错文案产自 acpAgent.ts，说明查询参数确实走完了 SDK → HTTP 路由 → workspace command → bridge → agent 子进程全链路；nosuch → 类型化 404。
Pool 路径（createOrAttachSession 真实建 session 后，GET /workspace/mcp 显示 docs entryCount:1、entrySummary:[{entryIndex:0, refs:1, status:'connected'}]）：无参 → pooled 形状 {serverName:'docs', entries:[{entryIndex:0, restarted:true, durationMs:299}]}；entryIndex:0 → 单条 entry 重启；entryIndex:7 → 软信号 entries: []（符合文档化行为）；'*' → 全部重启。
佐证：pooled 重启后 docs MCP 子进程 PID 真实轮换（12485 → 13631）；raw curl 下 entryIndex 取 -1/abc/1.5/0x2 均 → 400 invalid_entry_index；线上 /capabilities 广播 workspace_mcp_restart、mcp_workspace_pool、mcp_pool_restart（注意 features 是数组，预检应使用 caps.features.includes(...)）。运维备注（预存行为，非本 PR）：restart 属 strict-mutate 路由，即使 loopback 也要求 daemon 配置 bearer token，SDK 调用方需要 new DaemonClient({baseUrl, token})。
2. 真实触发 removeSessions stderr 路径（比单测 mock 更进一步）
前一份报告认为该行在其环境下不可触达；实际上无需模型对话即可触达：放置一个符合 pattern 的 session 文件（bbbb….jsonl，32 位 hex，首记录 cwd 与绑定 workspace 一致），再把 chats 目录设为只读，使 fs.unlinkSync 真实抛出 EACCES。将该 id 与一个含控制字符的敌意 id 放进同一批删除请求后，真实 daemon 上观察到：
- JSON-RPC 响应形状不变：EACCES 按 id 进入
errors:[{sessionId, error:"EACCES…"}]；敌意 id（"sess\nFAKE-LOG injected\r\x1b[31mRED"）以 JSON 转义形式进入notFound； - daemon stderr（
cat -v查看原始字节）：经过改写后的调用点输出恰好一行干净日志，无任何 CR/LF/ESC：qwen serve: /acp sessions/delete removeSessions(bbbbbbbb) failed: EACCES: permission denied, unlink '…'； - 敌意 id 未产生任何 stderr 输出。
closeSession 行复现了前一份报告的结论：SIGKILL 存活的 agent 子进程后，bridge 会先行拆除 session（channel exited (signal=SIGKILL, 1 session(s) torn down)），后续 close 落入 SessionNotFoundError → notFound。该路径继续由 PR 自带单测（FakeBridge 抛控制字符错误）覆盖，且 logSafe 是已有约 20 处调用点共用的原语。
3. Web shell pooled 消息 —— 真实编译产物 + 真实载荷
从构建出的 bundle（web-shell/dist/assets/index-*.js）中提取编译后的 mcp.restartEntries 英/中消息函数，按 McpDialog 的映射逻辑用上面捕获的真实 daemon 响应执行：真实 pooled（1 entry）→ Restarted 1/1 docs entries / 已重启 docs 的 1/1 个条目；真实 entries: [] → Restarted 0/0 docs entries / 已重启 docs 的 0/0 个条目；混合成功/失败（SDK 测试形状）→ Restarted 1/2 docs entries (failed: #4: in_flight) / 已重启 docs 的 1/2 个条目（失败：#4: in_flight）。
类型链检查：web shell 的 restartServer 经 @qwen-code/webui 类型化为 Promise<DaemonMcpRestartResult>，类型源自 @qwen-code/sdk/daemon —— 即 dialog 的 'entries' in result 收窄的正是本 PR 扩展的那个 union（不存在 any 洗型）。分支顺序正确（entries → restarted → skipped）；修复前 pooled 结果会落入 skipped 分支渲染出 reason 为空的 Skipped docs: 。0/0 只会在调用中途 pool 排空时出现（web shell 不发送 entryIndex），属外观问题，与 PR 声明的取舍一致。
4. mcp-pool-key.ts 注释
逐行对照 fingerprint()：canonical 对象 hash 的恰好是新注释列出的 14 个字段；新增的 4 个（tcp、authProviderType、targetAudience、targetServiceAccount）正是旧注释遗漏的。纯注释改动，现已准确。
结论
✅ 可以合并。 PR 的每项声明现在都有真实 daemon 证据：entryIndex 端到端转发、pooled entries union 与真实 daemon 返回一致；改写后的 removeSessions stderr 行在生产线路上被真实 fs 错误触发，保持单行干净、wire 响应不变；web shell 对三种结果形状的双语渲染全部正确；注释修正与代码一致。唯一无法从外部真实触发的是 closeSession 失败行（bridge 的 teardown-on-exit 健壮到无法从外部抢跑），该路径由 PR 单测覆盖。
wenshao
previously approved these changes
Jun 12, 2026
qwen-code-ci-bot
requested changes
Jun 12, 2026
qwen-code-ci-bot
previously approved these changes
Jun 12, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
previously approved these changes
Jun 12, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
previously approved these changes
Jun 12, 2026
Collaborator
Author
|
@qwen-code /review |
Contributor
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/27405038120)._ |
This was referenced Jun 12, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
reviewed
Jun 12, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running. LGTM! ✅ — qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@qwen-code /review |
Contributor
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/27429609482)._ |
wenshao
approved these changes
Jun 12, 2026
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jun 12, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
added a commit
that referenced
this pull request
Jun 15, 2026
* fix(daemon): Sanitize ACP delete logs and type MCP restarts Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(daemon): Cover PR review edge cases Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(web-shell): Show MCP restart entry failures Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(daemon): Harden ACP delete log sanitization Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(daemon): Fix ACP log sanitizer lint Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
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
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
This PR addresses the small daemon-mode follow-ups after the previous branch merged. It sanitizes ACP session deletion stderr messages before writing operator logs, exposes the MCP pool-entry restart response shape and
entryIndexquery parameter through the TypeScript SDK, keeps the web shell restart message compatible with pooled results, and corrects the MCP pool fingerprint comment so it matches the fields actually hashed.Why it's needed
The ACP session deletion path accepted client-controlled session ids and bridge error messages into stderr, which could split or forge log lines when those values contained control characters. SDK callers also could not target a specific pooled MCP entry or type the pooled restart result that the daemon can now return. The comment update keeps maintainers from relying on an incomplete fingerprint field list.
Reviewer Test Plan
How to verify
Reviewers can confirm that
_qwen/sessions/deletestill returns the same JSON-RPC response while stderr output no longer contains newlines, carriage returns, or escape characters from malicious session ids or close errors. Reviewers can also confirm thatrestartMcpServer('docs', { entryIndex: 3 })sends?entryIndex=3, accepts anentriesrestart payload, and that web shell code handles both single-server and pooled restart responses.Evidence (Before & After)
Before: ACP close failure logs could include raw control characters from the session id or error message, and the SDK type did not represent pooled MCP restart results. After: the focused ACP test verifies the sanitized one-line stderr output, the SDK test verifies entry-index forwarding and pooled result typing, and the web shell build accepts the new union shape with the pooled message rendered as
Restarted 1/2 docs entriesor已重启 docs 的 1/2 个条目.Tested on
Environment (optional)
macOS 26.4.1, Node.js v26.0.0, npm 11.12.1. Verified with
cd packages/cli && npx vitest run src/serve/acpHttp/transport.test.ts,cd packages/sdk-typescript && npx vitest run test/unit/DaemonClient.test.ts,npm run build,npm run typecheck, andnpm run build --workspace=@qwen-code/sdk.Risk & Scope
Linked Issues
N/A
中文说明
What this PR does
这个 PR 处理前一个 daemon-mode 分支合并后的几个小 follow-up。它会在写入操作员 stderr 日志前清洗 ACP session 删除路径的错误信息，把 MCP pool-entry restart 的返回结构和
entryIndex查询参数暴露给 TypeScript SDK，让 web shell 的重启提示兼容 pooled 结果，并修正 MCP pool fingerprint 注释，使注释列出的字段和实际参与 hash 的字段一致。Why it's needed
ACP session 删除路径会把客户端可控的 session id 和 bridge 错误信息写入 stderr；如果这些值包含控制字符，就可能拆分或伪造日志行。SDK 调用方之前也无法指定要重启的 pooled MCP entry，类型里也没有表达 daemon 已经可能返回的 pooled restart 结果。注释更新则避免维护者依赖一份不完整的 fingerprint 字段列表。
Reviewer Test Plan
How to verify
Reviewer 可以确认
_qwen/sessions/delete仍然返回相同的 JSON-RPC 响应，同时 stderr 输出不会包含来自恶意 session id 或 close error 的换行、回车或 escape 字符。Reviewer 也可以确认restartMcpServer('docs', { entryIndex: 3 })会发送?entryIndex=3，可以接收entriesrestart payload，并且 web shell 代码能同时处理单 server 和 pooled restart 响应。Evidence (Before & After)
Before：ACP close failure 日志可能包含来自 session id 或错误信息的原始控制字符，SDK 类型也没有表达 pooled MCP restart 结果。After：focused ACP 测试验证了清洗后的单行 stderr 输出，SDK 测试验证了 entry-index 转发和 pooled result typing，web shell build 可以接受新的 union 结构，并把 pooled message 渲染为
Restarted 1/2 docs entries或已重启 docs 的 1/2 个条目。Tested on
Environment (optional)
macOS 26.4.1，Node.js v26.0.0，npm 11.12.1。已使用
cd packages/cli && npx vitest run src/serve/acpHttp/transport.test.ts、cd packages/sdk-typescript && npx vitest run test/unit/DaemonClient.test.ts、npm run build、npm run typecheck和npm run build --workspace=@qwen-code/sdk验证。Risk & Scope
Linked Issues
N/A