Skip to content

fix(serve): Coordinate caller-supplied session IDs - #8415

Merged
doudouOUC merged 6 commits into
QwenLM:mainfrom
doudouOUC:codex/session-id-complete
Aug 9, 2026
Merged

fix(serve): Coordinate caller-supplied session IDs#8415
doudouOUC merged 6 commits into
QwenLM:mainfrom
doudouOUC:codex/session-id-complete

Conversation

@doudouOUC

Copy link
Copy Markdown
Collaborator

What this PR does

This PR completes caller-supplied daemon session IDs as one daemon-wide contract across REST, primary and workspace-qualified ACP, live workspace generations, the shared stdio agent, the official TypeScript and Java daemon SDKs, and daemon MCP. It validates RFC-variant UUID v1-v5 values, normalizes them to lowercase, forces explicit IDs to create fresh thread sessions, and verifies that downstream creation honored the requested ID.

Creation and recovery now share one admission coordinator. Creation rejects conflicts with live bridges, in-flight operations, active or archived transcripts, and worktree-backed history across every currently registered runtime. Recovery can share an in-flight claim only inside the same bridge generation; another workspace or generation receives the existing workspace-conflict contract. Draining and replaced bridge generations remain visible until their shutdown is confirmed, and runtime-specific persistence checks use each generation's pinned output directory.

ACP creation accepts session/new._meta["qwen-code/sessionId"], while load and resume participate in the same recovery admission as REST. The bridge and stdio agent provide defense in depth for direct callers, return structured conflicts without terminating the shared child, and clean up a newly created orphan if a downstream component returns a different ID. Clients negotiate the new session_id_override capability before mutation and verify successful responses; older daemons therefore cannot silently ignore the optional field.

Why it's needed

PR #7836 established the REST creation path, but route-local coordination could still allow REST and ACP to race, reuse an ID in another workspace, or overlap recovery across runtime generations. Those duplicates make session ownership ambiguous and can let one transport interfere with a session created by another. SDK callers also lacked a negotiated way to request an ID and detect a daemon that ignored it. This change closes those gaps as a single coherent behavior instead of leaving transport- or workspace-specific follow-ups.

Reviewer Test Plan

How to verify

  1. Create a session through raw REST with a mixed-case UUID and confirm the response returns the lowercase UUID with attached: false, even if the request asks for single scope. Repeat the same UUID through REST or ACP and confirm a stable 409 session_id_conflict without affecting the first session.
  2. Exercise primary and workspace-qualified ACP concurrently with the same requested ID, then repeat while an old workspace generation is draining. Confirm only one creation succeeds and cross-generation recovery returns session_workspace_conflict.
  3. Persist and archive sessions under runtime-specific output directories and worktree sidecars, restart the daemon, and confirm duplicate creation is rejected while load/resume still restores the known ID.
  4. Send invalid and duplicate requested-ID metadata directly to the stdio ACP agent. Confirm it returns structured INVALID_PARAMS, performs no settings or filesystem work before rejection, and keeps sibling sessions and the shared child alive.
  5. Use the TypeScript REST and ACP transports, Java builder, and daemon MCP session_create.session_id. Confirm they refuse to mutate when session_id_override is absent, serialize the requested ID when present, and reject a success response containing a different ID.

Local verification passed: CLI-focused Vitest 730/730, ACP bridge Vitest 469/469, TypeScript SDK Vitest 477/477, Java Maven tests 127/127 with 5 environment-dependent skips, and real bundled-daemon route integration 36/36. npm run build, npm run bundle, npm run typecheck, and npm run lint also passed.

Evidence (Before & After)

N/A — this changes daemon protocols, SDKs, and concurrency behavior without a TUI or other visual surface.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

macOS 26.4.1, Node.js 22.22.3, npm 10.9.8, and Java 21.0.8. The daemon route integration ran against the rebuilt production bundle with sandboxing disabled.

Risk & Scope

  • Main risk or tradeoff: The change coordinates session identity across several concurrency and runtime-lifecycle boundaries, so the main risk is an overly broad conflict or a reservation that is not released after failure; identity-bound releases, restore reference counts, generation guards, orphan cleanup, and cross-transport tests cover those paths.
  • Not validated / out of scope: Authenticated model prompting through every client, Windows and Linux local runs, UUID v7, migration of historical duplicate IDs, and a persistent daemon-global ID index are out of scope. The existing workspace-qualified owner resolution remains responsible for historical duplicates discovered when a workspace is registered later.
  • Breaking changes / migration notes: No existing request field or session format changes. The new field is optional; clients that use it must observe session_id_override. An explicit ID always means create a fresh thread session rather than idempotently attach, and callers with an ambiguous creation outcome should load or resume the known ID.

Linked Issues

Closes #8411

Completes the caller-supplied ID work introduced by #7836.

中文说明

本 PR 做了什么

本 PR 将调用方指定 daemon session ID 的能力完整收口为一个 daemon 级统一契约,覆盖 REST、primary 与 workspace-qualified ACP、仍存活的 workspace generation、共享 stdio agent、官方 TypeScript 与 Java daemon SDK,以及 daemon MCP。实现会校验 RFC variant 的 UUID v1-v5、统一转为小写、强制显式 ID 创建全新的 thread session,并核验下游实际创建结果是否采用了请求的 ID。

创建与恢复现在共享同一个 admission coordinator。创建会检查所有当前注册 runtime 中的 live bridge、进行中的操作、active/archived transcript 和 worktree-backed history 并拒绝冲突。恢复仅允许同一个 bridge generation 共享进行中的 claim;另一个 workspace 或 generation 会沿用现有 workspace conflict 契约。Draining 或已被替换的 bridge generation 在确认 shutdown 前始终可见,并且 runtime 级持久化检查使用各 generation 固定的输出目录。

ACP 创建支持 session/new._meta["qwen-code/sessionId"],load 和 resume 也会与 REST 一起进入同一恢复 admission。Bridge 与 stdio agent 为直接调用方提供纵深防御,返回结构化冲突而不终止共享 child,并在下游返回不同 ID 时清理本次新建的 orphan。客户端会在 mutation 前协商新的 session_id_override capability,并再次核验成功响应,因此旧 daemon 无法静默忽略这个可选字段。

为什么需要它

PR #7836 建立了 REST 创建路径,但路由局部的协调仍可能让 REST 与 ACP 竞态、在另一个 workspace 重用同一 ID,或让不同 runtime generation 的恢复过程重叠。这类重复会造成 session owner 歧义,也可能让一种 transport 干扰由另一种 transport 创建的 session。SDK 调用方同样缺少经过 capability 协商的指定 ID 能力,无法发现 daemon 静默忽略字段。本变更将这些缺口作为一个一致行为统一解决,不再遗留 transport 或 workspace 维度的 follow-up。

Reviewer 测试计划

如何验证

  1. 通过 raw REST 使用 mixed-case UUID 创建 session,确认响应返回小写 UUID 且 attached: false,即使请求指定 single scope 也一样。随后通过 REST 或 ACP 再次使用同一 UUID,确认稳定返回 409 session_id_conflict,且第一个 session 不受影响。
  2. 使用 primary 和 workspace-qualified ACP 并发请求同一个指定 ID,然后在旧 workspace generation draining 时重复验证。确认只有一个创建成功,跨 generation 恢复返回 session_workspace_conflict
  3. 在 runtime-specific 输出目录和 worktree sidecar 下持久化并归档 session,重启 daemon,确认重复创建被拒绝,而 load/resume 仍能恢复已知 ID。
  4. 直接向 stdio ACP agent 发送非法和重复的 requested-ID metadata。确认它返回结构化 INVALID_PARAMS,拒绝前不执行 settings 或 filesystem 工作,并保持 sibling session 和共享 child 存活。
  5. 分别使用 TypeScript REST/ACP transport、Java builder 与 daemon MCP 的 session_create.session_id。确认缺少 session_id_override 时不会发出 mutation,capability 存在时正确序列化指定 ID,并在成功响应包含不同 ID 时拒绝该响应。

本地验证已通过:CLI 定向 Vitest 730/730、ACP bridge Vitest 469/469、TypeScript SDK Vitest 477/477、Java Maven 测试 127/127(另有 5 个依赖环境的 skip),以及真实 bundle daemon 路由集成测试 36/36。npm run buildnpm run bundlenpm run typechecknpm run lint 也全部通过。

证据(Before & After)

N/A——本变更涉及 daemon protocol、SDK 与并发行为,不包含 TUI 或其他可视化界面变化。

测试平台

OS 状态
🍏 macOS ✅ 已测试
🪟 Windows ⚠️ 未测试
🐧 Linux ⚠️ 未测试

环境(可选)

macOS 26.4.1、Node.js 22.22.3、npm 10.9.8、Java 21.0.8。Daemon 路由集成测试使用重新构建的 production bundle,并关闭 sandbox。

风险与范围

  • 主要风险或权衡:本变更跨越多个 session identity 并发与 runtime lifecycle 边界,因此主要风险是冲突判定过宽,或失败后 reservation 未释放;identity-bound release、restore 引用计数、generation guard、orphan cleanup 和跨 transport 测试覆盖了这些路径。
  • 未验证/范围外:未在每个客户端上执行带真实认证的模型 prompt,未进行 Windows 与 Linux 本地运行;UUID v7、历史重复 ID 迁移和持久化 daemon-global ID 索引均不在范围内。以后注册 workspace 时发现的历史重复仍由现有 workspace-qualified owner resolution 处理。
  • Breaking change/迁移说明:没有修改现有请求字段或 session 格式。新字段是可选的;使用它的客户端必须检查 session_id_override。显式 ID 始终表示创建一个全新的 thread session,而不是幂等 attach;如果创建结果不确定,调用方应使用已知 ID 执行 load 或 resume。

关联 Issue

Closes #8411

本 PR 完成由 #7836 引入的 caller-supplied ID 工作。

@doudouOUC

Copy link
Copy Markdown
Collaborator Author

Verification report

Validated commit 6dbc0493ef21ea3034661ae74fe4d649e6792882 after rebasing onto the latest origin/main.

Automated checks

  • CLI caller-supplied ID parser, daemon admission, ACP HTTP, workspace-qualified ACP, and stdio-agent focused Vitest: 5 files, 730/730 passed.
  • ACP bridge focused Vitest: 469/469 passed.
  • TypeScript SDK ACP/REST transport, daemon client, route mapping, and MCP bridge focused Vitest: 4 files, 477/477 passed.
  • Java SDK Maven suite: 127/127 passed; 5 environment-dependent tests skipped.
  • Rebuilt production bundle with sandbox disabled, then ran the real daemon route integration suite: 36/36 passed, including mixed-case fixed-ID creation, capability advertisement, repeat conflict, TypeScript REST creation, and TypeScript ACP creation.
  • npm run build, npm run bundle, npm run typecheck, npm run lint, and git diff --check origin/main...HEAD passed.

Manual review

Two consecutive open-ended diff audits completed after the rebase. The audits covered runtime ownership, draining generation visibility, pending-state identity and reference counts, generation-change rollback, orphan cleanup, ACP/REST error mapping, capability gates, SDK response verification, public documentation, and test assertions. Both final passes were clean.

Not run locally

Windows and Linux local runs, plus authenticated real-model prompting through every client surface, were not run. The affected transport, persistence, concurrency, and shared-child failure paths are covered by focused tests; platform CI remains the final cross-platform check.

