Conversation
There was a problem hiding this comment.
Pull request overview
Adds first-pass CRUD support for Linear project milestones to the MCP server, wiring new tool definitions through handlers into LinearService, plus tests and documentation updates.
Changes:
- Introduces milestone tool definitions + handler registration for list/get/create/update/archive operations.
- Implements milestone service methods with a normalizer based on Linear’s
ProjectMilestoneSDK model. - Adds milestone-focused tests, bumps package version, and documents the new tools in
TOOLS.md.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/type-guards.ts | Adds type guards for milestone tool arguments. |
| src/tools/handlers/milestone-handlers.ts | New milestone tool handlers that validate args and delegate to LinearService. |
| src/tools/handlers/index.ts | Registers milestone handlers with the tool router and re-exports them. |
| src/tools/definitions/milestone-tools.ts | New MCP tool definitions for milestone CRUD operations. |
| src/tools/definitions/index.ts | Adds milestone tool definitions to the global tool list/exports. |
| src/services/linear-service.ts | Implements milestone list/get/create/update/archive methods and normalization. |
| src/tests/milestone-tools.test.ts | Verifies milestone tool registration, routing, and argument validation behavior. |
| src/tests/linear-service.test.ts | Adds sanitization/update-requirement tests for milestone create/update payloads. |
| package.json | Bumps version and adds milestone tools to Smithery tool list. |
| TOOLS.md | Documents milestone tools and removes them from the “planned” list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| typeof args === 'object' && | ||
| args !== null && | ||
| (!('includeArchived' in args) || | ||
| typeof (args as { includeArchived: boolean }).includeArchived === 'boolean') && | ||
| (!('limit' in args) || typeof (args as { limit: number }).limit === 'number') |
There was a problem hiding this comment.
isGetMilestonesArgs accepts any number for limit (including NaN, Infinity, negatives, and non-integers) and also treats arrays as valid objects. Since limit is passed to Linear pagination (first), this can allow invalid inputs past the type guard and trigger runtime errors. Consider requiring a plain object (non-array) and validating limit as a finite positive integer (you already have isJsonObject/isPositiveInteger helpers later in this file).
| typeof args === 'object' && | |
| args !== null && | |
| (!('includeArchived' in args) || | |
| typeof (args as { includeArchived: boolean }).includeArchived === 'boolean') && | |
| (!('limit' in args) || typeof (args as { limit: number }).limit === 'number') | |
| isJsonObject(args) && | |
| (!('includeArchived' in args) || | |
| typeof (args as { includeArchived: boolean }).includeArchived === 'boolean') && | |
| (!('limit' in args) || isPositiveInteger((args as { limit: number }).limit)) |
| default: false, | ||
| }, | ||
| limit: { | ||
| type: 'number', |
There was a problem hiding this comment.
linear_getMilestones defines limit as { type: 'number' }, but this value is used as a pagination first argument which is an integer in Linear/GraphQL. Defining this as type: 'integer' with minimum: 1 (similar to view-tools.ts) would prevent invalid non-integer/negative limits from being advertised/accepted.
| type: 'number', | |
| type: 'integer', | |
| minimum: 1, |
| output_schema: { | ||
| type: 'array', | ||
| items: { | ||
| type: 'object', | ||
| properties: { | ||
| id: { type: 'string' }, | ||
| name: { type: 'string' }, | ||
| description: { type: 'string' }, | ||
| status: { type: 'string' }, | ||
| progress: { type: 'number' }, | ||
| sortOrder: { type: 'number' }, | ||
| targetDate: { type: 'string' }, | ||
| createdAt: { type: 'string' }, | ||
| updatedAt: { type: 'string' }, | ||
| archivedAt: { type: 'string' }, | ||
| project: { | ||
| type: 'object', | ||
| properties: { | ||
| id: { type: 'string' }, | ||
| name: { type: 'string' }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The milestone output_schema marks several fields as always string/object, but the service normalizer can emit null for project (and Linear SDK fields like description, targetDate, archivedAt are commonly nullable). This makes the documented schema inaccurate and can break any downstream schema-based consumers. Consider updating these properties to allow null where applicable (e.g., type: ['string','null'] / type: ['object','null']) and applying the same pattern consistently across the milestone tool outputs.
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/itz4blitz/mcp-linear/sessions/7529291e-545e-4a97-93e5-ab7b7fff077f Co-authored-by: itz4blitz <60281870+itz4blitz@users.noreply.github.com>
Implemented all requested review-thread updates in 312f84b: tightened |
| description: 'Updated sort order for the milestone within its project', | ||
| }, | ||
| }, | ||
| required: ['id'], |
There was a problem hiding this comment.
linear_updateMilestone currently accepts { id } only in the tool schema, but LinearService.updateMilestone will throw At least one milestone field must be provided when no updatable fields are present. Consider encoding this constraint in input_schema (e.g., an anyOf requiring one of name, projectId, description, targetDate, or sortOrder, similar to linear_updateSavedView) so invalid calls are rejected by schema validation before reaching the service.
| required: ['id'], | |
| required: ['id'], | |
| anyOf: [ | |
| { required: ['name'] }, | |
| { required: ['projectId'] }, | |
| { required: ['description'] }, | |
| { required: ['targetDate'] }, | |
| { required: ['sortOrder'] }, | |
| ], |
| (!('projectId' in args) || typeof (args as { projectId: string }).projectId === 'string') && | ||
| (!('description' in args) || typeof (args as { description: string }).description === 'string') && | ||
| (!('targetDate' in args) || typeof (args as { targetDate: string }).targetDate === 'string') && | ||
| (!('sortOrder' in args) || typeof (args as { sortOrder: number }).sortOrder === 'number') |
There was a problem hiding this comment.
isUpdateMilestoneArgs returns true for { id: string } even though LinearService.updateMilestone rejects updates with no fields to change. Consider tightening the type guard to require at least one of name, projectId, description, targetDate, or sortOrder (mirroring the service check) so callers get a consistent Invalid arguments for updateMilestone error instead of a later service-level error.
| (!('sortOrder' in args) || typeof (args as { sortOrder: number }).sortOrder === 'number') | |
| (!('sortOrder' in args) || typeof (args as { sortOrder: number }).sortOrder === 'number') && | |
| ('name' in args || | |
| 'projectId' in args || | |
| 'description' in args || | |
| 'targetDate' in args || | |
| 'sortOrder' in args) |
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/itz4blitz/mcp-linear/sessions/d4c0c5a4-ec7a-4ff8-a33b-a5d7017879d1 Co-authored-by: itz4blitz <60281870+itz4blitz@users.noreply.github.com>
Applied in e7da114: |
| export function isGetMilestonesArgs(args: unknown): args is { | ||
| includeArchived?: boolean; | ||
| limit?: number; | ||
| } { | ||
| return ( | ||
| isJsonObject(args) && | ||
| (!('includeArchived' in args) || | ||
| typeof (args as { includeArchived: boolean }).includeArchived === 'boolean') && | ||
| (!('limit' in args) || isPositiveInteger((args as { limit: number }).limit)) | ||
| ); |
There was a problem hiding this comment.
isGetMilestonesArgs rejects null, but linear_getMilestones has no required args and the MCP server will pass null when arguments is omitted. Either allow null here (treat as {}) or ensure the handler coerces null to {} before calling this guard.
| export function handleGetMilestones(linearService: LinearService) { | ||
| return async (args: unknown) => { | ||
| try { | ||
| if (!isGetMilestonesArgs(args)) { | ||
| throw new Error('Invalid arguments for getMilestones'); | ||
| } | ||
|
|
||
| return await linearService.getMilestones(args); | ||
| } catch (error) { | ||
| logError('Error getting milestones', error); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
linear_getMilestones has only optional fields, but MCP server passes args = null when the caller omits arguments (see src/mcp-server.ts:70). With the current guard, null is rejected, so calling the tool without args will throw even though the schema has defaults. Consider treating null as {} in the handler (or updating the type guard to accept null for this tool) and add a test that handlers.linear_getMilestones(null) succeeds.
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/itz4blitz/mcp-linear/sessions/0bca453f-1e50-4bee-8292-e2ea05a6872b Co-authored-by: itz4blitz <60281870+itz4blitz@users.noreply.github.com>
Implemented in dabda83: |
|
@copilot resolve the merge conflicts in this pull request |
Agent-Logs-Url: https://github.com/itz4blitz/mcp-linear/sessions/b2522949-c340-41ba-af5a-2c668a7c7cd5 Co-authored-by: itz4blitz <60281870+itz4blitz@users.noreply.github.com>
Resolved in 7bc2664 by merging |
# Conflicts: # src/services/linear-service.ts # src/tools/definitions/index.ts # src/tools/handlers/index.ts # src/tools/type-guards.ts
Summary
linear_getMilestones,linear_getMilestoneById,linear_createMilestone,linear_updateMilestone, andlinear_archiveMilestoneProjectMilestoneSDK model and keep milestone-project association helpers out of scope for this first passTOOLS.mdValidation