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
6 changes: 6 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@

## Tool Schema Design Rules

### Output schemas must never use `.nullable()`

`structuredContent` is sanitised with `removeNullFields` before it leaves the server, so a null can never reach a client. A nullable output field therefore declares a value the tool cannot deliver: the MCP SDK validates the sanitised payload against the declared `outputSchema` and fails the whole call with `Output validation error` the moment that field is stripped.

Declare the field `.optional()` instead and leave the key out of `structuredContent` (`foo: value ?? undefined`) when there is nothing to report. `src/tools/output-schema-nullability.test.ts` enforces this across every registered tool.

### Removing/Clearing Optional Fields

When you need to support clearing an optional field:
Expand Down
2 changes: 1 addition & 1 deletion src/tools/get-overview.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ describe(`${GET_OVERVIEW} tool`, () => {
totalProjects: 0,
totalSections: 0,
hasNestedProjects: false,
inbox: null,
inbox: undefined,
})
})
})
Expand Down
7 changes: 3 additions & 4 deletions src/tools/get-overview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ const OutputSchema = {
name: z.string().describe('The inbox project name.'),
sections: z.array(SectionSchema).describe('Sections in the inbox project.'),
})
.nullable()
.optional()
.describe('Inbox information (account overview only).'),
projects: z
Expand Down Expand Up @@ -246,11 +245,11 @@ type ProjectStructure = {

type AccountOverviewStructured = Record<string, unknown> & {
type: 'account_overview'
inbox: {
inbox?: {
id: string
name: string
sections: SectionSummary[]
} | null
}
projects: ProjectStructure[]
totalProjects: number
totalSections: number
Expand Down Expand Up @@ -354,7 +353,7 @@ async function generateAccountOverview(
name: inbox.name,
sections: (sectionsByProject[inbox.id] || []).map(toSectionSummary),
}
: null,
: undefined,
projects: tree.map((project) =>
buildProjectStructure(project as ProjectWithChildren, sectionsByProject),
),
Expand Down
4 changes: 2 additions & 2 deletions src/tools/get-project-activity-stats.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ describe('get-project-activity-stats tool', () => {
{ date: '2026-03-28', totalCount: 8 },
{ date: '2026-03-27', totalCount: 3 },
],
weekItems: null,
weekItems: undefined,
})

expect(result.textContent).toContain('Daily Activity')
Expand Down Expand Up @@ -114,7 +114,7 @@ describe('get-project-activity-stats tool', () => {

expect(result.structuredContent).toMatchObject({
dayItems: [],
weekItems: null,
weekItems: undefined,
})
expect(result.textContent).toContain('No daily activity data available')
})
Expand Down
4 changes: 2 additions & 2 deletions src/tools/get-project-activity-stats.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const OutputSchema = {
totalCount: z.number().describe('Number of tasks completed in this week.'),
}),
)
.nullable()
.optional()
.describe('Weekly completion rollups. Only included when includeWeeklyCounts is true.'),
}