@github-actions github-actions Bot added the review/self-reported The linked issue was opened by the PR author (self-reported) label Aug 3, 2026
@doudouOUC doudouOUC self-assigned this Aug 3, 2026
@doudouOUC
doudouOUC requested a review from wenshao August 3, 2026 03:11
@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

🩺 serve daemon A/B

Built the PR base vs this PR head 7efde4d, drove a fixed endpoint set against each, and diffed the JSON responses. Only fields that changed are shown.

capabilities

field PR base (before) this PR (after)
features[] "session_id_override"

Qwen Code · serve A/B

@doudouOUC
doudouOUC marked this pull request as ready for review August 3, 2026 07:39
@doudouOUC
doudouOUC enabled auto-merge August 3, 2026 07:39
@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Re-run at 5a166aad (maintainer-triggered). The head moved since the last pass: the branch was rebased onto current main (d91c661) and gained fix(serve): normalize restored session IDs, which closes the last open Critical (the R1-1 restore-keying item). Gate re-evaluated at the new head:

  • Template ✓ — all sections present; Before/After stays "N/A (no TUI surface)", the honest call for a daemon/protocol PR.
  • Problem: observed, not theoretical — unchanged since the last pass. Linked issue Caller-supplied session IDs are not coordinated across daemon transports and workspaces #8411, plus @wenshao's live-daemon A/B against this PR's own base, which reproduced every failure mode it claims to close and one the description never mentioned (on main, an unvalidated caller-supplied ID through the shared stdio ACP agent writes the session transcript to an arbitrary filesystem path).
  • Direction: aligned — caller-supplied session IDs are now a daemon protocol feature, and one admission coordinator shared by every transport is the shape that closes the races. CHANGELOG direction signal: session-resume and session-identity integrity is active territory in the reference agent (recent fixes for session directory cross-contamination and stale-session resume), so canonical session identity is a live concern, not a speculative one.
  • Size: ~2,210 production lines / ~2,100 test lines / ~200 docs lines across 47 files at this head (packages/cli + packages/sdk-typescript + packages/sdk-java + packages/acp-bridge). fix-type, so no size block — but it crosses core paths (config/**, the daemon/ACP surface, both official SDKs), which keeps it on maintainer-awareness escalation: the bot will not auto-approve at this scale regardless of review outcome.
  • Approach: the scope is still right — admission, bridge generations, stdio defense-in-depth, and SDK capability negotiation are one contract. The delta since the last pass is exactly what the previous deferral asked for: the rebase onto current main (conflicts resolved additively) and the R1-1 normalization fix with regression coverage. No drive-by changes spotted in the delta.
  • Risk: packages/cli/src/acp-integration/acpAgent.ts matches the high-risk-path list — full review depth applied. All PR CI checks are green at this head; the sandboxed /verify round for this head is running as part of this triage run and will post its report in this thread.

Moving on to code review. 🔍

中文说明

5a166aad 重新运行(由 maintainer 触发)。head 自上一轮以来已移动:分支 rebase 到当前 maind91c661),并新增 fix(serve): normalize restored session IDs,关闭了最后一个未决 Critical(R1-1 restore 键控问题)。门禁在新 head 上重新评估:

  • 模板 ✓ —— 各节齐全;Before/After 维持 "N/A(无 TUI 界面)",对 daemon/协议类 PR 是诚实写法。
  • 问题:已观测、非理论——与上一轮一致。关联 issue Caller-supplied session IDs are not coordinated across daemon transports and workspaces #8411,加上 @wenshao 用真实 daemon 对本 PR 自身 base 做的 A/B,复现了其声称关闭的全部失效模式,还发现一个描述未提的问题(main 上未校验的调用方 ID 经共享 stdio ACP agent 可把 session transcript 写到任意文件系统路径)。
  • 方向:对齐——调用方指定 session ID 现在是 daemon 协议特性,所有传输层共享一个 admission coordinator 才是真正关闭竞态的形态。CHANGELOG 方向信号:参考 agent 中 session-resume 与 session 身份完整性是活跃领域(近期修复了 session 目录跨污染与陈旧 session 恢复问题),规范化 session 身份是现实关切而非臆测。
  • 规模:当前 head 约 2,210 行生产代码 / 约 2,100 行测试 / 约 200 行文档,共 47 个文件(packages/cli + packages/sdk-typescript + packages/sdk-java + packages/acp-bridge)。fix 类型,不按规模拦截——但跨越核心路径(config/**、daemon/ACP 面、两个官方 SDK),维持 maintainer 知会升级:无论评审结果如何,bot 不会在此规模上自动批准。
  • 方案:范围仍然正确——admission、bridge 代际、stdio 纵深防御、SDK 能力协商是一个整体契约。自上一轮以来的增量正是上次移交所要求的:rebase 到当前 main(冲突以叠加方式解决)加上带回归覆盖的 R1-1 归一化修复。增量中未发现夹带改动。
  • 风险packages/cli/src/acp-integration/acpAgent.ts 命中高风险路径清单——已按完整深度评审。当前 head 的 PR CI 全绿;针对该 head 的沙箱 /verify 正作为本次 triage 运行的一部分执行,报告将发布在本帖。

进入代码审查。🔍

Qwen Code · qwen3.8-max

Reviewed at 5a166aad81eee75c3469347413727478ed1ffa06 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Code review of 5a166aad. The head moved since the last attested pass (ed122b5a), so this pass reviews the delta rather than re-arguing settled ground: a rebase onto current main (d91c661) whose conflicts were resolved additively, and one new commit — fix(serve): normalize restored session IDs — which implements exactly the R1-1 follow-up the previous passes recommended, with real coverage. Unattended run: no PR code was built or executed here.

The normalization commit, reviewed line-by-line

Semantics are right. normalizeSessionIdForLookup (in the shared config/session-id.ts helper) lowercases only values matching the strict caller-supplied UUID regex (v1-v5, RFC variant, case-insensitive match). Internal -agent-* Arena IDs and legacy non-UUID IDs pass through with their spelling preserved — pinned by direct unit tests, and it matters because those IDs are compared by exact spelling elsewhere. parseCallerSuppliedSessionId now delegates to the same helper, so create-time and lookup-time normalization cannot drift apart.

Boundary coverage is complete. I enumerated every session-map lookup and admission call site at this head:

  • Admission coordinator — both reserveCreate and reserveRestore normalize at entry, so even a caller that bypassed every boundary would hit a normalized claim key. The mixed-case same-claim test pins it.
  • RESTrequireSessionId normalizes the route parameter, which feeds create, load, and resume routes.
  • ACP HTTP dispatch — normalizes the sessionId param and the session header once at dispatch entry, so the header-vs-param divergence check now compares canonical spellings (a mixed-case-only divergence would previously have been rejected as a mismatch). Load/resume handlers normalize again as defense in depth.
  • ACP HTTP mounts — SSE stream ownership, drain-correlation, and session-header paths all normalize.
  • stdio agent — load/resume/prompt/cancel/setMode/setModel/setConfigOption normalize before lookup, and extMethod normalizes before dispatching to extMethodInternal, whose only caller it is — that single wrapper covers every extension-method fan-out (status builders, rewind, branch, artifact persistence). The remaining session-map sites key on config.getSessionId(), which is canonical internally; any hypothetical missed lookup fails closed with session-not-found, so there is no open safety hole even in theory.

Follow-on operations stay on the canonical ID — the transport test restores with a mixed-case ID, then drives session/prompt on the session stream with the same mixed-case spelling and gets end_turn; the stdio test cancels with the original mixed-case ID after a normalized load. That is the behavior the commit message promises, pinned.

No new Criticals in the delta. The two standing Suggestion-grade items from earlier passes remain open and non-blocking: the _capabilities REST probe's bare catch also swallows caller aborts, and the three session_id_admission_unavailable wraps discard the underlying error cause (no cause: chaining). Both are polish for whenever those blocks are next touched.

Sequence: create with a caller-supplied ID

sequenceDiagram
    participant P1 as SDK client
    participant P2 as REST or ACP route
    participant P3 as Admission coordinator
    participant P4 as Bridge generation
    P1->>P2: create with sessionId (capability-gated)
    P2->>P3: reserveCreate with normalized id
    P3->>P4: scan live owners across all generations
    P3->>P3: scan pinned persistence targets
    P3-->>P2: claim installed synchronously
    P2->>P4: spawn with forced thread scope
    P4-->>P2: session id
    P2->>P2: verify honored, roll back orphan on mismatch
    P2->>P3: release claim
    P2-->>P1: lowercase id, attached false
Loading

Restore (load/resume) takes the same coordinator via reserveRestore — now normalized at entry on both sides — with shared claims only within one bridge generation; cross-workspace or cross-generation conflicts return without touching the foreign session.

Testing — the PR's own CI at 5a166aad (fetched via API, nothing re-run here)

All three pull_request-event workflow runs completed green at this head; the macOS/Windows Node legs are skipped by repo policy, and the Java matrix covers the SDK change:

Check Conclusion
Qwen Code CI / Test (ubuntu-latest, Node 22.x) ✅ success
Serve A/B (ubuntu-latest, Node 22.x) ✅ success
Real daemon E2E / Java 11 ✅ success
SDK Java / ubuntu-latest Java 11, 17, 21 ✅ success
SDK Java / macos-latest Java 21 ✅ success
SDK Java / windows-latest Java 21 ✅ success
Desktop Shell (ubuntu-22.04, windows-2022) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

The only in-flight check at this head is review-pr, a bot orchestration job — not PR CI. The green suite includes the new normalization tests (parser, admission claim identity, REST route param, wire-level ACP load/resume with follow-on prompt, stdio load/resume with follow-on cancel).

Sandboxed verification would settle the one open behavioural question at this head: the prior /verify merge-ready verdicts (2814/2814, then 2798/2798 assertions) were against pre-normalization heads, so whether the normalization commit preserves the full A/B gap is what @qwen-code /verify — already running for this head as part of this triage run, report to land in this thread — is set up to prove. Until that report lands, the delta's behavioural backing is the new regression tests plus CI, not an independent A/B round.

中文说明

5a166aad 的代码评审。head 自上次背书(ed122b5a)以来已移动,因此本轮评审增量而非重翻旧账:rebase 到当前 maind91c661),冲突以叠加方式解决;以及一个新提交——fix(serve): normalize restored session IDs——它正是此前各轮建议的 R1-1 后续修复,且带真实覆盖。无人值守运行:此处未构建或执行任何 PR 代码。

归一化提交逐行评审: 语义正确——normalizeSessionIdForLookup(位于共享的 config/session-id.ts)只对严格匹配调用方 UUID 正则(v1-v5、RFC variant、大小写不敏感匹配)的值转小写;内部 -agent-* Arena ID 与遗留非 UUID ID 原样保留拼写,有直接单测钉住——这一点很重要,因为这些 ID 在别处按精确拼写比较。parseCallerSuppliedSessionId 现委托同一 helper,创建时与查找时的归一化不会漂移。边界覆盖完整:枚举了当前 head 上全部 session-map 查找与 admission 调用点——admission coordinator 的 reserveCreate/reserveRestore 入口归一化(即使绕过所有边界也会命中原一化键,有 mixed-case 同 claim 测试钉住);REST 的 requireSessionId 归一化路由参数;ACP HTTP dispatch 在入口一次性归一化 param 与 session header(header 与 param 的分歧检查因此比较规范拼写,纯大小写差异不再被误拒),load/resume 处理器再次归一化作为纵深防御;ACP HTTP 挂载的 SSE 流归属、drain 关联、session header 路径均归一化;stdio agent 的 load/resume/prompt/cancel/setMode/setModel/setConfigOption 查找前归一化,extMethod 在分发到 extMethodInternal 前归一化且是其唯一调用者——一个包装覆盖全部扩展方法扇出;其余 session-map 点以 config.getSessionId() 为键(内部即规范);任何假想的漏网查找都以 session-not-found 失败关闭,理论上也不存在安全缺口。后续操作保持在规范 ID 上——transport 测试以 mixed-case ID restore 后用同一拼写驱动 prompt 得到 end_turn;stdio 测试在归一化 load 后以原 mixed-case ID cancel。增量中无新 Critical;两条既有的 Suggestion 级事项维持不阻塞(capabilities 探测裸 catch 吞中止、三处 503 包装丢失 cause 链)。

测试证据:当前 head 的 PR 自身 CI 全绿(三个 pull_request 工作流完成;macOS/Windows Node 腿按仓库策略跳过;Java 矩阵覆盖 SDK 改动),见上方 CI 表;唯一在跑的是 bot 编排作业 review-pr,非 PR CI。绿色套件包含全部新归一化测试。沙箱验证可收口当前 head 唯一未决的行为问题:此前 /verify 的 merge-ready 判定(2814/2814、2798/2798 断言)针对的是归一化之前的 head,归一化提交是否保持完整 A/B 差距由 @qwen-code /verify 证明——它已作为本次 triage 运行的一部分对该 head 执行,报告将发布在本帖。报告落地前,增量的行为支撑是新回归测试加 CI,而非独立 A/B 轮次。

Qwen Code · qwen3.8-max

Reviewed at 5a166aad81eee75c3469347413727478ed1ffa06 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — capped by the core-scale escalation policy, not by doubt: ~2,210 production lines reshaping the daemon protocol surface and both official SDKs, and the bot does not auto-approve at this scale. The human gate that cap exists to trigger has been passed — @wenshao verified the change end-to-end on real daemons and his approval stands on exactly this head (5a166aad, submitted minutes after the push).

Stepping back: this pass completed everything the last deferral asked for, and the delta is exactly what it should be. The branch is level with main again (rebased onto d91c661, conflicts resolved additively), CI re-ran green at the new head, and the last open Critical — restore keying on the raw, un-normalized ID — is no longer deferred to a follow-up: the normalization commit fixes it at every boundary plus the admission coordinator itself, with regression tests pinning each layer. My independent proposal for this problem (one daemon-wide coordinator keyed by normalized IDs, synchronous claim before any await, identity-bound releases, capability negotiation in the SDKs) remains exactly what this PR builds, and I could not find a simpler shape that closes the cross-transport races. The evidence chain at this head: green PR CI, new regression tests for the delta, three prior sandboxed /verify rounds merge-ready on earlier heads, a fresh /verify round running on this one, and a maintainer who reproduced every failure mode on the base arm and called it stronger than the description claims.

What remains before merge is mechanical, and maintainer-owned:

  • Two stale bot-authored CHANGES_REQUESTED reviews still formally hang on this PR and are what keeps the review decision red: the Aug 3 one flagged the workspace-setup-github.test.ts harness failure, fixed long ago and green ever since; the Aug 5 one is the /review bot's "Not reviewed" time-budget exhaustion, whose single ledger finding was R1-1 — now fixed in this head rather than deferred. Neither is a live substantive objection at 5a166aad; both need dismissing.
  • The in-flight /verify round for this head will post its report in this thread when it lands; treat any surprise in it as reopening what this pass settled.

Verdict: defer — with nothing left waiting on the bot and no blocking findings. I'm not adding an approval because the core-scale escalation policy forbids the bot approving at this size, and none is needed from me: a maintainer approval already stands on this exact head. Handing back to @wenshao for the review dismissals and the merge.

中文说明

置信度:3/5 —— 封顶来自核心规模升级政策,而非质疑:约 2,210 行生产代码重塑 daemon 协议面与两个官方 SDK,bot 不会在此规模上自动批准。该上限所要触发的人工关口已经通过——@wenshao 用真实 daemon 完成了端到端验证,其批准正落在本 head(5a166aad,推送后几分钟内提交)之上。

退一步看:本轮完成了上次移交所要求的全部事项,增量也恰如其分。分支重新与 main 齐平(rebase 到 d91c661,冲突以叠加方式解决),CI 在新 head 重跑全绿,最后一个未决 Critical——restore 以未归一化原始 ID 为键——不再留作后续:归一化提交在全部边界加 admission coordinator 自身将其修复,各层均有回归测试钉住。我对这个问题的独立方案(按归一化 ID 键控的 daemon 级 coordinator、await 前同步先占、身份绑定释放、SDK 能力协商)仍是本 PR 所构建的形态,且我找不到能关闭跨传输竞态的更简形态。当前 head 的证据链:PR CI 全绿、增量的新回归测试、此前三轮沙箱 /verify 在较早 head 上 merge-ready、针对本 head 的新一轮 /verify 正在运行,以及一位在 base 侧复现全部失效模式、并评价其强于描述声称的 maintainer。

合并之前剩余的是机械事项,且由 maintainer 主导:

  • PR 上仍正式挂着两条 bot 所提的 CHANGES_REQUESTED 评审,也是评审决定仍为红的原因:8 月 3 日那条指出 workspace-setup-github.test.ts harness 失败,早已修复并持续为绿;8 月 5 日那条是 /review bot 时间预算耗尽的"未审查",其唯一 ledger 发现即 R1-1——现已在本 head 修复而非延后。两者在 5a166aad 上都不是存活的实质性异议,但需要 dismiss。
  • 针对本 head 的在途 /verify 报告落地时会发布在本帖;若出现意外,视为重新打开本轮已定夺的事项。

结论:defer——已无任何事项等待 bot,也无阻塞性发现。不追加批准,因为核心规模升级政策禁止 bot 在此规模上批准,而且也不需要我批准:maintainer 的批准已落在同一 head 上。移交回 @wenshao:dismiss 评审并合并。

Qwen Code · qwen3.8-max

Reviewed at 5a166aad81eee75c3469347413727478ed1ffa06 · re-run with @qwen-code /triage

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Needs some rethinking before merge — details in my review comments above. Summary: the daemon-wide admission design is right and the tests pin the behavior, but this commit's own unit suite is red — the new createServeApp getSessionBridges invariant breaks two existing tests in workspace-setup-github.test.ts (the harness was not updated alongside the production wiring). Cross-package core scope also means maintainer sign-off is needed regardless. 🙏

@doudouOUC

Copy link
Copy Markdown
Collaborator Author

Review/CI follow-up — 191534f919

Item Decision Action
Linux unit suite: two workspace-setup-github tests failed because the hot-reload harness omitted the new live-bridge provider Fixed Wired the harness bridge through getSessionBridges, preserving the production fail-fast invariant instead of weakening it.
Suggest splitting TypeScript/Java SDK and MCP support into another PR Not taking #8411 defines capability-gated official client reachability and response verification as part of the same end-to-end contract; splitting would temporarily advertise a daemon feature without the official clients that negotiate it.
Repeated admission fallback construction / positional ACP dispatcher constructor Deferred Non-blocking maintainability observations outside this correctness fix; changing them would widen an already large review surface without altering the requested contract.
fix(serve) versus feat classification Not changing This PR completes and hardens the partially shipped behavior from #7836; no code or release behavior depends on changing the title type during this CI fix.

Verification on the pushed commit: npm run build, npm run typecheck, the affected CLI test file (16/16), file-scoped ESLint, and two focused diff audits all passed. There were no inline review threads to reply to or resolve (0/0).

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Qwen Code review timed out. Qwen review timed out after 14400 seconds (of the 240-minute budget). This run already used the maximum 240 minute timeout. See workflow logs.

@yiliang114

Copy link
Copy Markdown
Collaborator

@qwen-code /review --timeout=240

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor
_Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/30831049787)._

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Qwen Code review timed out. Qwen review timed out after 14400 seconds (of the 240-minute budget). This run already used the maximum 240 minute timeout. See workflow logs.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Qwen Code review timed out. Qwen review timed out after 14400 seconds (of the 240-minute budget). For large PRs, retry with a longer timeout by commenting: @qwen-code /review --timeout=360. See workflow logs.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Qwen Code review timed out. Qwen review timed out after 14400 seconds (of the 240-minute budget). This run already used the maximum 240 minute timeout. See workflow logs.

1 similar comment
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Qwen Code review timed out. Qwen review timed out after 14400 seconds (of the 240-minute budget). This run already used the maximum 240 minute timeout. See workflow logs.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Qwen Code review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs.

@doudouOUC
doudouOUC requested a review from yiliang114 August 5, 2026 03:13
@doudouOUC

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main (merge 94e78352) — the branch is now level with it and the conflict is gone.

On the blocking finding: already fixed, in 191534f9. The createServeApp getSessionBridges invariant did break the two workspace-setup-github.test.ts cases because the harness was not updated alongside the production wiring — that read was correct. The harness now wires session bridges, and Test (ubuntu-latest, Node 22.x) passes on head.

review-pr is red for an unrelated reason: the review bot hit its own ceiling — Qwen review timed out after 21600 seconds (of the 360-minute budget). Nothing to fix in the diff; re-running below.

Remaining is the maintainer sign-off you flagged for the cross-package core scope, which is not something I can resolve from this side.

@qwen-code /review

@doudouOUC

Copy link
Copy Markdown
Collaborator Author

@qwen-code /takeover

@qwen-code-dev-bot qwen-code-dev-bot added the autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+) label Aug 5, 2026
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤝 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 is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the autofix/takeover label (or comment @qwen-code /takeover stop) to release.

中文说明

🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。本 PR 来自 fork,首轮处理将由下一次定时扫描执行(通常几分钟内)。移除 autofix/takeover 标签(或评论 @qwen-code /takeover stop)即可释放。

@wenshao

wenshao commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Review: fix(serve): Coordinate caller-supplied session IDs

Overview

This consolidates caller-supplied session IDs into one daemon-wide contract. The core of the change is RequestedSessionIdAdmission (packages/cli/src/serve/session-id-admission.ts), a single in-memory claim map shared by REST and both ACP mounts, replacing the route-local inFlightSessionIds / inFlightRestoreOwners maps in routes/session.ts. Around it: a shared parseCallerSuppliedSessionId extracted to config/session-id.ts, session/new._meta["qwen-code/sessionId"] on the ACP dispatcher, generation-pinned runtime contexts so draining bridges stay visible, a per-child pending set in the stdio agent, and a negotiated session_id_override capability across the TS/Java SDKs and daemon MCP.

The design is coherent and the shape is right: a synchronous claim installed before the first await, identity-bound releases so a stale release can't evict a newer claim, refcounted restore claims, and fail-closed bridge enumeration. Test coverage is genuinely thorough — admission unit tests, cross-mount ACP tests, a REST↔ACP race test over a real WS connection, SDK tests on both languages, and an end-to-end integration test. The design doc and protocol docs are updated in the same PR.

I verified a few things that looked risky and they're fine, so noting them so they don't get re-raised: the bridge already rejects cross-action restore races with RestoreInProgressError and coalesces same-action ones, so the new startingSessionIds set in acpAgent.ts really is defense-in-depth and won't break the refcounted shared restore claim; Express 5 forwards async handler rejections, so the bare throw error after reserveCreate is handled; sendSessionWorkspaceConflict and the config.ts isValidSessionId import are both still live.


Issues

1. Three different outcomes for "we couldn't tell" — one of them is a permanent 409. session-id-admission.ts

catch (error) {
  if ((error as NodeJS.ErrnoException).code !== 'ENOENT') return true;
}

In persistedSessionExists, any non-ENOENT failure from the worktree sidecar access()EACCES, EPERM, ENOTDIR, EIO on a stale network mount, or a synchronous throw from getWorktreeSessionPathForArchiveState (no .code at all, so undefined !== 'ENOENT') — becomes return true, which surfaces as 409 session_id_conflict / Session "<id>" already exists. Meanwhile a rejection from sessionExistsInAnyState on the line above propagates unmapped (500), and a bridge-enumeration failure returns retryable 503 session_id_admission_unavailable. Three adjacent I/O failures, three different contracts.

The 409 is the damaging one: it's the only non-retryable answer, and it tells the caller their pre-allocated ID is permanently taken when in fact the daemon just couldn't read a directory. A caller that persisted that ID alongside its own workflow state has no recovery path. Since one workspace registered on an unreadable runtime dir poisons every requested ID, this isn't only a corner case. Suggest mapping unreadable-persistence to session_id_admission_unavailable (retryable 503), same as the live-bridge path, and letting only a genuine positive hit produce 409.

2. ACP session/load / session/resume hard-fail 503 when admission wasn't injected. acp-http/dispatch.ts

const reservation = this.requestedSessionIdAdmission?.reserveRestore(sessionId, {...});
if (!reservation) {
  throw new RequestedSessionIdAdmissionError('session_id_admission_unavailable', ...);
}

requestedSessionIdAdmission is an optional constructor param, but on the restore path its absence disables a pre-existing, non-opt-in feature entirely. The create path gets this right — it only demands admission when requestedSessionId is set. mountAcpHttp always supplies one so production is safe, but an optional parameter whose absence is a hard error is a footgun. Either make it required on the constructor, or skip the reservation when it's absent (matching pre-PR restore behavior).

3. AcpDispatcher's constructor is now 15 positional params, and this PR inserted one in the middle. acp-http/dispatch.ts

requestedSessionIdAdmission went in between archiveCoordinator and isWorkspaceTrusted, shifting three trailing params. TS will usually catch a mis-binding here, but sessionRuntimeBaseDir: string sitting next to two () => … params is exactly the arrangement where it eventually won't. Not a blocker for this PR, but this constructor has outgrown positional args — an options object would make the next insertion safe.

4. AcpWsTransport capabilities probe bypasses a caller-supplied fetch and swallows non-ok responses. sdk-typescript/src/daemon/AcpWsTransport.ts

this.restFetch = restFetch ?? globalThis.fetch.bind(globalThis);
...
try {
  const response = await this.restFetch(url, init);
  if (response.ok) return response;
} catch { /* ACP-only fallback */ }
return synthesizeResponse(200, this.initResult ?? { v: 1 });

