From 1f4d2dd004faf714c74fa107716ca4b5ed67b15d Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Tue, 18 Aug 2026 11:33:18 +0100 Subject: [PATCH 1/3] feat(comments): notify collaborators when adding a comment Comments posted through this server notified nobody. add-comments never sent uidsToNotify, so a teammate named in an agent's comment found out only if they happened to open the task. That also broke the next comment. Todoist's clients pick the recipients themselves and send them with each comment; the API notifies exactly who it is handed and derives nobody on its own. A reply takes its recipients from the comment before it, so a comment posted with an empty list silences the following comment too -- including one a human writes in the app. add-comments now takes notifyUsers, accepting user IDs, emails, names or "me" for each person, resolved through the existing user resolver. Omitting it mirrors the clients: the assignee, assigner and creator on a task's first comment, or the previous comment's participants on a reply. Passing ["none"] stays silent. Recipients are worked out once per distinct target, so several comments on one task in a single batch read the thread once and notify the same people. Comments now also report notifiedUserIds, which lets a caller see a thread's participants before replying. Refs #509 Co-Authored-By: Claude Opus 5 --- src/mcp-server.ts | 5 + src/tool-helpers.ts | 1 + src/tools/add-comments.test.ts | 256 ++++++++++++++++++++++++++- src/tools/add-comments.ts | 120 +++++++++++-- src/utils/comment-recipients.test.ts | 185 +++++++++++++++++++ src/utils/comment-recipients.ts | 87 +++++++++ src/utils/output-schemas.ts | 4 + src/utils/user-resolver.test.ts | 60 ++++++- src/utils/user-resolver.ts | 36 ++++ 9 files changed, 731 insertions(+), 23 deletions(-) create mode 100644 src/utils/comment-recipients.test.ts create mode 100644 src/utils/comment-recipients.ts diff --git a/src/mcp-server.ts b/src/mcp-server.ts index ab8b0235..5ccfb14f 100644 --- a/src/mcp-server.ts +++ b/src/mcp-server.ts @@ -42,6 +42,11 @@ What each tool does and how to fill its parameters is in the tool's own descript - To find out whether a task hides subtasks, use **fetch-object** with includeChildren rather than a speculative **find-tasks** call. - Filter tasks by label **name**. Label IDs are only for **delete-object** and **update-labels**. Shared labels can be renamed but not recoloured, reordered or favourited. +**Comments** + +- Comments notify only the people **add-comments** is handed. When a comment mentions someone, name them in \`notifyUsers\` — writing "@Ana" in the text notifies nobody. Omit \`notifyUsers\` to notify whoever the Todoist apps would, or pass \`["none"]\` to stay silent. +- Notification cannot be sent when editing a comment, only when adding one. + **Deleting and archiving** - **delete-object** removes every object type; there is no per-type delete tool. Reminders use type "reminder", location reminders "location_reminder". diff --git a/src/tool-helpers.ts b/src/tool-helpers.ts index 1585066e..3563c97f 100644 --- a/src/tool-helpers.ts +++ b/src/tool-helpers.ts @@ -377,6 +377,7 @@ function mapComment(comment: Comment) { content: comment.content, postedAt: comment.postedAt.toISOString(), postedUid: comment.postedUid ?? undefined, + notifiedUserIds: comment.uidsToNotify?.length ? comment.uidsToNotify : undefined, fileAttachment: comment.fileAttachment ? { resourceType: comment.fileAttachment.resourceType, diff --git a/src/tools/add-comments.test.ts b/src/tools/add-comments.test.ts index c20ebeed..4730a2d2 100644 --- a/src/tools/add-comments.test.ts +++ b/src/tools/add-comments.test.ts @@ -1,13 +1,21 @@ -import type { Comment, TodoistApi } from '@doist/todoist-sdk' +import type { Comment, Task, TodoistApi } from '@doist/todoist-sdk' import { type Mocked, vi } from 'vitest' -import { createMockUser } from '../utils/test-helpers.js' +import { createMockTask, createMockUser } from '../utils/test-helpers.js' import { ToolNames } from '../utils/tool-names.js' +import { resolveUserRefs } from '../utils/user-resolver.js' import { addComments } from './add-comments.js' +vi.mock('../utils/user-resolver.js', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, resolveUserRefs: vi.fn() } +}) + // Mock the Todoist API const mockTodoistApi = { addComment: vi.fn(), getUser: vi.fn(), + getComments: vi.fn(), + getTask: vi.fn(), } as unknown as Mocked const { ADD_COMMENTS } = ToolNames @@ -28,10 +36,26 @@ function createMockComment(overrides: Partial = {}): Comment { } } +const CURRENT_USER_ID = 'current-user' + +function createMockTaskWithUids(overrides: Partial = {}): Task { + return { + ...createMockTask(), + addedByUid: CURRENT_USER_ID, + assignedByUid: null, + responsibleUid: null, + ...overrides, + } +} + describe(`${ADD_COMMENTS} tool`, () => { beforeEach(() => { vi.clearAllMocks() - mockTodoistApi.getUser.mockResolvedValue(createMockUser()) + mockTodoistApi.getUser.mockResolvedValue(createMockUser({ id: CURRENT_USER_ID })) + // Default: an empty thread on an unassigned, self-created task, so + // nobody is notified unless a test says otherwise. + mockTodoistApi.getComments.mockResolvedValue({ results: [], nextCursor: null }) + mockTodoistApi.getTask.mockResolvedValue(createMockTaskWithUids()) }) describe('adding comments to tasks', () => { @@ -295,6 +319,232 @@ describe(`${ADD_COMMENTS} tool`, () => { }) }) + describe('notifying users', () => { + const mockResolveUserRefs = vi.mocked(resolveUserRefs) + + function mockAddedComment(uidsToNotify: string[] | null = null) { + mockTodoistApi.addComment.mockResolvedValue( + createMockComment({ taskId: 'task456', uidsToNotify }), + ) + } + + it('should notify explicitly named users, resolving IDs, emails and names alike', async () => { + mockResolveUserRefs.mockResolvedValue([ + { userId: '111', displayName: 'Ana', email: 'ana@example.com' }, + { userId: '222', displayName: 'Bo', email: 'bo@example.com' }, + { userId: '333', displayName: 'Cleo', email: 'cleo@example.com' }, + ]) + mockAddedComment(['111', '222', '333']) + + const result = await addComments.execute( + { + comments: [ + { + taskId: 'task456', + content: 'Please review', + notifyUsers: ['111', 'bo@example.com', 'Cleo'], + }, + ], + }, + mockTodoistApi, + ) + + expect(mockResolveUserRefs).toHaveBeenCalledWith(mockTodoistApi, [ + '111', + 'bo@example.com', + 'Cleo', + ]) + expect(mockTodoistApi.addComment).toHaveBeenCalledWith({ + content: 'Please review', + taskId: 'task456', + uidsToNotify: ['111', '222', '333'], + }) + // Explicit recipients mean the thread never needs reading. + expect(mockTodoistApi.getComments).not.toHaveBeenCalled() + expect(result.textContent).toContain('Notified 3 people') + }) + + it('should surface an unresolvable user as an error', async () => { + mockResolveUserRefs.mockRejectedValue( + new Error('Could not find user(s): "Nobody", "Nobody Else".'), + ) + + await expect( + addComments.execute( + { + comments: [ + { + taskId: 'task456', + content: 'Please review', + notifyUsers: ['Nobody', 'Nobody Else'], + }, + ], + }, + mockTodoistApi, + ), + ).rejects.toThrow('Could not find user(s): "Nobody", "Nobody Else".') + }) + + it('should omit uidsToNotify entirely when told to notify nobody', async () => { + mockAddedComment() + + await addComments.execute( + { + comments: [{ taskId: 'task456', content: 'Quiet note', notifyUsers: ['none'] }], + }, + mockTodoistApi, + ) + + // Nobody to notify means no recipient field at all, rather than + // an empty one. + expect(mockTodoistApi.addComment).toHaveBeenCalledWith({ + content: 'Quiet note', + taskId: 'task456', + }) + expect(mockResolveUserRefs).not.toHaveBeenCalled() + expect(mockTodoistApi.getComments).not.toHaveBeenCalled() + }) + + it('should default a first task comment to the assignee, assigner and creator', async () => { + mockTodoistApi.getTask.mockResolvedValue( + createMockTaskWithUids({ + responsibleUid: 'assignee', + assignedByUid: 'assigner', + addedByUid: 'creator', + }), + ) + mockAddedComment(['assignee', 'assigner', 'creator']) + + await addComments.execute( + { comments: [{ taskId: 'task456', content: 'First comment' }] }, + mockTodoistApi, + ) + + expect(mockTodoistApi.addComment).toHaveBeenCalledWith({ + content: 'First comment', + taskId: 'task456', + uidsToNotify: ['assignee', 'assigner', 'creator'], + }) + }) + + it('should exclude the comment author and drop unset uids from the defaults', async () => { + mockTodoistApi.getTask.mockResolvedValue( + createMockTaskWithUids({ + responsibleUid: CURRENT_USER_ID, + assignedByUid: null, + addedByUid: 'creator', + }), + ) + mockAddedComment(['creator']) + + await addComments.execute( + { comments: [{ taskId: 'task456', content: 'First comment' }] }, + mockTodoistApi, + ) + + expect(mockTodoistApi.addComment).toHaveBeenCalledWith( + expect.objectContaining({ uidsToNotify: ['creator'] }), + ) + }) + + it("should default a reply to the previous comment's participants", async () => { + mockTodoistApi.getComments.mockResolvedValue({ + results: [ + createMockComment({ + id: 'older', + postedAt: new Date('2024-01-01T09:00:00Z'), + postedUid: 'stale-author', + uidsToNotify: ['stale-recipient'], + }), + createMockComment({ + id: 'newest', + postedAt: new Date('2024-01-01T15:00:00Z'), + postedUid: 'previous-author', + uidsToNotify: ['participant', CURRENT_USER_ID], + }), + ], + nextCursor: null, + }) + mockAddedComment(['participant', 'previous-author']) + + await addComments.execute( + { comments: [{ taskId: 'task456', content: 'Reply' }] }, + mockTodoistApi, + ) + + expect(mockTodoistApi.addComment).toHaveBeenCalledWith({ + content: 'Reply', + taskId: 'task456', + uidsToNotify: ['participant', 'previous-author'], + }) + // A thread that already has comments settles the recipients, so the + // task's own assignee/creator are never consulted. + expect(mockTodoistApi.getTask).not.toHaveBeenCalled() + }) + + it('should notify nobody on a first project comment', async () => { + mockTodoistApi.addComment.mockResolvedValue( + createMockComment({ taskId: undefined, projectId: 'project789' }), + ) + + await addComments.execute( + { comments: [{ projectId: 'project789', content: 'Project note' }] }, + mockTodoistApi, + ) + + expect(mockTodoistApi.addComment).toHaveBeenCalledWith({ + content: 'Project note', + projectId: 'project789', + }) + expect(mockTodoistApi.getTask).not.toHaveBeenCalled() + }) + + it('should read a shared thread once for several comments on the same task', async () => { + mockTodoistApi.getTask.mockResolvedValue( + createMockTaskWithUids({ responsibleUid: 'assignee' }), + ) + mockAddedComment(['assignee']) + + await addComments.execute( + { + comments: [ + { taskId: 'task456', content: 'First' }, + { taskId: 'task456', content: 'Second' }, + ], + }, + mockTodoistApi, + ) + + expect(mockTodoistApi.getComments).toHaveBeenCalledTimes(1) + expect(mockTodoistApi.addComment).toHaveBeenCalledTimes(2) + expect(mockTodoistApi.addComment).toHaveBeenCalledWith( + expect.objectContaining({ content: 'Second', uidsToNotify: ['assignee'] }), + ) + }) + + it('should surface who was notified, and omit the field when nobody was', async () => { + mockAddedComment(['111', '222']) + const notified = await addComments.execute( + { + comments: [{ taskId: 'task456', content: 'Heads up', notifyUsers: ['none'] }], + }, + mockTodoistApi, + ) + expect(notified.structuredContent?.comments[0]).toEqual( + expect.objectContaining({ notifiedUserIds: ['111', '222'] }), + ) + + mockAddedComment() + const silent = await addComments.execute( + { + comments: [{ taskId: 'task456', content: 'Quiet note', notifyUsers: ['none'] }], + }, + mockTodoistApi, + ) + expect(silent.structuredContent?.comments[0]?.notifiedUserIds).toBeUndefined() + }) + }) + describe('validation', () => { it('should throw error when neither taskId nor projectId provided', async () => { await expect( diff --git a/src/tools/add-comments.ts b/src/tools/add-comments.ts index abd6d0f1..b632e21c 100644 --- a/src/tools/add-comments.ts +++ b/src/tools/add-comments.ts @@ -1,9 +1,16 @@ -import type { AddCommentArgs } from '@doist/todoist-sdk' +import type { AddCommentArgs, TodoistApi } from '@doist/todoist-sdk' import { z } from 'zod' import type { TodoistTool } from '../todoist-tool.js' import { isInboxProjectId, mapComment, resolveInboxProjectId } from '../tool-helpers.js' +import { + type CommentTarget, + NO_NOTIFY_KEYWORD, + getDefaultCommentRecipients, + isNoNotifyList, +} from '../utils/comment-recipients.js' import { CommentSchema as CommentOutputSchema } from '../utils/output-schemas.js' import { ToolNames } from '../utils/tool-names.js' +import { resolveUserRefs } from '../utils/user-resolver.js' const CommentSchema = z.object({ taskId: z.string().optional().describe('The ID of the task to comment on.'), @@ -14,6 +21,12 @@ const CommentSchema = z.object({ 'The ID of the project to comment on. Project ID should be an ID string, or the text "inbox", for inbox tasks.', ), content: z.string().min(1).describe('The content of the comment.'), + notifyUsers: z + .array(z.string().min(1)) + .optional() + .describe( + `Who to notify about this comment — a user ID, email, full name, or "me" for each person. Set this whenever the comment mentions someone; the text of an @mention notifies nobody on its own. Omit to notify whoever the Todoist apps would (the task's assignee, assigner and creator on a first comment, or the previous comment's participants on a reply). Pass ["${NO_NOTIFY_KEYWORD}"] to notify nobody.`, + ), }) const ArgsSchema = { @@ -26,10 +39,13 @@ const OutputSchema = { addedCommentIds: z.array(z.string()).describe('The IDs of the added comments.'), } +type CommentInput = z.infer +type TodoistUser = Awaited> + const addComments = { name: ToolNames.ADD_COMMENTS, description: - 'Add multiple comments to tasks or projects. Each comment must specify either taskId or projectId.', + 'Add multiple comments to tasks or projects, optionally notifying collaborators. Each comment must specify either taskId or projectId.', parameters: ArgsSchema, outputSchema: OutputSchema, annotations: { readOnlyHint: false, destructiveHint: false, idempotentHint: false }, @@ -50,25 +66,48 @@ const addComments = { } } - // Check if any comment needs inbox resolution + // Every comment needs the current user, either to resolve "inbox" or to + // keep the author out of their own comment's recipients. const needsInboxResolution = comments.some((comment) => isInboxProjectId(comment.projectId)) - const todoistUser = needsInboxResolution ? await client.getUser() : undefined - - const addCommentPromises = comments.map(async ({ content, taskId, projectId }) => { - // Resolve "inbox" to actual inbox project ID if needed - const resolvedProjectId = await resolveInboxProjectId({ - projectId, - user: todoistUser, - client: todoistUser ? undefined : client, - }) - - return await client.addComment({ - content, - ...(taskId ? { taskId } : { projectId: resolvedProjectId }), - } as AddCommentArgs) + const needsDefaultRecipients = comments.some((comment) => !comment.notifyUsers) + const todoistUser = + needsInboxResolution || needsDefaultRecipients ? await client.getUser() : undefined + + const targets = await Promise.all( + comments.map(async (comment) => { + // Resolve "inbox" to actual inbox project ID if needed + const resolvedProjectId = await resolveInboxProjectId({ + projectId: comment.projectId, + user: todoistUser, + client: todoistUser ? undefined : client, + }) + + return ( + comment.taskId ? { taskId: comment.taskId } : { projectId: resolvedProjectId } + ) as CommentTarget + }), + ) + + const recipients = await resolveRecipientsPerComment({ + comments, + targets, + client, + currentUser: todoistUser, }) - const newComments = await Promise.all(addCommentPromises) + const newComments = await Promise.all( + comments.map(async ({ content }, index) => { + const uidsToNotify = recipients[index] ?? [] + return await client.addComment({ + content, + ...targets[index], + // Nobody to notify means no recipient field at all, + // rather than an empty one. + ...(uidsToNotify.length > 0 && { uidsToNotify }), + } as AddCommentArgs) + }), + ) + const mappedComments = newComments.map(mapComment) const textContent = generateTextContent({ comments: mappedComments }) @@ -83,6 +122,44 @@ const addComments = { }, } satisfies TodoistTool +/** + * Work out the recipients for every comment in the batch, reading each distinct + * target's thread only once. Two comments on the same task within one call are + * a single conversation, so they notify the same people. + */ +async function resolveRecipientsPerComment({ + comments, + targets, + client, + currentUser, +}: { + comments: CommentInput[] + targets: CommentTarget[] + client: TodoistApi + currentUser: TodoistUser | undefined +}): Promise { + const byTarget = new Map>() + + return await Promise.all( + comments.map(async ({ notifyUsers }, index) => { + if (notifyUsers) { + if (isNoNotifyList(notifyUsers)) return [] + const resolved = await resolveUserRefs(client, notifyUsers) + return resolved.map((user) => user.userId) + } + + const target = targets[index] + if (!target || !currentUser) return [] + + const key = target.taskId ? `task:${target.taskId}` : `project:${target.projectId}` + const pending = + byTarget.get(key) ?? getDefaultCommentRecipients(client, target, currentUser.id) + byTarget.set(key, pending) + return await pending + }), + ) +} + function generateTextContent({ comments }: { comments: ReturnType[] }): string { // Group comments by entity type and count const taskComments = comments.filter((c) => c.taskId).length @@ -100,7 +177,12 @@ function generateTextContent({ comments }: { comments: ReturnType 0 ? `Added ${parts.join(' and ')}` : 'No comments added' - return summary + const notified = new Set(comments.flatMap((c) => c.notifiedUserIds ?? [])) + if (notified.size === 0) { + return summary + } + const peopleLabel = notified.size > 1 ? 'people' : 'person' + return `${summary}. Notified ${notified.size} ${peopleLabel}` } export { addComments } diff --git a/src/utils/comment-recipients.test.ts b/src/utils/comment-recipients.test.ts new file mode 100644 index 00000000..52c16d7d --- /dev/null +++ b/src/utils/comment-recipients.test.ts @@ -0,0 +1,185 @@ +import type { Comment, TodoistApi } from '@doist/todoist-sdk' +import { type Mocked, vi } from 'vitest' +import { + NO_NOTIFY_KEYWORD, + getDefaultCommentRecipients, + isNoNotifyList, +} from './comment-recipients.js' +import { createMockTask } from './test-helpers.js' + +const CURRENT_USER_ID = 'current-user' + +function createComment(overrides: Partial = {}): Comment { + return { + id: 'comment-1', + content: 'Existing comment', + postedAt: new Date('2024-01-01T12:00:00Z'), + postedUid: 'previous-author', + taskId: 'task-1', + projectId: undefined, + fileAttachment: null, + uidsToNotify: null, + reactions: null, + isDeleted: false, + ...overrides, + } +} + +describe('isNoNotifyList', () => { + it.each([[NO_NOTIFY_KEYWORD], ['None'], [' NONE ']])('treats %j as the opt-out', (value) => { + expect(isNoNotifyList([value])).toBe(true) + }) + + it.each([[[]], [['none', 'Ana']], [['nobody']]])('treats %j as a real list', (value) => { + expect(isNoNotifyList(value)).toBe(false) + }) +}) + +describe('getDefaultCommentRecipients', () => { + let mockClient: Mocked + + beforeEach(() => { + mockClient = { + getComments: vi.fn().mockResolvedValue({ results: [], nextCursor: null }), + getTask: vi.fn().mockResolvedValue(createMockTask()), + } as unknown as Mocked + }) + + describe('a thread that already has comments', () => { + it("takes the newest comment's participants, not the first page's", async () => { + mockClient.getComments.mockResolvedValue({ + results: [ + createComment({ + id: 'newest', + postedAt: new Date('2024-03-01T09:00:00Z'), + postedUid: 'recent-author', + uidsToNotify: ['recent-participant'], + }), + createComment({ + id: 'oldest', + postedAt: new Date('2024-01-01T09:00:00Z'), + postedUid: 'stale-author', + uidsToNotify: ['stale-participant'], + }), + ], + nextCursor: null, + }) + + const recipients = await getDefaultCommentRecipients( + mockClient, + { taskId: 'task-1' }, + CURRENT_USER_ID, + ) + + expect(recipients).toEqual(['recent-participant', 'recent-author']) + expect(mockClient.getTask).not.toHaveBeenCalled() + }) + + it('keeps the chain alive from a comment that notified nobody', async () => { + mockClient.getComments.mockResolvedValue({ + results: [createComment({ postedUid: 'agent', uidsToNotify: null })], + nextCursor: null, + }) + + const recipients = await getDefaultCommentRecipients( + mockClient, + { taskId: 'task-1' }, + CURRENT_USER_ID, + ) + + expect(recipients).toEqual(['agent']) + }) + + it('excludes the author of the comment being posted', async () => { + mockClient.getComments.mockResolvedValue({ + results: [ + createComment({ + postedUid: CURRENT_USER_ID, + uidsToNotify: [CURRENT_USER_ID, 'other'], + }), + ], + nextCursor: null, + }) + + const recipients = await getDefaultCommentRecipients( + mockClient, + { taskId: 'task-1' }, + CURRENT_USER_ID, + ) + + expect(recipients).toEqual(['other']) + }) + }) + + describe('a task with no comments yet', () => { + it('takes the assignee, assigner and creator', async () => { + mockClient.getTask.mockResolvedValue( + createMockTask({ + responsibleUid: 'assignee', + assignedByUid: 'assigner', + addedByUid: 'creator', + }), + ) + + const recipients = await getDefaultCommentRecipients( + mockClient, + { taskId: 'task-1' }, + CURRENT_USER_ID, + ) + + expect(recipients).toEqual(['assignee', 'assigner', 'creator']) + }) + + it('drops unset uids and collapses a person filling two roles', async () => { + mockClient.getTask.mockResolvedValue( + createMockTask({ + responsibleUid: 'ana', + assignedByUid: null, + addedByUid: 'ana', + }), + ) + + const recipients = await getDefaultCommentRecipients( + mockClient, + { taskId: 'task-1' }, + CURRENT_USER_ID, + ) + + expect(recipients).toEqual(['ana']) + }) + + it('notifies nobody when the author is the only party', async () => { + mockClient.getTask.mockResolvedValue( + createMockTask({ + responsibleUid: CURRENT_USER_ID, + assignedByUid: null, + addedByUid: CURRENT_USER_ID, + }), + ) + + const recipients = await getDefaultCommentRecipients( + mockClient, + { taskId: 'task-1' }, + CURRENT_USER_ID, + ) + + expect(recipients).toEqual([]) + }) + }) + + describe('a project with no comments yet', () => { + it('notifies nobody, as a project has no assignee to fall back on', async () => { + const recipients = await getDefaultCommentRecipients( + mockClient, + { projectId: 'project-1' }, + CURRENT_USER_ID, + ) + + expect(mockClient.getComments).toHaveBeenCalledWith( + expect.objectContaining({ projectId: 'project-1' }), + ) + expect(recipients).toEqual([]) + expect(mockClient.getTask).not.toHaveBeenCalled() + }) + }) +}) diff --git a/src/utils/comment-recipients.ts b/src/utils/comment-recipients.ts new file mode 100644 index 00000000..bb137029 --- /dev/null +++ b/src/utils/comment-recipients.ts @@ -0,0 +1,87 @@ +import type { + Comment, + GetCommentsResponse, + GetTaskCommentsArgs, + TodoistApi, +} from '@doist/todoist-sdk' +import { fetchAllPages } from '../tool-helpers.js' + +/** Value that tells `add-comments` to post without notifying anyone. */ +export const NO_NOTIFY_KEYWORD = 'none' + +export type CommentTarget = + | { taskId: string; projectId?: never } + | { projectId: string; taskId?: never } + +/** Whether a `notifyUsers` list is the explicit "notify nobody" opt-out. */ +export function isNoNotifyList(notifyUsers: string[]): boolean { + return notifyUsers.length === 1 && notifyUsers[0]?.trim().toLowerCase() === NO_NOTIFY_KEYWORD +} + +/** + * Work out who Todoist's own clients would notify about a new comment. + * + * The API notifies exactly the people it is handed and derives nobody itself, + * so a comment posted without recipients notifies no one — and, because a reply + * takes its recipients from the comment before it, silences the next comment in + * the thread too. Mirroring the clients here keeps that chain intact: + * + * - replying to an existing thread notifies the previous comment's participants + * - the first comment on a task notifies its assignee, assigner and creator + * - the first comment on a project notifies nobody, as a project has no assignee + * + * The comment's own author is never a recipient. + * + * @param client - Todoist API client + * @param target - The task or project being commented on + * @param currentUserId - The authenticated user, excluded from the result + * @returns User IDs to notify, deduplicated + */ +export async function getDefaultCommentRecipients( + client: TodoistApi, + target: CommentTarget, + currentUserId: string, +): Promise { + const previousComment = await getLatestComment(client, target) + + if (previousComment) { + return dedupe( + [...(previousComment.uidsToNotify ?? []), previousComment.postedUid], + currentUserId, + ) + } + + if (!target.taskId) { + return [] + } + + const task = await client.getTask(target.taskId) + return dedupe([task.responsibleUid, task.assignedByUid, task.addedByUid], currentUserId) +} + +async function getLatestComment( + client: TodoistApi, + target: CommentTarget, +): Promise { + // The SDK brands the unused key as `never` on each half of + // `GetTaskCommentsArgs | GetProjectCommentsArgs`, which a generic cannot + // infer across, so pin one half and let the target supply either key. + const comments = await fetchAllPages({ + apiMethod: (args) => client.getComments(args), + args: target as GetTaskCommentsArgs, + }) + + // The API does not guarantee an order, so pick the newest explicitly. + return comments.reduce( + (latest, comment) => (!latest || comment.postedAt > latest.postedAt ? comment : latest), + undefined, + ) +} + +function dedupe(userIds: (string | null | undefined)[], currentUserId: string): string[] { + const seen = new Set() + for (const userId of userIds) { + if (userId && userId !== currentUserId) seen.add(userId) + } + return [...seen] +} diff --git a/src/utils/output-schemas.ts b/src/utils/output-schemas.ts index 5088b168..1119b2bb 100644 --- a/src/utils/output-schemas.ts +++ b/src/utils/output-schemas.ts @@ -98,6 +98,10 @@ const CommentSchema = z.object({ content: z.string(), postedAt: z.string().describe('ISO 8601.'), postedUid: z.string().optional(), + notifiedUserIds: z + .array(z.string()) + .optional() + .describe('Users notified about this comment. Absent when nobody was notified.'), fileAttachment: AttachmentSchema.optional(), }) diff --git a/src/utils/user-resolver.test.ts b/src/utils/user-resolver.test.ts index f7d6d124..4a2edc5f 100644 --- a/src/utils/user-resolver.test.ts +++ b/src/utils/user-resolver.test.ts @@ -1,6 +1,12 @@ import type { TodoistApi } from '@doist/todoist-sdk' import { type Mocked, vi } from 'vitest' -import { BoundedTtlCache, SELF_USER_KEYWORD, UserResolver } from './user-resolver.js' +import { + BoundedTtlCache, + SELF_USER_KEYWORD, + UserResolver, + resolveUserRefs, + userResolver, +} from './user-resolver.js' describe('BoundedTtlCache', () => { it('evicts the least recently used entry at capacity', () => { @@ -252,3 +258,55 @@ describe('UserResolver', () => { }) }) }) + +describe('resolveUserRefs', () => { + const collaborators = [ + { id: '111', name: 'Ana Lovelace', email: 'ana@example.com' }, + { id: '222', name: 'Bo Turing', email: 'bo@example.com' }, + ] + + let mockClient: Mocked + + beforeEach(() => { + userResolver.clearCache() + mockClient = { + getUser: vi + .fn() + .mockResolvedValue({ id: '999', fullName: 'Me', email: 'me@example.com' }), + getProjects: vi + .fn() + .mockResolvedValue({ results: [{ id: 'p1', isShared: true }], nextCursor: null }), + getProjectCollaborators: vi + .fn() + .mockResolvedValue({ results: collaborators, nextCursor: null }), + } as unknown as Mocked + }) + + it('resolves IDs, emails, names and "me" in one pass', async () => { + const resolved = await resolveUserRefs(mockClient, ['111', 'bo@example.com', 'me']) + + expect(resolved.map((user) => user.userId)).toEqual(['111', '222', '999']) + }) + + it('preserves input order and collapses duplicates', async () => { + const resolved = await resolveUserRefs(mockClient, [ + 'Bo Turing', + 'ana@example.com', + 'bo@example.com', + ]) + + expect(resolved.map((user) => user.userId)).toEqual(['222', '111']) + }) + + it('names every unresolvable reference in a single error', async () => { + await expect( + resolveUserRefs(mockClient, ['Ana Lovelace', 'Ghost', 'Phantom']), + ).rejects.toThrow( + 'Could not find user(s): "Ghost", "Phantom". Make sure they are collaborators on a shared project.', + ) + }) + + it('resolves nothing for an empty list', async () => { + await expect(resolveUserRefs(mockClient, [])).resolves.toEqual([]) + }) +}) diff --git a/src/utils/user-resolver.ts b/src/utils/user-resolver.ts index 3ea8d87a..3419d04d 100644 --- a/src/utils/user-resolver.ts +++ b/src/utils/user-resolver.ts @@ -381,3 +381,39 @@ export async function resolveUserNameToId( ): Promise { return userResolver.resolveUser(client, nameOrId) } + +/** + * Resolve a list of user references — IDs, emails, full names, or the "me" + * keyword — to users, in one pass. + * + * Order of the input is preserved and duplicates are collapsed, so callers can + * hand the result straight to an API that expects a recipient list. Every + * reference that cannot be resolved is reported in a single error rather than + * failing on the first one, so the caller learns about all the bad references + * at once. + * + * @param client - Todoist API client + * @param refs - User references to resolve + * @returns The resolved users, deduplicated by user ID + * @throws Error naming every reference that could not be resolved + */ +export async function resolveUserRefs(client: TodoistApi, refs: string[]): Promise { + const resolutions = await Promise.all(refs.map((ref) => userResolver.resolveUser(client, ref))) + + const unresolved = refs.filter((_ref, index) => !resolutions[index]) + if (unresolved.length > 0) { + const names = unresolved.map((ref) => `"${ref}"`).join(', ') + throw new Error( + `Could not find user(s): ${names}. Make sure they are collaborators on a shared project.`, + ) + } + + const seen = new Set() + const resolved: ResolvedUser[] = [] + for (const user of resolutions) { + if (!user || seen.has(user.userId)) continue + seen.add(user.userId) + resolved.push(user) + } + return resolved +} From 452c3e579a34f5b65c2ac622672a574b3cc7b9b1 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Tue, 18 Aug 2026 11:38:37 +0100 Subject: [PATCH 2/3] chore(deps): bump @doist/todoist-sdk to 14.0.1 Carries the fix for addComment sending uidsToNotify as a comma-joined string, which the API rejected outright. Comment notifications could not work without it. Co-Authored-By: Claude Opus 5 --- package-lock.json | 8 ++++---- package.json | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package-lock.json b/package-lock.json index 8685a474..23556f0f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,7 +9,7 @@ "version": "12.5.7", "license": "MIT", "dependencies": { - "@doist/todoist-sdk": "14.0.0", + "@doist/todoist-sdk": "14.0.1", "@modelcontextprotocol/ext-apps": "1.2.2", "date-fns": "4.1.0", "dompurify": "3.3.3", @@ -499,9 +499,9 @@ } }, "node_modules/@doist/todoist-sdk": { - "version": "14.0.0", - "resolved": "https://registry.npmjs.org/@doist/todoist-sdk/-/todoist-sdk-14.0.0.tgz", - "integrity": "sha512-dapFZq8Kf+1X+6rWucy22qvGIPWR+yQZzjXMXg6RPq9LXrI94hdtUbLNzEf1KMFJO2+l2BkvLygZWAP4JZANyg==", + "version": "14.0.1", + "resolved": "https://registry.npmjs.org/@doist/todoist-sdk/-/todoist-sdk-14.0.1.tgz", + "integrity": "sha512-l2mXCZHFkKWHyVeqGj9dZONAyOLCP5Qy89msUH4/45SDfubpFN+lx6D5Q52w2Qkc8sy6nV8rnTR7E6U7jS9myg==", "license": "MIT", "dependencies": { "camelcase": "6.3.0", diff --git a/package.json b/package.json index 8402a8c9..8b0f82b4 100644 --- a/package.json +++ b/package.json @@ -65,7 +65,7 @@ "prepare": "husky" }, "dependencies": { - "@doist/todoist-sdk": "14.0.0", + "@doist/todoist-sdk": "14.0.1", "@modelcontextprotocol/ext-apps": "1.2.2", "date-fns": "4.1.0", "dompurify": "3.3.3", From fa0d4a76bd3faf55bdb4e62b334f36a1b90058cd Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Tue, 18 Aug 2026 11:55:26 +0100 Subject: [PATCH 3/3] fix(comments): bound and dedupe comment notification lookups Four issues from review, all on the cost of resolving recipients: resolveUserRefs fanned every reference out with Promise.all, so each non-ID name missed the collaborator cache before any call had populated it and ran its own full project-and-collaborator fetch. References are now deduplicated and resolved one at a time, letting the first lookup warm the cache the rest read. notifyUsers had no upper bound, so one argument could multiply into unbounded parallel requests against the operator's token. Capped at ApiLimits.NOTIFY_USERS_MAX, matching the other array inputs. An empty notifyUsers was truthy, so it skipped the default recipients and posted silently -- an undocumented second opt-out that cut off the notification chain. Rejected by the schema, leaving ["none"] as the only way to stay silent. Reading a thread materialised its entire comment history to pick one item from it. Pages are now walked keeping only the newest comment seen. Comments come back oldest-first, so reaching the last page is unavoidable, but holding the whole history is not. Co-Authored-By: Claude Opus 5 --- src/tools/add-comments.test.ts | 23 ++++++++++++++++ src/tools/add-comments.ts | 3 +++ src/utils/comment-recipients.test.ts | 34 ++++++++++++++++++++++++ src/utils/comment-recipients.ts | 39 ++++++++++++++-------------- src/utils/constants.ts | 2 ++ src/utils/user-resolver.test.ts | 15 +++++++++++ src/utils/user-resolver.ts | 31 +++++++++++++++------- 7 files changed, 119 insertions(+), 28 deletions(-) diff --git a/src/tools/add-comments.test.ts b/src/tools/add-comments.test.ts index 4730a2d2..35262a0f 100644 --- a/src/tools/add-comments.test.ts +++ b/src/tools/add-comments.test.ts @@ -1,5 +1,7 @@ import type { Comment, Task, TodoistApi } from '@doist/todoist-sdk' import { type Mocked, vi } from 'vitest' +import { z } from 'zod' +import { ApiLimits } from '../utils/constants.js' import { createMockTask, createMockUser } from '../utils/test-helpers.js' import { ToolNames } from '../utils/tool-names.js' import { resolveUserRefs } from '../utils/user-resolver.js' @@ -545,6 +547,27 @@ describe(`${ADD_COMMENTS} tool`, () => { }) }) + describe('notifyUsers schema bounds', () => { + const parse = (notifyUsers: unknown) => + z.object(addComments.parameters).safeParse({ + comments: [{ taskId: 'task456', content: 'x', notifyUsers }], + }) + + it('rejects an empty list, so ["none"] stays the only way to stay silent', () => { + expect(parse([]).success).toBe(false) + }) + + it('rejects more recipients than a single comment may notify', () => { + const tooMany = Array.from( + { length: ApiLimits.NOTIFY_USERS_MAX + 1 }, + (_, i) => `u${i}`, + ) + + expect(parse(tooMany).success).toBe(false) + expect(parse(tooMany.slice(0, ApiLimits.NOTIFY_USERS_MAX)).success).toBe(true) + }) + }) + describe('validation', () => { it('should throw error when neither taskId nor projectId provided', async () => { await expect( diff --git a/src/tools/add-comments.ts b/src/tools/add-comments.ts index b632e21c..efcec6d1 100644 --- a/src/tools/add-comments.ts +++ b/src/tools/add-comments.ts @@ -8,6 +8,7 @@ import { getDefaultCommentRecipients, isNoNotifyList, } from '../utils/comment-recipients.js' +import { ApiLimits } from '../utils/constants.js' import { CommentSchema as CommentOutputSchema } from '../utils/output-schemas.js' import { ToolNames } from '../utils/tool-names.js' import { resolveUserRefs } from '../utils/user-resolver.js' @@ -23,6 +24,8 @@ const CommentSchema = z.object({ content: z.string().min(1).describe('The content of the comment.'), notifyUsers: z .array(z.string().min(1)) + .min(1) + .max(ApiLimits.NOTIFY_USERS_MAX) .optional() .describe( `Who to notify about this comment — a user ID, email, full name, or "me" for each person. Set this whenever the comment mentions someone; the text of an @mention notifies nobody on its own. Omit to notify whoever the Todoist apps would (the task's assignee, assigner and creator on a first comment, or the previous comment's participants on a reply). Pass ["${NO_NOTIFY_KEYWORD}"] to notify nobody.`, diff --git a/src/utils/comment-recipients.test.ts b/src/utils/comment-recipients.test.ts index 52c16d7d..cd68fb58 100644 --- a/src/utils/comment-recipients.test.ts +++ b/src/utils/comment-recipients.test.ts @@ -167,6 +167,40 @@ describe('getDefaultCommentRecipients', () => { }) }) + it('walks every page of a long thread to reach the newest comment', async () => { + mockClient.getComments + .mockResolvedValueOnce({ + results: [ + createComment({ postedAt: new Date('2024-01-01T09:00:00Z'), postedUid: 'p1' }), + ], + nextCursor: 'page-2', + }) + .mockResolvedValueOnce({ + results: [ + createComment({ postedAt: new Date('2024-02-01T09:00:00Z'), postedUid: 'p2' }), + ], + nextCursor: 'page-3', + }) + .mockResolvedValueOnce({ + results: [ + createComment({ postedAt: new Date('2024-03-01T09:00:00Z'), postedUid: 'p3' }), + ], + nextCursor: null, + }) + + const recipients = await getDefaultCommentRecipients( + mockClient, + { taskId: 'task-1' }, + CURRENT_USER_ID, + ) + + expect(mockClient.getComments).toHaveBeenCalledTimes(3) + expect(mockClient.getComments).toHaveBeenLastCalledWith( + expect.objectContaining({ cursor: 'page-3' }), + ) + expect(recipients).toEqual(['p3']) + }) + describe('a project with no comments yet', () => { it('notifies nobody, as a project has no assignee to fall back on', async () => { const recipients = await getDefaultCommentRecipients( diff --git a/src/utils/comment-recipients.ts b/src/utils/comment-recipients.ts index bb137029..6ee99019 100644 --- a/src/utils/comment-recipients.ts +++ b/src/utils/comment-recipients.ts @@ -1,10 +1,4 @@ -import type { - Comment, - GetCommentsResponse, - GetTaskCommentsArgs, - TodoistApi, -} from '@doist/todoist-sdk' -import { fetchAllPages } from '../tool-helpers.js' +import type { Comment, GetTaskCommentsArgs, TodoistApi } from '@doist/todoist-sdk' /** Value that tells `add-comments` to post without notifying anyone. */ export const NO_NOTIFY_KEYWORD = 'none' @@ -63,19 +57,26 @@ async function getLatestComment( client: TodoistApi, target: CommentTarget, ): Promise { - // The SDK brands the unused key as `never` on each half of - // `GetTaskCommentsArgs | GetProjectCommentsArgs`, which a generic cannot - // infer across, so pin one half and let the target supply either key. - const comments = await fetchAllPages({ - apiMethod: (args) => client.getComments(args), - args: target as GetTaskCommentsArgs, - }) + let latest: Comment | undefined + let cursor: string | null = null - // The API does not guarantee an order, so pick the newest explicitly. - return comments.reduce( - (latest, comment) => (!latest || comment.postedAt > latest.postedAt ? comment : latest), - undefined, - ) + // Walked a page at a time, keeping only the newest comment seen. A thread + // can be arbitrarily long and all we want from it is its last participant, + // so there is no reason to hold the whole history in memory. Page order is + // not guaranteed, so every page is still compared. + do { + const response = await client.getComments({ + ...(target.taskId ? { taskId: target.taskId } : { projectId: target.projectId }), + cursor, + } as GetTaskCommentsArgs) + + for (const comment of response.results) { + if (!latest || comment.postedAt > latest.postedAt) latest = comment + } + cursor = response.nextCursor + } while (cursor) + + return latest } function dedupe(userIds: (string | null | undefined)[], currentUserId: string): string[] { diff --git a/src/utils/constants.ts b/src/utils/constants.ts index 317090c3..73bb4f85 100644 --- a/src/utils/constants.ts +++ b/src/utils/constants.ts @@ -29,6 +29,8 @@ export const ApiLimits = { COMMENTS_DEFAULT: 10, /** Maximum limit for comment search and list operations */ COMMENTS_MAX: 10, + /** Maximum number of users a single comment can notify */ + NOTIFY_USERS_MAX: 25, /** Default limit for activity log listings */ ACTIVITY_DEFAULT: 20, /** Maximum limit for activity log search and list operations */ diff --git a/src/utils/user-resolver.test.ts b/src/utils/user-resolver.test.ts index 4a2edc5f..822af3c1 100644 --- a/src/utils/user-resolver.test.ts +++ b/src/utils/user-resolver.test.ts @@ -309,4 +309,19 @@ describe('resolveUserRefs', () => { it('resolves nothing for an empty list', async () => { await expect(resolveUserRefs(mockClient, [])).resolves.toEqual([]) }) + + it('looks a repeated reference up only once', async () => { + await resolveUserRefs(mockClient, ['Ana Lovelace', 'ana lovelace', ' Ana Lovelace ']) + + // The collaborator lookup is shared, so a repeated name must not send + // the whole project list off to be fetched again. + expect(mockClient.getProjects).toHaveBeenCalledTimes(1) + }) + + it('reuses the warmed collaborator cache across distinct references', async () => { + await resolveUserRefs(mockClient, ['Ana Lovelace', 'Bo Turing']) + + expect(mockClient.getProjects).toHaveBeenCalledTimes(1) + expect(mockClient.getProjectCollaborators).toHaveBeenCalledTimes(1) + }) }) diff --git a/src/utils/user-resolver.ts b/src/utils/user-resolver.ts index 3419d04d..d125b8b7 100644 --- a/src/utils/user-resolver.ts +++ b/src/utils/user-resolver.ts @@ -398,9 +398,29 @@ export async function resolveUserNameToId( * @throws Error naming every reference that could not be resolved */ export async function resolveUserRefs(client: TodoistApi, refs: string[]): Promise { - const resolutions = await Promise.all(refs.map((ref) => userResolver.resolveUser(client, ref))) + const seenRefs = new Set() + const seenUserIds = new Set() + const resolved: ResolvedUser[] = [] + const unresolved: string[] = [] + + // Resolved one at a time on purpose: the first lookup populates the + // collaborator cache that the rest read, where resolving in parallel would + // send every reference through its own full collaborator fetch. + for (const ref of refs) { + const dedupeKey = ref.trim().toLowerCase() + if (seenRefs.has(dedupeKey)) continue + seenRefs.add(dedupeKey) + + const user = await userResolver.resolveUser(client, ref) + if (!user) { + unresolved.push(ref) + continue + } + if (seenUserIds.has(user.userId)) continue + seenUserIds.add(user.userId) + resolved.push(user) + } - const unresolved = refs.filter((_ref, index) => !resolutions[index]) if (unresolved.length > 0) { const names = unresolved.map((ref) => `"${ref}"`).join(', ') throw new Error( @@ -408,12 +428,5 @@ export async function resolveUserRefs(client: TodoistApi, refs: string[]): Promi ) } - const seen = new Set() - const resolved: ResolvedUser[] = [] - for (const user of resolutions) { - if (!user || seen.has(user.userId)) continue - seen.add(user.userId) - resolved.push(user) - } return resolved }