feat(serve): support caller-supplied sessionId in POST /session - #7836
Merged
Conversation
Pass through a caller-supplied session id across the three layers that previously dropped it silently: 1. REST layer (session.ts): parse `sessionId` from request body with validation (non-empty string, max 128 chars). Return 400 on invalid input instead of silently ignoring it. 2. Bridge (bridge.ts): thread `sessionId` through `BridgeSpawnRequest` → `doSpawn` → `connection.newSession()` via ACP `_meta` extension key `qwen-code.sessionId`. The ACP protocol's `NewSessionRequest` has no native sessionId field, so `_meta` is the spec-compliant extension point. 3. Agent (acpAgent.ts): extract `qwen-code.sessionId` from `_meta` in the `newSession` handler and pass it as the 4th argument to `newSessionConfig`, which already supports it (falls back to randomUUID when absent). The core Config constructor already accepts `params.sessionId` with format validation and duplicate detection, so no changes needed there.
Collaborator
qwen-code-ci-bot
requested changes
Jul 27, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
@qwen-code-dev-bot Two things need attention before this can move forward:
1. PR template not followed. The body uses freeform headings (## Summary, ## Changes, ## Tests) instead of the required template. Missing sections: ## What this PR does, ## Why it's needed, ## Reviewer Test Plan (with How to verify / Evidence / Tested on), ## Risk & Scope, ## Linked Issues, and the <details> Chinese translation. Please restructure the body to follow the template.
2. Linked issue #7831 does not describe this problem. The PR says "Fixes the silent-drop issue described in #7831," but #7831 reports repeated ECONNRESET on streaming responses when context exceeds ~150k tokens — a server-side gateway timeout. It says nothing about sessionId being silently dropped by POST /session. If there is a real use case for caller-supplied session IDs, please link the correct issue or feature request, or describe the concrete scenario (who calls POST /session with a sessionId, and what breaks today when it is ignored).
中文说明
@qwen-code-dev-bot 有两个问题需要先处理：
1. 未遵循 PR 模板。 PR 正文使用了自由格式标题（## Summary、## Changes、## Tests），而非模板要求的结构。缺少：## What this PR does、## Why it's needed、## Reviewer Test Plan（含 How to verify / Evidence / Tested on）、## Risk & Scope、## Linked Issues，以及 <details> 中文翻译。请按模板重新组织正文。
2. 关联的 issue #7831 与此 PR 无关。 PR 声称"修复 #7831 中描述的静默丢弃问题"，但 #7831 报告的是大上下文（150k+ tokens）流式响应时的 ECONNRESET 错误——是服务端网关超时问题，与 POST /session 忽略 sessionId 完全无关。如果确实存在调用方指定 sessionId 的需求，请关联正确的 issue 或 feature request，或描述具体场景（谁在调用 POST /session 时传入 sessionId，当前忽略它会导致什么问题）。
— Qwen Code · qwen3.8-max-preview
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
Collaborator
|
I completed the implementation hardening and regression coverage in git cherry-pick 27091356aIn addition to the three requested pass-throughs, the commit shares the existing CLI session-ID validator with REST/ACP, prevents a mismatched requested ID from silently attaching to a live or in-flight single-scope session, and converts duplicate IDs in ACP mode into a structured Local verification on macOS: affected test files passed (bridge 451, ACP agent 322, CLI config 302, REST server 841), targeted ESLint and formatting passed, and the full build and workspace typecheck passed. Concrete motivation for the open triage request: daemon REST clients can preallocate deterministic IDs to correlate a newly created Qwen session with an external record or deterministic Arena agent identity. Before this change, |
qwen-code-ci-bot
requested changes
Jul 27, 2026
Collaborator
Author
Prevent path traversal via unsanitized sessionId and ensure compatibility with SessionService's SESSION_FILE_PATTERN by validating UUID format at the route boundary. Extract a shared REQUESTED_SESSION_ID_META_KEY constant (using the qwen-code/ slash convention) and fix the JSDoc that incorrectly claimed Config validates the id.
Collaborator
|
Follow-up review fixes are available in 2444b3ada on the same fork branch. Because the in-repository PR head is still not writable by this GitHub identity, please cherry-pick 2709135 and then 2444b3a (or cherry-pick the range fa5604c..2444b3a from xurik:codex/session-id-passthrough-7836). This follow-up makes explicit IDs live-unique across workspace runtimes, bounds and shares the UUID-agent suffix validator across persistence consumers, introduces the distinct session_id_exists error, and adds the session_id_override capability plus TypeScript/Java SDK support. Verification: full affected CLI files 1767 tests, bridge 451 tests, core 252 tests, TypeScript SDK 301 tests, and the full Java DaemonSessionClientTest passed. npm run build, npm run typecheck, and npm run lint also passed. |
Collaborator
Author
|
🤖 Addressed the latest review feedback (round 1/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/10 轮）。改动内容与我反驳保留之处如下： Review feedback addressedInline findings[rc:3656581071] Critical — Path traversal via unsanitized Enforced UUID format ( [rc:3656581085] Critical — Non-UUID session IDs orphaned by The same UUID format enforcement ensures every caller-supplied [rc:3656581090] Suggestion — Incorrect JSDoc on Updated the JSDoc in [rc:3656581097] Suggestion — Duplicated inline meta key with wrong naming convention → Resolved Defined Review-level findingsPR template not followed → Noted. The PR body needs restructuring to follow the repository template ( Linked issue #7831 does not describe this problem → Noted. The reviewer is correct that #7831 reports Verification
中文说明已处理的审查反馈行内发现[rc:3656581071] Critical — 未过滤的 在 [rc:3656581085] Critical — 非 UUID 会话 ID 被 同样的 UUID 格式强制确保每个调用方提供的 [rc:3656581090] Suggestion — 将 [rc:3656581097] Suggestion — 重复的内联 meta 键且命名约定错误 → 已解决 在 审查级别发现未遵循 PR 模板 → 已记录。PR 正文需要按仓库模板重新组织（ 关联的 issue #7831 与此 PR 无关 → 已记录。审查者正确地指出 #7831 报告的是大上下文流式传输时的 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
Collaborator
Author
|
🔀 Base updated: red check(s) [review-pr] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [review-pr] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
requested changes
Jul 27, 2026
Collaborator
Author
|
🤖 Addressed the latest review feedback (round 2/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/10 轮）。改动内容与我反驳保留之处如下： Review feedback addressed[Critical] Duplicate
|
Collaborator
|
Qwen Code review timed out. Qwen review timed out after 300 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
Collaborator
Author
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action takenNo code change was made this round because there is no actionable review feedback to address. What this round contained:
Conclusion: there is nothing in the PR code to fix. The timed-out review is an operator/workflow matter (retry with a longer timeout), not a change to this branch. The branch is left as-is. 中文说明Autofix 审查轮次 —— 未采取任何操作本轮未做任何代码改动，因为没有可处理的审查反馈。 本轮包含的内容：
结论： PR 代码中没有需要修复的内容。超时的审查属于操作/工作流层面的事项（以更长的超时时间重试），而非对本分支的改动。分支保持原样。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Collaborator
Author
|
🔀 Base updated: red check(s) [review-pr] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [review-pr] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
…ence from isValidSessionId (#7836)
Collaborator
Author
|
🤖 Addressed the latest review feedback (round 3/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/10 轮）。改动内容与我反驳保留之处如下： Review feedback addressed[rc:3659161045] Divergent UUID validation regex vs.
|
qwen-code-ci-bot
requested changes
Jul 27, 2026
Collaborator
Author
|
🤖 AutoFix updated a stale base — the fix did not pass verification, but this PR was behind Review feedback addressed[Critical] TOCTOU race on concurrent duplicate
|
doudouOUC
reviewed
Jul 27, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
🤖 Addressed the latest review feedback (round 5/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/10 轮）。改动内容与我反驳保留之处如下： Review feedback addressed[rc:3659744026] Critical — TOCTOU race on concurrent duplicate sessionIdsDecision: Implemented. Added an The fix has two layers:
This ensures the second concurrent request gets a [rc:3660041603] Suggestion — Same TOCTOU race (duplicate of rc:3659744026)Decision: Resolved by the same fix above. The suggestion correctly identified the Changes
Conflict notesNo conflicts ( Verification
中文说明已处理的审查反馈[rc:3659744026] Critical — 并发重复 sessionId 的 TOCTOU 竞态决定： 已实现。 添加了 修复分为两层：
这确保第二个并发请求收到 [rc:3660041603] Suggestion — 相同的 TOCTOU 竞态（与 rc:3659744026 重复）决定： 由上述相同修复解决。 该建议正确识别了 变更内容
冲突说明无冲突（ 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max-preview via Qwen Code /review
Collaborator
|
@qwen-code /triage |
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. |
Collaborator
Author
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Round summaryFeedback: misleading "daemon-wide" comment on session liveness check[rc:3689312643] Decision: Implemented (option 2 — correct the comment). Changes: Rewrote the comment to accurately describe the per-workspace scope: the check queries only the current workspace's bridge; the daemon-wide Making the check truly daemon-wide (option 1) would be a behavioral change beyond this PR's scope — the per-workspace check plus Verification
中文说明本轮总结反馈：会话存活检查注释中"daemon 级别"的描述具有误导性[rc:3689312643] 决定： 已实施（方案二——修正注释）。 变更内容： 重写注释以准确描述按工作区的作用域：该检查仅查询当前工作区的 bridge；daemon 级别的 将检查改为真正的 daemon 级别（方案一）属于超出本 PR 范围的行为变更——按工作区检查加上 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
doudouOUC
previously approved these changes
Jul 31, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed at 0b89e9c (review started at c600a9d; the only delta is the comment-accuracy commit, which I diffed line by line — no code change). The thread-through is real at every layer — I traced the id from route to config: BridgeSpawnRequest.sessionId → _meta[REQUESTED_SESSION_ID_META_KEY] on newSession → acpAgent reads the meta and passes it into newSessionConfig → loadCliConfig argv, with the route verifying session.sessionId === requestedSessionId afterward and returning 500 session_id_not_honored (plus orphan-session reap and branch/worktree rollback replicated into that early-return, since a return skips the outer catch) if the agent came back with a different id — no layer silently drops or substitutes the id. Validation checks out character by character: HTTP_SESSION_ID_REGEX is the claimed strict subset of config.ts's SESSION_ID_REGEX (identical v4 version/variant nibbles, minus the -agent- suffix arm), nil-UUID and v7 rejected by both (test-pinned), lowercase normalization after the case-insensitive match. The three-source 409 ordering is correct: live-on-bridge first (catches created-but-never-prompted sessions with no transcript), then on-disk via Storage.runWithRuntimeBaseDir with the workspace's own runtimeOutputDir (closing the earlier default-dir mismatch — and the agent side now throws SessionIdConflictError mapped to a per-request RPC error instead of process.exit(1)-ing the shared ACP child), then the daemon-wide inFlightSessionIds reservation released in finally on every path.
On the liveness-scope thread: I independently confirmed the bot was right — in multi-workspace mode each secondary workspace gets its own bridge (runtime.createAcpSessionBridge in run-qwen-serve), so the old "daemon-wide" comment was wrong and sequential cross-workspace id reuse of a never-prompted session does slip past all three guards into ambiguous_session_owner. The head commit takes exactly the right-sized fix: the comment now states per-workspace scope and discloses the residual sequential case honestly. The exposure needs a caller deliberately reusing a custom id across workspaces, so documenting rather than building a cross-bridge sweep is the correct trade at round 12+ of this PR; a daemon-wide liveness sweep can be a follow-up if that scenario ever materialises. The three remaining unresolved threads are all outdated (2026-07-28) and their findings are verified fixed at this HEAD (side-effect rollback in the early-return, regex subset, runtime-dir pinning) — they just need closing.
Ran the touched suites locally: bridge 467/467, config 318/318, acpAgent 355/355; all 12 tests this PR adds to server.test.ts passed identically across two full runs (the few full-suite failures were in untouched areas — transcript timeout, CORS, device-flow — non-reproducing between runs and green in CI, i.e. local load flake, not this diff). CI green at this HEAD; maintainer approved at c600a9d and the only change since is the comment fix.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max-preview via Qwen Code /review
Collaborator
Author
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed on PR #7836SummaryOne inline suggestion from the automated reviewer. It is implemented; no other Feedback points[rc:3689917781] Pin the
|
Collaborator
Author
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
wenshao
approved these changes
Jul 31, 2026
Collaborator
|
@qwen-code /resolve |
Collaborator
|
@qwen-code /triage |
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. |
Collaborator
Author
|
Qwen Code did not run conflict resolution for this request. PR #7836 does not currently have merge conflicts with main. |
ytahdn
approved these changes
Jul 31, 2026
Collaborator
|
Released in v0.21.3. |
This was referenced Aug 3, 2026
Closed
This was referenced Aug 11, 2026
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
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
POST /sessionpreviously ignoredsessionIdin the request body. The REST → bridge → agent chain never carried it, so a caller that asked for a specific id silently got a random UUID back — no error, no warning. This threads it through and validates it at the route boundary.Changes (5 source files, 3 packages)
1. REST layer —
packages/cli/src/serve/routes/session.tssessionIdfrom the request body and validate it as a UUID; return400 invalid_session_idon bad input instead of silently ignoring itHTTP_SESSION_ID_REGEXis a strict subset ofconfig.ts'sisValidSessionId: same v4 version/variant nibbles, minus the-agent-{suffix}form. The subset relationship matters in both directions — an id the daemon accepts but the CLI rejects would create a session/resume <id>can never address (resumeCommand.tsgates onisValidSessionIdand falls through to title matching), and a-agent--suffixed id would write a transcriptSessionService.SESSION_FILE_PATTERNcan never see409 session_id_conflict, from three sources:SessionService.sessionExistsInAnyState(covers archived sessions too)getSessionSummarylookup. The disk check cannot see these: the transcript JSONL is only written on a session's first message, so a created-but-never-prompted session leaves no file behind for its whole lifetime. Without this, a sequential retry falls through to the agent's ownSession <id> is already active.guard and surfaces as an opaque500 / -32603inFlightSessionIdsset that closes the TOCTOU between two concurrent requests that both pass the checks above before either session existssessionScope: 'thread', so it never coalesces onto an existingsingle-scope session500 session_id_not_honored) and roll back the branch/worktree rather than returning a surprise idsessionIdtobridge.spawnOrAttach()2. Bridge types —
packages/acp-bridge/src/bridgeTypes.tssessionId?: stringtoBridgeSpawnRequestREQUESTED_SESSION_ID_META_KEY = 'qwen-code/sessionId'3. Bridge —
packages/acp-bridge/src/bridge.tssessionIdthroughdoSpawn→connection.newSession()via the ACP_metaextension keyqwen-code/sessionIdNewSessionRequesthas no nativesessionIdfield, so_metais the spec-compliant extension point4. Agent —
packages/cli/src/acp-integration/acpAgent.tsqwen-code/sessionIdfrom_metain thenewSessionhandler and pass it tonewSessionConfigSessionIdConflictErrortoRequestError.invalidParamsso a duplicate fails the one request5. CLI config —
packages/cli/src/config/config.tsSessionIdConflictErrorand athrowOnSessionIdConflictparameter onloadCliConfigloadCliConfigcallsprocess.exit(1)when a caller-supplied session id already exists. That is correct for the interactive CLI, but the daemon multiplexes every session in a workspace onto one shared ACP child — a single duplicate id would kill all of them. Embedded callers set the flag to get a throw instead, andnewSessionConfigmaps it to aRequestError.What's NOT changed
The core
Configconstructor (packages/core/src/config/config.ts:2036) doesthis.sessionId = params.sessionId ?? randomUUID()— it uses the value verbatim, with no validation, and the sessionId becomes a filename (<sessionId>.jsonl,<sessionId>.runtime.json, …). Format validation and duplicate detection live in the CLI'sloadCliConfig, not in core, so the route-boundary regex is the only barrier between a caller-supplied string and a filesystem path. No core changes needed, but that is why the validation is strict.Tests
server.test.ts: forwarding, lowercase normalization,400on non-UUID/traversal/empty/non-string/-agent-suffix,400on UUID shapesisValidSessionIdrejects,409on existing-on-disk,409on live-but-unflushed,409on concurrent in-flight, in-flight release after a spawn failure,500 session_id_not_honoredacpAgent.test.ts:_metaextraction, duplicate mapped toRequestErrorinstead of crashing the childbridge.test.ts:_metainjection onnewSessionconfig.test.ts: exit on duplicate by default, throw underthrowOnSessionIdConflict, no throw for a fresh id