Skip to content

refactor(mcp): convert handlers to Result-based error handling #282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
galligan wants to merge 1 commit into
base: outfitter/v2-cli-handlers
Choose a base branch
from
Open

refactor(mcp): convert handlers to Result-based error handling #282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
125 changes: 71 additions & 54 deletions apps/mcp/src/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand All @@ -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;
Expand Down Expand Up @@ -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",
Expand All @@ -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;
Expand All @@ -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(
Expand All @@ -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");
Expand All @@ -155,15 +161,17 @@ describe("handleAddWaymark", () => {
);

const tracker = createNotifyTracker();
const response = await handleAddWaymark({
const result = await handleAddWaymark({
notifyResourceChanged: tracker.notify,
filePath: file,
type: "idea",
content: "capture ideas for follow-up",
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;
};
Expand All @@ -187,7 +195,7 @@ describe("handleAddWaymark", () => {
);

const tracker = createNotifyTracker();
await handleAddWaymark({
const result = await handleAddWaymark({
notifyResourceChanged: tracker.notify,
filePath: file,
type: "todo",
Expand All @@ -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]]");
Expand All @@ -213,7 +222,7 @@ describe("handleAddWaymark", () => {
);

const tracker = createNotifyTracker();
await handleAddWaymark({
const result = await handleAddWaymark({
notifyResourceChanged: tracker.notify,
filePath: file,
type: "todo",
Expand All @@ -222,14 +231,15 @@ 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]]");

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(
Expand All @@ -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 });
});
Expand All @@ -263,7 +275,7 @@ describe("handleAddWaymark", () => {
);

const tracker = createNotifyTracker();
await handleAddWaymark({
const result = await handleAddWaymark({
notifyResourceChanged: tracker.notify,
filePath: file,
type: "todo",
Expand All @@ -272,14 +284,15 @@ 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]]");

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(
Expand All @@ -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 });
});
Expand Down
8 changes: 6 additions & 2 deletions apps/mcp/src/resources/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}`);
});
}
12 changes: 8 additions & 4 deletions apps/mcp/src/resources/todos.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down
Loading