Repository navigation
fix(serve): support reserved characters in virtual subagent ids #8717
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,55 +79,170 @@ function activeTarget(sessions: VirtualSubagentSessions): { | |
| } | ||
|
|
||
| describe('VirtualSubagentSessions', () => { | ||
| it('rejects invalid, oversized, or non-round-trippable id parts', () => { | ||
| expect(() => | ||
| createVirtualSubagentSessionId('parent session', 'agent-1'), | ||
| ).toThrow('valid id parts'); | ||
| expect(() => | ||
| createVirtualSubagentSessionId('parent/session', 'agent-1'), | ||
| ).toThrow('valid id parts'); | ||
| expect(() => | ||
| createVirtualSubagentSessionId('parent:session', 'agent-1'), | ||
| ).toThrow('valid id parts'); | ||
| expect(() => | ||
| createVirtualSubagentSessionId('parent.session', 'agent-1'), | ||
| ).toThrow('valid id parts'); | ||
| expect(() => createVirtualSubagentSessionId('', 'agent-1')).toThrow( | ||
| 'valid id parts', | ||
| ); | ||
| expect(() => | ||
| createVirtualSubagentSessionId('a'.repeat(501), 'agent-1'), | ||
| ).toThrow('valid id parts'); | ||
| expect(() => createVirtualSubagentSessionId('parent-session', '')).toThrow( | ||
| 'valid id parts', | ||
| ); | ||
| expect(() => | ||
| createVirtualSubagentSessionId('parent-session', 'a'.repeat(501)), | ||
| ).toThrow('valid id parts'); | ||
| expect(() => | ||
| createVirtualSubagentSessionId('parent-session', '界'.repeat(493)), | ||
| ).toThrow('exceeds 2000 characters'); | ||
| expect(() => | ||
| createVirtualSubagentSessionId('parent-session', '\ud800'), | ||
| ).toThrow('valid id parts'); | ||
| }); | ||
|
|
||
| it.each([ | ||
| ['a'.repeat(500), 696], | ||
| [`${'界'.repeat(492)}aa`, 2_000], | ||
| ])( | ||
| 'round-trips an agent id at an accepted length boundary', | ||
| (agentId, expectedSessionIdLength) => { | ||
| const sessionId = createVirtualSubagentSessionId( | ||
| 'parent-session', | ||
| agentId, | ||
| ); | ||
|
|
||
| expect(sessionId).toHaveLength(expectedSessionIdLength); | ||
| expect(parseVirtualSubagentSessionId(sessionId)).toEqual({ | ||
| parentSessionId: 'parent-session', | ||
| agentId, | ||
| }); | ||
| }, | ||
| ); | ||
|
|
||
| it.each([ | ||
| ['non-canonical parent encoding', true, false], | ||
| ['non-canonical agent encoding', false, true], | ||
| ])('rejects %s', (_name, padParent, padAgent) => { | ||
| const sessionId = createVirtualSubagentSessionId( | ||
| 'parent-session', | ||
| 'agent-1', | ||
| ); | ||
| const [parentPart, agentPart] = sessionId | ||
| .slice('subagent.'.length) | ||
| .split('.'); | ||
| const nonCanonicalSessionId = `subagent.${parentPart}${padParent ? '=' : ''}.${agentPart}${padAgent ? '=' : ''}`; | ||
|
|
||
| expect( | ||
| parseVirtualSubagentSessionId(nonCanonicalSessionId), | ||
| ).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('rejects a non-canonical agent encoding that decodes as garbage', () => { | ||
| const sessionId = createVirtualSubagentSessionId( | ||
| 'parent-session', | ||
| 'agent-1', | ||
| ); | ||
| const [parentPart] = sessionId.slice('subagent.'.length).split('.'); | ||
|
|
||
| expect( | ||
| parseVirtualSubagentSessionId(`subagent.${parentPart}.garbage`), | ||
| ).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('rejects a parent part outside the strict charset', () => { | ||
| const parentPart = Buffer.from('../foo', 'utf8').toString('base64url'); | ||
| const agentPart = Buffer.from('agent-1', 'utf8').toString('base64url'); | ||
|
|
||
| expect( | ||
| parseVirtualSubagentSessionId(`subagent.${parentPart}.${agentPart}`), | ||
| ).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('rejects an oversized parent part under the total session id cap', () => { | ||
| const parentPart = Buffer.from('a'.repeat(600), 'utf8').toString( | ||
| 'base64url', | ||
| ); | ||
| const agentPart = Buffer.from('agent-1', 'utf8').toString('base64url'); | ||
|
|
||
| expect( | ||
| parseVirtualSubagentSessionId(`subagent.${parentPart}.${agentPart}`), | ||
| ).toBeUndefined(); | ||
| }); | ||
|
|
||
| it.each(['general-purpose-agent:8', 'general-purpose-agent/8'])( | ||
| 'round-trips an existing agent id containing reserved characters: %s', | ||
|
Comment on lines
+199
to
+200
Collaborator
There was a problem hiding this comment. [Suggestion] R2-1: No test chains a reserved-character task ID through the real Suggested fix — add one real- 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 经由真实的 建议修复——为真实 — qwen3.8-max via Qwen Code /review (v0.21.8)
Collaborator
Author
There was a problem hiding this comment. Addressed in a4c7df2. The existing out-of-band resolve test is now parameterized with |
||
| (agentId) => { | ||
| const sessionId = createVirtualSubagentSessionId( | ||
| 'parent-session', | ||
| agentId, | ||
| ); | ||
|
|
||
| expect(parseVirtualSubagentSessionId(sessionId)).toEqual({ | ||
| parentSessionId: 'parent-session', | ||
| agentId, | ||
| }); | ||
| }, | ||
| ); | ||
|
|
||
| it.each(['fork-agent-1', 'general-purpose-agent:8'])( | ||
| 'resolves an out-of-band task by agent task id: %s', | ||
| async (taskId) => { | ||
| const runtime = { | ||
| workspaceId: 'workspace-1', | ||
| workspaceCwd: '/workspace', | ||
| env: { mode: 'parent-process', overlayKeys: [] }, | ||
| bridge: { | ||
| getSessionTasksStatus: async () => ({ | ||
| v: 1 as const, | ||
| sessionId: 'parent-session', | ||
| now: Date.now(), | ||
| tasks: [ | ||
| { | ||
| kind: 'agent' as const, | ||
| id: taskId, | ||
| label: 'Review current changes', | ||
| description: 'Review current changes', | ||
| status: 'running' as const, | ||
| startTime: Date.now(), | ||
| runtimeMs: 1, | ||
| outputFile: `/tmp/${taskId}.jsonl`, | ||
| isBackgrounded: true, | ||
| }, | ||
| ], | ||
| }), | ||
| }, | ||
| } as unknown as WorkspaceRuntime; | ||
|
|
||
| const resolved = await new VirtualSubagentSessions().resolve( | ||
| runtime, | ||
| 'parent-session', | ||
| taskId, | ||
| ); | ||
|
|
||
| expect(resolved).toMatchObject({ | ||
| taskId, | ||
| title: 'Review current changes', | ||
| status: 'running', | ||
| }); | ||
| expect(parseVirtualSubagentSessionId(resolved!.sessionId)).toEqual({ | ||
| parentSessionId: 'parent-session', | ||
| agentId: taskId, | ||
| }); | ||
| }, | ||
| ); | ||
|
|
||
| it('resolves, fully loads, and independently streams an agent transcript', async () => { | ||
| const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-subagent-')); | ||
| tempDirs.push(dir); | ||
|
|
||
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] Route parameter name
:toolCallIdis misleading since it also accepts agent task IDs — Failure scenario: A maintainer sees the route/session/:id/subagents/:toolCallIdand the error message "toolCallIdmust 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)