-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(serve): support reserved characters in virtual subagent ids #8717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
860a334
6df743f
1713e9a
2ba6d37
0945d22
977e6d8
b69cf27
2ba3bbc
4c2c421
9750746
a4c7df2
1c27914
d425d1f
6688f96
53265f2
a91880c
3d0f6e0
44f0e4c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -108,6 +108,7 @@ import { | |||||||||
| deleteBranch, | ||||||||||
| } from '../server/git-branch-ops.js'; | ||||||||||
| import { | ||||||||||
| MAX_VIRTUAL_SESSION_ID_PART_LENGTH, | ||||||||||
| parseVirtualSubagentSessionId, | ||||||||||
| type VirtualSubagentSessions, | ||||||||||
| } from '../virtual-subagent-sessions.js'; | ||||||||||
|
|
@@ -2392,8 +2393,8 @@ export function registerSessionRoutes( | |||||||||
| app.post('/session/:id/load', mutate(), restoreSessionHandler('load')); | ||||||||||
| app.post('/session/:id/resume', mutate(), restoreSessionHandler('resume')); | ||||||||||
|
|
||||||||||
| app.get('/session/:id/subagents/:toolCallId', async (req, res) => { | ||||||||||
| const route = 'GET /session/:id/subagents/:toolCallId'; | ||||||||||
| app.get('/session/:id/subagents/:subagentRef', async (req, res) => { | ||||||||||
| const route = 'GET /session/:id/subagents/:subagentRef'; | ||||||||||
| const sessionId = requireSessionId(req, res); | ||||||||||
| if (!sessionId) return; | ||||||||||
| if (!virtualSubagentSessions) { | ||||||||||
|
|
@@ -2404,11 +2405,14 @@ export function registerSessionRoutes( | |||||||||
| }); | ||||||||||
| return; | ||||||||||
| } | ||||||||||
| const toolCallId = req.params['toolCallId']; | ||||||||||
| if (!toolCallId || toolCallId.length > 500) { | ||||||||||
| const subagentRef = req.params['subagentRef']; | ||||||||||
| if ( | ||||||||||
| !subagentRef || | ||||||||||
| subagentRef.length > MAX_VIRTUAL_SESSION_ID_PART_LENGTH | ||||||||||
| ) { | ||||||||||
|
Comment on lines
+2408
to
+2412
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The Extract a shared helper called from both handlers, e.g. beside 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)中, 提取一个两个 handler 共用的辅助函数,例如放在 — qwen3.8-max via Qwen Code /review (v0.21.10) |
||||||||||
| res.status(400).json({ | ||||||||||
| error: '`toolCallId` must be a non-empty tool call id', | ||||||||||
| code: 'invalid_tool_call_id', | ||||||||||
| error: '`subagentRef` must be a non-empty subagent reference', | ||||||||||
| code: 'invalid_subagent_ref', | ||||||||||
|
Comment on lines
+2414
to
+2415
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The 400 message only mentions the non-empty constraint, but this diff's guard also rejects refs longer than
Suggested change
中文说明[建议] 这个 400 错误信息只提到了非空约束,但本次改动的守卫还会拒绝超过 — qwen3.8-max via Qwen Code /review (v0.21.10) |
||||||||||
| }); | ||||||||||
| return; | ||||||||||
| } | ||||||||||
|
|
@@ -2424,14 +2428,14 @@ export function registerSessionRoutes( | |||||||||
| const resolved = await virtualSubagentSessions.resolve( | ||||||||||
| runtime, | ||||||||||
| sessionId, | ||||||||||
| toolCallId, | ||||||||||
| subagentRef, | ||||||||||
| ); | ||||||||||
|
Comment on lines
2428
to
2432
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The The fix belongs in 中文说明[建议] 修复位置在 — qwen3.8-max via Qwen Code /review (v0.21.10) |
||||||||||
| if (!resolved) { | ||||||||||
| res.status(404).json({ | ||||||||||
| error: 'Subagent session not found', | ||||||||||
| code: 'session_not_found', | ||||||||||
| sessionId, | ||||||||||
| toolCallId, | ||||||||||
| subagentRef, | ||||||||||
| }); | ||||||||||
| return; | ||||||||||
| } | ||||||||||
|
|
@@ -2442,10 +2446,10 @@ export function registerSessionRoutes( | |||||||||
| }); | ||||||||||
|
|
||||||||||
| app.post( | ||||||||||
| '/session/:id/subagents/:toolCallId/cancel', | ||||||||||
| '/session/:id/subagents/:subagentRef/cancel', | ||||||||||
| mutate(), | ||||||||||
| async (req, res) => { | ||||||||||
| const route = 'POST /session/:id/subagents/:toolCallId/cancel'; | ||||||||||
| const route = 'POST /session/:id/subagents/:subagentRef/cancel'; | ||||||||||
| const sessionId = requireSessionId(req, res); | ||||||||||
| if (!sessionId) return; | ||||||||||
| if (!virtualSubagentSessions) { | ||||||||||
|
|
@@ -2456,11 +2460,14 @@ export function registerSessionRoutes( | |||||||||
| }); | ||||||||||
| return; | ||||||||||
| } | ||||||||||
| const toolCallId = req.params['toolCallId']; | ||||||||||
| if (!toolCallId || toolCallId.length > 500) { | ||||||||||
| const subagentRef = req.params['subagentRef']; | ||||||||||
| if ( | ||||||||||
| !subagentRef || | ||||||||||
| subagentRef.length > MAX_VIRTUAL_SESSION_ID_PART_LENGTH | ||||||||||
| ) { | ||||||||||
| res.status(400).json({ | ||||||||||
| error: '`toolCallId` must be a non-empty tool call id', | ||||||||||
| code: 'invalid_tool_call_id', | ||||||||||
| error: '`subagentRef` must be a non-empty subagent reference', | ||||||||||
| code: 'invalid_subagent_ref', | ||||||||||
| }); | ||||||||||
| return; | ||||||||||
| } | ||||||||||
|
|
@@ -2476,14 +2483,14 @@ export function registerSessionRoutes( | |||||||||
| const resolved = await virtualSubagentSessions.resolve( | ||||||||||
| runtime, | ||||||||||
| sessionId, | ||||||||||
| toolCallId, | ||||||||||
| subagentRef, | ||||||||||
| ); | ||||||||||
| if (!resolved) { | ||||||||||
| res.status(404).json({ | ||||||||||
| error: 'Subagent session not found', | ||||||||||
| code: 'session_not_found', | ||||||||||
| sessionId, | ||||||||||
| toolCallId, | ||||||||||
| subagentRef, | ||||||||||
| }); | ||||||||||
| return; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8785,53 +8785,70 @@ describe('createServeApp', () => { | |||||||||||||||||||
| ]); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('resolves and cancels a virtual subagent through its routes', async () => { | ||||||||||||||||||||
| const bridge = fakeBridge({ | ||||||||||||||||||||
| cancelSessionTaskImpl: async () => ({ cancelled: true }), | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| const resolveSpy = vi | ||||||||||||||||||||
| .spyOn(VirtualSubagentSessions.prototype, 'resolve') | ||||||||||||||||||||
| .mockResolvedValue({ | ||||||||||||||||||||
| sessionId: createVirtualSubagentSessionId('s-1', 'agent-1'), | ||||||||||||||||||||
| taskId: 'agent-1', | ||||||||||||||||||||
| title: 'Investigate', | ||||||||||||||||||||
| status: 'running', | ||||||||||||||||||||
| it.each([ | ||||||||||||||||||||
| ['agent%3A8', 'agent:8'], | ||||||||||||||||||||
| ['agent%2F8', 'agent/8'], | ||||||||||||||||||||
| ])( | ||||||||||||||||||||
|
Comment on lines
+8788
to
+8791
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Both added route cases decode to forms containing no
Suggested change
The added case passes today and fails under the double-decode mutant; optionally also restore the pre-diff plain scenario with 中文说明[建议] 新增的两个路由用例解码后都不含 新增的 — qwen3.8-max via Qwen Code /review (v0.21.10) |
||||||||||||||||||||
| 'resolves and cancels a virtual subagent through its routes: %s', | ||||||||||||||||||||
| async (encodedSubagentRef, subagentRef) => { | ||||||||||||||||||||
| const taskId = `general-purpose-${subagentRef}`; | ||||||||||||||||||||
| const bridge = fakeBridge({ | ||||||||||||||||||||
| cancelSessionTaskImpl: async () => ({ cancelled: true }), | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| const tokenOpts: ServeOptions = { ...baseOpts, token: 'secret' }; | ||||||||||||||||||||
| const app = createServeApp( | ||||||||||||||||||||
| { ...tokenOpts, workspace: WS_BOUND }, | ||||||||||||||||||||
| undefined, | ||||||||||||||||||||
| { bridge }, | ||||||||||||||||||||
| ); | ||||||||||||||||||||
| const resolveSpy = vi | ||||||||||||||||||||
| .spyOn(VirtualSubagentSessions.prototype, 'resolve') | ||||||||||||||||||||
| .mockResolvedValue({ | ||||||||||||||||||||
| sessionId: createVirtualSubagentSessionId('s-1', taskId), | ||||||||||||||||||||
| taskId, | ||||||||||||||||||||
| title: 'Investigate', | ||||||||||||||||||||
| status: 'running', | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| const tokenOpts: ServeOptions = { ...baseOpts, token: 'secret' }; | ||||||||||||||||||||
| const app = createServeApp( | ||||||||||||||||||||
| { ...tokenOpts, workspace: WS_BOUND }, | ||||||||||||||||||||
| undefined, | ||||||||||||||||||||
| { bridge }, | ||||||||||||||||||||
| ); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| try { | ||||||||||||||||||||
| const resolveRes = await request(app) | ||||||||||||||||||||
| .get('/session/s-1/subagents/tool-1') | ||||||||||||||||||||
| .set('Host', `127.0.0.1:${tokenOpts.port}`) | ||||||||||||||||||||
| .set('Authorization', 'Bearer secret'); | ||||||||||||||||||||
| const cancelRes = await request(app) | ||||||||||||||||||||
| .post('/session/s-1/subagents/tool-1/cancel') | ||||||||||||||||||||
| .set('Host', `127.0.0.1:${tokenOpts.port}`) | ||||||||||||||||||||
| .set('Authorization', 'Bearer secret'); | ||||||||||||||||||||
| try { | ||||||||||||||||||||
| const resolveRes = await request(app) | ||||||||||||||||||||
| .get(`/session/s-1/subagents/${encodedSubagentRef}`) | ||||||||||||||||||||
| .set('Host', `127.0.0.1:${tokenOpts.port}`) | ||||||||||||||||||||
| .set('Authorization', 'Bearer secret'); | ||||||||||||||||||||
| const cancelRes = await request(app) | ||||||||||||||||||||
| .post(`/session/s-1/subagents/${encodedSubagentRef}/cancel`) | ||||||||||||||||||||
| .set('Host', `127.0.0.1:${tokenOpts.port}`) | ||||||||||||||||||||
| .set('Authorization', 'Bearer secret'); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| expect(resolveRes.status).toBe(200); | ||||||||||||||||||||
| expect(resolveRes.headers['cache-control']).toBe('no-store'); | ||||||||||||||||||||
| expect(resolveRes.body).toMatchObject({ | ||||||||||||||||||||
| taskId: 'agent-1', | ||||||||||||||||||||
| status: 'running', | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| expect(cancelRes.status).toBe(200); | ||||||||||||||||||||
| expect(cancelRes.body).toEqual({ cancelled: true }); | ||||||||||||||||||||
| expect(resolveSpy).toHaveBeenCalledTimes(2); | ||||||||||||||||||||
| expect(resolveSpy.mock.calls[0]?.slice(1)).toEqual(['s-1', 'tool-1']); | ||||||||||||||||||||
| expect(resolveSpy.mock.calls[1]?.slice(1)).toEqual(['s-1', 'tool-1']); | ||||||||||||||||||||
| expect(bridge.cancelSessionTaskCalls).toEqual([ | ||||||||||||||||||||
| { sessionId: 's-1', taskId: 'agent-1', taskKind: 'agent' }, | ||||||||||||||||||||
| ]); | ||||||||||||||||||||
| } finally { | ||||||||||||||||||||
| resolveSpy.mockRestore(); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| expect(resolveRes.status).toBe(200); | ||||||||||||||||||||
| expect(resolveRes.headers['cache-control']).toBe('no-store'); | ||||||||||||||||||||
| expect(resolveRes.body).toMatchObject({ | ||||||||||||||||||||
| taskId, | ||||||||||||||||||||
| status: 'running', | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| expect(cancelRes.status).toBe(200); | ||||||||||||||||||||
| expect(cancelRes.body).toEqual({ cancelled: true }); | ||||||||||||||||||||
| expect(resolveSpy).toHaveBeenCalledTimes(2); | ||||||||||||||||||||
| expect(resolveSpy.mock.calls[0]?.slice(1)).toEqual([ | ||||||||||||||||||||
| 's-1', | ||||||||||||||||||||
| subagentRef, | ||||||||||||||||||||
| ]); | ||||||||||||||||||||
| expect(resolveSpy.mock.calls[1]?.slice(1)).toEqual([ | ||||||||||||||||||||
| 's-1', | ||||||||||||||||||||
| subagentRef, | ||||||||||||||||||||
| ]); | ||||||||||||||||||||
| expect(bridge.cancelSessionTaskCalls).toEqual([ | ||||||||||||||||||||
| { | ||||||||||||||||||||
| sessionId: 's-1', | ||||||||||||||||||||
| taskId, | ||||||||||||||||||||
| taskKind: 'agent', | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| ]); | ||||||||||||||||||||
| } finally { | ||||||||||||||||||||
| resolveSpy.mockRestore(); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| ); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('requires the parent runtime for virtual heartbeat and detach', async () => { | ||||||||||||||||||||
| const primaryBridge = fakeBridge(); | ||||||||||||||||||||
|
|
||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] The route-level 400 guard this PR rewrote (constant-based cap + new
invalid_subagent_refcode) has zero test coverage — the only route tests are the two happy-pathit.eachcases 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 reachesresolve()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 intorequest-helpers.ts) that drops or mis-bounds the length check ships green, letting >500-char refs reachresolve()— 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:中文说明
[建议] 本 PR 重写的路由级 400 守卫(基于常量的上限 + 新的
invalid_subagent_refcode)没有任何测试覆盖 —— 路由测试只有下面两个 happy-pathit.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)