-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(core): disambiguate send_message destinations #10083
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
f1b7f3b
c032b1c
740d786
90e2fdf
3267e34
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 |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ const DEFAULT_MODE = 'default' as ApprovalMode; | |
| const PLAN_MODE = 'plan' as ApprovalMode; | ||
|
|
||
| function makeTeamConfig(opts?: { | ||
| registry?: BackgroundTaskRegistry; | ||
| teamManager?: { | ||
| sendMessage: (...args: unknown[]) => Promise<void>; | ||
| broadcast: (...args: unknown[]) => Promise<BroadcastResult>; | ||
|
|
@@ -43,7 +44,8 @@ function makeTeamConfig(opts?: { | |
| : null; | ||
| return { | ||
| getTeamManager: () => teamManager, | ||
| getBackgroundTaskRegistry: () => new BackgroundTaskRegistry(), | ||
| getBackgroundTaskRegistry: () => | ||
| opts?.registry ?? new BackgroundTaskRegistry(), | ||
| getApprovalMode: () => opts?.approvalMode ?? DEFAULT_MODE, | ||
| } as unknown as Config; | ||
| } | ||
|
|
@@ -211,6 +213,40 @@ describe('SendMessageTool — team mode', () => { | |
| expect(() => tool.build({} as never)).toThrow(); | ||
| expect(() => tool.build({ to: 'alice' } as never)).toThrow(); | ||
| }); | ||
|
|
||
| it('rejects ambiguous teammate and background-task destinations', async () => { | ||
| const registry = new BackgroundTaskRegistry(); | ||
| registry.register({ | ||
| agentId: 'agent-1', | ||
| description: 'test agent', | ||
| status: 'running', | ||
| startTime: Date.now(), | ||
| abortController: new AbortController(), | ||
| isBackgrounded: true, | ||
| outputFile: '/tmp/test.jsonl', | ||
| }); | ||
| const sendMessage = vi.fn().mockResolvedValue(undefined); | ||
| const tool = new SendMessageTool( | ||
| makeTeamConfig({ | ||
| registry, | ||
| teamManager: { sendMessage, broadcast: vi.fn() }, | ||
| }), | ||
| ); | ||
|
|
||
| const result = await tool.validateBuildAndExecute( | ||
| { | ||
| to: 'alice', | ||
| task_id: 'agent-1', | ||
| message: 'ambiguous destination', | ||
| }, | ||
| new AbortController().signal, | ||
| ); | ||
|
|
||
| expect(result.error?.type).toBe(ToolErrorType.INVALID_TOOL_PARAMS); | ||
| expect(result.llmContent).toContain('Only one of "to" or "task_id"'); | ||
| expect(registry.get('agent-1')!.pendingMessages).toEqual([]); | ||
| expect(sendMessage).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('SendMessageTool — background-task mode', () => { | ||
|
|
@@ -226,7 +262,10 @@ describe('SendMessageTool — background-task mode', () => { | |
| reviveCompletedBackgroundAgent = vi.fn(); | ||
| config = { | ||
| getBackgroundTaskRegistry: () => registry, | ||
| getTeamManager: () => null, | ||
| getTeamManager: () => | ||
| ({ | ||
| getTeamFile: () => ({ members: [{ name: 'qa-reviewer' }] }), | ||
| }) as ReturnType<Config['getTeamManager']>, | ||
| resumeBackgroundAgent, | ||
| reviveCompletedBackgroundAgent, | ||
| } as unknown as Config; | ||
|
|
@@ -319,8 +358,44 @@ describe('SendMessageTool — background-task mode', () => { | |
| new AbortController().signal, | ||
| ); | ||
|
|
||
| expect(result.error?.type).toBe(ToolErrorType.SEND_MESSAGE_NOT_FOUND); | ||
| expect(result.error?.message).toBe('Task not found: nope'); | ||
| expect(result.llmContent).toContain('No background task found'); | ||
| expect(result.llmContent).not.toContain('use `to:'); | ||
| expect(result.returnDisplay).toContain('Task not found.'); | ||
| expect(result.returnDisplay).not.toContain('use "to"'); | ||
| }); | ||
|
|
||
| it('returns error for non-existent task without an active team', async () => { | ||
| const noTeamTool = new SendMessageTool( | ||
| makeTeamConfig({ registry, teamManager: null }), | ||
| ); | ||
| const result = await noTeamTool.validateBuildAndExecute( | ||
| { task_id: 'nope', message: 'hello' }, | ||
| new AbortController().signal, | ||
| ); | ||
|
|
||
| expect(result.error?.type).toBe(ToolErrorType.SEND_MESSAGE_NOT_FOUND); | ||
| expect(result.llmContent).toContain('No background task found'); | ||
| expect(result.llmContent).not.toContain('use `to:'); | ||
| expect(result.returnDisplay).toContain('Task not found.'); | ||
| expect(result.returnDisplay).not.toContain('use "to"'); | ||
| }); | ||
|
|
||
| it('suggests the teammate destination for a matching task ID', async () => { | ||
| const result = await tool.validateBuildAndExecute( | ||
| { task_id: 'QA Reviewer', message: 'hello' }, | ||
| new AbortController().signal, | ||
| ); | ||
|
|
||
| expect(result.error?.type).toBe(ToolErrorType.SEND_MESSAGE_NOT_FOUND); | ||
| expect(result.error?.message).toContain( | ||
| 'use `to: "qa-reviewer"` instead of `task_id`', | ||
| ); | ||
| expect(result.llmContent).toContain('use `to: "qa-reviewer"`'); | ||
| expect(result.returnDisplay).toContain( | ||
| 'use "to" for teammate "qa-reviewer"', | ||
| ); | ||
|
Comment on lines
+396
to
+398
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] This round adds Consider adding display assertions to both no-hint tests, e.g.: expect(result.returnDisplay).toContain('Task not found.');
expect(result.returnDisplay).not.toContain('use "to"');Fix witness: mutate the not-found branch in 中文说明本轮为正向提示测试新增了 建议在两个无提示测试中各加一条显示断言,例如: expect(result.returnDisplay).toContain('Task not found.');
expect(result.returnDisplay).not.toContain('use "to"');修复见证:将 — qwen3.8-max via Qwen Code /review (v0.22.2) |
||
| }); | ||
|
|
||
| it('returns error for a failed (non-running, non-revivable) task', async () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ import type { PermissionDecision } from '../permissions/types.js'; | |
| import { ToolErrorType } from './tool-error.js'; | ||
| import { ToolNames, ToolDisplayNames } from './tool-names.js'; | ||
| import { getAgentName } from '../agents/team/identity.js'; | ||
| import { findMemberByName } from '../agents/team/teamHelpers.js'; | ||
| import { LEADER_NAME } from '../agents/team/types.js'; | ||
| import type { ApprovalMode } from '../config/approval-mode.js'; | ||
| import { | ||
|
|
@@ -224,11 +225,23 @@ class SendMessageInvocation extends BaseToolInvocation< | |
| const entry = registry.get(this.params.task_id); | ||
|
|
||
| if (!entry) { | ||
| const teamManager = this.config.getTeamManager(); | ||
| const teammate = teamManager | ||
| ? findMemberByName( | ||
| teamManager.getTeamFile().members, | ||
| this.params.task_id, | ||
| ) | ||
| : undefined; | ||
| const teammateHint = teammate | ||
| ? ` Did you mean to message teammate "${teammate.name}"? If so, use \`to: "${teammate.name}"\` instead of \`task_id\`.` | ||
| : ''; | ||
| return { | ||
| llmContent: `Error: No background task found with ID "${this.params.task_id}".`, | ||
| returnDisplay: 'Task not found.', | ||
| llmContent: `Error: No background task found with ID "${this.params.task_id}".${teammateHint}`, | ||
| returnDisplay: teammate | ||
| ? `Task not found; use "to" for teammate "${teammate.name}".` | ||
| : 'Task not found.', | ||
|
Comment on lines
+240
to
+242
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] This new teammate-hint branch of The assertion belongs in the expect(result.returnDisplay).toContain('use "to" for teammate "qa-reviewer"');中文说明
在 expect(result.returnDisplay).toContain('use "to" for teammate "qa-reviewer"');— qwen3.8-max via Qwen Code /review (v0.22.0) |
||
| error: { | ||
| message: `Task not found: ${this.params.task_id}`, | ||
| message: `Task not found: ${this.params.task_id}${teammate ? `.${teammateHint}` : ''}`, | ||
| type: ToolErrorType.SEND_MESSAGE_NOT_FOUND, | ||
| }, | ||
| }; | ||
|
|
@@ -536,6 +549,15 @@ export class SendMessageTool extends BaseDeclarativeTool< | |
| return new SendMessageInvocation(this.config, params); | ||
| } | ||
|
|
||
| protected override validateToolParamValues( | ||
| params: SendMessageParams, | ||
| ): string | null { | ||
| if (params.to && params.task_id) { | ||
| return 'Only one of "to" or "task_id" may be provided.'; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Forward the routing fields and the message verbatim to the classifier — | ||
| * `to`/`task_id` identify the privileged sink and the `message` itself is | ||
|
|
||
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] Swapping this fixture from
getTeamManager: () => nullto a non-null stub removed the only test coverage of the new hint code's no-team branch (teamManager ? findMemberByName(...) : undefinedinsend-message.ts). After the swap, no test exercises atask_idmiss while no team is active — the production-common solo-session path — so a follow-up edit that drops the null guard would ship green.A mutation probe confirmed the gap: removing the
teamManager ?guard leaves all 23 existing tests green, while a null-manager{task_id: 'nope', message}call then crashes intoEXECUTION_FAILEDinstead of returningSEND_MESSAGE_NOT_FOUND:Keep the stub for the hint test, but add one not-found case with a null team manager: build a config with
getTeamManager: () => null, callvalidateBuildAndExecute({ task_id: 'nope', message: 'hello' }, ...), and expectToolErrorType.SEND_MESSAGE_NOT_FOUNDwith 'No background task found' and no hint.中文说明
将此处 fixture 从
getTeamManager: () => null换成非空 stub,移除了新增提示代码"无团队"分支(send-message.ts中的teamManager ? findMemberByName(...) : undefined)的唯一测试覆盖。替换之后,没有任何测试覆盖"无活动团队时task_id未命中"这一场景——而这正是生产上常见的单人会话路径——因此后续若有编辑移除空值保护,测试仍会全绿通过。变异探针确认了该缺口:移除
teamManager ?保护后,现有 23 个测试仍全部通过,而空团队管理器下的{task_id: 'nope', message}调用会崩溃为EXECUTION_FAILED,而不是返回SEND_MESSAGE_NOT_FOUND:建议保留该 stub 用于提示测试,但补充一个空团队管理器的未命中用例:构造
getTeamManager: () => null的 config,调用validateBuildAndExecute({ task_id: 'nope', message: 'hello' }, ...),断言ToolErrorType.SEND_MESSAGE_NOT_FOUND、包含 'No background task found' 且不含提示文本。— qwen3.8-max via Qwen Code /review (v0.22.0)