Two consequences worth a second look:

  • DaemonClient resolves opts.fetch ?? opts.transport?.restFetch ?? globalThis.fetch, so a caller who passes a custom fetch (auth proxy, instrumentation, test double) and constructs AcpWsTransport themselves — which is the documented pattern — now has that custom fetch silently bypassed for the capabilities call only. Threading the client's _fetch into the transport, or leaving restFetch undefined and letting the client supply it, would avoid the split.
  • A 401/403/404 from REST is indistinguishable from "ACP-only deployment": both fall through to { v: 1 } with no features. The user-visible result of a bad token is then DaemonCapabilityMissingError: session_id_override, which points the caller at the wrong problem entirely. Consider only falling back on network-level failure and 404, and propagating auth statuses.

5. Both SDKs throw on ID mismatch without releasing the session they just created. DaemonClient.ts, DaemonClient.java

DaemonSessionIdProtocolError / SessionCreationOutcomeUnknownException fire after a 200 response that already contains sessionId and clientId — everything needed to detach. The daemon now rolls back mismatches itself, so this path only fires against a nonconforming daemon, but that's precisely when leaving a live session and an attached client behind matters most. A best-effort detachClient before throwing would cost little. (Java also recomputes request.getSessionId().toLowerCase(Locale.ROOT) three times and fully-qualifies java.util.Locale inline rather than importing it.)

