fix(serve): support reserved characters in virtual subagent ids - #8717
Conversation
E2E verification report
Verdict: PASS. |
🩺 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 |
|
Thanks for the PR! Template looks good ✓ Problem: real, and verified against the code. The Web Shell resolves a subagent detail view through the parent tool-call ID, and the agent task ID is built directly from the provider-assigned tool-call ID ( Direction: aligned. The subagent detail view is a shipped Web Shell feature, and this is a hard failure for providers that emit IDs with reserved characters. Treating the tool-call ID as opaque while keeping the locally-generated parent session ID under strict validation is the right distinction. Size: ~110 production lines vs ~307 test lines. The change spans two packages: Approach: scope feels right. Splitting the shared validator — strict regex for parent IDs, and for agent IDs only non-empty, ≤500 code units, and lossless round-trip through the existing base64url encoding — is the minimal fix, and bounding the final encoded session ID on creation makes the 2,000-char limit symmetric with parsing. The follow-up rounds strengthened it without expanding scope: a canonical-encoding check on the parse side, a shared part-length constant replacing the bare Risk: no elevated risk signals (none of the changed files match the revert-correlated paths). Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：真实存在，且已在代码中核实。Web Shell 通过父工具调用 ID 解析 Subagent 详情视图，而 agent task ID 直接由 provider 分配的工具调用 ID 拼接生成（ 方向：对齐。Subagent 详情视图是 Web Shell 已上线的功能，而这对会生成含保留字符 ID 的 provider 来说是硬性失败。把工具调用 ID 视为不透明标识、同时对本地生成的父会话 ID 保持严格校验，这个区分是正确的。 规模：约 110 行生产代码、307 行测试。改动跨两个包： 方案：范围合理。拆分共享校验器——父 ID 保持严格正则，agent ID 仅要求非空、不超过 500 个代码单元、且能经现有 base64url 编码无损往返——是最小修复；在创建时对最终编码后的会话 ID 加长度上限，也使 2,000 字符限制与解析侧对称。后续几轮加强了实现但没有扩大范围：解析侧新增规范编码校验、用共享常量替换路由守卫里裸写的 风险：无升级风险信号（改动文件均未命中与 revert 相关的高风险路径）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-read the full diff at the current head, which has grown several rounds since my first pass. My independent take is unchanged: keep the strict validator for locally-generated parent session IDs, relax the agent ID to non-empty + bounded + losslessly representable through the existing base64url encoding, and make the length bounds symmetric between creation and parsing. That is exactly what this PR does, and I still don't see a simpler path. What I verified in the current state:
Open non-blocking items from the latest review round, for follow-up rather than merge-blocking: naming Test evidence (PR's own CI at
|
| Check | Conclusion |
|---|---|
| Test (ubuntu-latest, Node 22.x) | ✅ success |
| web-shell E2E Smoke (ubuntu-latest, Node 22.x) | ✅ success |
| Desktop Shell (ubuntu-22.04) | ✅ success |
| Desktop Shell (windows-2022) | ✅ success |
| Serve A/B (ubuntu-latest, Node 22.x) | ✅ success |
| Real daemon E2E / Java 11 | ✅ success |
| SDK Java (ubuntu × Java 11/17/21, macOS/Windows × Java 21) | ✅ success |
| Test (macos-latest / windows-latest, Node 22.x) | ⏭️ skipped |
| Integration Tests (CLI, No Sandbox) | ⏭️ skipped |
The skipped legs are the repo's normal job layout, not a gap introduced here — the most recently merged PR on main (#8942) shows the same skips, and this PR additionally ran the web-shell E2E smoke and both Desktop Shell legs green. The Serve A/B job (run against this exact head) reported no response changes against the PR base across its scenarios. Not verified independently: the author's end-to-end walk (resolve → virtual session load → SSE stream) was a single-platform macOS run; nothing in this review executed PR code.
Sandboxed verification would settle the remaining behavioural claim: @qwen-code /verify — that the 500→200 fix holds end-to-end for a colon-bearing task ID and oversized/lossy IDs still fail closed. The author has no write access, so this is a sponsored run: a maintainer's comment approves the head it runs against, it carries a pre-execution risk screen and a full workspace wipe, and its report should be read with the same skepticism as the fork's own CI logs.
中文说明
代码审查：已在当前 head 重新通读 diff。我的独立方案与此 PR 一致——父会话 ID 保留严格校验，agent ID 放宽为非空、有界、且能经现有 base64url 编码无损往返，创建与解析的长度上限对称——也没有找到更简的路径。已核实的关键点：base64url 字母表不含 .，保留字符不会破坏 split('.') 分段；往返校验真实有效（拒绝孤立代理项，避免两个不同 ID 编码成同一会话 ID）；解析侧新增的规范编码校验是使 ID 空间单射的关键——Node 的 base64 解码器对非法字符是宽容的，不加 re-encode 相等判断，手工构造的填充或畸形编码也能通过解析，测试已钉住填充变体与垃圾片段；父 ID 严格字符集是承重的：解码后的 parentSessionId 会未经净化地拼入文件系统路径（getSubagentSessionDir、chats/${parentSessionId}.jsonl），而 agentId 只做字符串比较，测试已钉住编码后的 ../foo 父 ID 被拒绝；创建侧 2,000 字符上限与解析侧对称、路由守卫改用共享常量，边界数字已手工核算（500 字符 agent ID → 696；界×492+aa → 恰好 2,000；双侧均达上限 → 2,677 被总长上限拒绝）；toolCallId → subagentRef 改名彻底，无遗留旧引用，遥测目录与 SDK 同步更新，REST URL 形状未变、SDK 形参重命名按位置传参不受影响；路由测试用 %3A/%2F 编码段驱动真实 Express 应用，单元往返用例在旧校验器下会失败，覆盖确实钉住了改动。最新一轮评审遗留的非阻塞项（resolve() 第三参数命名、400 文案提及长度约束、把重复的守卫提取到 request-helpers.ts、单次解码与 400 守卫的路由测试）留作后续打磨。
测试证据（PR 自身 CI，提交 44f0e4c，经 API 获取——本次审查未执行任何 PR 代码）：所有 pull_request 事件的工作流（Qwen Code CI、Serve A/B、SDK Java）均绿色完成。跳过的 macOS/Windows 单测与 CLI 集成测试是仓库常规任务布局——最近合入 main 的 #8942 呈现相同的跳过，且本 PR 额外跑绿了 web-shell E2E 冒烟与两个 Desktop Shell 腿。Serve A/B（正是针对当前 head 运行）未发现响应差异。未独立核实：作者的端到端演练（解析 → 虚拟会话加载 → SSE 流）仅在 macOS 单平台执行。
沙箱验证可补齐剩余的行为性声明：@qwen-code /verify——验证含冒号 task ID 的 500→200 修复在端到端成立、超长/有损 ID 仍被拒绝。作者无写权限，因此这是 sponsored run：由 maintainer 评论触发并锁定其针对的 head，运行前有执行前风险筛查与完整工作区清理，报告应与 fork 自身 CI 日志同等审慎地阅读。
— Qwen Code · qwen3.8-max
Reviewed at 44f0e4c56d707057a0d4083a23c5cb58f8dcb27c · re-run with @qwen-code /triage
|
Confidence: 4/5 — a real, code-verified bug with a minimal fix and tests that pin the change; the only reservation remains that the end-to-end evidence is single-platform (the author's macOS run), with CI covering the rest. Stepping back: this is still the kind of PR the gate should wave through, and it has gotten better with each round. The problem is real — agent task IDs trace back to provider-assigned tool-call IDs, so reserved characters are not hypothetical — and the before/after matches the actual throw path. The fix doesn't try too hard: one validator split, one symmetric length bound, reuse of the existing encoding instead of anything new. The follow-up rounds closed the soft spots rather than adding scope: the parse side now enforces canonical encodings (the base64 decoder's permissiveness made that check genuinely necessary), the parent charset is pinned by traversal-shaped tests, and the route param rename from human review makes the API honest about what it accepts. The open inline suggestions are polish — naming, message wording, a helper extraction, two more route tests — and a maintainer already approved this head with them standing, which I read as agreement that they don't block. CI is green on the repo's normal job layout, Serve A/B saw no response regressions on this exact head, and the tests fail against the old validator, so the coverage is load-bearing rather than decorative. In six months this reads as two clearly-named validators and a comment explaining why they differ — I'd thank the author, not curse them. Approving, pinned to the reviewed commit. ✅ 中文说明回顾整体：这仍然是应当放行的 PR，而且每一轮都在变好。问题真实存在——agent task ID 可追溯到 provider 分配的工具调用 ID，保留字符并非假设——before/after 与实际抛错路径吻合。修复克制：一次校验器拆分、一个对称的长度上限，复用现有编码而非新造轮子。后续几轮补上的是薄弱点而非扩大范围：解析侧强制规范编码（base64 解码器的宽容性使该校验确有必要）、用遍历形态的测试钉住父 ID 字符集、来自人工评审的路由参数改名让 API 如实反映其接受的输入。遗留的行内建议属于打磨——命名、错误文案、辅助函数提取、再补两个路由测试——且已有 maintainer 在这些建议存在的情况下批准了当前 head，可视为认同它们不构成阻塞。CI 在仓库常规任务布局下全绿，Serve A/B 在当前 head 上无响应回归，测试在旧校验器下会失败，说明覆盖是有效的而非摆设。六个月后回看，这是两个命名清晰的校验器加一条解释差异的注释。 予以批准，锁定在被审查的提交上。✅ — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not explored to full depth (tool budget reached): This PR relaxes virtual subagent session ID validation: p...: Could not execute the changed unit tests against the PR code — the worktree ( /Users/wenshao/git/qwen-code/.qwen/tmp/review-pr-8717 ) has no installed node_mod…; This PR relaxes virtual subagent session ID validation: p...: Did not trace client-side (web-shell) rendering of taskId / sessionId into HTML — out of the daemon diff's scope; the values reach the client as JSON/SSE data…; This PR relaxes virtual subagent session ID validation: p...: exact failure detail of the one failed server.test.ts run (lost to harness file flip-flop; re-run passed); This PR relaxes virtual subagent session ID validation: p...: full-file server.test.ts run under a stable (non-flipping) source checkout; This PR relaxes virtual subagent session ID validation: p...: attribution of the two unchanged failing tests to a specific environmental cause (missing coverage/.tmp dir, ordering interference, or harness flip) — verified ….
中文说明
已审查。 建议见行内评论。 未探索到全部深度（达到工具调用预算）：This PR relaxes virtual subagent session ID validation: p...：Could not execute the changed unit tests against the PR code — the worktree ( /Users/wenshao/git/qwen-code/.qwen/tmp/review-pr-8717 ) has no installed node_mod…；This PR relaxes virtual subagent session ID validation: p...：Did not trace client-side (web-shell) rendering of taskId / sessionId into HTML — out of the daemon diff's scope; the values reach the client as JSON/SSE data…；This PR relaxes virtual subagent session ID validation: p...：exact failure detail of the one failed server.test.ts run (lost to harness file flip-flop; re-run passed)；This PR relaxes virtual subagent session ID validation: p...：full-file server.test.ts run under a stable (non-flipping) source checkout；This PR relaxes virtual subagent session ID validation: p...：attribution of the two unchanged failing tests to a specific environmental cause (missing coverage/.tmp dir, ordering interference, or harness flip) — verified …。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
| const sessionId = `${PREFIX}${encodePart(parentSessionId)}.${encodePart(agentId)}`; | ||
| if (sessionId.length > MAX_VIRTUAL_SESSION_ID_LENGTH) { | ||
| throw new Error('Virtual subagent session ids require valid id parts'); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The new encoded-length cap branch throws the same "Virtual subagent session ids require valid id parts" message as the invalid-parts branch — Failure scenario: a bridge agent task ID in the ~374–500 multibyte range passes every isValidVirtualAgentId check (≤500 chars, lossless round-trip), then resolve() → createVirtualSubagentSessionId trips the encoded-length cap and the route answers HTTP 500 with "require valid id parts" even though no part is invalid; the route-level pre-guard (toolCallId.length > 500 → 400) measures a different quantity, so the caller cannot diagnose from the message. The PR's own '界'.repeat(500) test exercises and blesses this branch.
| const sessionId = `${PREFIX}${encodePart(parentSessionId)}.${encodePart(agentId)}`; | |
| if (sessionId.length > MAX_VIRTUAL_SESSION_ID_LENGTH) { | |
| throw new Error('Virtual subagent session ids require valid id parts'); | |
| } | |
| const sessionId = `${PREFIX}${encodePart(parentSessionId)}.${encodePart(agentId)}`; | |
| if (sessionId.length > MAX_VIRTUAL_SESSION_ID_LENGTH) { | |
| throw new Error( | |
| `Virtual subagent session id exceeds ${MAX_VIRTUAL_SESSION_ID_LENGTH} characters`, | |
| ); | |
| } |
中文说明
新增的编码后总长度上限分支与无效部件分支抛出了相同的 "Virtual subagent session ids require valid id parts" 错误信息，混淆了两种失败模式：部件确实非法，与部件各自合法但编码后组合长度超过 2,000 字符。触发场景：约 374–500 个多字节字符的 agent task ID 能通过 isValidVirtualAgentId 的全部检查（≤500 字符、无损往返），随后 resolve() → createVirtualSubagentSessionId 触发长度上限，路由返回 HTTP 500 且提示 "require valid id parts"，实际没有任何部件非法；路由前置校验（toolCallId.length > 500 → 400）与创建侧上限度量的量不一致，调用方无法仅凭报错定位。PR 自带的 '界'.repeat(500) 测试恰好命中并固化了这条分支。建议为长度分支使用独立错误信息，并同步更新 toThrow 断言。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Addressed in 4c2c421. The encoded-length branch now reports a distinct Virtual subagent session id exceeds 2000 characters error, while invalid components continue to use the existing valid-parts error. The oversized multibyte-agent test now asserts the distinct message.
| function isValidVirtualAgentId(value: string): boolean { | ||
| return ( | ||
| value.length > 0 && | ||
| value.length <= 500 && | ||
| decodePart(encodePart(value)) === value | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Suggestion] On the parse path the "lossless round-trip" check is vacuous — Failure scenario: parseVirtualSubagentSessionId runs isValidVirtualAgentId against the decoded value, which is always well-formed UTF-8 (invalid bytes become U+FFFD) and therefore always re-encodes losslessly, so the effective parse-side agent gate is only "non-empty and ≤500 chars". A token-bearing client can address a virtual subagent via infinitely many distinct session-ID spellings — subagent.<b64parent>.garbage decodes to \ufffd\ufffd\ufffdj\u0007 and now parses where the old strict regex returned undefined; the context/heartbeat routes branch on parse success without verifying the task exists, so a nonexistent virtual session answers 200 instead of the old 404. Create and parse now accept different sets, contradicting the PR's stated "round-trip losslessly" invariant. No privilege boundary is crossed, but the parse-side round-trip term is dead code beyond the length check.
| function isValidVirtualAgentId(value: string): boolean { | |
| return ( | |
| value.length > 0 && | |
| value.length <= 500 && | |
| decodePart(encodePart(value)) === value | |
| ); | |
| } | |
| if ( | |
| !parentSessionId || | |
| !agentId || | |
| !isValidVirtualParentSessionId(parentSessionId) || | |
| !isValidVirtualAgentId(agentId) || | |
| encodePart(agentId) !== parts[1] | |
| ) { | |
| return undefined; | |
| } |
中文说明
在解析路径上，"无损往返" 校验是空转的：parseVirtualSubagentSessionId 对解码后的值执行 isValidVirtualAgentId，而解码输出永远是合法 UTF-8（非法字节被替换为 U+FFFD），必然能无损再编码，因此解析侧对 agent 部件的有效校验只剩 "非空且 ≤500 字符"。持有 token 的客户端可以用无数种不同的会话 ID 拼写寻址同一虚拟 subagent——例如 subagent.<b64parent>.garbage 解码为 \ufffd\ufffd\ufffdj\u0007，旧的正则校验会返回 undefined，现在却能解析成功；context/heartbeat 路由只依据解析成功就放行且不校验任务是否存在，导致不存在的虚拟会话返回 200 而非旧行为的 404。创建与解析现在接受不同的集合，与 PR 描述中 "经 UTF-8 Base64URL 无损往返" 的不变量不符。未跨越任何权限边界，但解析侧的往返项除长度检查外是死代码。建议在解析侧要求编码部件为规范形式（如 encodePart(agentId) !== parts[1] 时返回 undefined），并补充非规范部件解析失败的测试。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Addressed in 4c2c421. Parsing now re-encodes and compares both the parent and agent components with their original Base64URL parts, so only canonical spellings are accepted. The regression tests cover padded aliases for each component as well as a garbage agent part that Node decodes permissively.
| expect(() => | ||
| createVirtualSubagentSessionId('parent-session', '界'.repeat(500)), | ||
| ).toThrow('valid id parts'); |
There was a problem hiding this comment.
[Suggestion] The new 2,000-char cap is tested only in the rejection direction — Failure scenario: MAX_VIRTUAL_SESSION_ID_LENGTH drifts down and, because create and parse share the one constant, mutual consistency and every round-trip test stay green (all test inputs encode to ≤ ~700 chars, so even cap=500 passes the suite); a legitimate ≤500-char agent ID whose base64url encoding crosses the cap is then rejected in createVirtualSubagentSessionId → resolve() throws → the route answers HTTP 500 with no test failing. The exact-500-char agent boundary is likewise unpinned in the acceptance direction.
| expect(() => | |
| createVirtualSubagentSessionId('parent-session', '界'.repeat(500)), | |
| ).toThrow('valid id parts'); | |
| it('accepts a session at the encoded-length boundary', () => { | |
| const agentId = '界'.repeat(492); | |
| const sessionId = createVirtualSubagentSessionId('parent-session', agentId); | |
| expect(sessionId.length).toBe(1998); | |
| expect(parseVirtualSubagentSessionId(sessionId)).toEqual({ | |
| parentSessionId: 'parent-session', | |
| agentId, | |
| }); | |
| }); |
中文说明
新增的 2,000 字符上限只在拒绝方向被测试（2,029 字符的会话）；没有任何测试断言接近上限的会话能被接受并完成往返，精确 500 字符的 agent 边界在接受方向上也没有被钉住。触发场景：MAX_VIRTUAL_SESSION_ID_LENGTH 被误改小——创建与解析共用同一个常量，相互一致性和全部往返测试仍然全绿（所有测试输入的编码会话 ≤ ~700 字符，即使上限降到 500 整套测试也通过）；此时一个合法 ≤500 字符、但 base64url 编码后跨过上限的 agent ID 会在 createVirtualSubagentSessionId 中被拒绝 → resolve() 抛错 → 路由返回 HTTP 500，而没有任何测试失败。建议补充边界接受测试，例如 '界'.repeat(492)（会话约 1998 字符，为可容纳的最大值）并断言 parseVirtualSubagentSessionId 能往返，另加一个恰好 500 字符 ASCII agent 的接受用例。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Addressed in 2ba3bbc and 9750746. The tests now accept and round-trip a 500-character ASCII agent ID, pin an exactly 2,000-character encoded session ID, and reject the next multibyte boundary. The original 界 × 492 example produces 1,997 characters with parent-session, so the exact-cap case uses 界 × 492 plus aa and explicitly asserts a length of 2,000.
|
Qwen Code review timed out. Qwen review timed out after 10800 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not explored to full depth (tool budget reached): This PR relaxes virtual subagent session ID validation so...: did not inspect getSubagentSessionDir 's internals for the raw- sessionId path that reaches findTask via the single-workspace requireSessionRuntime fast p…; This PR relaxes virtual subagent session ID validation so...: could not execute the vitest suite ( packages/cli/.../virtual-subagent-sessions.test.ts ) — the worktree has no node_modules and even the parent checkout is m….
中文说明
已审查。 建议见行内评论。 未探索到全部深度（达到工具调用预算）：This PR relaxes virtual subagent session ID validation so...：did not inspect getSubagentSessionDir 's internals for the raw- sessionId path that reaches findTask via the single-workspace requireSessionRuntime fast p…；This PR relaxes virtual subagent session ID validation so...：could not execute the vitest suite ( packages/cli/.../virtual-subagent-sessions.test.ts ) — the worktree has no node_modules and even the parent checkout is m…。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| it.each(['general-purpose-agent:8', 'general-purpose-agent/8'])( | ||
| 'round-trips an existing agent id containing reserved characters: %s', |
There was a problem hiding this comment.
[Suggestion] R2-1: No test chains a reserved-character task ID through the real VirtualSubagentSessions.resolve() — the reserved-char round-trip tests below call createVirtualSubagentSessionId/parseVirtualSubagentSessionId directly, the server route test mocks resolve, and every real-resolve unit test uses plain IDs only (fork-agent-1, general-purpose-call-1, ...). — Failure scenario: a future change at the resolve() call site that re-restricts or transforms task.id before ID creation (e.g. reintroducing a per-part charset filter) silently ships this PR's exact bug — HTTP 500 for agent:8-style provider IDs — while every current test stays green (verified by probe: a strict charset gate injected on task.id left all existing tests green; adding the missing chain test failed with the original valid id parts error).
Suggested fix — add one real-resolve case with a reserved-char id, e.g. a sibling of 'resolves an out-of-band fork by agent task id' where the bridge task's id is general-purpose-agent:8, asserting the resolved sessionId parses back to that agent ID:
it('resolves an out-of-band task whose id contains reserved characters', async () => {
// same scaffold as the fork test, bridge task id 'general-purpose-agent:8'
const target = await sessions.resolve(runtime, 'parent-1', 'general-purpose-agent:8');
expect(parseVirtualSubagentSessionId(target.sessionId)).toMatchObject({
agentId: 'general-purpose-agent:8',
});
});中文说明
没有测试把含保留字符的 task ID 经由真实的 VirtualSubagentSessions.resolve() 串到 createVirtualSubagentSessionId：下方的保留字符往返测试直接调用 create/parse，server 路由测试 mock 了 resolve，而所有真实 resolve 单测只用普通 ID（fork-agent-1、general-purpose-call-1 等）。— 失败场景：未来在 resolve() 调用点对 task.id 重新收紧或变换（例如重新引入按部件的字符集过滤），会在所有现有测试保持绿色的情况下悄悄复发本 PR 修复的 bug——agent:8 这类 provider ID 返回 HTTP 500（已用探针验证：在 task.id 上注入严格字符集门槛后全部现有测试仍为绿色；补上缺失的链路测试则以原始的 valid id parts 报错失败）。
建议修复——为真实 resolve 补一个含保留字符 ID 的用例，例如在 'resolves an out-of-band fork by agent task id' 旁新增一个 bridge task id 为 general-purpose-agent:8 的用例，断言解析出的 sessionId 能 parse 回该 agent ID。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Addressed in a4c7df2. The existing out-of-band resolve test is now parameterized with general-purpose-agent:8 and asserts that the sessionId returned by the real VirtualSubagentSessions.resolve() path parses back to the same task ID. I also verified the test's efficacy by temporarily reintroducing a strict charset gate at the resolve call site: only the new reserved-ID case failed with the original valid id parts error. The focused suite passes 19/19 on the final code.
| value.length <= 500 && | ||
| decodePart(encodePart(value)) === value |
There was a problem hiding this comment.
[Suggestion] R2-2: The validator split duplicates the 500-char part cap in two spellings — {1,500} in isValidVirtualParentSessionId's regex and value.length <= 500 here — while the sibling 2000 bound was extracted into MAX_VIRTUAL_SESSION_ID_LENGTH in the same change. The route-level toolCallId.length > 500 guards in routes/session.ts are behaviorally coupled to the same bound and equally unpinned. — Failure scenario: a future change adjusting the cap for one part type (e.g. longer provider-assigned agent IDs) edits one literal and leaves the other; the boundary tests only exercise agent ids at 500/501 with a short parent, so a divergent parent cap ships silently.
Suggested fix — name the bound once and use it in both validators (leave the route-level toolCallId caps alone unless they are meant to track the same bound):
const MAX_ID_PART_LENGTH = 500;
function isValidVirtualParentSessionId(value: string): boolean {
return (
value.length > 0 &&
value.length <= MAX_ID_PART_LENGTH &&
/^[a-zA-Z0-9_-]+$/.test(value)
);
}
function isValidVirtualAgentId(value: string): boolean {
return (
value.length > 0 &&
value.length <= MAX_ID_PART_LENGTH &&
decodePart(encodePart(value)) === value
);
}中文说明
校验器拆分后，500 字符的部件上限出现了两种写法——isValidVirtualParentSessionId 正则里的 {1,500} 和这里的 value.length <= 500——而同一次改动中，2000 的姊妹上限却被提取成了 MAX_VIRTUAL_SESSION_ID_LENGTH。routes/session.ts 路由层的 toolCallId.length > 500 守卫与同一上限存在行为耦合，同样没有被钉住。— 失败场景：未来为某一类部件调整上限（例如更长的 provider 分配 agent ID）时只改了一处字面量而漏掉另一处；边界测试只用短父 ID 验证了 agent id 在 500/501 的行为，父 ID 上限不一致会静默漏过。
建议修复——把该上限命名为一个常量并在两个校验器中使用（路由层的 toolCallId 上限除非有意与之保持一致，否则不必改动）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Addressed in 1c27914. MAX_VIRTUAL_SESSION_ID_PART_LENGTH now defines the 500-character cap used by both the parent and agent validators; the parent regex retains only the character-class check. I intentionally left the route-level toolCallId limits unchanged because they guard a different request input boundary and changing them would widen this follow-up beyond the two validator definitions. The focused suite passes 19/19.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not explored to full depth (tool budget reached): Context: this PR relaxes virtual subagent session ID vali...: could not run the actual vitest suites ( virtual-subagent-sessions.test.ts , server.test.ts ) — dependencies not installed in the review worktree; logic verifi….
中文说明
已审查。 建议见行内评论。 未探索到全部深度（达到工具调用预算）：Context: this PR relaxes virtual subagent session ID vali...：could not run the actual vitest suites ( virtual-subagent-sessions.test.ts , server.test.ts ) — dependencies not installed in the review worktree; logic verifi…。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| return ( | ||
| value.length > 0 && | ||
| value.length <= MAX_VIRTUAL_SESSION_ID_PART_LENGTH && | ||
| /^[a-zA-Z0-9_-]+$/.test(value) |
There was a problem hiding this comment.
[Suggestion] The strict parent-session-ID charset is not pinned by any test — Failure scenario: mutating this regex to also accept :, /, . (or reusing isValidVirtualAgentId for the parent) survives the entire suite, because the only invalid parent ID exercised anywhere is 'parent session', which either variant rejects (verified by probe: with the mutated regex, 19/19 tests still pass). The strictness is load-bearing: the decoded parentSessionId is interpolated into filesystem paths — getSubagentSessionDir(projectDir, parentSessionId) + readdir in findTask, and ${projectDir}/chats/${parentSessionId}.jsonl in the legacy path — so a future regression would let a crafted subagent.<base64url('../foo')>.<part> session ID put path-active characters into those paths on a green suite.
Suggested fix — add rejection cases in virtual-subagent-sessions.test.ts:
expect(() =>
createVirtualSubagentSessionId('parent/session', 'agent-1'),
).toThrow('valid id parts');
expect(() =>
createVirtualSubagentSessionId('parent:session', 'agent-1'),
).toThrow('valid id parts');
expect(
parseVirtualSubagentSessionId(
`subagent.${Buffer.from('../foo', 'utf8').toString('base64url')}.${Buffer.from('agent-1', 'utf8').toString('base64url')}`,
),
).toBeUndefined();中文说明
严格的父会话 ID 字符集目前没有任何测试钉住——失败场景：把该正则放宽为也接受 :、/、.（或让父 ID 复用 isValidVirtualAgentId），整个测试套件仍会全部通过，因为现有用例中唯一的非法父 ID 是 'parent session'，而两种变体都会拒绝它（已通过探针验证：变异后 19/19 测试仍通过）。这一严格性是关键保障：解码出的 parentSessionId 会被插入文件系统路径——findTask 中的 getSubagentSessionDir(projectDir, parentSessionId) + readdir，以及遗留路径中的 ${projectDir}/chats/${parentSessionId}.jsonl——未来一旦回归，构造的 subagent.<base64url('../foo')>.<part> 会话 ID 就能把路径敏感字符带进这些路径，且套件仍然全绿。
建议修复——在 virtual-subagent-sessions.test.ts 中补充拒绝用例（见上方英文代码块）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Thanks — agreed this would be useful regression coverage for the parent-session ID trust boundary. The production validator already enforces the strict parent charset, so this is not a current correctness fix. Since this PR has already gone through several review rounds, I’m going to defer this test-only hardening to a follow-up rather than widen the bugfix further.
| try { | ||
| const resolveRes = await request(app) | ||
| .get('/session/s-1/subagents/tool-1') | ||
| .get('/session/s-1/subagents/agent%3A8') |
There was a problem hiding this comment.
[Suggestion] The reserved-character path this PR enables is route-tested only with : (%3A) — Failure scenario: / is the riskier newly-accepted character inside a path segment (%2F); if path matching/raw-URL handling (or an intervening proxy on a remote-daemon deployment) later regresses encoded-slash handling, resolve/cancel for /-bearing task IDs would 404 or misroute while this %3A-only test stays green. / is an intended case — the unit tests round-trip general-purpose-agent/8 and the SDK ships an agent%2F1 fixture — and a probe against this branch confirmed agent%2F8 resolves and cancels correctly today, so this is a coverage gap worth pinning, not a live defect.
Suggested fix — extend this route test (or add an it.each case) to drive one resolve + cancel round-trip through /session/s-1/subagents/agent%2F8, asserting the decoded agent/8 reaches resolve and the cancel bridge call:
const resolveRes = await request(app)
.get('/session/s-1/subagents/agent%2F8')
.set('Host', `127.0.0.1:${tokenOpts.port}`)
.set('Authorization', 'Bearer secret');
// ... then assert:
expect(resolveSpy).toHaveBeenCalledWith('s-1', 'agent/8');中文说明
本 PR 启用的保留字符路径在路由层只用 :（%3A）做了测试——失败场景：/ 是路径段中风险更高的新接受字符（%2F）；如果未来的路径匹配/原始 URL 处理变化（或远程 daemon 部署中插入的代理）使编码斜杠的处理发生回归，含 / 的 task ID 的 resolve/cancel 会 404 或被错误路由，而这个只测 %3A 的测试仍会保持绿色。/ 是预期支持的用例——单元测试已对 general-purpose-agent/8 做了往返验证，SDK 也带有 agent%2F1 的 URL 夹具——且对本分支的探针确认 agent%2F8 目前能正确解析与取消，因此这是一个值得钉住的覆盖缺口，而非当前缺陷。
建议修复——扩展该路由测试（或增加一个 it.each 用例），通过 /session/s-1/subagents/agent%2F8 驱动一次 resolve + cancel 往返，并断言解码后的 agent/8 到达 resolve 及 cancel 桥接调用（见上方英文代码块）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Thanks — agreed that an encoded-slash route case would strengthen coverage. The current implementation already handles %2F correctly (and the review probe confirmed resolve/cancel works), so this is a test coverage gap rather than a live defect. Since this PR has already gone through several review rounds, I’m going to defer this test-only addition to a follow-up rather than widen the bugfix further.
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🔀 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 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; Context: this PR relaxes virtual subagent session ID vali...: could not run vitest on the two changed test files (no node_modules in the worktree; monorepo npm install exceeded the review budget) — substituted the No…; Context: this PR relaxes virtual subagent session ID vali...: did not run npm run typecheck /lint on packages/cli for the same reason.; Context: this PR relaxes virtual subagent session ID vali...: none — all checks I needed completed within budget.; Context: this PR relaxes virtual subagent session ID vali...: did not run the two updated test files ( virtual-subagent-sessions.test.ts , server.test.ts ) — my conclusions rest on source reading plus the empirical Node b…, and 3 more.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；Context: this PR relaxes virtual subagent session ID vali...：could not run vitest on the two changed test files (no node_modules in the worktree; monorepo npm install exceeded the review budget) — substituted the No…；Context: this PR relaxes virtual subagent session ID vali...：did not run npm run typecheck /lint on packages/cli for the same reason.；Context: this PR relaxes virtual subagent session ID vali...：none — all checks I needed completed within budget.；Context: this PR relaxes virtual subagent session ID vali...：did not run the two updated test files ( virtual-subagent-sessions.test.ts , server.test.ts ) — my conclusions rest on source reading plus the empirical Node b…，另有 3 条。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const MAX_VIRTUAL_SESSION_ID_PART_LENGTH = 500; | ||
| const MAX_VIRTUAL_SESSION_ID_LENGTH = 2_000; |
There was a problem hiding this comment.
[Suggestion] The 500-char part cap this new constant names is still hardcoded as a bare literal at the two upstream route guards — routes/session.ts:2408 and :2460 both read if (!toolCallId || toolCallId.length > 500). The route's toolCallId is matched against task.id in resolve() (including via the endsWith(-${toolCallId}) suffix branch, which can resolve a ≤500-char toolCallId to a longer task.id), and task.id is exactly the agentId part MAX_VIRTUAL_SESSION_ID_PART_LENGTH bounds — two expressions of one domain limit on the same string, now unsynchronised. Failure scenario: if MAX_VIRTUAL_SESSION_ID_PART_LENGTH is raised to admit longer agent task IDs, the unit tests pass but both routes still 400 with invalid_tool_call_id above 500 chars — the feature appears implemented yet silently rejects at the old bound, with no compile-time signal tying the sites together.
Suggested fix (routes/session.ts, both guards — the constant would need to be exported):
if (!toolCallId || toolCallId.length > MAX_VIRTUAL_SESSION_ID_PART_LENGTH) {中文说明
[建议] 这个新常量所命名的 500 字符部件上限，在上游的两处路由守卫中仍以裸字面量硬编码 —— routes/session.ts:2408 和 :2460 都是 if (!toolCallId || toolCallId.length > 500)。路由的 toolCallId 会在 resolve() 中与 task.id 匹配（包括通过 endsWith(-${toolCallId}) 后缀分支，它可以让 ≤500 字符的 toolCallId 解析到更长的 task.id），而 task.id 正是 MAX_VIRTUAL_SESSION_ID_PART_LENGTH 所约束的 agentId 部件 —— 同一字符串上同一领域上限的两种表达，如今互不同步。失败场景：如果调高 MAX_VIRTUAL_SESSION_ID_PART_LENGTH 以接受更长的 agent task ID，单元测试会通过，但两个路由仍会在超过 500 字符时返回 400 invalid_tool_call_id —— 功能看似已实现，却在旧边界处被悄悄拒绝，且没有任何编译期信号把这些位置关联起来。
建议修复（routes/session.ts 的两处守卫 —— 需要先导出该常量）：
if (!toolCallId || toolCallId.length > MAX_VIRTUAL_SESSION_ID_PART_LENGTH) {— qwen3.8-max via Qwen Code /review (v0.21.10)
| if (sessionId.length > MAX_VIRTUAL_SESSION_ID_LENGTH) { | ||
| throw new Error('Virtual subagent session id exceeds 2000 characters'); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The added error message hardcodes 2000 even though MAX_VIRTUAL_SESSION_ID_LENGTH is defined a few lines above; the serve directory's house convention is to interpolate the constant into such messages (e.g. server/request-helpers.ts:145 interpolates MAX_WORKSPACE_PATH_LENGTH). Failure scenario: a future change raises MAX_VIRTUAL_SESSION_ID_LENGTH; parse follows the constant, but the thrown error still tells callers the limit is 2000 characters, and this PR's own test .toThrow('exceeds 2000 characters') silently pins the stale text — a caller debugging an oversized id is given the wrong limit.
| if (sessionId.length > MAX_VIRTUAL_SESSION_ID_LENGTH) { | |
| throw new Error('Virtual subagent session id exceeds 2000 characters'); | |
| } | |
| if (sessionId.length > MAX_VIRTUAL_SESSION_ID_LENGTH) { | |
| throw new Error(`Virtual subagent session id exceeds ${MAX_VIRTUAL_SESSION_ID_LENGTH} characters`); | |
| } |
中文说明
[建议] 新增的错误信息硬编码了 2000，而 MAX_VIRTUAL_SESSION_ID_LENGTH 就定义在几行之上；serve 目录的惯例是把常量插值进此类信息（例如 server/request-helpers.ts:145 会插值 MAX_WORKSPACE_PATH_LENGTH）。失败场景：未来某次修改调高了 MAX_VIRTUAL_SESSION_ID_LENGTH；parse 会跟随常量，但抛出的错误仍告诉调用方上限是 2000 字符，而本 PR 自己的测试 .toThrow('exceeds 2000 characters') 会悄悄钉住这段过期文本 —— 调试超长 id 的调用方会得到错误的上限值。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| function isValidVirtualAgentId(value: string): boolean { | ||
| return ( | ||
| value.length > 0 && | ||
| value.length <= MAX_VIRTUAL_SESSION_ID_PART_LENGTH && | ||
| decodePart(encodePart(value)) === value | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Suggestion] decodePart(encodePart(value)) === value reads like a tautology but is the only create-time guard against lone-surrogate aliasing — verified by probe: encodePart('a\ud800b') === encodePart('a\ufffdb') (both Ye-_vWI), so without this check two distinct agent ids would encode to byte-identical session ids and alias onto one VirtualSubagentTarget (targets map keyed by ${workspaceId}:${sessionId}), cross-contaminating status updates and event streams. The '\ud800' test pins the behaviour, but neither the test name nor the code explains the collision it prevents; a maintainer "simplifying" the apparently always-true round-trip away would silently reintroduce the aliasing (the parse-side canonical checks run too late to prevent create-time collisions).
| function isValidVirtualAgentId(value: string): boolean { | |
| return ( | |
| value.length > 0 && | |
| value.length <= MAX_VIRTUAL_SESSION_ID_PART_LENGTH && | |
| decodePart(encodePart(value)) === value | |
| ); | |
| } | |
| function isValidVirtualAgentId(value: string): boolean { | |
| return ( | |
| value.length > 0 && | |
| value.length <= MAX_VIRTUAL_SESSION_ID_PART_LENGTH && | |
| // Round-trip rejects lone surrogates: UTF-8 maps them to U+FFFD, so two | |
| // distinct agent ids would otherwise encode to the same session id. | |
| decodePart(encodePart(value)) === value | |
| ); | |
| } |
中文说明
[建议] decodePart(encodePart(value)) === value 读起来像恒真式，但它是创建时防止孤立代理项（lone surrogate）别名的唯一守卫 —— 已用探针验证：encodePart('a\ud800b') === encodePart('a\ufffdb')（两者都是 Ye-_vWI），因此没有这个检查，两个不同的 agent id 会编码出字节完全相同的 session id，并别名到同一个 VirtualSubagentTarget（targets 映射以 ${workspaceId}:${sessionId} 为键），交叉污染状态更新和事件流。'\ud800' 测试钉住了这个行为，但测试名和代码都没有解释它所防止的冲突；一位维护者若"简化"掉这个看似恒真的往返检查，就会悄悄重新引入别名问题（parse 侧的规范化检查为时已晚，无法防止创建时的冲突）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| return ( | ||
| value.length > 0 && | ||
| value.length <= MAX_VIRTUAL_SESSION_ID_PART_LENGTH && | ||
| /^[a-zA-Z0-9_-]+$/.test(value) |
There was a problem hiding this comment.
[Suggestion] R1-1: Still standing from the previous round — the strict parent-session-ID validation remains unpinned by tests. Every rejection case in virtual-subagent-sessions.test.ts varies only the agent part; the single parent case ('parent session') does not pin the charset — mutating this regex to also accept :, /, . (or reusing isValidVirtualAgentId for the parent) survives the whole suite. This round's mutation analysis extends the gap: deleting value.length > 0 && or the <= MAX_VIRTUAL_SESSION_ID_PART_LENGTH check also survives, letting createVirtualSubagentSessionId emit an id the same module's parser refuses — downstream routes then 404 on the server's own freshly-created id. Failure scenario: any such mutation keeps the suite green while create() emits unparseable ids.
Suggested tests:
expect(() => createVirtualSubagentSessionId('', 'agent-1')).toThrow('valid id parts');
expect(() => createVirtualSubagentSessionId('a'.repeat(501), 'agent-1')).toThrow('valid id parts');
expect(() => createVirtualSubagentSessionId('parent:session', 'agent-1')).toThrow('valid id parts');中文说明
[建议] R1-1：上一轮的发现仍然存在 —— 严格的父会话 ID 校验依然没有被测试钉住。virtual-subagent-sessions.test.ts 中的每个拒绝用例都只变化 agent 部件；唯一的 parent 用例（'parent session'）并没有钉住字符集 —— 把这个正则变异为也接受 :、/、.（或对 parent 复用 isValidVirtualAgentId）在整个测试套件下依然存活。本轮的变异分析进一步扩展了这个缺口：删除 value.length > 0 && 或 <= MAX_VIRTUAL_SESSION_ID_PART_LENGTH 检查同样存活，使得 createVirtualSubagentSessionId 可以产出同一模块的解析器拒绝接受的 id —— 下游路由会对服务器自己刚创建的 id 返回 404。失败场景：任何此类变异都会让套件保持绿色，而 create() 却产出无法解析的 id。
建议补充的测试见上方代码块。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| try { | ||
| const resolveRes = await request(app) | ||
| .get('/session/s-1/subagents/tool-1') | ||
| .get('/session/s-1/subagents/agent%3A8') |
There was a problem hiding this comment.
[Suggestion] R1-2: Still standing from the previous round — the reserved-character route path this PR enables is route-tested only with : (%3A); / (%2F), the riskier newly-accepted character inside a path segment, never travels through the route param even though general-purpose-agent/8 round-trips at unit level. Re-verified this round that Express 5.2.1 decodes %2F correctly today, so this remains a regression-coverage gap rather than a live bug — but the %3A test cannot detect a %2F-specific regression (router upgrade or middleware/proxy rejecting encoded slashes), which would silently 404 the exact scenario this PR targets while the suite certifies reserved-character support.
Suggested fix — parametrize the route test:
it.each([
['agent%3A8', 'agent:8'],
['agent%2F8', 'agent/8'],
])('resolves and cancels a reserved-character task id: %s', async (encoded, decoded) => {
// same body, using encoded in the URLs and decoded in the resolve/cancel assertions
});中文说明
[建议] R1-2：上一轮的发现仍然存在 —— 本 PR 启用的保留字符路由路径在路由层只用 :（%3A）测试过；/（%2F）—— 路径段中风险更高的新接受字符 —— 尽管 general-purpose-agent/8 在单元层可以往返，却从未通过路由参数传递。本轮重新验证了 Express 5.2.1 目前能正确解码 %2F，所以这仍是回归覆盖缺口而非现存 bug —— 但 %3A 测试无法发现 %2F 特有的回归（路由器升级或中间件/代理拒绝编码斜杠），届时本 PR 针对的确切场景会被悄悄 404，而套件却为保留字符支持背书。
建议修复 —— 参数化路由测试（见上方代码块）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Round summaryNew commit: Feedback pointsImplemented this round (test-only)
Both findings had been deferred to a follow-up in the previous round, but they were still presented as actionable this round. Since each is a small test-only pin on behavior this PR itself introduces, they were implemented to close them out rather than deferred again. Re-verified as already fixed (earlier commits survived the merge of main)
Failed check:
|
Wire MAX_VIRTUAL_SESSION_ID_PART_LENGTH into the two subagent route guards and interpolate MAX_VIRTUAL_SESSION_ID_LENGTH into the oversized session id error so each limit keeps a single source of truth. Pin the parent part's empty and length rejections in tests and document the lone-surrogate round-trip guard.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): Context: this PR relaxes virtual subagent session ID vali...: none — all checks above were completed within budget.; Context: this PR relaxes virtual subagent session ID vali...: did not run the full server.test.ts suite (only the filtered virtual-subagent route tests), nor typecheck/lint.; Context: this PR relaxes virtual subagent session ID vali...: did not audit the web-shell client UI code paths that consume these routes beyond confirming the SDK client percent-encodes params.; Context: this PR relaxes virtual subagent session ID vali...: none — all planned checks completed within budget..
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）：Context: this PR relaxes virtual subagent session ID vali...：none — all checks above were completed within budget.；Context: this PR relaxes virtual subagent session ID vali...：did not run the full server.test.ts suite (only the filtered virtual-subagent route tests), nor typecheck/lint.；Context: this PR relaxes virtual subagent session ID vali...：did not audit the web-shell client UI code paths that consume these routes beyond confirming the SDK client percent-encodes params.；Context: this PR relaxes virtual subagent session ID vali...：none — all planned checks completed within budget.。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| function isValidVirtualParentSessionId(value: string): boolean { | ||
| return ( | ||
| value.length > 0 && | ||
| value.length <= MAX_VIRTUAL_SESSION_ID_PART_LENGTH && |
There was a problem hiding this comment.
[Suggestion] The length bound of the new strict parent-session-ID validator (value.length <= MAX_VIRTUAL_SESSION_ID_PART_LENGTH) is pinned by no test on either the create or the parse path — mutation-verified: deleting only that line from isValidVirtualParentSessionId survives the whole suite (colocated suite 20/20, virtual-subagent route subset 7/7), while the sibling isValidVirtualAgentId is pinned on all of its properties. (The empty-bound half was probed and dropped: /^[a-zA-Z0-9_-]+$/ already rejects empty strings, so value.length > 0 is redundant defense-in-depth and that mutant is equivalent.) — Failure scenario: under the mutant, createVirtualSubagentSessionId('a'.repeat(501), 'agent-1') stops throwing (the probe produced a 688-char id), and parse accepts a hand-crafted subagent.<800-char canonical base64url parent>.<agent> (~820 chars total, under the 2,000 cap) at the six URL-fed route call sites — all with a green suite, so the regression ships.
Suggested fix — parent-side boundary tests alongside the existing agent-side ones:
expect(() =>
createVirtualSubagentSessionId('a'.repeat(501), 'agent-1'),
).toThrow('valid id parts');
// parse side: a crafted oversized canonical parent part stays under the 2,000 total cap
expect(
parseVirtualSubagentSessionId(
`subagent.${Buffer.from('a'.repeat(600)).toString('base64url')}.YWdlbnQtMQ`,
),
).toBeUndefined();中文说明
[建议] 新的严格父会话 ID 校验器的长度边界（value.length <= MAX_VIRTUAL_SESSION_ID_PART_LENGTH）在创建路径和解析路径上都没有任何测试钉住 —— 已经过变异验证：只从 isValidVirtualParentSessionId 中删除这一行，整个测试套件仍然通过（同目录套件 20/20，虚拟 subagent 路由子集 7/7），而 sibling isValidVirtualAgentId 的所有属性都有测试钉住。（空值边界那一半经过探针验证后排除：/^[a-zA-Z0-9_-]+$/ 本身就会拒绝空字符串，所以 value.length > 0 是冗余的防御性检查，该变异体是等价变异体。）—— 失败场景：在该变异体下，createVirtualSubagentSessionId('a'.repeat(501), 'agent-1') 不再抛错（探针产生了 688 字符的 id），且 parse 会在六个由 URL 输入的路由调用点接受手工构造的 subagent.<800 字符规范 base64url 父部件>.<agent>（总计约 820 字符，低于 2,000 上限）—— 整套测试仍然是绿色的，因此回归会悄悄上线。
建议修复 —— 在现有 agent 侧测试旁补充父侧边界测试（见上方代码块）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #8717All five inline suggestions were addressed; four by this round's commit Feedback points and dispositions
Nothing was declined or escalated this round. VerificationCommands actually run this round, after the changes, all from the repository root unless noted:
中文说明Autofix 审查轮次 — PR #8717五条行内建议全部处理完毕：四条由本轮提交 反馈点及处理结论
本轮没有拒绝或升级（交由维护者决策）的反馈点。 验证本轮修改后实际运行的命令（除特别说明外均在仓库根目录执行）：
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/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Not explored to full depth (tool budget reached): Context: PR #8717 (QwenLM/qwen-code) relaxes virtual suba...: did not run the two changed vitest suites locally (fresh review worktree, no node_modules); boundary expectations were verified by independent arithmetic instea…; Context: PR #8717 (QwenLM/qwen-code) relaxes virtual suba...: I did not run a full npm run typecheck (repo-wide tsc) — I relied on the vitest transform/execution of the changed modules as type-level evidence. Also, I did…; Context: PR #8717 (QwenLM/qwen-code) relaxes virtual suba...: full-repo npm run typecheck — relied on successful vitest transform+execution of all changed modules as type-level evidence instead.; Context: PR #8717 (QwenLM/qwen-code) relaxes virtual suba...: full unfiltered server.test.ts run (923 tests) — deliberately skipped per the repo's "run individual/filtered tests" guidance; only the two tests changed by t….
中文说明
未发现问题。LGTM！✅
未探索到全部深度（达到工具调用预算）：Context: PR #8717 (QwenLM/qwen-code) relaxes virtual suba...：did not run the two changed vitest suites locally (fresh review worktree, no node_modules); boundary expectations were verified by independent arithmetic instea…；Context: PR #8717 (QwenLM/qwen-code) relaxes virtual suba...：I did not run a full npm run typecheck (repo-wide tsc) — I relied on the vitest transform/execution of the changed modules as type-level evidence. Also, I did…；Context: PR #8717 (QwenLM/qwen-code) relaxes virtual suba...：full-repo npm run typecheck — relied on successful vitest transform+execution of all changed modules as type-level evidence instead.；Context: PR #8717 (QwenLM/qwen-code) relaxes virtual suba...：full unfiltered server.test.ts run (923 tests) — deliberately skipped per the repo's "run individual/filtered tests" guidance; only the two tests changed by t…。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Autofix review round summary — PR #8717Feedback points and dispositionsrc:3761713110 — [Suggestion] Parent-session-ID length bound unpinned by tests (virtual-subagent-sessions.ts:212)Disposition: Addressed (RESOLVED). The finding asked for the
Change: added one colocated test, Mutation verification (applied and reverted in this round): deleting only the Note: the finding's remark that Changes
Commit: Verification
中文说明Autofix 审查轮次总结 — PR #8717反馈点及处理rc:3761713110 — [建议] 父会话 ID 长度边界缺少测试钉住（virtual-subagent-sessions.ts:212）处理：已解决（RESOLVED）。 该 finding 要求用测试在创建路径和解析路径上同时钉住
改动：新增一个同目录测试 变异验证（本轮内应用并已还原）：仅删除 说明：finding 中提到在 变更内容
提交： 验证
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
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
中文说明
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
| const toolCallId = req.params['toolCallId']; | ||
| if (!toolCallId || toolCallId.length > 500) { | ||
| if (!toolCallId || toolCallId.length > MAX_VIRTUAL_SESSION_ID_PART_LENGTH) { |
There was a problem hiding this comment.
[Suggestion] Route parameter name :toolCallId is misleading since it also accepts agent task IDs — Failure scenario: A maintainer sees the route /session/:id/subagents/:toolCallId and the error message "toolCallId must be a non-empty tool call id", reasonably assumes the value is always a tool call ID, and adds a regex guard that rejects : and /. The feature that resolves agent task IDs with these characters would silently return 400 errors.
中文说明
路由参数名 :toolCallId 具有误导性，因为它同时也接受 agent task ID——当维护者看到 /session/:id/subagents/:toolCallId 路由时，会合理认为该值始终是工具调用 ID，可能会添加拒绝 : 和 / 的正则校验，导致该功能静默返回 400 错误。建议将路由参数重命名为 :taskRef 或 :subagentRef，并更新相关的错误消息。
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all planned checks completed within budget..
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all planned checks completed within budget.。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if ( | ||
| !sessionId.startsWith(PREFIX) || | ||
| sessionId.length > MAX_VIRTUAL_SESSION_ID_LENGTH | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] R5-1: The parse-side total-length rejection branch has no decisive test — every 2,000-char cap assertion exercises createVirtualSubagentSessionId, and the test named "rejects an oversized parent part under the total session id cap" builds an ~820-char id that is rejected by the 500-char part cap, not by this branch. Probe-confirmed: removing only this condition leaves all 21 tests green. — Failure scenario: parseVirtualSubagentSessionId consumes client-supplied session ids at six route call sites; if a future change drops this check (rationale: create enforces the cap — but create only sees server-generated inputs while parse sees untrusted request strings), every existing test stays green and oversized attacker-crafted ids flow into the virtual-subagent lookup path.
// decisive parse-side case: ASCII parent at the part cap + multibyte agent part
const parentPart = Buffer.from('a'.repeat(500), 'utf8').toString('base64url'); // 667 chars
const agentPart = Buffer.from('界'.repeat(500), 'utf8').toString('base64url'); // 2,000 chars
expect(
parseVirtualSubagentSessionId(`subagent.${parentPart}.${agentPart}`), // 2,677 chars
).toBeUndefined();Also rename the mislabeled test to reflect the part cap it actually exercises.
中文说明
解析侧的总长度拒绝分支没有决定性测试——所有 2,000 字符上限的断言都走 createVirtualSubagentSessionId；名为 "rejects an oversized parent part under the total session id cap" 的测试构造的 ~820 字符 ID 实际是被 500 字符部件上限拒绝的，而非此分支。已用探针确认：仅删除该条件后全部 21 个测试仍然通过。— 失败场景：parseVirtualSubagentSessionId 在 6 个路由调用点消费客户端提供的会话 ID；若未来改动删除此检查（理由可能是 create 已强制上限——但 create 只见到服务端生成的输入，而 parse 面对的是不可信的请求字符串），所有现有测试仍为绿色，超长的攻击构造 ID 会流入虚拟 subagent 查找路径。建议补充决定性的解析侧用例（ASCII 父部件达到部件上限 + 多字节 agent 部件，编码后总长 2,677 字符 → undefined，已双向验证），并将名称不实的测试重命名以反映其实际覆盖的部件上限。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| function isValidVirtualParentSessionId(value: string): boolean { | ||
| return ( | ||
| value.length > 0 && | ||
| value.length <= MAX_VIRTUAL_SESSION_ID_PART_LENGTH && | ||
| /^[a-zA-Z0-9_-]+$/.test(value) | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Suggestion] R5-2: The strict-parent / relaxed-agent validator asymmetry is load-bearing but undocumented — the decoded parentSessionId is interpolated into filesystem paths (getSubagentSessionDir, ${projectDir}/chats/${parentSessionId}.jsonl — the latter unsanitized), while agentId is only ever string-compared, and requireSessionRuntime skips its membership lookup entirely in the single-workspace case, so no gate stands behind the charset there. — Failure scenario: a future maintainer "harmonizes" the two adjacent same-shape validators and relaxes the parent to the round-trip check → a crafted id like subagent.<b64url("../../foo")>.<...> carries a traversal-shaped parent into the path constructions; the existing '../foo' test would fail, but nothing in production code tells the maintainer why that charset matters before they change it.
| function isValidVirtualParentSessionId(value: string): boolean { | |
| return ( | |
| value.length > 0 && | |
| value.length <= MAX_VIRTUAL_SESSION_ID_PART_LENGTH && | |
| /^[a-zA-Z0-9_-]+$/.test(value) | |
| ); | |
| } | |
| // Parent ids reach filesystem paths, so they keep the strict charset; | |
| // agent ids are comparison-only and may use the round-trippable space. | |
| function isValidVirtualParentSessionId(value: string): boolean { | |
| return ( | |
| value.length > 0 && | |
| value.length <= MAX_VIRTUAL_SESSION_ID_PART_LENGTH && | |
| /^[a-zA-Z0-9_-]+$/.test(value) | |
| ); | |
| } |
中文说明
严格父 ID / 宽松 agent ID 的校验器不对称是关键设计但没有文档说明——解码出的 parentSessionId 会被插入文件系统路径（getSubagentSessionDir、${projectDir}/chats/${parentSessionId}.jsonl，后者未经净化），而 agentId 只做字符串比较；并且 requireSessionRuntime 在单工作区场景会完全跳过成员查找，因此那里没有任何网关兜底该字符集。— 失败场景：未来的维护者"统一"这两个相邻且同形的校验器，把父 ID 放宽为往返校验 → 形如 subagent.<b64url("../../foo")>.<...> 的构造 ID 会把穿越形状的父 ID 带入路径构造；现有的 '../foo' 测试会失败，但生产代码中没有任何东西能在维护者动手之前告诉他这个字符集为何重要。建议在函数上方加一行注释说明原因。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Autofix review round summary — PR #8717Feedback points and dispositions1. Review-level comment by @doudouOUC (deepseek-v4-flash via Qwen Code /review)
Disposition: informational — no code action available. This comment is a certification statement about the review run's own internal bookkeeping (which audit passes were launched/recorded), not a code-level request. The only finding that run produced is the inline suggestion below, which is addressed in this round. 2. Inline [rc:3762339069] — [Suggestion] Route parameter name
|
Add a decisive parse-side test for the 2,000-char total length cap where both decoded parts sit at the part length cap, rename the mislabeled part-cap test, and document why the parent validator keeps the strict charset while the agent validator only requires round-tripping.
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Autofix review response — PR #8717 (round 5)Two R5-1 (rc:3762433531) — parse-side total-length branch has no decisive test → AddressedVerified the finding first: the test named "rejects an oversized parent part under the total session id cap" builds an 820-char id (600-char parent → 800-char base64url part, total well under 2,000), which is rejected by the 500-char per-part cap in Changes:
R5-2 (rc:3762433535) — strict-parent / relaxed-agent asymmetry is load-bearing but undocumented → AddressedVerified the claim: the decoded ConflictNone ( Commit
Verification
中文说明Autofix 评审响应 — PR #8717（第 5 轮）自动化评审器提出了两条 R5-1 (rc:3762433531) — 解析侧总长度分支没有决定性测试 → 已处理先核实了该发现：名为 "rejects an oversized parent part under the total session id cap" 的测试构造了一个 820 字符的 ID（600 字符父部件 → 800 字符 base64url 编码，总长远低于 2,000），实际是被 改动内容：
R5-2 (rc:3762433535) — 严格父 ID / 宽松 agent ID 的不对称是关键设计但没有文档 → 已处理已核实该论断：解码出的 冲突无（ 提交
验证
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/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const resolved = await virtualSubagentSessions.resolve( | ||
| runtime, | ||
| sessionId, | ||
| toolCallId, | ||
| subagentRef, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The toolCallId → subagentRef rename stops one layer short: VirtualSubagentSessions.resolve()'s third parameter is still named toolCallId, although after this change the value flowing in is routinely a provider task id like agent:8 rather than a tool-call id — the PR's own tests pass task ids here. — Failure scenario: a maintainer tracing subagentRef from the route into resolve(runtime, parentSessionId, toolCallId) is told the wrong contract at the seam and may reason the ref must be a transcript tool-call id when analyzing the candidate.id === toolCallId || candidate.toolUseId === toolCallId || candidate.id.endsWith(...) matching; a grep for subagentRef also misses the layer where matching actually happens.
The fix belongs in virtual-subagent-sessions.ts (not at this call site): rename resolve()'s third parameter to subagentRef. The transcript-level helpers (findLegacyTaskByToolCall, readParentToolCallMetrics) genuinely compare transcript tool-call ids and can keep their names.
中文说明
[建议] toolCallId → subagentRef 的重命名还差一层：VirtualSubagentSessions.resolve() 的第三个参数仍然叫 toolCallId，而本次改动之后，流入这个参数的值通常是 agent:8 这样的 provider task id，而不是工具调用 id —— 本 PR 自己的测试也是在这里传入 task id。—— 失败场景：维护者从路由追踪 subagentRef 进入 resolve(runtime, parentSessionId, toolCallId) 时，会在这个接缝处得到错误的契约，在分析 candidate.id === toolCallId || candidate.toolUseId === toolCallId || candidate.id.endsWith(...) 匹配时可能误以为 ref 必须是 transcript 工具调用 id；grep subagentRef 也会漏掉真正发生匹配的这一层。
修复位置在 virtual-subagent-sessions.ts（而不是这个调用点）：把 resolve() 的第三个参数重命名为 subagentRef。transcript 层的辅助函数（findLegacyTaskByToolCall、readParentToolCallMetrics）确实是在比较 transcript 的工具调用 id，可以保持不变。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| error: '`subagentRef` must be a non-empty subagent reference', | ||
| code: 'invalid_subagent_ref', |
There was a problem hiding this comment.
[Suggestion] The 400 message only mentions the non-empty constraint, but this diff's guard also rejects refs longer than MAX_VIRTUAL_SESSION_ID_PART_LENGTH (500) — and an Express :subagentRef segment can never be empty for a matched route, so the length bound is the only practically reachable trigger. The sibling parseClientIdHeader in server/request-helpers.ts states both constraints ("must be a non-empty token of 128 characters or fewer"). The identical block in the cancel route (~line 2469) has the same issue. — Failure scenario: a client sends a 501-character subagentRef (a long provider task id); the daemon answers 400 with a message implying the ref was empty, so the debugging engineer investigates the wrong property.
| error: '`subagentRef` must be a non-empty subagent reference', | |
| code: 'invalid_subagent_ref', | |
| error: `\`subagentRef\` must be a non-empty subagent reference of ${MAX_VIRTUAL_SESSION_ID_PART_LENGTH} characters or fewer`, | |
| code: 'invalid_subagent_ref', |
中文说明
[建议] 这个 400 错误信息只提到了非空约束，但本次改动的守卫还会拒绝超过 MAX_VIRTUAL_SESSION_ID_PART_LENGTH（500）的 ref —— 而 Express 的 :subagentRef 段在匹配到路由时不可能为空，所以长度上限才是实际唯一可能触发的条件。同目录的 server/request-helpers.ts 中 parseClientIdHeader 会同时声明两个约束（"must be a non-empty token of 128 characters or fewer"）。cancel 路由中相同的代码块（约第 2469 行）存在同样的问题。—— 失败场景：客户端发送 501 个字符的 subagentRef（比如一个很长的 provider task id）；daemon 返回 400 并提示 ref 为空，调试的工程师会因此排查错误的方向。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const subagentRef = req.params['subagentRef']; | ||
| if ( | ||
| !subagentRef || | ||
| subagentRef.length > MAX_VIRTUAL_SESSION_ID_PART_LENGTH | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] The subagentRef extraction + length validation + coded-400 payload is duplicated verbatim in the two subagent routes this PR rewrites (GET resolve and POST cancel). server/request-helpers.ts is the established home for exactly this shape — requireSessionId and parseClientIdHeader (extraction + length cap + coded 400) — and this file already imports from it. — Failure scenario: any future change to this validation must be applied to both copies in lockstep — this very diff had to edit both copies identically twice (the rename and the constant). One missed copy silently diverges the resolve and cancel routes' acceptance boundaries, so a ref accepted by resolve is rejected by cancel (or vice versa).
Extract a shared helper called from both handlers, e.g. beside requireSessionId:
function requireSubagentRef(req: Request, res: Response): string | null {
const subagentRef = req.params['subagentRef'];
if (
!subagentRef ||
subagentRef.length > MAX_VIRTUAL_SESSION_ID_PART_LENGTH
) {
res.status(400).json({
error: '`subagentRef` must be a non-empty subagent reference',
code: 'invalid_subagent_ref',
});
return null;
}
return subagentRef;
}中文说明
[建议] 本 PR 重写的两个 subagent 路由（GET resolve 和 POST cancel）中，subagentRef 的提取 + 长度校验 + 带 code 的 400 响应体是逐字重复的。server/request-helpers.ts 正是这类逻辑的既定归属 —— requireSessionId 和 parseClientIdHeader（提取 + 长度上限 + 带 code 的 400）—— 而且本文件已经从该模块导入。—— 失败场景：未来对这个校验的任何修改都必须同步应用到两处 —— 本 diff 自己就不得不两次同步修改这两份副本（重命名和常量）。漏改一处会悄悄让 resolve 与 cancel 路由的接受边界产生分歧，导致 resolve 接受的 ref 被 cancel 拒绝（反之亦然）。
提取一个两个 handler 共用的辅助函数，例如放在 requireSessionId 旁边（示例代码见上）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| it.each([ | ||
| ['agent%3A8', 'agent:8'], | ||
| ['agent%2F8', 'agent/8'], | ||
| ])( |
There was a problem hiding this comment.
[Suggestion] Both added route cases decode to forms containing no %, so the tests cannot pin that the router decodes subagentRef exactly once (the plain unencoded case the replaced test had was also dropped). Mutation-probe verified: layering decodeURIComponent(req.params['subagentRef']!) on top of Express's built-in decode — a plausible "fix" in exactly this code area — passes both existing cases green, because decodeURIComponent('agent:8') === 'agent:8'. — Failure scenario: the relaxed validator accepts any round-trippable UTF-8 agent id, which includes %: for a task whose literal id is agent%3A8, the SDK sends agent%253A8, Express decodes once to agent%3A8, and a double-decoding handler resolves/cancels agent:8 instead — the wrong subagent session (or 404 on a valid one).
| it.each([ | |
| ['agent%3A8', 'agent:8'], | |
| ['agent%2F8', 'agent/8'], | |
| ])( | |
| it.each([ | |
| ['agent%3A8', 'agent:8'], | |
| ['agent%2F8', 'agent/8'], | |
| ['agent%253A8', 'agent%3A8'], | |
| ])( |
The added case passes today and fails under the double-decode mutant; optionally also restore the pre-diff plain scenario with ['tool-1', 'tool-1'].
中文说明
[建议] 新增的两个路由用例解码后都不含 %，因此测试无法钉住"路由器只对 subagentRef 解码一次"这一点（被替换的旧测试中的纯文本未编码用例也被删掉了）。已用变异探针验证：在 Express 内建解码之上再叠一层 decodeURIComponent(req.params['subagentRef']!) —— 在这段代码区域是一个很"合理"的"修复" —— 两个现有用例仍然全绿，因为 decodeURIComponent('agent:8') === 'agent:8'。—— 失败场景：放宽后的校验器接受任何可无损往返的 UTF-8 agent id，其中包括含 % 的：若某个 task 的字面 id 是 agent%3A8，SDK 会发送 agent%253A8，Express 解码一次得到 agent%3A8，而二次解码的 handler 会解析/取消 agent:8 —— 命中错误的 subagent 会话（或对一个有效会话返回 404）。
新增的 ['agent%253A8', 'agent%3A8'] 用例在当前代码下通过，在二次解码变异体下失败；也可以选择用 ['tool-1', 'tool-1'] 恢复改动前的纯文本场景。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| !subagentRef || | ||
| subagentRef.length > MAX_VIRTUAL_SESSION_ID_PART_LENGTH |
There was a problem hiding this comment.
[Suggestion] The route-level 400 guard this PR rewrote (constant-based cap + new invalid_subagent_ref code) has zero test coverage — the only route tests are the two happy-path it.each cases below. Probe-verified: removing this guard flips an oversized ref's response from 400 to 500 (Virtual subagent session ids require valid id parts), because the ref reaches resolve() and suffix-matches a long task id. The cancel route's copy (~line 2465) is equally untested. — Failure scenario: with nothing pinning the 400 contract, a follow-up refactor (e.g. deduplicating this guard into request-helpers.ts) that drops or mis-bounds the length check ships green, letting >500-char refs reach resolve() — reintroducing via the back door the HTTP-500 failure class this PR exists to fix.
Suggested test alongside the happy-path cases, following this file's request(app) + Host/Bearer scaffolding:
it('rejects an oversized subagentRef with a 400', async () => {
const oversized = 'a'.repeat(MAX_VIRTUAL_SESSION_ID_PART_LENGTH + 1);
const tokenOpts: ServeOptions = { ...baseOpts, token: 'secret' };
const app = createServeApp(
{ ...tokenOpts, workspace: WS_BOUND },
undefined,
{ bridge: fakeBridge() },
);
const resolveRes = await request(app)
.get(`/session/s-1/subagents/${oversized}`)
.set('Host', `127.0.0.1:${tokenOpts.port}`)
.set('Authorization', 'Bearer secret');
const cancelRes = await request(app)
.post(`/session/s-1/subagents/${oversized}/cancel`)
.set('Host', `127.0.0.1:${tokenOpts.port}`)
.set('Authorization', 'Bearer secret');
expect(resolveRes.status).toBe(400);
expect(resolveRes.body).toMatchObject({ code: 'invalid_subagent_ref' });
expect(cancelRes.status).toBe(400);
expect(cancelRes.body).toMatchObject({ code: 'invalid_subagent_ref' });
});中文说明
[建议] 本 PR 重写的路由级 400 守卫（基于常量的上限 + 新的 invalid_subagent_ref code）没有任何测试覆盖 —— 路由测试只有下面两个 happy-path it.each 用例。已用探针验证：移除该守卫后，超长 ref 的响应会从 400 变为 500（Virtual subagent session ids require valid id parts），因为 ref 会进入 resolve() 并与长 task id 发生后缀匹配。cancel 路由中的副本（约第 2465 行）同样没有测试。—— 失败场景：在没有任何测试钉住 400 契约的情况下，后续的 refactor（比如把这个守卫去重提取到 request-helpers.ts）如果漏掉或写错长度检查，仍然能全绿合入，让超过 500 字符的 ref 进入 resolve() —— 从后门重新引入本 PR 要修复的 HTTP-500 失败类别。
建议在 happy-path 用例旁补充测试，沿用本文件 request(app) + Host/Bearer 的既有写法（示例代码见上）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
Independent verification (maintainer, local real-stack A/B)I verified this PR locally with a real daemon + Web Shell + browser stack on both the PR head ( Setup
Result 1 — real-stack before/after
Full-size screenshots: before (base) · after (head) Result 2 — boundary probes on PR head (raw HTTP)
Result 3 — tests are non-hollow
Code notes
Environment: macOS (Darwin 25.6.0), Node 24. Windows/Linux not covered here — CI remains the source of truth there. 中文版本（Chinese version）独立验证（维护者本地真实栈 A/B）我在本地用真实 daemon + Web Shell + 浏览器栈对 PR head（ 环境搭建
结果 1 — 真实栈前后对比
结果 2 — PR head 边界探测(原始 HTTP)
结果 3 — 测试非空洞
代码备注
验证环境:macOS(Darwin 25.6.0)、Node 24。Windows/Linux 未覆盖,以 CI 为准。 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round: no action requiredNo actionable feedback arrived in this round, so no changes were made.
Working tree and branch head are unchanged. 中文说明Autofix 评审轮次：无需处理本轮没有收到需要处理的反馈，因此未做任何改动。
工作区与分支 head 均未变化。 Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
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: 112 passed · 0 failed · 112 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：112 通过 · 0 失败 · 112 总计 Verification reportPR #8717 Deep Verification — fix(serve): support reserved characters in virtual subagent idsVerdict: merge-ready — 112 scripted assertions executed, 112 passed, 0 failed. 中文摘要
Scope selection
Central claim — A/B tableEnvironment per cell: base = scratch worktree at
Counts: unit matrix 34/34 head, 29/29 base; route E2E 6/6 per arm. Evidence: Vacuity & mutation matrixHead's new tests run against base source (coarse mutant = the whole fix reverted), with positive controls:
Every guard the PR introduces is pinned by a test that dies when the guard is deleted; no survivors among the new guards. Coarse-mutant survivors (15 + 921) are shared-behavior regression pins — base behaves identically on them (verified by the green base controls), classified as completeness, not merge conditions. Evidence: Findings (none blocking)
Additional checks, all clean:
Not covered
MethodologyRan in the CI verify container on Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Released in v0.21.11. |
5 participants
What this PR does
Allows virtual subagent session IDs to represent existing agent task IDs that contain reserved characters such as
:and/. Parent session IDs remain strictly validated, while agent IDs must be non-empty, bounded, and losslessly round-trip through UTF-8 Base64URL encoding. The final encoded session ID is also bounded consistently during creation and parsing.Why it's needed
Web Shell resolves a subagent detail view from the parent tool-call ID and then creates a virtual session from the resolved agent task ID. Some providers produce IDs such as
agent:8andgeneral-purpose-agent:8; the previous shared validator rejected those IDs, causing the detail request to fail with HTTP 500 even though the subagent existed.The tool-call ID arrives with the parent model's function-call response and may be assigned by the model provider, or generated and normalized locally when missing or duplicated. Its concrete format is therefore not a stable application contract and must be treated as an opaque identifier. This differs from the parent session ID, which is locally generated or restored under a controlled UUID-shaped format and can remain strictly validated.
Reviewer Test Plan
How to verify
Open a session containing a completed subagent whose tool-call ID includes a colon, then open that subagent's detail view. The resolution request should return HTTP 200, the virtual session should load, and the detail stream should connect. IDs containing
/should also round-trip through virtual session creation and parsing, while empty, oversized, over-encoded, or lossy Unicode IDs remain rejected.Evidence (Before & After)
Before: opening
/session/<parent>/subagents/agent%3A8returned HTTP 500 withVirtual subagent session ids require valid id parts.After: the same route returned HTTP 200, the generated virtual subagent session loaded with HTTP 200, and its SSE detail stream connected successfully.
Tested on
Environment (optional)
Local daemon and Web Shell through
npm run dev:daemon; focused Vitest coverage for virtual-session validation and the subagent resolve/cancel routes.Risk & Scope
Linked Issues
N/A
中文说明
这个 PR 做了什么
允许虚拟 Subagent 会话 ID 表示包含
:、/等保留字符的现有 agent task ID。父会话 ID 仍采用严格校验；agent ID 必须非空、长度受限，并且能够通过 UTF-8 Base64URL 编码无损往返。创建和解析时还会一致地限制最终编码后的会话 ID 长度。为什么需要
Web Shell 会通过父工具调用 ID 解析 Subagent 详情视图，然后使用解析后的 agent task ID 创建虚拟会话。部分 provider 会产生
agent:8、general-purpose-agent:8这类 ID；原来的共享校验器会拒绝它们，导致 Subagent 明明存在，详情请求却返回 HTTP 500。工具调用 ID 会随父 Agent 模型的 function-call 响应一起到达，它可能由模型 provider 分配，也可能在缺失或重复时由本地生成或规范化。因此，其具体格式不是稳定的应用契约，应将它视为不透明标识符。这与父会话 ID 不同：父会话 ID 由本地以受控的 UUID 格式生成或恢复，因此仍可保持严格校验。
Reviewer 测试计划
如何验证
打开一个包含已完成 Subagent 的会话，且该 Subagent 的工具调用 ID 包含冒号，然后打开 Subagent 详情。解析请求应返回 HTTP 200，虚拟会话应成功加载，详情流应成功连接。包含
/的 ID 也应能通过虚拟会话创建和解析往返，而空 ID、超长 ID、编码后超长 ID 以及会损失的 Unicode ID 仍应被拒绝。前后对比证据
修复前：打开
/session/<parent>/subagents/agent%3A8会返回 HTTP 500，错误为Virtual subagent session ids require valid id parts。修复后：相同路由返回 HTTP 200，生成的虚拟 Subagent 会话以 HTTP 200 成功加载，SSE 详情流也成功连接。
测试平台
环境
使用
npm run dev:daemon启动本地 daemon 和 Web Shell；对虚拟会话校验以及 Subagent 解析/取消路由运行了定向 Vitest。风险与范围
关联 Issue
无。