From c48475715203551fd96300be0061aaf83873f860 Mon Sep 17 00:00:00 2001 From: Matt Galligan Date: Sat, 21 Feb 2026 22:30:53 -0500 Subject: [PATCH] refactor(mcp): convert handlers to Result-based error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace all throw statements in MCP handler and utility files with Result return types. Security-boundary throws in filesystem.ts are converted to ValidationError results propagated through callers. Tests updated from rejects.toThrow() to isErr() checks. 🤘🏻 In-collaboration-with: [Claude Code](https://claude.com/claude-code) --- apps/mcp/src/index.test.ts | 125 +++++++++++++---------- apps/mcp/src/resources/index.ts | 8 +- apps/mcp/src/resources/todos.test.ts | 12 ++- apps/mcp/src/resources/todos.ts | 42 +++++--- apps/mcp/src/tools/add.ts | 142 +++++++++++++++++++-------- apps/mcp/src/tools/graph.ts | 40 +++++--- apps/mcp/src/tools/index.ts | 7 +- apps/mcp/src/tools/scan.ts | 40 +++++--- apps/mcp/src/tools/waymark.test.ts | 12 ++- apps/mcp/src/tools/waymark.ts | 8 +- apps/mcp/src/utils/filesystem.ts | 104 ++++++++++++++------ 11 files changed, 360 insertions(+), 180 deletions(-) diff --git a/apps/mcp/src/index.test.ts b/apps/mcp/src/index.test.ts index c86b3bd0..977cae6c 100644 --- a/apps/mcp/src/index.test.ts +++ b/apps/mcp/src/index.test.ts @@ -21,16 +21,16 @@ function createNotifyTracker() { }; } -const TLDR_EXISTS_REGEX = /already contains a tldr waymark/u; -const WM_ID_REGEX = /wm:/u; -const EMPTY_ID_REGEX = /cannot be empty/u; -const DIFFERENT_ID_REGEX = /different waymark id/u; const ABOUT_INSERT_LINE = 3; const SAMPLE_SOURCE = ["line 1", "line 2", "line 3", "line 4", "line 5"].join( "\n" ); const TRUNCATION_LIMIT = 3; const EXPECTED_TRUNCATED_LINES = 4; +const TLDR_EXISTS_REGEX = /already contains a tldr waymark/u; +const DIFFERENT_ID_REGEX = /different waymark id/u; +const EMPTY_ID_REGEX = /cannot be empty/u; +const WM_ID_REGEX = /wm:/u; describe("handleAddWaymark", () => { test("inserts TLDR at top of file", async () => { @@ -50,15 +50,17 @@ describe("handleAddWaymark", () => { ); const tracker = createNotifyTracker(); - const response = await handleAddWaymark({ + const result = await handleAddWaymark({ notifyResourceChanged: tracker.notify, filePath: file, type: "tldr", content: "Summarizes example module export", }); + expect(result.isOk()).toBe(true); + const response = result.isOk() ? result.value : null; expect(tracker.changes).toBe(1); - const payload = JSON.parse(String(response.content?.[0]?.text ?? "")) as { + const payload = JSON.parse(String(response?.content?.[0]?.text ?? "")) as { type: string; startLine: number; content: string; @@ -92,7 +94,7 @@ describe("handleAddWaymark", () => { const signals: SignalFlags = { flagged: true }; const tracker = createNotifyTracker(); - const response = await handleAddWaymark({ + const result = await handleAddWaymark({ notifyResourceChanged: tracker.notify, filePath: file, type: "about", @@ -101,7 +103,9 @@ describe("handleAddWaymark", () => { signals, }); - const payload = JSON.parse(String(response.content?.[0]?.text ?? "")) as { + expect(result.isOk()).toBe(true); + const response = result.isOk() ? result.value : null; + const payload = JSON.parse(String(response?.content?.[0]?.text ?? "")) as { type: string; startLine: number; content: string; @@ -119,7 +123,7 @@ describe("handleAddWaymark", () => { await rm(dir, { recursive: true, force: true }); }); - test("throws when TLDR already exists", async () => { + test("returns error when TLDR already exists", async () => { const dir = await mkdtemp(join(tmpdir(), "waymark-mcp-duplicate-")); const file = join(dir, "duplicate.ts"); await writeFile( @@ -129,14 +133,16 @@ describe("handleAddWaymark", () => { ); const tracker = createNotifyTracker(); - await expect( - handleAddWaymark({ - notifyResourceChanged: tracker.notify, - filePath: file, - type: "tldr", - content: "another summary", - }) - ).rejects.toThrow(TLDR_EXISTS_REGEX); + const result = await handleAddWaymark({ + notifyResourceChanged: tracker.notify, + filePath: file, + type: "tldr", + content: "another summary", + }); + + expect(result.isErr()).toBe(true); + const errorMessage = result.isErr() ? result.error.message : ""; + expect(errorMessage).toMatch(TLDR_EXISTS_REGEX); const updated = await readFile(file, "utf8"); expect(updated).toContain("existing summary"); @@ -155,7 +161,7 @@ describe("handleAddWaymark", () => { ); const tracker = createNotifyTracker(); - const response = await handleAddWaymark({ + const result = await handleAddWaymark({ notifyResourceChanged: tracker.notify, filePath: file, type: "idea", @@ -163,7 +169,9 @@ describe("handleAddWaymark", () => { line: 1, }); - const payload = JSON.parse(String(response.content?.[0]?.text ?? "")) as { + expect(result.isOk()).toBe(true); + const response = result.isOk() ? result.value : null; + const payload = JSON.parse(String(response?.content?.[0]?.text ?? "")) as { type: string; startLine: number; }; @@ -187,7 +195,7 @@ describe("handleAddWaymark", () => { ); const tracker = createNotifyTracker(); - await handleAddWaymark({ + const result = await handleAddWaymark({ notifyResourceChanged: tracker.notify, filePath: file, type: "todo", @@ -196,6 +204,7 @@ describe("handleAddWaymark", () => { id: "Auth-Refresh", }); + expect(result.isOk()).toBe(true); const updated = await readFile(file, "utf8"); const [firstLine] = updated.split("\n"); expect(firstLine).toBe("// todo ::: add retry [[auth-refresh]]"); @@ -213,7 +222,7 @@ describe("handleAddWaymark", () => { ); const tracker = createNotifyTracker(); - await handleAddWaymark({ + const result = await handleAddWaymark({ notifyResourceChanged: tracker.notify, filePath: file, type: "todo", @@ -222,6 +231,7 @@ describe("handleAddWaymark", () => { id: "[[Auth-Refresh]]", }); + expect(result.isOk()).toBe(true); const updated = await readFile(file, "utf8"); const [firstLine] = updated.split("\n"); expect(firstLine).toBe("// todo ::: add retry [[auth-refresh]]"); @@ -229,7 +239,7 @@ describe("handleAddWaymark", () => { await rm(dir, { recursive: true, force: true }); }); - test("throws when content already contains a different id", async () => { + test("returns error when content already contains a different id", async () => { const dir = await mkdtemp(join(tmpdir(), "waymark-mcp-id-conflict-")); const file = join(dir, "feature.ts"); await writeFile( @@ -239,16 +249,18 @@ describe("handleAddWaymark", () => { ); const tracker = createNotifyTracker(); - await expect( - handleAddWaymark({ - notifyResourceChanged: tracker.notify, - filePath: file, - type: "todo", - content: "add retry [[existing-id]]", - line: 1, - id: "other-id", - }) - ).rejects.toThrow(DIFFERENT_ID_REGEX); + const result = await handleAddWaymark({ + notifyResourceChanged: tracker.notify, + filePath: file, + type: "todo", + content: "add retry [[existing-id]]", + line: 1, + id: "other-id", + }); + + expect(result.isErr()).toBe(true); + const errorMessage = result.isErr() ? result.error.message : ""; + expect(errorMessage).toMatch(DIFFERENT_ID_REGEX); await rm(dir, { recursive: true, force: true }); }); @@ -263,7 +275,7 @@ describe("handleAddWaymark", () => { ); const tracker = createNotifyTracker(); - await handleAddWaymark({ + const result = await handleAddWaymark({ notifyResourceChanged: tracker.notify, filePath: file, type: "todo", @@ -272,6 +284,7 @@ describe("handleAddWaymark", () => { id: " AUTH ", }); + expect(result.isOk()).toBe(true); const updated = await readFile(file, "utf8"); const [firstLine] = updated.split("\n"); expect(firstLine).toBe("// todo ::: add retry [[auth]]"); @@ -279,7 +292,7 @@ describe("handleAddWaymark", () => { await rm(dir, { recursive: true, force: true }); }); - test("rejects empty ids", async () => { + test("returns error for empty ids", async () => { const dir = await mkdtemp(join(tmpdir(), "waymark-mcp-id-empty-")); const file = join(dir, "feature.ts"); await writeFile( @@ -289,35 +302,39 @@ describe("handleAddWaymark", () => { ); const tracker = createNotifyTracker(); - await expect( - handleAddWaymark({ - notifyResourceChanged: tracker.notify, - filePath: file, - type: "todo", - content: "add retry", - line: 1, - id: " ", - }) - ).rejects.toThrow(EMPTY_ID_REGEX); + const result = await handleAddWaymark({ + notifyResourceChanged: tracker.notify, + filePath: file, + type: "todo", + content: "add retry", + line: 1, + id: " ", + }); + + expect(result.isErr()).toBe(true); + const errorMessage = result.isErr() ? result.error.message : ""; + expect(errorMessage).toMatch(EMPTY_ID_REGEX); await rm(dir, { recursive: true, force: true }); }); - test("rejects wm ids", async () => { + test("returns error for wm ids", async () => { const dir = await mkdtemp(join(tmpdir(), "waymark-mcp-wm-")); const file = join(dir, "wm-id.ts"); await writeFile(file, ["export const sample = true;"].join("\n"), "utf8"); const tracker = createNotifyTracker(); - await expect( - handleAddWaymark({ - notifyResourceChanged: tracker.notify, - filePath: file, - type: "todo", - content: "add retry", - id: "wm:abc123", - }) - ).rejects.toThrow(WM_ID_REGEX); + const result = await handleAddWaymark({ + notifyResourceChanged: tracker.notify, + filePath: file, + type: "todo", + content: "add retry", + id: "wm:abc123", + }); + + expect(result.isErr()).toBe(true); + const errorMessage = result.isErr() ? result.error.message : ""; + expect(errorMessage).toMatch(WM_ID_REGEX); await rm(dir, { recursive: true, force: true }); }); diff --git a/apps/mcp/src/resources/index.ts b/apps/mcp/src/resources/index.ts index 14b5f531..84f7a2dc 100644 --- a/apps/mcp/src/resources/index.ts +++ b/apps/mcp/src/resources/index.ts @@ -23,12 +23,16 @@ export function registerResources(server: Server): void { ], })); - // biome-ignore lint/suspicious/useAwait: handler must return Promise per SDK contract server.setRequestHandler(ReadResourceRequestSchema, async (request) => { const { uri } = request.params; if (uri === todosResourceDefinition.uri) { - return handleTodosResource(); + const result = await handleTodosResource(); + if (result.isErr()) { + throw new Error(result.error.message); + } + return result.value; } + // SDK boundary: unknown resource URIs are reported as protocol errors throw new Error(`Unknown resource: ${uri}`); }); } diff --git a/apps/mcp/src/resources/todos.test.ts b/apps/mcp/src/resources/todos.test.ts index cac91b2d..2e1d4c88 100644 --- a/apps/mcp/src/resources/todos.test.ts +++ b/apps/mcp/src/resources/todos.test.ts @@ -43,10 +43,12 @@ describe("handleTodosResource", () => { ].join("\n") ); - const response = await withWorkspace(workspace, () => + const result = await withWorkspace(workspace, () => handleTodosResource() ); - const text = String(response.contents?.[0]?.text ?? ""); + expect(result.isOk()).toBe(true); + const response = result.isOk() ? result.value : null; + const text = String(response?.contents?.[0]?.text ?? ""); const payload = JSON.parse(text) as { todos: Array<{ content: string }>; truncated: boolean; @@ -71,10 +73,12 @@ describe("handleTodosResource", () => { ); await writeFixture(join(workspace, "src", "many.ts"), lines.join("\n")); - const response = await withWorkspace(workspace, () => + const result = await withWorkspace(workspace, () => handleTodosResource() ); - const text = String(response.contents?.[0]?.text ?? ""); + expect(result.isOk()).toBe(true); + const response = result.isOk() ? result.value : null; + const text = String(response?.contents?.[0]?.text ?? ""); const payload = JSON.parse(text) as { todos: Array<{ content: string }>; truncated: boolean; diff --git a/apps/mcp/src/resources/todos.ts b/apps/mcp/src/resources/todos.ts index e13f4fe8..de92fe26 100644 --- a/apps/mcp/src/resources/todos.ts +++ b/apps/mcp/src/resources/todos.ts @@ -1,6 +1,8 @@ // tldr ::: todos resource handler for waymark MCP server import { readFile } from "node:fs/promises"; +import type { OutfitterError } from "@outfitter/contracts"; +import { InternalError, Result } from "@outfitter/contracts"; import { parse, type WaymarkRecord } from "@waymarks/core"; import { MARKERS } from "@waymarks/grammar"; import { TODOS_RESOURCE_URI } from "../types"; @@ -14,12 +16,22 @@ import { export const MAX_TODOS_CONCURRENCY = 8; export const MAX_TODOS_RESULTS = 2000; +type TodosResourceContents = { + contents: Array<{ uri: string; mimeType: string; text: string }>; +}; + /** * Handle the todos resource request. - * @returns MCP resource response with todo records. + * @returns Result containing MCP resource response with todo records, or an OutfitterError. */ -export async function handleTodosResource() { - const { records, truncated } = await collectRecords(["."], {}); +export async function handleTodosResource(): Promise< + Result +> { + const collectResult = await collectRecords(["."], {}); + if (collectResult.isErr()) { + return Result.err(collectResult.error); + } + const { records, truncated } = collectResult.value; const todos = records.map((record) => ({ file: record.file, line: record.startLine, @@ -27,7 +39,7 @@ export async function handleTodosResource() { raw: record.raw, })); - return { + return Result.ok({ contents: [ { uri: TODOS_RESOURCE_URI, @@ -35,16 +47,22 @@ export async function handleTodosResource() { text: JSON.stringify({ todos, truncated }, null, 2), }, ], - }; + }); } async function collectRecords( inputs: string[], options: { configPath?: string; scope?: string } -): Promise<{ records: WaymarkRecord[]; truncated: boolean }> { - let filePaths = await expandInputPaths(inputs); +): Promise< + Result<{ records: WaymarkRecord[]; truncated: boolean }, OutfitterError> +> { + const expandResult = await expandInputPaths(inputs); + if (expandResult.isErr()) { + return Result.err(expandResult.error); + } + let filePaths = expandResult.value; if (filePaths.length === 0) { - return { records: [], truncated: false }; + return Result.ok({ records: [], truncated: false }); } const configResult = await loadConfig({ @@ -52,8 +70,10 @@ async function collectRecords( ...(options.configPath ? { configPath: options.configPath } : {}), }); if (configResult.isErr()) { - throw new Error( - `Failed to load config: ${configResult.error instanceof Error ? configResult.error.message : String(configResult.error)}` + return Result.err( + InternalError.create( + `Failed to load config: ${configResult.error instanceof Error ? configResult.error.message : String(configResult.error)}` + ) ); } const config = configResult.value; @@ -92,7 +112,7 @@ async function collectRecords( records.push(...todos); }); - return { records, truncated }; + return Result.ok({ records, truncated }); } export const todosResourceDefinition = { diff --git a/apps/mcp/src/tools/add.ts b/apps/mcp/src/tools/add.ts index 5c0de1dd..95371bb3 100644 --- a/apps/mcp/src/tools/add.ts +++ b/apps/mcp/src/tools/add.ts @@ -3,6 +3,13 @@ import { existsSync } from "node:fs"; import { readFile, writeFile } from "node:fs/promises"; import { resolve } from "node:path"; +import type { OutfitterError } from "@outfitter/contracts"; +import { + InternalError, + NotFoundError, + Result, + ValidationError, +} from "@outfitter/contracts"; import type { ConfigScope, WaymarkRecord } from "@waymarks/core"; import { formatText, parse } from "@waymarks/core"; import { MARKERS } from "@waymarks/grammar"; @@ -81,23 +88,37 @@ type InsertWaymarkResult = { /** * Handle the add action for the MCP tool. * @param input - Raw tool input payload. - * @param server - MCP server interface for notifications. - * @returns MCP tool result with insertion payload. + * @param notifyResourceChanged - Callback invoked after a successful write. + * @returns Result containing MCP tool result with insertion payload, or an OutfitterError. */ // biome-ignore lint/complexity/noExcessiveCognitiveComplexity: sequential MCP input handling with necessary branching export async function handleAdd( input: unknown, notifyResourceChanged: () => void -): Promise { - const params = addWaymarkInputSchema.parse(input); +): Promise> { + const parseResult = addWaymarkInputSchema.safeParse(input); + if (!parseResult.success) { + return Result.err(ValidationError.fromMessage(parseResult.error.message)); + } + const params = parseResult.data; const { filePath, type, content, line, signals, id, configPath, scope } = params; - const normalizedId = id ? normalizeWaymarkId(id) : undefined; - const normalizedContent = applyIdToContent(content, normalizedId); + const normalizedIdResult = id + ? normalizeWaymarkId(id) + : Result.ok(undefined); + if (normalizedIdResult.isErr()) { + return Result.err(normalizedIdResult.error); + } + const normalizedId = normalizedIdResult.value; + const normalizedContentResult = applyIdToContent(content, normalizedId); + if (normalizedContentResult.isErr()) { + return Result.err(normalizedContentResult.error); + } + const normalizedContent = normalizedContentResult.value; const absolutePath = resolve(process.cwd(), filePath); if (!existsSync(absolutePath)) { - throw new Error(`File not found: ${filePath}`); + return Result.err(NotFoundError.create("file", filePath)); } const normalizedPath = normalizePathForOutput(absolutePath); @@ -106,8 +127,10 @@ export async function handleAdd( ...(configPath ? { configPath } : {}), }); if (configResult.isErr()) { - throw new Error( - `Failed to load config: ${configResult.error instanceof Error ? configResult.error.message : String(configResult.error)}` + return Result.err( + InternalError.create( + `Failed to load config: ${configResult.error instanceof Error ? configResult.error.message : String(configResult.error)}` + ) ); } const config = configResult.value; @@ -121,11 +144,15 @@ export async function handleAdd( markerLower === MARKERS.tldr && existingRecords.some((record) => record.type.toLowerCase() === MARKERS.tldr) ) { - throw new Error(`File ${filePath} already contains a tldr waymark.`); + return Result.err( + ValidationError.fromMessage( + `File ${filePath} already contains a tldr waymark.` + ) + ); } const commentStyle = resolveCommentStyle(absolutePath, existingRecords); - const insertion = _addWaymark({ + const insertionResult = _addWaymark({ source: originalSource, type, content: normalizedContent, @@ -135,6 +162,10 @@ export async function handleAdd( ...(signals ? { signals } : {}), markerLower, }); + if (insertionResult.isErr()) { + return Result.err(insertionResult.error); + } + const insertion = insertionResult.value; const formatted = formatText(insertion.text, { file: normalizedPath, @@ -155,17 +186,21 @@ export async function handleAdd( notifyResourceChanged(); - return toJsonResponse({ - filePath: normalizedPath, - type: insertedRecord?.type ?? type, - startLine: insertedRecord?.startLine ?? insertion.lineNumber, - endLine: insertedRecord?.endLine ?? insertion.lineNumber, - content: insertedRecord?.contentText ?? normalizedContent, - signals: insertedRecord?.signals, - }); + return Result.ok( + toJsonResponse({ + filePath: normalizedPath, + type: insertedRecord?.type ?? type, + startLine: insertedRecord?.startLine ?? insertion.lineNumber, + endLine: insertedRecord?.endLine ?? insertion.lineNumber, + content: insertedRecord?.contentText ?? normalizedContent, + signals: insertedRecord?.signals, + }) + ); } -function _addWaymark(params: InsertWaymarkParams): InsertWaymarkResult { +function _addWaymark( + params: InsertWaymarkParams +): Result { const { source, type, @@ -188,7 +223,11 @@ function _addWaymark(params: InsertWaymarkParams): InsertWaymarkResult { const zeroBased = Math.max(0, line - 1); insertIndex = Math.min(zeroBased, lines.length); } else if (markerLower === MARKERS.about) { - throw new Error("line is required when inserting an `about` waymark"); + return Result.err( + ValidationError.fromMessage( + "line is required when inserting an `about` waymark" + ) + ); } const indentString = @@ -213,10 +252,10 @@ function _addWaymark(params: InsertWaymarkParams): InsertWaymarkResult { updatedText += newline; } - return { + return Result.ok({ text: updatedText, lineNumber: insertIndex + 1, - }; + }); } function renderWaymarkLine(params: { @@ -346,60 +385,77 @@ function toJsonResponse(value: unknown): ToolContent { }; } -function normalizeWaymarkId(id: string): string { +function normalizeWaymarkId(id: string): Result { const trimmed = id.trim(); if (trimmed.length === 0) { - throw new Error("Waymark id cannot be empty."); + return Result.err( + ValidationError.fromMessage("Waymark id cannot be empty.") + ); } const lower = trimmed.toLowerCase(); if (lower.startsWith(WM_ID_PREFIX)) { - throw new Error( - "wm: ids are not supported. Use [[hash]] or [[hash|alias]]." + return Result.err( + ValidationError.fromMessage( + "wm: ids are not supported. Use [[hash]] or [[hash|alias]]." + ) ); } if (trimmed.startsWith("[[") && trimmed.endsWith("]]")) { const inner = trimmed.slice(2, -2).trim(); if (inner.length === 0) { - throw new Error(`Invalid waymark id format: ${id}`); + return Result.err( + ValidationError.fromMessage(`Invalid waymark id format: ${id}`) + ); } const normalizedInner = inner.toLowerCase(); if (normalizedInner.startsWith(WM_ID_PREFIX)) { - throw new Error( - "wm: ids are not supported. Use [[hash]] or [[hash|alias]]." + return Result.err( + ValidationError.fromMessage( + "wm: ids are not supported. Use [[hash]] or [[hash|alias]]." + ) ); } - return `[[${normalizedInner}]]`; + return Result.ok(`[[${normalizedInner}]]`); } - return `[[${lower}]]`; + return Result.ok(`[[${lower}]]`); } -function applyIdToContent(content: string, id?: string): string { +function applyIdToContent( + content: string, + id?: string +): Result { const trimmed = content.trim(); if (!id) { - return trimmed; + return Result.ok(trimmed); } const match = trimmed.match(ID_TRAIL_REGEX); const existingId = match?.[1]; if (existingId) { - const normalizedExisting = normalizeWaymarkId(existingId); + const normalizedExistingResult = normalizeWaymarkId(existingId); + if (normalizedExistingResult.isErr()) { + return Result.err(normalizedExistingResult.error); + } + const normalizedExisting = normalizedExistingResult.value; if (normalizedExisting !== id) { - throw new Error( - `Content already contains a different waymark id: ${existingId}` + return Result.err( + ValidationError.fromMessage( + `Content already contains a different waymark id: ${existingId}` + ) ); } const base = trimmed.replace(ID_TRAIL_REGEX, "").trimEnd(); - return base.length > 0 - ? `${base} ${normalizedExisting}` - : normalizedExisting; + return Result.ok( + base.length > 0 ? `${base} ${normalizedExisting}` : normalizedExisting + ); } - return trimmed.length > 0 ? `${trimmed} ${id}` : id; + return Result.ok(trimmed.length > 0 ? `${trimmed} ${id}` : id); } /** * Wrapper to invoke the add tool handler in tests. * @param params - The add waymark parameters including file path, type, content, and server context. - * @returns A promise resolving to the call tool result. + * @returns A promise resolving to Result containing the tool content or an OutfitterError. */ export function handleAddWaymark(params: { filePath: string; @@ -411,6 +467,6 @@ export function handleAddWaymark(params: { configPath?: string | undefined; scope?: ConfigScope | undefined; notifyResourceChanged: () => void; -}): Promise { +}): Promise> { return handleAdd(params, params.notifyResourceChanged); } diff --git a/apps/mcp/src/tools/graph.ts b/apps/mcp/src/tools/graph.ts index eee4b166..9b913610 100644 --- a/apps/mcp/src/tools/graph.ts +++ b/apps/mcp/src/tools/graph.ts @@ -1,5 +1,7 @@ // tldr ::: graph tool handler for waymark MCP server +import type { OutfitterError } from "@outfitter/contracts"; +import { InternalError, Result, ValidationError } from "@outfitter/contracts"; import type { ConfigScope, WaymarkRecord } from "@waymarks/core"; import { buildRelationGraph, parse } from "@waymarks/core"; import type { ToolContent } from "../types"; @@ -15,10 +17,16 @@ import { /** * Handle the graph action for the MCP tool. * @param input - Raw tool input payload. - * @returns MCP tool result with graph output. + * @returns Result containing MCP tool result with graph output, or an OutfitterError. */ -export async function handleGraph(input: unknown): Promise { - const { paths, configPath, scope } = graphInputSchema.parse(input); +export async function handleGraph( + input: unknown +): Promise> { + const parseResult = graphInputSchema.safeParse(input); + if (!parseResult.success) { + return Result.err(ValidationError.fromMessage(parseResult.error.message)); + } + const { paths, configPath, scope } = parseResult.data; const collectOptions: { configPath?: string; scope?: ConfigScope } = {}; if (configPath) { collectOptions.configPath = configPath; @@ -26,18 +34,26 @@ export async function handleGraph(input: unknown): Promise { if (scope) { collectOptions.scope = scope; } - const { records } = await collectRecords(paths, collectOptions); + const collectResult = await collectRecords(paths, collectOptions); + if (collectResult.isErr()) { + return Result.err(collectResult.error); + } + const { records } = collectResult.value; const edges = buildRelationGraph(records).edges; - return toJsonResponse(edges); + return Result.ok(toJsonResponse(edges)); } async function collectRecords( inputs: string[], options: { configPath?: string; scope?: ConfigScope } -): Promise<{ records: WaymarkRecord[] }> { - let filePaths = await expandInputPaths(inputs); +): Promise> { + const expandResult = await expandInputPaths(inputs); + if (expandResult.isErr()) { + return Result.err(expandResult.error); + } + let filePaths = expandResult.value; if (filePaths.length === 0) { - return { records: [] }; + return Result.ok({ records: [] }); } const configResult = await loadConfig({ @@ -45,8 +61,10 @@ async function collectRecords( ...(options.configPath ? { configPath: options.configPath } : {}), }); if (configResult.isErr()) { - throw new Error( - `Failed to load config: ${configResult.error instanceof Error ? configResult.error.message : String(configResult.error)}` + return Result.err( + InternalError.create( + `Failed to load config: ${configResult.error instanceof Error ? configResult.error.message : String(configResult.error)}` + ) ); } const config = configResult.value; @@ -65,7 +83,7 @@ async function collectRecords( }) ); - return { records }; + return Result.ok({ records }); } function toJsonResponse(value: unknown): ToolContent { diff --git a/apps/mcp/src/tools/index.ts b/apps/mcp/src/tools/index.ts index ab1f3860..5fa364e7 100644 --- a/apps/mcp/src/tools/index.ts +++ b/apps/mcp/src/tools/index.ts @@ -1,9 +1,7 @@ // tldr ::: tool registry for waymark MCP server -import { Result } from "@outfitter/contracts"; import type { McpServer } from "@outfitter/mcp"; import { defineTool } from "@outfitter/mcp"; -import type { ToolContent } from "../types"; import { waymarkToolInputSchema } from "../types"; import { handleWaymarkTool, waymarkToolDescription } from "./waymark"; @@ -21,10 +19,7 @@ export function registerTools( name: "waymark", description: waymarkToolDescription, inputSchema: waymarkToolInputSchema, - handler: async (input, _ctx) => { - const result = await handleWaymarkTool(input, notifyResourceChanged); - return Result.ok(result) as Result; - }, + handler: (input, _ctx) => handleWaymarkTool(input, notifyResourceChanged), }) ); } diff --git a/apps/mcp/src/tools/scan.ts b/apps/mcp/src/tools/scan.ts index 64f91566..0fd6b9b3 100644 --- a/apps/mcp/src/tools/scan.ts +++ b/apps/mcp/src/tools/scan.ts @@ -1,5 +1,7 @@ // tldr ::: scan tool handler for waymark MCP server +import type { OutfitterError } from "@outfitter/contracts"; +import { InternalError, Result, ValidationError } from "@outfitter/contracts"; import type { ConfigScope, WaymarkRecord } from "@waymarks/core"; import { parse } from "@waymarks/core"; import type { RenderFormat, ToolContent } from "../types"; @@ -15,10 +17,16 @@ import { /** * Handle the scan action for the MCP tool. * @param input - Raw tool input payload. - * @returns MCP tool result with scan output. + * @returns Result containing MCP tool result with scan output, or an OutfitterError. */ -export async function handleScan(input: unknown): Promise { - const { paths, format, configPath, scope } = scanInputSchema.parse(input); +export async function handleScan( + input: unknown +): Promise> { + const parseResult = scanInputSchema.safeParse(input); + if (!parseResult.success) { + return Result.err(ValidationError.fromMessage(parseResult.error.message)); + } + const { paths, format, configPath, scope } = parseResult.data; const collectOptions: { configPath?: string; scope?: ConfigScope } = {}; if (configPath) { collectOptions.configPath = configPath; @@ -26,18 +34,26 @@ export async function handleScan(input: unknown): Promise { if (scope) { collectOptions.scope = scope; } - const { records } = await collectRecords(paths, collectOptions); + const collectResult = await collectRecords(paths, collectOptions); + if (collectResult.isErr()) { + return Result.err(collectResult.error); + } + const { records } = collectResult.value; const rendered = renderRecords(records, format); - return toTextResponse(rendered, mimeForFormat(format)); + return Result.ok(toTextResponse(rendered, mimeForFormat(format))); } async function collectRecords( inputs: string[], options: { configPath?: string; scope?: ConfigScope } -): Promise<{ records: WaymarkRecord[] }> { - let filePaths = await expandInputPaths(inputs); +): Promise> { + const expandResult = await expandInputPaths(inputs); + if (expandResult.isErr()) { + return Result.err(expandResult.error); + } + let filePaths = expandResult.value; if (filePaths.length === 0) { - return { records: [] }; + return Result.ok({ records: [] }); } const configResult = await loadConfig({ @@ -45,8 +61,10 @@ async function collectRecords( ...(options.configPath ? { configPath: options.configPath } : {}), }); if (configResult.isErr()) { - throw new Error( - `Failed to load config: ${configResult.error instanceof Error ? configResult.error.message : String(configResult.error)}` + return Result.err( + InternalError.create( + `Failed to load config: ${configResult.error instanceof Error ? configResult.error.message : String(configResult.error)}` + ) ); } const config = configResult.value; @@ -65,7 +83,7 @@ async function collectRecords( }) ); - return { records }; + return Result.ok({ records }); } function renderRecords(records: WaymarkRecord[], format: RenderFormat): string { diff --git a/apps/mcp/src/tools/waymark.test.ts b/apps/mcp/src/tools/waymark.test.ts index 47e76b39..e632bdf5 100644 --- a/apps/mcp/src/tools/waymark.test.ts +++ b/apps/mcp/src/tools/waymark.test.ts @@ -11,21 +11,25 @@ const noopNotify = () => { describe("handleWaymarkTool", () => { test("returns help text", async () => { - const response = await handleWaymarkTool( + const result = await handleWaymarkTool( { action: "help" } as WaymarkToolInput, noopNotify ); - const text = String(response.content?.[0]?.text ?? ""); + expect(result.isOk()).toBe(true); + const response = result.isOk() ? result.value : null; + const text = String(response?.content?.[0]?.text ?? ""); expect(text).toContain("Waymark MCP Tool"); expect(text).toContain("scan"); }); test("returns topic-specific help", async () => { - const response = await handleWaymarkTool( + const result = await handleWaymarkTool( { action: "help", topic: "scan" } as WaymarkToolInput, noopNotify ); - const text = String(response.content?.[0]?.text ?? ""); + expect(result.isOk()).toBe(true); + const response = result.isOk() ? result.value : null; + const text = String(response?.content?.[0]?.text ?? ""); expect(text).toContain("Action: scan"); }); }); diff --git a/apps/mcp/src/tools/waymark.ts b/apps/mcp/src/tools/waymark.ts index 551cc828..080a1d8e 100644 --- a/apps/mcp/src/tools/waymark.ts +++ b/apps/mcp/src/tools/waymark.ts @@ -1,5 +1,7 @@ // tldr ::: single MCP tool handler for waymark actions +import type { OutfitterError } from "@outfitter/contracts"; +import { Result } from "@outfitter/contracts"; import type { ToolContent, WaymarkToolInput } from "../types"; import { handleAdd } from "./add"; import { handleGraph } from "./graph"; @@ -11,12 +13,12 @@ import { handleScan } from "./scan"; * Input is pre-validated by the framework's discriminatedUnion schema. * @param input - Validated tool input payload. * @param notifyResourceChanged - Callback to signal resource list changed. - * @returns MCP tool result promise. + * @returns Result containing MCP tool result, or an OutfitterError. */ export function handleWaymarkTool( input: WaymarkToolInput, notifyResourceChanged: () => void -): Promise { +): Promise> { switch (input.action) { case "scan": return handleScan(input); @@ -25,7 +27,7 @@ export function handleWaymarkTool( case "add": return handleAdd(input, notifyResourceChanged); case "help": - return Promise.resolve(handleHelp(input)); + return Promise.resolve(Result.ok(handleHelp(input))); default: // Exhaustive: input.action is validated by the framework's discriminatedUnion schema return input satisfies never; diff --git a/apps/mcp/src/utils/filesystem.ts b/apps/mcp/src/utils/filesystem.ts index 64f34a23..a8d1f72f 100644 --- a/apps/mcp/src/utils/filesystem.ts +++ b/apps/mcp/src/utils/filesystem.ts @@ -3,6 +3,7 @@ import { existsSync, realpathSync } from "node:fs"; import { lstat, readdir, realpath, stat } from "node:fs/promises"; import { join, relative, resolve } from "node:path"; +import { Result, ValidationError } from "@outfitter/contracts"; import { Glob } from "bun"; const SKIP_DIRECTORY_NAMES = new Set([ @@ -34,15 +35,16 @@ function getWorkspaceBounds(): WorkspaceBounds { /** * Expands user-supplied paths into a normalized list of files while enforcing * that every resolved path (including symlink targets) remains inside the - * current workspace. Inputs that escape the workspace throw an error. + * current workspace. Inputs that escape the workspace produce a ValidationError. * * @param inputs - Relative or absolute paths provided to the MCP server - * @returns A de-duplicated array of files under the workspace root - * @throws Error when an input resolves outside the workspace + * @returns Result containing a de-duplicated array of files under the workspace root */ -export async function expandInputPaths(inputs: string[]): Promise { +export async function expandInputPaths( + inputs: string[] +): Promise> { if (inputs.length === 0) { - return []; + return Result.ok([]); } const bounds = getWorkspaceBounds(); @@ -52,17 +54,27 @@ export async function expandInputPaths(inputs: string[]): Promise { for (const input of inputs) { const resolved = resolve(bounds.root, input); if (escapesWorkspace(resolved, bounds)) { - throw new Error( - `Input "${input}" resolves outside workspace: ${resolved}` + return Result.err( + ValidationError.fromMessage( + `Input "${input}" resolves outside workspace: ${resolved}` + ) ); } if (!existsSync(resolved)) { continue; } - await collectFilesRecursive(resolved, files, bounds, visited); + const collectResult = await collectFilesRecursive( + resolved, + files, + bounds, + visited + ); + if (collectResult.isErr()) { + return Result.err(collectResult.error); + } } - return Array.from(files); + return Result.ok(Array.from(files)); } /** @@ -73,52 +85,75 @@ export async function expandInputPaths(inputs: string[]): Promise { * @param files - Mutable set that accumulates normalized file paths * @param bounds - Workspace boundary information * @param visited - Set of resolved paths already processed to avoid cycles + * @returns Result representing success or a ValidationError for workspace escapes */ export async function collectFilesRecursive( path: string, files: Set, bounds: WorkspaceBounds, visited: Set -): Promise { - const resolution = await resolveWithinWorkspace(path, bounds); +): Promise> { + const resolutionResult = await resolveWithinWorkspace(path, bounds); + if (resolutionResult.isErr()) { + return Result.err(resolutionResult.error); + } + + const resolution = resolutionResult.value; if (!resolution) { - return; + return Result.ok(undefined); } const { targetPath, stats } = resolution; if (visited.has(targetPath)) { - return; + return Result.ok(undefined); } visited.add(targetPath); if (stats.isFile()) { files.add(normalizePathForOutput(targetPath)); - return; + return Result.ok(undefined); } if (!stats.isDirectory() || shouldSkipDirectory(targetPath)) { - return; + return Result.ok(undefined); } const entries = await readdir(targetPath, { withFileTypes: true }); - await Promise.all( - entries.map(async (entry) => { + const results = await Promise.all( + entries.map(async (entry): Promise> => { const child = join(targetPath, entry.name); if (entry.isDirectory() || entry.isSymbolicLink()) { if (SKIP_DIRECTORY_NAMES.has(entry.name)) { - return; + return Result.ok(undefined); + } + return collectFilesRecursive(child, files, bounds, visited); + } + if (entry.isFile()) { + const fileResolutionResult = await resolveWithinWorkspace( + child, + bounds + ); + if (fileResolutionResult.isErr()) { + return Result.err(fileResolutionResult.error); } - await collectFilesRecursive(child, files, bounds, visited); - } else if (entry.isFile()) { - const fileResolution = await resolveWithinWorkspace(child, bounds); + const fileResolution = fileResolutionResult.value; if (!fileResolution) { - return; + return Result.ok(undefined); } files.add(normalizePathForOutput(fileResolution.targetPath)); + return Result.ok(undefined); } + return Result.ok(undefined); }) ); + + for (const r of results) { + if (r.isErr()) { + return Result.err(r.error); + } + } + return Result.ok(undefined); } type PathResolution = { @@ -132,13 +167,12 @@ type PathResolution = { * * @param path - The path to resolve * @param bounds - Workspace traversal limits - * @returns The resolved target path with filesystem stats - * @throws Error when the path resolves outside the workspace + * @returns Result containing the resolved target path with filesystem stats, or null if not found */ async function resolveWithinWorkspace( path: string, bounds: WorkspaceBounds -): Promise { +): Promise> { const lstatInfo = await lstat(path).catch((error) => { if (isEnoent(error)) { return null; @@ -147,16 +181,20 @@ async function resolveWithinWorkspace( }); if (!lstatInfo) { - return null; + return Result.ok(null); } // Check the nominal path first so absolute inputs outside the workspace fail. if (escapesWorkspace(path, bounds)) { - throw new Error(`Input "${path}" resolves outside workspace: ${path}`); + return Result.err( + ValidationError.fromMessage( + `Input "${path}" resolves outside workspace: ${path}` + ) + ); } if (!lstatInfo.isSymbolicLink()) { - return { targetPath: path, stats: lstatInfo }; + return Result.ok({ targetPath: path, stats: lstatInfo }); } const realPath = await realpath(path).catch((error) => { @@ -167,11 +205,15 @@ async function resolveWithinWorkspace( }); if (!realPath) { - return { targetPath: path, stats: lstatInfo }; + return Result.ok({ targetPath: path, stats: lstatInfo }); } if (escapesWorkspace(realPath, bounds)) { - throw new Error(`Input "${path}" resolves outside workspace: ${realPath}`); + return Result.err( + ValidationError.fromMessage( + `Input "${path}" resolves outside workspace: ${realPath}` + ) + ); } const targetStats = await stat(realPath).catch((error) => { @@ -181,7 +223,7 @@ async function resolveWithinWorkspace( throw error; }); - return { targetPath: realPath, stats: targetStats }; + return Result.ok({ targetPath: realPath, stats: targetStats }); } /**