6. liveWorkspaceCwd is used to report a workspace that isn't live. session-id-admission.ts

if (persisted) throw conflict(sessionId, 'persisted', persisted.workspaceCwd);
// → details: { conflict: 'persisted', liveWorkspaceCwd: '/two' }

conflict() puts the workspace into liveWorkspaceCwd regardless of kind, so a persisted-history hit reports a path under a field named "live". This is in the public REST/ACP error payload and the tests bake it in. Worth renaming (conflictWorkspaceCwd) before it ships and becomes load-bearing.

7. Restore now compares a bridge-reported cwd against a route-canonicalized one. session-id-admission.ts

const foreignLive = liveOwners(sessionId).find(
  (owner) => owner.bridge !== target.bridge || owner.workspaceCwd !== target.workspaceCwd,
);

owner.workspaceCwd comes from bridge.getSessionSummary() (whatever was passed at spawn); target.workspaceCwd comes from the route after realpathSync.native canonicalization. The replaced enterRestoreOwner compared two values that both originated from runtime, so this is a new cross-source string comparison. If those ever diverge — symlinked workspace root, case-insensitive FS spelling — a legitimate reload of a live session becomes 409 session_workspace_conflict. Is the bridge's stored workspaceCwd guaranteed to be the canonicalized form?

8. Is 409 session_workspace_conflict right for a merely-draining generation? By design, a session live on a replaced-but-not-yet-shut-down bridge blocks load/resume on the new generation (the uses the concrete primary bridge generation for restore admission test asserts this). But that condition self-resolves once the drain completes, which makes it a retryable state dressed up as a permanent conflict. The admission layer already has a retryable code for exactly this shape. Worth a deliberate decision rather than inheriting the pre-existing workspace-conflict contract by default.


Code quality

Design rationale was dropped during the extraction. routes/session.ts deleted this comment along with HTTP_SESSION_ID_REGEX:

Keeping it a subset in BOTH directions matters: every id the daemon accepts must also be a valid --session-id and /resume <id> argument, otherwise a session created over HTTP is unreachable from the CLI (resumeCommand.ts gates on isValidSessionId and falls through to title matching when it fails). That rules out UUIDv7, the nil UUID, and non-RFC-4122 variants even though they are harmless as filenames.

The isValidSessionId JSDoc explaining the Arena -agent- suffix was also dropped from config.ts. Neither landed in the new config/session-id.ts, which has no comments at all — just two bare regexes whose relationship (CALLER_SUPPLIED_* must stay a strict subset of INTERNAL_*) is now undocumented in the one file where it's actually enforced. That constraint is the whole reason UUIDv7 is rejected; without the comment the next person will read it as an oversight and "fix" it. The design doc covers it, but the invariant belongs next to the code. This is the one change I'd ask for before merge.

Otherwise the code reads well and matches surrounding conventions: no any, discriminated-union parse result rather than nullable string, readonly on the admission interfaces, Promise.all over persistence targets with a dedupe keyed on runtimeBaseDir\0workspaceCwd. The integration-tests/vitest.config.ts alias-ordering comment ("most specific first … would swallow it") is exactly the kind of note that saves someone an hour.

Performance

reserveCreate costs O(registered workspaces) × 3 fs ops, but only for creates that actually supply an ID, and it runs under the archive coordinator's shared lock — acceptable. reserveRestore runs on every load/resume but is fully synchronous (map lookups across bridges). The new Set(bridges) dedupe in liveOwners correctly handles a bridge appearing in both the registry and the draining list.

Security

Nothing concerning. Validation stays at the transport boundary, path characters and the -agent- suffix are rejected before any filesystem access, the stdio agent validates before touching settings or disk (well covered by rejects invalid sessionId meta before settings access without closing the child), and error payloads leak only workspace paths that the caller is already authorized for. Fail-closed is the default everywhere I looked. Worth noting the session_id_conflict response does disclose liveWorkspaceCwd for a session in another workspace — fine within a single-tenant daemon, but it's a cross-workspace information channel if the daemon is ever shared.

Test coverage

Strong. Gaps I'd suggest closing:

  • No test for the non-ENOENT branch of persistedSessionExists — the exact branch flagged in issue 1. A test forcing EACCES would pin down whichever contract you land on.
  • AcpWsTransport tests the REST-capabilities happy path but not the fallback, so the 401-becomes-empty-envelope behavior in issue 4 is uncovered in both directions.
  • No coverage of AcpDispatcher constructed without requestedSessionIdAdmission (issue 2).
  • The PR reports Windows and Linux untested, and issues 1 and 7 are both fs-semantics-sensitive (access errno, path canonicalization). At minimum the CI Linux leg should exercise the new admission suite before merge.

Solid, well-scoped work on a genuinely tricky concurrency surface. Issue 1 (unreadable persistence → permanent 409) and the missing subset-invariant comment are the two I'd want resolved before merge; the rest are worth a reply but not blocking.

yiliang114
yiliang114 previously approved these changes Aug 5, 2026

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, no blockers. The coordination model is verified sound: synchronous claim before async scan closes the TOCTOU, identity-checked stale release, generation pinning holds (guard object captured at dispatch entry, no await before runtime-context read), orphan rollback correct, and caller-ID validation is strict RFC-UUID (injection-safe, no Arena squatting). Cross-transport shared admission instance is real. One P2: on the REST create path, reserveCreate non-admission errors (e.g. disk-scan failure) are rethrown OUTSIDE the route's main try/catch, so Express 5 returns a non-conforming 500 (no {code} envelope) unlike every other failure mode — move the reservation inside the main try or map to a structured 503/500. P3s: conflict errors leak liveWorkspaceCwd to token-authenticated callers (consider omitting client-side); restore-path admission keys use raw (non-lowercased) IDs, asymmetric with create.

@doudouOUC

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 5, 2026

Copy link
Copy Markdown
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: 2664 passed · 0 failed · 2664 total

