diff --git a/packages/core/src/agents/team/tasks.test.ts b/packages/core/src/agents/team/tasks.test.ts index 5fa71f4fb3c..ecafad60f5c 100644 --- a/packages/core/src/agents/team/tasks.test.ts +++ b/packages/core/src/agents/team/tasks.test.ts @@ -24,6 +24,7 @@ import { notifyTasksUpdated, TaskOwnershipError, RECIPROCAL_CALLER, + normalizeTaskId, } from './tasks.js'; import { mockCompromisedLock } from '../../test-utils/mock-compromised-lock.js'; @@ -53,6 +54,23 @@ function setMockDir(dir: string): void { ).__setMockGlobalDir(dir); } +describe('normalizeTaskId', () => { + it('trims whitespace and strips one leading #', () => { + expect(normalizeTaskId(' 1 ')).toBe('1'); + expect(normalizeTaskId('#1')).toBe('1'); + expect(normalizeTaskId(' #42 ')).toBe('42'); + // Only one leading # is stripped. + expect(normalizeTaskId('##1')).toBe('#1'); + }); + + it('returns undefined when nothing remains', () => { + expect(normalizeTaskId('')).toBeUndefined(); + expect(normalizeTaskId(' ')).toBeUndefined(); + expect(normalizeTaskId('#')).toBeUndefined(); + expect(normalizeTaskId(' # ')).toBeUndefined(); + }); +}); + describe('tasks', () => { let tmpDir: string; diff --git a/packages/core/src/agents/team/tasks.ts b/packages/core/src/agents/team/tasks.ts index 93f27fec91d..3f0dbf50f76 100644 --- a/packages/core/src/agents/team/tasks.ts +++ b/packages/core/src/agents/team/tasks.ts @@ -187,6 +187,19 @@ export function assertValidTaskId(taskId: string): void { } } +/** + * Normalize a model-supplied task-ID reference: trim surrounding + * whitespace and strip one leading `#` (the rendered display form, + * e.g. `#1`). Returns `undefined` when nothing remains, so callers + * can treat blank references as absent instead of forwarding `''` + * to filters that activate on `!== undefined`. The result must + * still pass `assertValidTaskId` before use. + */ +export function normalizeTaskId(raw: string): string | undefined { + const id = raw.trim().replace(/^#/, ''); + return id === '' ? undefined : id; +} + /** Path to a single task file. */ export function getTaskPath(teamName: string, taskId: string): string { assertValidTaskId(taskId); diff --git a/packages/core/src/tools/task-list.test.ts b/packages/core/src/tools/task-list.test.ts index 49ffd01f07e..222f8cd1604 100644 --- a/packages/core/src/tools/task-list.test.ts +++ b/packages/core/src/tools/task-list.test.ts @@ -137,6 +137,120 @@ describe('TaskListTool', () => { expect(String(result.llmContent)).toContain('owner must include'); }); + // #9281: blank optional filters must behave like absent filters, + // matching how the tool's description/schema present them. + describe('blank filters are treated as absent (#9281)', () => { + it('treats an empty owner filter as no filter', async () => { + await createTask(TEAM, { + subject: 'Owned', + description: 'desc', + owner: 'alice', + }); + await createTask(TEAM, { + subject: 'Unowned', + description: 'desc', + }); + + const invocation = tool.build({ owner: '' }); + const result = await invocation.execute(new AbortController().signal); + expect(result.error).toBeUndefined(); + expect(result.llmContent).toContain('Owned'); + expect(result.llmContent).toContain('Unowned'); + }); + + it('treats a whitespace-only owner filter as no filter', async () => { + await createTask(TEAM, { + subject: 'Owned', + description: 'desc', + owner: 'alice', + }); + + const invocation = tool.build({ owner: ' ' }); + const result = await invocation.execute(new AbortController().signal); + expect(result.error).toBeUndefined(); + expect(result.llmContent).toContain('Owned'); + expect(invocation.getDescription()).toBe('List all tasks'); + }); + + it('treats an empty blockedBy filter as no filter', async () => { + await createTask(TEAM, { + subject: 'Task A', + description: 'desc', + }); + + const invocation = tool.build({ blockedBy: '' }); + const result = await invocation.execute(new AbortController().signal); + expect(result.error).toBeUndefined(); + expect(result.llmContent).toContain('Task A'); + }); + + it('treats a whitespace-only blockedBy filter as no filter', async () => { + await createTask(TEAM, { + subject: 'Task A', + description: 'desc', + }); + + const invocation = tool.build({ blockedBy: ' ' }); + const result = await invocation.execute(new AbortController().signal); + expect(result.error).toBeUndefined(); + expect(result.llmContent).toContain('Task A'); + expect(invocation.getDescription()).toBe('List all tasks'); + }); + + it('still filters precisely by a non-empty blockedBy', async () => { + const blocker = await createTask(TEAM, { + subject: 'Blocker', + description: 'desc', + }); + const blocked = await createTask(TEAM, { + subject: 'Blocked', + description: 'desc', + }); + await createTask(TEAM, { + subject: 'Free', + description: 'desc', + }); + await updateTask(TEAM, blocked.id, { addBlockedBy: [blocker.id] }); + + const invocation = tool.build({ blockedBy: blocker.id }); + const result = await invocation.execute(new AbortController().signal); + expect(result.error).toBeUndefined(); + expect(result.llmContent).toContain('Blocked'); + expect(result.llmContent).not.toContain('Blocker'); + expect(result.llmContent).not.toContain('Free'); + + const formatted = tool.build({ blockedBy: ` #${blocker.id} ` }); + const formattedResult = await formatted.execute( + new AbortController().signal, + ); + expect(formattedResult.error).toBeUndefined(); + expect(formattedResult.llmContent).toContain('Blocked'); + + const invalid = await tool + .build({ blockedBy: 'task-1' }) + .execute(new AbortController().signal); + expect(invalid.error).toBeDefined(); + }); + + it('rejects a non-blank blockedBy that normalizes to nothing', async () => { + await createTask(TEAM, { + subject: 'Task A', + description: 'desc', + }); + + // A bare '#' must fail closed like the owner path, not activate + // a never-matching '' filter (silent empty board) nor behave as + // absent while getDescription() still advertises the filter. + for (const blockedBy of ['#', ' #']) { + const result = await tool + .build({ blockedBy }) + .execute(new AbortController().signal); + expect(result.error).toBeDefined(); + expect(result.llmContent).toContain('blockedBy'); + } + }); + }); + it('returns TaskListResultDisplay', async () => { await createTask(TEAM, { subject: 'Task X', diff --git a/packages/core/src/tools/task-list.ts b/packages/core/src/tools/task-list.ts index 2d339772cc0..1c38da4ebf9 100644 --- a/packages/core/src/tools/task-list.ts +++ b/packages/core/src/tools/task-list.ts @@ -18,7 +18,11 @@ import { ToolNames, ToolDisplayNames } from './tool-names.js'; import type { Config } from '../config/config.js'; import { getTeamName, resolveActiveTeamName } from '../agents/team/identity.js'; import { sanitizeName } from '../agents/team/teamHelpers.js'; -import { listTasks } from '../agents/team/tasks.js'; +import { + assertValidTaskId, + listTasks, + normalizeTaskId, +} from '../agents/team/tasks.js'; export interface TaskListParams { status?: 'pending' | 'in_progress' | 'completed'; @@ -42,10 +46,10 @@ class TaskListInvocation extends BaseToolInvocation< if (this.params.status) { filters.push(`status=${this.params.status}`); } - if (this.params.owner) { + if (this.params.owner?.trim()) { filters.push(`owner=${this.params.owner}`); } - if (this.params.blockedBy) { + if (this.params.blockedBy?.trim()) { filters.push(`blockedBy=${this.params.blockedBy}`); } return filters.length > 0 @@ -66,8 +70,12 @@ class TaskListInvocation extends BaseToolInvocation< }; } + // Blank (empty/whitespace-only) optional filters behave as absent: + // the schema marks them optional and `getDescription()` only shows + // truthy filter values, so a blank value must not activate a + // filter (#9281). let ownerFilter: string | undefined; - if (this.params.owner !== undefined) { + if (this.params.owner !== undefined && this.params.owner.trim() !== '') { ownerFilter = sanitizeName(this.params.owner); if (!ownerFilter) { const msg = @@ -81,10 +89,38 @@ class TaskListInvocation extends BaseToolInvocation< } } + let blockedByFilter: string | undefined; + if (this.params.blockedBy?.trim()) { + blockedByFilter = normalizeTaskId(this.params.blockedBy); + if (blockedByFilter === undefined) { + // Non-blank input that normalizes to nothing (e.g. a bare + // '#'): fail closed like the owner path instead of + // activating a filter that can never match. + const msg = + 'Cannot filter by blockedBy: blockedBy must be a task ID, ' + + 'optionally prefixed with #.'; + return { + llmContent: msg, + returnDisplay: msg, + error: { message: msg }, + }; + } + try { + assertValidTaskId(blockedByFilter); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + return { + llmContent: msg, + returnDisplay: msg, + error: { message: msg }, + }; + } + } + const tasks = await listTasks(teamName, { status: this.params.status, owner: ownerFilter, - blockedBy: this.params.blockedBy, + blockedBy: blockedByFilter, }); if (tasks.length === 0) {