Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions packages/core/src/agents/team/tasks.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
notifyTasksUpdated,
TaskOwnershipError,
RECIPROCAL_CALLER,
normalizeTaskId,
} from './tasks.js';
import { mockCompromisedLock } from '../../test-utils/mock-compromised-lock.js';

Expand Down Expand Up @@ -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;

Expand Down
13 changes: 13 additions & 0 deletions packages/core/src/agents/team/tasks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
114 changes: 114 additions & 0 deletions packages/core/src/tools/task-list.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
46 changes: 41 additions & 5 deletions packages/core/src/tools/task-list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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
Expand All @@ -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() !== '') {
Comment thread
yiliang114 marked this conversation as resolved.
ownerFilter = sanitizeName(this.params.owner);
if (!ownerFilter) {
const msg =
Expand All @@ -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) {
Expand Down