中文 — 判定:✅ 通过 · 可合入(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:2664 通过 · 0 失败 · 2664 总计

Verification report

PR 8415 Deep Verification — fix(serve): Coordinate caller-supplied session IDs

Verdict: merge-ready — 2664/2664 scripted assertions passed, 0 failures.
Verified head OID: 07fa2ef30647a2119913f9729812d0b507baead5 (merge commit 99fcb2c3, base tip 8b0e8b819).

The central claim was proven load-bearing with a real-daemon A/B against a
control build differing only by this PR: on head, REST and ACP creation with a
caller-supplied UUID coordinate through one admission object (duplicates get a
structured session_id_conflict, exactly one of N concurrent requests wins); on
base, the ACP surface silently ignores the requested ID and there is no
cross-transport coordination. Every PR-added guard survived a mutation kill, and a
positive control proved the suites are live.

中文摘要

结论:merge-ready(2664/2664 断言通过,0 失败)。

A/B 结论:用真实 qwen serve 进程对 PR 版与基线版(仅差本 PR)做了对照。

  • 头版(head):REST 与 ACP 用同一个调用方指定 UUID 创建会话时,统一走一个 admission
    协调器——重复创建稳定返回 409 session_id_conflict;两路并发同 ID 只有一个成功;
    已存在的 REST 会话会挡住 ACP 同 ID 新建;非法 _meta ID 返回结构化 INVALID_PARAMS
  • 基线(base):ACP 完全忽略 qwen-code/sessionId(各自用随机 ID 建出新会话),无跨
    transport 协调,/capabilities 也不声明 session_id_override。差异正是本 PR 要补的洞。

主要 findings:无阻断性问题。四个新增守卫(live-bridge 检查、pending 检查、bridge 层
thread 强制、stdio agent 预留)分别被定点 mutation 杀死(对应测试转红),且正控
(改测试期望本身)也转红,证明测试非空洞。UUID v1–v5 / RFC variant 校验 32/32 通过
(含大小写归一、nil/越界版本/变体位/尾随空白/路径穿越均被拒)。TS SDK 能力协商 4/4
(无能力时拒绝 mutate、有能力时透传并复核响应 ID、ID 不一致抛 DaemonSessionIdProtocolError)。

未覆盖:容器内无 java/mvn,Java SDK 测试未跑;base 侧 integration 套件被
globalSetup 强制指回 head bundle,故 base 的 integration 对照以 daemon-ab 为准;
多 commit 归因(depth-2 浅克隆,仅 merge/base/head 三点可达)未做。详见 Not covered

Central claim + A/B table

Central claim: caller-supplied session IDs are coordinated daemon-wide — REST,
primary ACP, and workspace-qualified ACP share one admission coordinator, so a
duplicate (live, pending, or persisted) is rejected with a structured conflict and an
explicit ID always creates a fresh thread session. Secondary claims: (1) clients
negotiate session_id_override before mutating and verify the returned ID; (2) IDs are
validated as RFC-variant UUID v1–v5 and normalized to lowercase.

Driven against a real node <cli> serve daemon (temp HOME, fake OpenAI env,
--port 0), REST via fetch, ACP via a raw WS JSON-RPC client (no SDK, so the
capability-less base daemon is drivable). Cells C5/C6/C7/C11 use arm-specific oracles:
the base arm is expected to exhibit the uncoordinated behavior, so a base cell
"passing" means the control correctly shows the pre-PR gap.

Cell Scenario Head result Base result (control)
C00 /capabilities advertises session_id_override does not advertise it
C1 REST create, mixed-case UUID, scope single 200, lowercased, attached:false same (REST path pre-existed via #7836)
C2 REST sequential duplicate 409 session_id_conflict 409 (route-local guard pre-existed)
C3a/b explicit id + single scope fresh thread session, attached:false parity (base already forced thread at route level)
C4 REST invalid id 400 invalid_session_id 400
C5 ACP session/new w/ live REST id -32602 session_id_conflict silently ignored — new random-id session
C6 2× concurrent ACP same id exactly 1 winner at requested id, 1 conflict 2 winners, both random ids
C7 ACP sequential duplicate 2nd → session_id_conflict 2nd also created (ignored)
C8 first session still live after all 200 200
C9 failed load then create with same id 404 then 200 (claim released) same
C10 reservation released after 400 (branch+worktree) retry 200 same
C11 ACP invalid id _meta -32602 invalid_session_id accepted verbatim — random-id session

Result: head 13/13, base 13/13 (control cells red/green in the expected
direction). The load-bearing delta is C5/C6/C7/C11 + capability advertisement — absent
on base, enforced on head.

Witnesses: evidence/01-ab-head-arm.png, evidence/02-ab-base-arm.png.

Secondary claim 1 — SDK capability negotiation (wire-oracle, refusing fake daemon)

evidence/03-sdk-capability-negotiation.png. A loopback fake daemon records every
request; the built TS SDK talks real HTTP. 4/4:

  • B1 capability absent → createOrAttachSession({sessionId}) rejects with
    DaemonCapabilityMissingError before any POST /session reaches the daemon.
  • B2 capability present → requested id serialized into the POST body; matching
    response id accepted.
  • B3 capability present but response id differs → DaemonSessionIdProtocolError.
  • B4 control — no sessionId, capability absent → POST proceeds (negotiation only
    gates the field).

Secondary claim 2 — UUID validation sweep (32/32)

evidence/04-validation-sweep.png. v1–v5 accepted; v0/v6/v7/v8/v9 rejected; variant
nibbles 8/9/a/b accepted, 0/7/c/f rejected; nil rejected; mixed case normalized to
lowercase; trailing/leading whitespace, -agent- suffix, astral char, short form, and
../escape all rejected. (First pass showed 6 "failures" that were a digit-placement bug
in my fixture generator — the version nibble must lead group 3 — not PR behavior; fixed
and re-run clean.)

Mutation matrix (no survivors; positive control included)

# Guard mutated Suite that caught it Killed by (expected red observed)
M0 positive control — test expectation itself session-id-admission red as required (proves suite live)
M1 disable live-bridge check in reserveCreate session-id-admission "checks every live bridge…" (rejects.toMatchObject got a resolved reservation)
M2 disable pending check in reserveCreate session-id-admission "claims synchronously…", "…stale release…"
M3 revert bridge thread-forcing (effectiveScope) acp-bridge bridge.test.ts "forces caller-supplied session ids…" (second.attached true vs expected false)
M4 disable stdio-agent reserveStartingSessionId acpAgent.test.ts "rejects a concurrent duplicate requested sessionId…" (promise resolved, not rejected)

All four PR-added guards are pinned by a real test; deleting any one turns a suite red
with the intended behavioral assertion. Witness: evidence/05-mutation-livecheck-disabled.png.

Targeted gates (unmodified head, exact counts)

  • cli session-id-admission + session-id + acp-http/transport + workspace-qualified-acp: 369/369
  • cli server + acpAgent + workspace-setup-github: 1243/1243
  • acp-bridge bridge.test.ts: 472/472
  • sdk-typescript (AcpWsTransport, DaemonClient, acpRouteTable, serve-bridge): 477/477
  • integration cli/qwen-serve-routes.test.ts (real daemon, includes the new
    "honors and reserves a normalized caller-supplied session ID"): 36/36

Corrections

None required — no earlier round or bot comment misdescribed the code here (first round).

Findings

No blocking findings. Two clarifications that a reviewer may find useful, neither a defect:

  1. The bridge-level thread-forcing is defense-in-depth for the REST path but
    load-bearing for direct/ACP callers.
    The REST route already forced thread scope for
    a caller-supplied id (pre-existing, from feat(serve): support caller-supplied sessionId in POST /session #7836), so my initial A/B hypothesis that REST
    would diverge on sessionScope:'single' was wrong — base and head are at parity there.
    The bridge hunk (effectiveScope = sessionId ? 'thread' : …) is nonetheless real and is
    pinned by bridge.test.ts (mutation M3 killed it). Not a problem; just calibrating where
    the change actually bites.
  2. dist/cli.js is a chunked esbuild bundle. Grepping dist/cli.js alone for
    session_id_override/session_id_conflict returns nothing; the markers live in
    dist/chunks/*.js, which I confirmed present. Bundle is current head code (built
    2026-08-05 08:20, matches CI window). Methodology note, not a finding.

Not covered

  • Java SDK tests (DaemonSessionClientTest, etc.) — the container has no java or
    mvn (command not found), so the Java side (capability gate + ID-mismatch →
    SessionCreationOutcomeUnknownException) was verified by reading the diff only, not executed.
  • Base-arm integration suiteintegration-tests/globalSetup.ts unconditionally sets
    TEST_CLI_PATH to the head dist/cli.js, overriding my base-CLI env, so a base run of the
    vitest integration file actually exercised the head bundle. I did not count it as a base
    comparison; the base contrast for the same behavior comes from daemon-ab.mjs --arm base
    instead.
  • Per-commit attribution — depth-2 shallow checkout exposes only the merge commit,
    HEAD^1 (base tip), and HEAD^2 (PR head); the 5 intermediate commits in the metadata are
    unreachable, so I verified the aggregate HEAD^1..HEAD diff, not each commit.
  • Workspace-qualified / draining-generation and worktree-sidecar persistence paths were
    exercised through the PR's own unit + integration tests (all green) but not re-driven through
    a hand-built multi-workspace daemon in this round.
  • No repo-wide test sweep; I ran only the affected workspaces' relevant files.

Methodology

Environment: GitHub-Actions node:22-bookworm container, repo at
/__w/qwen-code/qwen-code checked out at refs/pull/8415/merge (depth 2), npm ci +
npm run build pre-run. Harnesses live in this artifact dir and are re-runnable:
daemon-ab.mjs (real-daemon REST+WS A/B), sdk-capability.mjs (refusing fake daemon),
validation-probe.mjs (dist parseCallerSuppliedSessionId). Base control: git worktree add tmp/base-tree HEAD^1, node_modules symlinked from the root install with each
@&#8203;qwen-code/* link re-pointed into the base tree (realpath asserted: core/bridge/cli all
resolved to tmp/base-tree/...); web-templates and cli rebuilt in-tree after generating
the gitignored git-commit.ts. Base-arm daemon purity confirmed by absence of
session-id-admission.js/session-id.js and session_id_override in the base dist.
Raw per-cell stdout/stderr are in logs/ (ab-head.txt, ab-base.txt, sdk-head.txt,
validation-probe.txt) and build logs under tmp/. Mutation reverts were done on scratch
edits and restored with git checkout -- after each run; git status is clean.

Evidence images

01-ab-head-arm

02-ab-base-arm

03-sdk-capability-negotiation

04-validation-sweep

05-mutation-livecheck-disabled

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

yiliang114
yiliang114 previously approved these changes Aug 8, 2026

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM — approving at 5a166aad81. Spot-checked the two items I raised in earlier rounds: the restore/load/resume admission now keys on normalizeSessionIdForLookup at both reserveCreate and reserveRestore (case-variant restore claims can no longer slip past live/pending admission, while internal -agent-* and legacy IDs stay untouched), and the disk-scan failure path is wrapped into a retryable session_id_admission_unavailable that the route maps to a structured 503 — so no non-conforming bare 500 escapes the reservation block, and the admission-side catch still releases the reservation on any unexpected throw. Synchronous-claim-before-async-scan, generation pinning via the captured assertion, and orphan cleanup all hold at this head. Deferred Suggestions per the review-budget rule are fine as follow-ups.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not reviewed: reverse audit — stopped before round 2 by the review time budget.

中文说明

未审查:反向审计——评审时间预算不足,未能开始第 2 轮。

— qwen3.8-max via Qwen Code /review (v0.21.7)

Comment thread packages/cli/src/config/session-id.ts
Comment on lines +1718 to +1719
const rollbackRestore = async (): Promise<void> => {
if (restored.attached) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] rollbackRestore swallows every cleanup error, making the teardown-race diagnostic unreachable. Both branches attach .catch(() => {}) before awaiting, so the function can never reject and the call-site void rollbackRestore().catch((err) => writeStderrLine(... teardown-race ...)) (~line 1771) is dead — orphan-cleanup failures on the restore race path are silently dropped. The pre-diff code logged qwen serve: /acp orphan kill(<id>) teardown-race: <err> on rejection, and the sibling session/new path still preserves the diagnostic via removeOrphanSession; the still-written-but-dead writeStderrLine shows the diagnostic was meant to survive this refactor. — Concrete cost: during session/load, a racing session/close whose killSession fails (bridge error, bridge draining) leaves a lingering orphan session process with no operator signal, where operators previously had a stderr trace. Suggested fix: drop the two inner .catch(() => {}) so the existing call-site .catch fires; swallow explicitly (await rollbackRestore().catch(() => {})) at the two generation-assert rollback sites so the original error still propagates.

中文说明

rollbackRestore 吞掉所有清理错误,使 teardown-race 诊断不可达。两个分支都在 await 之前挂上了 .catch(() => {}),因此该函数永远不会 reject,调用点 void rollbackRestore().catch((err) => writeStderrLine(... teardown-race ...))(约第 1771 行)成为死代码——restore 竞态路径上的 orphan 清理失败会被静默丢弃。改动前的代码会在 rejection 时记录 qwen serve: /acp orphan kill(<id>) teardown-race: <err>,且同级的 session/new 路径仍通过 removeOrphanSession 保留该诊断;仍然写出但已不可达的 writeStderrLine 表明本意是让该诊断在重构后继续存在。— 具体代价:在 session/load 期间,一个竞态的 session/close 若其 killSession 失败(bridge 错误、bridge 正在排空),会留下一个残留的 orphan session 进程且没有任何运维信号,而此前运维人员有一条 stderr 线索。建议修复:去掉两处内部 .catch(() => {}),让既有的调用点 .catch 生效;在两处 generation-assert 回滚点显式吞错(await rollbackRestore().catch(() => {})),使原始错误仍然传播。

— qwen3.8-max via Qwen Code /review (v0.21.7)

Comment on lines 5323 to 5326
async prompt(params: PromptRequest): Promise<PromptResponse> {
const session = this.sessions.get(params.sessionId);
const sessionId = normalizeSessionIdForLookup(params.sessionId);
const session = this.sessions.get(sessionId);
if (!session) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] Five normalized stdio-agent entry points have no test. This diff adds normalizeSessionIdForLookup to prompt, setSessionMode, unstable_setSessionModel, setSessionConfigOption, and extMethod, but only load/resume/cancel normalization is exercised (the file's single toUpperCase call is in the load/resume test). The mixed-case session/prompt in transport.test.ts does not count: the daemon dispatcher normalizes params before anything reaches the child, so that test cannot exercise this child-side normalization for direct-stdio clients. — Failure scenario: a client driving the ACP child directly sends session/prompt or session/set_session_mode with the mixed-case spelling it originally requested; if any of these five calls is later deleted, this.sessions.get(UPPERCASE) misses → "Session not found", and no test in the repo turns red. Suggested fix: extend the existing mixed-case it.each test: create the session, then call agent.prompt / agent.setSessionMode with sessionId.toUpperCase() and assert the call reaches the session (sendUpdate/setMode invoked) instead of throwing.

中文说明

五个做了归一化的 stdio agent 入口没有测试。本 diff 为 promptsetSessionModeunstable_setSessionModelsetSessionConfigOptionextMethod 添加了 normalizeSessionIdForLookup,但只有 load/resume/cancel 的归一化被测试覆盖(文件中唯一的 toUpperCase 调用在 load/resume 测试里)。transport.test.ts 中的混合大小写 session/prompt 不算数:daemon dispatcher 在请求到达子进程之前就已归一化参数,因此该测试无法覆盖为直连 stdio 客户端提供的这层子进程侧归一化。— 失败场景:直接驱动 ACP 子进程的客户端用其最初请求时的混合大小写拼写发送 session/promptsession/set_session_mode;若这五处调用中任何一处日后被删除,this.sessions.get(大写) 会未命中 → "Session not found",且仓库中没有任何测试变红。建议修复:扩展现有的混合大小写 it.each 测试:先创建 session,再用 sessionId.toUpperCase() 调用 agent.prompt / agent.setSessionMode,断言调用到达了该 session(sendUpdate/setMode 被调用)而不是抛错。

— qwen3.8-max via Qwen Code /review (v0.21.7)

expect(res.status).toBe(400);
});

it('honors and reserves a normalized caller-supplied session ID', async () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The integration coverage for this feature is gated only by the E2E workflow, which is skipped at this commit. The new/changed integration tests live outside every npm workspace, so npm run test never collects them; the pre-submission check report for this commit lists Integration Tests (CLI, No Sandbox) among the skipped checks, so the behaviors exercised only here (UUID normalization + reservation, 409 session_id_conflict, session_id_override capability) would merge with no CI gate executing them. Mitigating evidence: this review ran the changed file against a freshly built production bundle — 36/36 passed. — Concrete cost: if the E2E job stays skipped (or is not required) on this PR, a regression in these integration-only paths ships uncaught by any unit gate. Suggested fix: no code change needed — run/require the integration workflow (test:integration:sandbox:none) at this head before merge.

中文说明

本特性的集成覆盖只由 E2E workflow 把关,而该 job 在本 commit 上被跳过。新增/修改的集成测试位于所有 npm workspace 之外,因此 npm run test 永远不会收集它们;本 commit 的提交前检查报告将 Integration Tests (CLI, No Sandbox) 列在被跳过的检查中,所以只在这里覆盖的行为(UUID 归一化 + 预约、409 session_id_conflictsession_id_override capability)将在没有任何 CI 门禁执行它们的情况下合并。缓解证据:本次评审用新构建的 production bundle 运行了被修改的文件——36/36 通过。— 具体代价:如果 E2E job 在本 PR 上保持跳过(或非必需),这些仅集成覆盖的路径上的回归将不会被任何单测门禁捕获。建议修复:无需代码改动——合并前在本 head 上运行/要求集成 workflow(test:integration:sandbox:none)。

— qwen3.8-max via Qwen Code /review (v0.21.7)

Comment on lines +1351 to +1353
if (typeof params['sessionId'] === 'string') {
params['sessionId'] = normalizeSessionIdForLookup(params['sessionId']);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] Plural sessionIds batch surfaces skip the normalization the singular surfaces apply. The singular sessionId param/header is normalized here (and at the REST requireSessionId), but parseSessionIds (dispatch, serving qwen/sessions/archive|delete|unarchive) and parseSessionIdsBody (routes/session.ts, serving REST /sessions/delete|archive|unarchive) pass caller IDs verbatim; downstream bridge.closeSession and the exact-path persisted probes then miss case-variant IDs. The protocol doc's own ACP example sends an uppercase UUID, so the asymmetry is reachable by documented usage. — Failure scenario: a client creates a session with a mixed-case caller ID (stored lowercase), then archives/deletes it using its original spelling: the batch op reports notFound and silently no-ops, while session/load with the identical spelling succeeds. Suggested fix: normalize each element where the plural IDs enter — sessionIds.map((s) => normalizeSessionIdForLookup(s)) in both parseSessionIds and parseSessionIdsBody.

中文说明

复数 sessionIds 批处理入口跳过了单数入口所应用的归一化。单数 sessionId 参数/请求头在此处(以及 REST 的 requireSessionId)被归一化,但 parseSessionIds(dispatch 中,服务 qwen/sessions/archive|delete|unarchive)与 parseSessionIdsBodyroutes/session.ts 中,服务 REST /sessions/delete|archive|unarchive)原样传递调用方 ID;下游 bridge.closeSession 与精确路径的持久化探测因此会未命中大小写变体的 ID。协议文档自己的 ACP 示例就发送大写 UUID,所以该不对称按文档用法即可触达。— 失败场景:客户端用混合大小写的调用方 ID 创建 session(以小写存储),随后用其原始拼写执行 archive/delete:批处理操作报告 notFound 并静默不生效,而用完全相同拼写的 session/load 却能成功。建议修复:在复数 ID 入口处对每个元素做归一化——在 parseSessionIdsparseSessionIdsBody 中使用 sessionIds.map((s) => normalizeSessionIdForLookup(s))

— qwen3.8-max via Qwen Code /review (v0.21.7)

Comment on lines +3995 to +3996
getSessionBridges: () =>
registry.listManaged().map((runtime) => runtime.bridge),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] No test wiring of getSessionBridges exercises draining-generation visibility. Every test wiring copies createServeApp's registry-managed-only fallback (here and at ~line 3739) or a static single bridge, so the suite never exercises the property the dependency exists for — admission seeing sessions live on a replaced-but-draining bridge generation. listManaged() drops the old runtime the moment activateReplacement swaps entry.current, while production's runtimeBridges retains it until shutdown confirms. — Failure scenario: if run-qwen-serve.ts's getSessionBridges: () => runtimeBridges regressed to a registry-derived enumeration, sessions live on a draining bridge would become invisible to create admission between replacement activation and confirmed shutdown — POST /session reusing such an ID would pass the live scan and spawn a duplicate live session, and every existing test would stay green. Suggested fix: add one createServeApp test injecting getSessionBridges: () => [newBridge, oldDrainingBridge] where oldDrainingBridge is not part of the injected registry, have its getSessionSummary report a live session, and assert POST /session with that ID returns 409 session_id_conflict (conflict: 'live').

中文说明

getSessionBridges 的所有测试接线都没有覆盖排空代际(draining generation)可见性。每处测试接线要么复制 createServeApp 的仅 registry 管理回退(此处与约第 3739 行),要么是静态单个 bridge,因此套件从未覆盖该依赖存在的意义——准入要能看到仍存活于被替换但正在排空的 bridge 代际上的 session。listManaged()activateReplacement 换掉 entry.current 的瞬间就丢弃旧 runtime,而生产环境的 runtimeBridges 会保留它直到 shutdown 确认。— 失败场景:如果 run-qwen-serve.tsgetSessionBridges: () => runtimeBridges 回归为基于 registry 的枚举,在替换激活与确认 shutdown 之间,排空 bridge 上存活的 session 将对 create 准入不可见——复用该 ID 的 POST /session 会通过存活扫描并派生重复的存活 session,而所有现有测试仍为绿色。建议修复:新增一个 createServeApp 测试,注入 getSessionBridges: () => [newBridge, oldDrainingBridge],其中 oldDrainingBridge 不属于注入的 registry,让其 getSessionSummary 报告一个存活 session,并断言使用该 ID 的 POST /session 返回 409 session_id_conflictconflict: 'live')。

— qwen3.8-max via Qwen Code /review (v0.21.7)

Comment on lines +187 to +189
const restFetch = vi
.fn<typeof globalThis.fetch>()
.mockResolvedValueOnce(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] vi.fn<typeof globalThis.fetch>() is a TS2344 under this package's pinned vitest. packages/sdk-typescript pins vitest ^1.6.0 (the lockfile resolves a nested 1.6.1, whose @vitest/spy declares fn<TArgs extends any[], R>() — a function type violates the any[] constraint; the single-function-type form arrived in vitest 2). Measured with tsc over this file: error TS2344 at exactly this line. Latent today because the package's typecheck excludes test/ and vitest's esbuild transform erases types (the suite passes 34/34 on 1.6.1), but the file is red in any IDE TS server and becomes a hard CI error once typechecking is extended to test/. The sibling recordingFetch in DaemonClient.test.ts uses the implementation form, valid under both majors. — Concrete cost: red squiggles in every IDE today; a CI break the moment someone extends typechecking to tests; the mismatch also masks that this package is still on vitest 1.6 while every other workspace is on ^3.x.

Suggested change
const restFetch = vi
.fn<typeof globalThis.fetch>()
.mockResolvedValueOnce(
const restFetch = vi
.fn()
.mockResolvedValueOnce(
中文说明

vi.fn<typeof globalThis.fetch>() 在本包锁定的 vitest 下是 TS2344。packages/sdk-typescript 锁定 vitest ^1.6.0(lockfile 解析出嵌套的 1.6.1,其 @vitest/spy 声明为 fn<TArgs extends any[], R>()——函数类型不满足 any[] 约束;单函数类型泛型形式是 vitest 2 才引入的)。用 tsc 对该文件实测:恰好在这一行报 error TS2344。当前处于潜伏状态,因为该包的 typecheck 排除了 test/,且 vitest 的 esbuild 转换会擦除类型(套件在 1.6.1 下 34/34 通过),但该文件在任何 IDE 的 TS server 中都是红的,一旦 typecheck 扩展到 test/ 就会成为硬性 CI 错误。DaemonClient.test.ts 中的同类 recordingFetch 使用实现形式,在两个大版本下均合法。— 具体代价:当前每个 IDE 都会报红;一旦有人把 typecheck 扩展到测试就会 break CI;该错配还掩盖了本包仍停留在 vitest 1.6 而其他所有 workspace 都在 ^3.x 的事实。

— qwen3.8-max via Qwen Code /review (v0.21.7)

doudouOUC and others added 5 commits August 9, 2026 10:07
Complete daemon-wide admission across REST, ACP, workspace generations, SDKs, and MCP.

Closes QwenLM#8411

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…nLM#8415)

Restore the observability and fail-loud guarantees flagged in review:
log every session-id admission routing failure, name the live foreign
owner workspace in restore conflicts, make the ACP dispatcher's
admission dependency required so load/resume cannot run on a mount
without one, and require mountAcpHttp hosts to inject the daemon-wide
admission instead of silently building a weak fallback. Harden the SDK
WS transport against environments without global fetch and against
non-capabilities 200 envelopes, and align the design doc with the
implemented restore-sharing and persistence-failure semantics.
Preserve REST capability errors, fail closed on malformed envelopes, retain restore routing diagnostics, and align retry documentation.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@doudouOUC
doudouOUC dismissed stale reviews from yiliang114 and wenshao via 13021d2 August 9, 2026 02:18
@doudouOUC
doudouOUC force-pushed the codex/session-id-complete branch from 5a166aa to 13021d2 Compare August 9, 2026 02:18
@doudouOUC

Copy link
Copy Markdown
Collaborator Author

Resolved the merge conflict by rebasing onto origin/main at 4a79517. The only manual resolution kept main’s expanded integration-test TypeScript path map, which already includes the two SDK source mappings this PR added; range-diff confirmed the remaining four commits are patch-equivalent. Local verification passed: build, bundle, typecheck, lint, integration TypeScript compile, 36/36 real-daemon route tests, 499/499 ACP bridge tests, targeted CLI/TypeScript SDK tests, and Java SDK Maven tests (127 tests, 5 skipped).

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@doudouOUC

Copy link
Copy Markdown
Collaborator Author

Fixed the mixed-case legacy session regression in 7efde4d.

Item Action
Mixed-case persisted transcript lookup Added case-insensitive legacy transcript resolution for load/resume and create admission.

Validated with targeted core and CLI tests plus build and typecheck. Resolved 1 review thread.

@wenshao

wenshao commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Local verification on real daemons — merge-ready

I built both sides from source and drove them over the wire. Everything below is a live exchange with a daemon started from that side's own bundle; the probe scripts speak raw HTTP / WebSocket JSON-RPC / ndjson and import no PR code, so the same probe file runs unchanged against base and head.

Setup. Two isolated worktrees, each with its own npm ci + npm run bundle. base = 4a79517 (merge-base of this branch with main), head = 7efde4d. Each daemon runs with its own throwaway HOME/QWEN_HOME against a shared fixture git workspace, so both sides compute identical project transcript paths. macOS 26.6, Node 24.18.1, npm 11.16.0, JDK 26.0.2, Maven 3.9.16.

1. Daemon surface — REST + ACP/WebSocket

before/after matrix

The four rows that change are the ones worth merging for:

  • session_id_override is now advertised. On base the feature is unreachable to any client that negotiates.
  • ACP session/new._meta["qwen-code/sessionId"] was silently ignored on base. The daemon returned a freshly minted random ID and a 200. The caller believes it owns the ID it asked for; it does not. On head the ID is honored and lowercased.
  • Cross-transport conflict. REST creates X, then ACP asks for X: base hands back a different session, so the ACP caller ends up holding a session nobody asked for. Head returns -32602 carrying httpStatus: 409, errorKind: session_id_conflict, conflict: "live" and the owning liveWorkspaceCwd.
  • Concurrent REST + ACP for the same fresh ID. Head admits exactly one and the loser gets a structured conflict — in my run ACP won and REST got 409 session_id_conflict, which is the right shape either way. On base "only one won" only because ACP never tried to use the requested ID at all.

Rows 6–8 are unchanged base vs head, i.e. no regression in the paths #7836 already shipped: mixed-case → lowercase, attached: false even with sessionScope: single, duplicate → 409, and after a daemon restart all three of active-transcript / archived-transcript / mixed-case-over-lowercase-persisted are rejected while POST /session/:id/load still restores the known ID. nil UUID, UUIDv7, <uuid>-agent-x, ../escape and a non-string all still get 400 invalid_session_id on both sides.

2. The shared stdio ACP agent — this is the sharp edge

stdio agent traversal

On base, qwen --experimental-acp takes _meta["qwen-code/sessionId"] verbatim, unvalidated. I sent ../../../../qwen8415-stdio-escape-6wp25i, got it back as the session ID, then ran one real turn against a local fake OpenAI server — and the session transcript was written four levels above the project directory, at a path the caller chose. An absolute path (/tmp/...), the nil UUID, and an Arena-namespace <uuid>-agent-x ID were all accepted verbatim too. A duplicate of a live sibling ID came back as an unstructured -32603 "Internal error".

On head every one of those is -32602 with a structured errorKind (invalid_session_id / session_id_conflict), nothing lands outside the project directory, and the sibling session plus the shared child process both survive the rejections — a fresh session/new right after still succeeds.

3. Official TypeScript SDK against real daemons

SDK negotiation

  • New SDK → old daemon: DaemonCapabilityMissingError, and I counted the requests — zero POST /session reached the old daemon. It refuses before mutating rather than letting the daemon silently drop the field. Same client with no requested ID still creates normally.
  • New SDK → new daemon: honored and normalized over both the REST transport and the ACP WebSocket transport.
  • New SDK → lying daemon: I put a proxy in front of the head daemon that advertises the capability but rewrites sessionId in the 200 body. The SDK rejects it with DaemonSessionIdProtocolError naming both IDs.

4. Suites re-run at this head

suite result
packages/acp-bridge Vitest (all 25 files) 1124 / 1124
packages/sdk-typescript focused Vitest (4 files) 487 / 487
Java SDK Maven 127 run, 0 failures, 0 errors, 5 skipped
Real bundled-daemon route integration 36 / 36
CLI focused Vitest (7 files, incl. full server.test.ts) 1671 / 1672 — see below
npm run typecheck pass
ESLint on the changed production files pass

5. One flake, and it is not yours

The single CLI failure was server.test.ts > POST /channels/:channelName/webhooks/:source > keeps valid webhook sources when a sibling source is malformed, asserting 202 but getting 404. It passes in isolation, so I chased it: the same flake reproduces on base. Three sequential full-file runs of 8 failed on base, and under 6-way parallel load base failed on two different tests with the same signature — GET /capabilities > omits mcp_workspace_pool / mcp_pool_restart … (expected 200, got 404) and read-only status routes > rejects extension mutations when the operation queue is full (expected 429, got 404). Same shape, different tests, present without this PR: a pre-existing load-sensitive 404 in server.test.ts on main, worth its own issue but not a reason to hold this PR.

6. Not covered by this round

Live workspace-qualified ACP against a draining bridge generation (I exercised the primary ACP endpoint only; the generation-guard paths are covered by the new unit tests, not by my live probes), Windows and Linux, authenticated real-model prompting through every client, and UUID v7 / historical duplicate migration — all consistent with what the description already scopes out.

Verdict: merge-ready. The behaviour the description claims is the behaviour the daemons actually exhibit, the previously silent ACP paths now fail loudly and structurally, and the unvalidated stdio ID is closed.

中文版本

本地真实 daemon 验证 —— 可以合并

我在本地把两侧都从源码构建出来,然后走真实协议驱动。下面每一条都是与「用该侧自己 bundle 启动的 daemon」的一次真实交互;探针脚本只讲原始 HTTP / WebSocket JSON-RPC / ndjson,不 import 任何 PR 代码,所以同一个探针文件对 base 和 head 原样运行。

环境。 两个隔离 worktree,各自 npm ci + npm run bundlebase = 4a79517(本分支与 main 的 merge-base),head = 7efde4d。每个 daemon 使用独立的临时 HOME/QWEN_HOME,共用同一个 fixture git workspace,因此两侧计算出的 project transcript 路径完全一致。macOS 26.6、Node 24.18.1、npm 11.16.0、JDK 26.0.2、Maven 3.9.16。

1. Daemon 层面 —— REST + ACP/WebSocket(截图 1)

真正发生变化、也是值得合并的四行:

  • session_id_override 现在会被广播。 在 base 上,任何做 capability 协商的客户端都够不到这个能力。
  • ACP session/new._meta["qwen-code/sessionId"] 在 base 上被静默忽略。 daemon 返回一个新随机 ID 加 200。调用方以为自己拿到了请求的 ID,其实没有。head 上该 ID 被采纳并转小写。
  • 跨 transport 冲突。 REST 先创建 X,随后 ACP 请求 X:base 返回的是另一个 session,ACP 调用方拿到一个自己没要过的会话。head 返回 -32602,data 里带 httpStatus: 409errorKind: session_id_conflictconflict: "live" 以及归属方 liveWorkspaceCwd
  • REST 与 ACP 并发抢同一个新 ID。 head 只允许一个成功,失败方拿到结构化冲突 —— 我这次是 ACP 胜出、REST 得到 409 session_id_conflict,两个方向都是正确形状。base 上「只有一个成功」只是因为 ACP 压根没使用请求的 ID。

第 6–8 行 base 与 head 一致,即 #7836 已交付的路径没有回归:mixed-case 转小写、即使 sessionScope: single 也返回 attached: false、重复 ID 返回 409;daemon 重启后,active transcript、archived transcript、以及「mixed-case 请求命中已持久化的小写 ID」三种情况全部被拒,而 POST /session/:id/load 仍能恢复已知 ID。nil UUIDUUIDv7<uuid>-agent-x../escape 和非字符串在两侧都仍是 400 invalid_session_id

2. 共享 stdio ACP agent —— 这里是真正的锋利处(截图 2)

在 base 上,qwen --experimental-acp_meta["qwen-code/sessionId"] 原样接受、完全不校验。我传入 ../../../../qwen8415-stdio-escape-6wp25i,它原样返回作为 session ID;随后我用本地 fake OpenAI server 跑了一轮真实对话 —— 会话 transcript 被写到了 project 目录之上四层,路径由调用方指定。绝对路径(/tmp/...)、nil UUID、以及占用 Arena 命名空间的 <uuid>-agent-x 同样被原样接受。复用一个存活 sibling 的 ID 则返回无结构的 -32603 "Internal error"

在 head 上,这些全部变成带结构化 errorKind-32602invalid_session_id / session_id_conflict),project 目录之外没有任何产物,并且 sibling session 与共享 child 进程都在拒绝后存活 —— 紧接着发一个新的 session/new 仍然成功。

3. 官方 TypeScript SDK 对真实 daemon(截图 3)

  • 新 SDK → 旧 daemon:抛 DaemonCapabilityMissingError,而且我统计了实际请求数 —— 到达旧 daemon 的 POST /session0。它在 mutation 之前就拒绝,而不是让 daemon 静默丢弃该字段。同一个 client 不带指定 ID 时仍可正常创建。
  • 新 SDK → 新 daemon:REST transport 与 ACP WebSocket transport 两条腿都正确采纳并归一化。
  • 新 SDK → 说谎的 daemon:我在 head daemon 前面放了一个代理,广播该 capability 但把 200 响应体里的 sessionId 改掉。SDK 以 DaemonSessionIdProtocolError 拒绝,并同时点出两个 ID。

4. 在当前 head 重跑的测试

套件 结果
packages/acp-bridge Vitest(全部 25 个文件) 1124 / 1124
packages/sdk-typescript 定向 Vitest(4 个文件) 487 / 487
Java SDK Maven 127 运行,0 失败,0 错误,5 跳过
真实 bundle daemon 路由集成测试 36 / 36
CLI 定向 Vitest(7 个文件,含完整 server.test.ts 1671 / 1672 —— 见下
npm run typecheck 通过
对改动的生产文件跑 ESLint 通过

5. 有一个 flake,但不是这个 PR 的

CLI 唯一那条失败是 server.test.ts > POST /channels/:channelName/webhooks/:source > keeps valid webhook sources when a sibling source is malformed,断言 202 实际拿到 404。它单独跑是过的,于是我追了下去:同一个 flake 在 base 上同样复现。 base 上连续跑 8 次整文件有 3 次失败;在 6 路并行压力下,base 还在另外两个测试上以相同签名失败 —— GET /capabilities > omits mcp_workspace_pool / mcp_pool_restart …(期望 200 得到 404)与 read-only status routes > rejects extension mutations when the operation queue is full(期望 429 得到 404)。形状相同、测试不同、没有本 PR 也存在:这是 mainserver.test.ts 既有的、对负载敏感的 404 抖动,值得单开 issue,但不构成阻塞本 PR 的理由。

6. 本轮未覆盖

针对正在 draining 的 bridge generation 的 workspace-qualified ACP 实测(我只驱动了 primary ACP 端点;generation guard 路径由新增单测覆盖,不在我的实时探针内)、Windows 与 Linux、带真实认证的模型 prompt 全客户端串联,以及 UUID v7 / 历史重复 ID 迁移 —— 这些与描述中已声明的范围外内容一致。

结论:可以合并。 描述所声称的行为,就是 daemon 实际表现出来的行为;此前静默的 ACP 路径现在会大声且结构化地失败;stdio 侧未校验的 session ID 已经关闭。

@doudouOUC
doudouOUC requested a lite review from Copilot August 9, 2026 07:30
@doudouOUC
doudouOUC dismissed qwen-code-ci-bot’s stale review August 9, 2026 07:31

already have 2 approve,3ks

@doudouOUC
doudouOUC added this pull request to the merge queue Aug 9, 2026
Merged via the queue into QwenLM:main with commit 60458f5 Aug 9, 2026
92 checks passed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the “caller-supplied daemon session ID” contract into a single daemon-wide behavior, coordinating create/load/resume admission across REST, ACP (primary + workspace-qualified), draining runtime generations, stdio agent validation, and official TypeScript/Java SDKs (plus daemon MCP).

Changes:

  • Introduces a shared requested-session-id admission coordinator to reject live/pending/persisted conflicts across all registered runtimes and bridge generations.
  • Adds capability-gated caller-supplied session ID support (session_id_override) across REST, ACP metadata (qwen-code/sessionId), daemon MCP tooling, and TypeScript/Java SDK clients with response verification.
  • Improves normalization/compat handling for mixed-case UUID filenames and session ID routing across transports and runtime lifecycle events.

Reviewed changes

Copilot reviewed 48 out of 48 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/sdk-typescript/test/unit/serve-bridge.test.ts Adds MCP bridge tool tests for capability-gated session_create.session_id forwarding.
packages/sdk-typescript/test/unit/DaemonClient.test.ts Adds SDK tests for gating, serialization, and response verification for caller-supplied sessionId.
packages/sdk-typescript/test/unit/AcpWsTransport.test.ts Adds tests ensuring capability discovery prefers REST and handles missing/malformed REST responses.
packages/sdk-typescript/test/unit/acpRouteTable.test.ts Tests mapping POST /session sessionId into ACP _meta["qwen-code/sessionId"].
packages/sdk-typescript/src/utils/validation.ts Updates UUID validation comment to reflect v1–v5.
packages/sdk-typescript/src/index.ts Exports DaemonSessionIdProtocolError.
packages/sdk-typescript/src/daemon/index.ts Re-exports DaemonSessionIdProtocolError from daemon entrypoint.
packages/sdk-typescript/src/daemon/DaemonClient.ts Gates caller-supplied sessionId on session_id_override and verifies returned ID matches.
packages/sdk-typescript/src/daemon/AcpWsTransport.ts Prefers REST /capabilities over ACP initialize result; adds robust fallback.
packages/sdk-typescript/src/daemon/acpRouteTable.ts Maps REST sessionId onto ACP session/new metadata key.
packages/sdk-typescript/src/daemon-mcp/serve-bridge/tools/session.ts Exposes session_create.session_id and forwards into SDK create request.
packages/sdk-typescript/src/daemon-mcp/serve-bridge/README.md Documents session_create.session_id behavior and capability gating.
packages/sdk-java/qwencode/src/test/java/com/alibaba/qwen/code/daemon/DaemonSessionClientTest.java Adds Java SDK tests for capability gating, serialization, and mismatch handling.
packages/sdk-java/qwencode/src/main/java/com/alibaba/qwen/code/daemon/DaemonClient.java Enforces session_id_override capability and validates returned sessionId vs requested.
packages/sdk-java/qwencode/src/main/java/com/alibaba/qwen/code/daemon/CreateSessionRequest.java Adds sessionId to create request builder and JSON serialization.
packages/sdk-java/qwencode/README.md Documents caller-supplied session ID usage and mismatch outcome semantics.
packages/core/src/services/sessionService.ts Adds findSessionIdIgnoringCase for legacy mixed-case transcript filenames.
packages/core/src/services/sessionService.test.ts Adds unit test for findSessionIdIgnoringCase.
packages/cli/src/serve/session-id-admission.ts Introduces daemon-wide requested-session-id admission coordinator (live/pending/persisted).
packages/cli/src/serve/session-id-admission.test.ts Adds tests for admission behavior, conflicts, failure modes, and release semantics.
packages/cli/src/serve/server/request-helpers.ts Normalizes route session IDs via normalizeSessionIdForLookup.
packages/cli/src/serve/server/request-helpers.test.ts Tests UUID route parameter normalization behavior.
packages/cli/src/serve/server.ts Wires shared admission into serve app and requires live bridge enumeration when generations can change.
packages/cli/src/serve/server.test.ts Updates serve tests for new capability tag and cross-transport admission behaviors.
packages/cli/src/serve/run-qwen-serve.ts Injects getSessionBridges to allow admission to see draining generations.
packages/cli/src/serve/routes/workspace-setup-github.test.ts Updates harness to pass getSessionBridges under hot reload.
packages/cli/src/serve/routes/session.ts Replaces per-route guards with shared admission; adds generation-open assertions before side effects.
packages/cli/src/serve/capabilities.ts Advertises session_id_override capability.
packages/cli/src/serve/acp-http/workspace-qualified-acp.test.ts Adds tests for admission sharing and generation guard behaviors in qualified ACP mounts.
packages/cli/src/serve/acp-http/transport.test.ts Extends ACP transport tests for requested session ID validation/normalization and orphan rollback.
packages/cli/src/serve/acp-http/index.ts Normalizes session IDs in ACP HTTP plumbing and injects shared admission + runtime context getters.
packages/cli/src/serve/acp-http/dispatch.ts Adds admission coordination + validation for ACP session/new meta and load/resume restore claims.
packages/cli/src/serve/acp-http/client-mcp-ws.test.ts Updates client-MCP-over-WS setup to provide admission dependencies.
packages/cli/src/config/session-id.ts Adds shared session-id parsing/normalization utilities (internal vs caller-supplied).
packages/cli/src/config/session-id.test.ts Adds unit tests for parser/normalizer and internal ID validity.
packages/cli/src/config/config.ts Moves isValidSessionId to shared session-id.ts module and re-exports it.
packages/cli/src/acp-integration/acpAgent.ts Adds defense-in-depth validation, concurrency guards, normalization, and structured conflicts in stdio agent.
packages/cli/src/acp-integration/acpAgent.test.ts Adds tests for invalid meta rejection, duplicate startup conflicts, and normalization on restore paths.
packages/acp-bridge/src/bridge.ts Forces caller-supplied session IDs to thread scope and includes them in fresh-session admission context.
packages/acp-bridge/src/bridge.test.ts Extends bridge test coverage for thread scope forcing + admission context + ACP meta injection.
integration-tests/vitest.config.ts Adds alias for built daemon transports bundle in integration tests.
integration-tests/cli/qwen-serve-routes.test.ts Adds integration coverage for end-to-end session ID override behavior and ACP WS transport path.
docs/developers/sdk-typescript.md Documents TypeScript SDK caller-supplied sessionId with capability gating and response verification.
docs/developers/sdk-java.md Documents Java SDK caller-supplied session IDs and mismatch outcome behavior.
docs/developers/qwen-serve-protocol.md Updates protocol docs for session_id_override and requested-ID semantics across REST/ACP.
docs/developers/daemon/11-capabilities-versioning.md Adds session_id_override to capability tag inventory.
docs/developers/daemon/08-session-lifecycle.md Adds session_id_override to lifecycle capability tag list.
docs/design/2026-08-01-caller-supplied-session-id.md Adds design doc describing the unified daemon-wide contract and ownership boundaries.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

headers: this.headers({ 'Content-Type': 'application/json' }, clientId),
body: JSON.stringify({
cwd: req.workspaceCwd,
...(req.sessionId !== undefined ? { sessionId: req.sessionId } : {}),
Comment on lines +116 to +118
if (sessionId === undefined) {
return { ...rest, ...(_meta !== undefined ? { _meta } : {}) };
}
qqqys added a commit to qqqys/qwen-code that referenced this pull request Aug 9, 2026
Resolves the conflict in acpAgent.ts left by 60458f5 (fix(serve):
Coordinate caller-supplied session IDs, QwenLM#8415).

Both sides touched `loadSession` and `unstable_resumeSession`. Upstream
wrapped each in a caller-supplied-id reservation — `reserveStartingSessionId`
plus a `normalizeSessionIdForLookup` / `findSessionIdIgnoringCase` preamble —
which re-indented the whole body into a new try/finally. This branch's change
to the same two functions is the Goal v3 migration: drop the
`#restoreGoalOnResume` hook, its two call sites, and the
`supersedeUnrestorableGoal` replay option, since the Goal runtime now owns
restore and the option no longer exists on `collectHistoryReplayUpdates`.

The two are orthogonal, so the resolution keeps upstream's control flow
verbatim and re-applies this branch's deletions inside it.

Verified: npm run build, npm run typecheck, packages/cli
src/acp-integration/ (28 files, 1344 tests) and packages/core src/goals/
(15 files, 353 tests) all pass.
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.9.

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

Labels

autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+) review/self-reported The linked issue was opened by the PR author (self-reported)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Caller-supplied session IDs are not coordinated across daemon transports and workspaces

6 participants