Expand Down Expand Up @@ -89,7 +89,7 @@ const getProjectActivityStats = {
structuredContent: {
projectId,
dayItems: stats.dayItems,
weekItems: stats.weekItems ?? null,
weekItems: stats.weekItems ?? undefined,
},
}
},
Expand Down
32 changes: 16 additions & 16 deletions src/tools/get-project-health.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ const TaskContextOutputSchema = z.object({
id: z.string().describe('The task ID.'),
content: z.string().describe('The task content/title.'),
priority: z.string().describe('The task priority (1-4).'),
due: z.string().nullable().optional().describe('The due date string, if set.'),
deadline: z.string().nullable().optional().describe('The deadline date string, if set.'),
due: z.string().optional().describe('The due date string, if set.'),
deadline: z.string().optional().describe('The deadline date string, if set.'),
isCompleted: z.boolean().describe('Whether the task is completed.'),
labels: z.array(z.string()).describe('Labels applied to this task.'),
})
Expand All @@ -49,17 +49,14 @@ const OutputSchema = {
status: z.enum(HEALTH_STATUSES).describe('The overall health status of the project.'),
description: z
.string()
.nullable()
.optional()
.describe('Detailed description of the health assessment.'),
descriptionSummary: z
.string()
.nullable()
.optional()
.describe('Brief summary of the health assessment.'),
taskRecommendations: z
.array(TaskRecommendationOutputSchema)
.nullable()
.optional()
.describe('Specific recommendations for individual tasks.'),
isStale: z
Expand All @@ -70,14 +67,13 @@ const OutputSchema = {
.describe('Whether a health analysis update is currently in progress.'),
updatedAt: z
.string()
.nullable()
.optional()
.describe('When the health assessment was last updated.'),
})
.describe('Project health assessment.'),
context: z
.object({
projectDescription: z.string().nullable().describe('The project description, if any.'),
projectDescription: z.string().optional().describe('The project description, if any.'),
projectMetrics: z
.object({
totalTasks: z.number().describe('Total number of tasks in the project.'),
Expand All @@ -89,7 +85,7 @@ const OutputSchema = {
.describe('Tasks completed in the current week.'),
averageCompletionTime: z
.number()
.nullable()
.optional()
.describe('Average task completion time in days, if available.'),
})
.describe('Aggregated project metrics.'),
Expand Down Expand Up @@ -222,14 +218,18 @@ const getProjectHealth = {

const context = data.context
? {
projectDescription: data.context.projectDescription,
projectMetrics: data.context.projectMetrics,
projectDescription: data.context.projectDescription ?? undefined,
projectMetrics: {
...data.context.projectMetrics,
averageCompletionTime:
data.context.projectMetrics.averageCompletionTime ?? undefined,
},
tasks: data.context.tasks.map((task) => ({
id: task.id,
content: task.content,
priority: task.priority,
due: task.due ?? null,
deadline: task.deadline ?? null,
due: task.due ?? undefined,
deadline: task.deadline ?? undefined,
isCompleted: task.isCompleted,
labels: task.labels,
})),
Expand All @@ -248,12 +248,12 @@ const getProjectHealth = {
},
health: {
status: data.health.status,
description: data.health.description ?? null,
descriptionSummary: data.health.descriptionSummary ?? null,
taskRecommendations: data.health.taskRecommendations ?? null,
description: data.health.description ?? undefined,
descriptionSummary: data.health.descriptionSummary ?? undefined,
taskRecommendations: data.health.taskRecommendations ?? undefined,
isStale: data.health.isStale,
updateInProgress: data.health.updateInProgress,
updatedAt: data.health.updatedAt?.toISOString() ?? null,
updatedAt: data.health.updatedAt?.toISOString(),
},
context,
},
Expand Down
22 changes: 15 additions & 7 deletions src/tools/get-workspace-insights.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import type { TodoistApi, WorkspaceInsights } from '@doist/todoist-sdk'
import { type Mocked, vi } from 'vitest'
import { z } from 'zod'
import { removeNullFields } from '../utils/sanitize-data.js'
import { TEST_ERRORS } from '../utils/test-helpers.js'
import { ToolNames } from '../utils/tool-names.js'
import { workspaceResolver } from '../utils/workspace-resolver.js'
Expand All @@ -18,8 +20,9 @@ const mockTodoistApi = {
const mockResolveWorkspace = vi.mocked(workspaceResolver.resolveWorkspace)

function createMockInsights(overrides: Partial<WorkspaceInsights> = {}): WorkspaceInsights {
// The API dropped `folder_id` from this endpoint, so the SDK no longer
// receives one.
return {
folderId: null,
projectInsights: [
{
projectId: 'proj-1',
Expand Down Expand Up @@ -94,7 +97,6 @@ describe('get-workspace-insights tool', () => {
expect(result.structuredContent).toMatchObject({
workspaceId: 'ws-123',
workspaceName: 'Engineering',
folderId: null,
projectInsights: [
{
projectId: 'proj-1',
Expand All @@ -108,12 +110,12 @@ describe('get-workspace-insights tool', () => {
},
{
projectId: 'proj-3',
health: null,
progress: null,
},
],
})

expect(result.structuredContent).not.toHaveProperty('folderId')

expect(result.textContent).toContain('Engineering')
expect(result.textContent).toContain('**Projects:** 3')
})
Expand Down Expand Up @@ -142,12 +144,18 @@ describe('get-workspace-insights tool', () => {
mockTodoistApi,
)

expect(result.structuredContent.projectInsights[0]).toMatchObject({
expect(result.structuredContent.projectInsights[0]).toEqual({
projectId: 'proj-1',
health: null,
progress: null,
health: undefined,
progress: undefined,
})

// Nulls would be stripped on the way out, leaving structured content
// that no longer matches the declared output schema and failing the
// call with an MCP output validation error.
const sanitized = removeNullFields(result.structuredContent)
expect(() => z.object(getWorkspaceInsights.outputSchema).parse(sanitized)).not.toThrow()

expect(result.textContent).toContain('status=N/A')
expect(result.textContent).toContain('progress=N/A')
})
Expand Down
14 changes: 6 additions & 8 deletions src/tools/get-workspace-insights.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,21 @@ const ProjectInsightSchema = z.object({
.boolean()
.describe('Whether a health analysis update is in progress.'),
})
.nullable()
.describe('Health data for this project, if available.'),
.optional()
.describe('Health data for this project. Omitted when the project has none.'),
progress: z
.object({
completedCount: z.number().describe('Number of completed tasks.'),
activeCount: z.number().describe('Number of active tasks.'),
progressPercent: z.number().describe('Completion percentage (0-100).'),
})
.nullable()
.describe('Progress data for this project, if available.'),
.optional()
.describe('Progress data for this project. Omitted when the project has none.'),
})

const OutputSchema = {
workspaceId: z.string().describe('The resolved workspace ID.'),
workspaceName: z.string().describe('The resolved workspace name.'),
folderId: z.string().nullable().describe('The folder ID, if applicable.'),
projectInsights: z
.array(ProjectInsightSchema)
.describe('Health and progress insights for each project in the workspace.'),
Expand Down Expand Up @@ -74,14 +73,14 @@ const getWorkspaceInsights = {
isStale: p.health.isStale,
updateInProgress: p.health.updateInProgress,
}
: null,
: undefined,
progress: p.progress
? {
completedCount: p.progress.completedCount,
activeCount: p.progress.activeCount,
progressPercent: p.progress.progressPercent,
}
: null,
: undefined,
}))

const lines: string[] = [
Expand All @@ -102,7 +101,6 @@ const getWorkspaceInsights = {
structuredContent: {
workspaceId: resolved.workspaceId,
workspaceName: resolved.workspaceName,
folderId: insights.folderId ?? null,
projectInsights,
},
}
Expand Down
69 changes: 69 additions & 0 deletions src/tools/output-schema-nullability.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import { describe, expect, it } from 'vitest'
import { z } from 'zod'
import { registeredTools } from '../tool-registry.js'

/**
* Structured content is sanitised with `removeNullFields` on its way out, so no
* null ever reaches a client. A tool that declares an output field as nullable
* therefore promises a value it can never send: the MCP SDK validates the
* sanitised payload against the declared output schema and fails the whole call
* with "Output validation error" as soon as that field is stripped.
*
* The shape that survives sanitisation is `.optional()` with the key left out of
* `structuredContent` when there is nothing to report.
*/
function findNullablePaths(node: unknown, path: string): string[] {
if (!node || typeof node !== 'object') {
return []
}

if (Array.isArray(node)) {
return node.flatMap((item) => findNullablePaths(item, path))
}

const schema = node as Record<string, unknown>
const { type } = schema

if (type === 'null' || (Array.isArray(type) && type.includes('null'))) {
return [path]
}

const paths: string[] = []

const properties = schema.properties
if (properties && typeof properties === 'object') {
for (const [key, value] of Object.entries(properties)) {
paths.push(...findNullablePaths(value, path ? `${path}.${key}` : key))
}
}

for (const key of ['items', 'additionalProperties', 'not']) {
paths.push(...findNullablePaths(schema[key], key === 'items' ? `${path}[]` : path))
}

for (const key of ['anyOf', 'oneOf', 'allOf', 'prefixItems']) {
paths.push(...findNullablePaths(schema[key], path))
}

const defs = schema.$defs
if (defs && typeof defs === 'object') {
for (const [key, value] of Object.entries(defs)) {
paths.push(...findNullablePaths(value, `$defs.${key}`))
}
}

return paths
}

describe('tool output schemas', () => {
it.each(registeredTools.map((tool) => [tool.name, tool] as const))(
'%s declares no nullable output fields',
(_name, tool) => {
const jsonSchema = z.toJSONSchema(z.object(tool.outputSchema))

// Nullable output fields cannot survive `removeNullFields`; use
// `.optional()` and omit the key instead.
expect(findNullablePaths(jsonSchema, '')).toEqual([])
},
)
})
Loading