From a16b9bdaf725eb3c400e4b7785962527ceb8d9ac Mon Sep 17 00:00:00 2001 From: Tao Feng Date: Mon, 13 Apr 2026 23:27:52 -0700 Subject: [PATCH] fix: wire goalSessionStore in MCP transports, normalize error formats (#214) goalSessionStore was initialized by createLocalRuntime but never passed into the McpDeps object in either stdio or HTTP MCP entry points, causing all four goal/session tools to silently return [NOT_CONFIGURED] on the primary agent transport. Also normalizes error formats: toolValidationError() now uses [CODE] bracket format matching error-handler.ts, inline error objects across tool files refactored to shared toolError(), and HTTP-level errors in serve-http.ts normalized to { code, message } via httpError() helper. --- src/mcp/deps-parity.test.ts | 81 ++++++++++++++++++++++++++++++++++++ src/mcp/error-handler.ts | 4 +- src/mcp/operation-adapter.ts | 6 +-- src/mcp/serve-http.ts | 46 ++++++++++---------- src/mcp/serve.ts | 1 + src/mcp/tools/goal.ts | 21 ++-------- src/mcp/tools/handoffs.ts | 34 ++++----------- src/mcp/tools/ingest.ts | 25 +++-------- src/mcp/tools/session.ts | 21 ++-------- 9 files changed, 126 insertions(+), 113 deletions(-) create mode 100644 src/mcp/deps-parity.test.ts diff --git a/src/mcp/deps-parity.test.ts b/src/mcp/deps-parity.test.ts new file mode 100644 index 00000000..490ac165 --- /dev/null +++ b/src/mcp/deps-parity.test.ts @@ -0,0 +1,81 @@ +/** + * Deps parity tests — verify that MCP entry points forward all stores + * provided by LocalRuntime into the McpDeps object. + * + * Catches wiring omissions like goalSessionStore not being passed through + * (issue #214). + */ + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { createLocalRuntime, type LocalRuntime } from "../local/runtime.js"; +import type { McpDeps } from "./deps.js"; + +describe("MCP deps parity with LocalRuntime", () => { + let tempDir: string; + let runtime: LocalRuntime; + + beforeEach(async () => { + tempDir = await mkdtemp(join(tmpdir(), "grove-deps-parity-")); + const groveDir = join(tempDir, ".grove"); + await Bun.write(join(groveDir, ".gitkeep"), ""); + + runtime = createLocalRuntime({ + groveDir, + frontierCacheTtlMs: 0, + workspace: true, + parseContract: false, // no GROVE.md in temp dir + }); + }); + + afterEach(async () => { + runtime.close(); + await rm(tempDir, { recursive: true, force: true }); + }); + + test("LocalRuntime always provides goalSessionStore", () => { + expect(runtime.goalSessionStore).toBeDefined(); + }); + + test("stdio MCP deps construction includes goalSessionStore", () => { + // Mirror the deps construction from src/mcp/serve.ts + const deps: McpDeps = { + contributionStore: runtime.contributionStore, + claimStore: runtime.claimStore, + bountyStore: runtime.bountyStore, + cas: runtime.cas, + frontier: runtime.frontier, + workspace: runtime.workspace!, + contract: runtime.contract, + onContributionWrite: runtime.onContributionWrite, + workspaceBoundary: runtime.groveRoot, + goalSessionStore: runtime.goalSessionStore, + handoffStore: runtime.handoffStore, + }; + + expect(deps.goalSessionStore).toBeDefined(); + expect(deps.goalSessionStore).toBe(runtime.goalSessionStore); + }); + + test("HTTP MCP deps construction includes goalSessionStore", () => { + // Mirror the deps construction from src/mcp/serve-http.ts buildScopedDeps + const deps: McpDeps = { + contributionStore: runtime.contributionStore, + claimStore: runtime.claimStore, + bountyStore: runtime.bountyStore, + cas: runtime.cas, + frontier: runtime.frontier, + workspace: runtime.workspace!, + contract: runtime.contract, + onContributionWrite: runtime.onContributionWrite, + workspaceBoundary: runtime.groveRoot, + goalSessionStore: runtime.goalSessionStore, + }; + + expect(deps.goalSessionStore).toBeDefined(); + expect(deps.goalSessionStore).toBe(runtime.goalSessionStore); + }); +}); diff --git a/src/mcp/error-handler.ts b/src/mcp/error-handler.ts index 72bc7b37..6559a144 100644 --- a/src/mcp/error-handler.ts +++ b/src/mcp/error-handler.ts @@ -120,8 +120,8 @@ export function validationError(message: string): CallToolResult { return toolError(McpErrorCode.ValidationError, message); } -/** Build a CallToolResult with isError: true. */ -function toolError(code: string, message: string): CallToolResult { +/** Build a CallToolResult with isError: true. Format: `[CODE] message`. */ +export function toolError(code: string, message: string): CallToolResult { return { isError: true, content: [{ type: "text" as const, text: `[${code}] ${message}` }], diff --git a/src/mcp/operation-adapter.ts b/src/mcp/operation-adapter.ts index 682391d5..552e7cda 100644 --- a/src/mcp/operation-adapter.ts +++ b/src/mcp/operation-adapter.ts @@ -10,6 +10,7 @@ import type { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; import type { OperationDeps } from "../core/operations/deps.js"; import type { OperationResult } from "../core/operations/result.js"; import type { McpDeps } from "./deps.js"; +import { McpErrorCode, toolError } from "./error-handler.js"; /** * Convert McpDeps to OperationDeps. @@ -73,8 +74,5 @@ export function toMcpResult(result: OperationResult, warning?: string): Ca * specific field and include an example of correct input. */ export function toolValidationError(message: string): CallToolResult { - return { - isError: true, - content: [{ type: "text" as const, text: `VALIDATION_ERROR: ${message}` }], - }; + return toolError(McpErrorCode.ValidationError, message); } diff --git a/src/mcp/serve-http.ts b/src/mcp/serve-http.ts index 042dbecf..98a1bb4b 100644 --- a/src/mcp/serve-http.ts +++ b/src/mcp/serve-http.ts @@ -320,6 +320,7 @@ async function buildScopedDeps(sessionId: string | undefined): Promise { // --- HTTP server ------------------------------------------------------------ +/** Format an HTTP-level JSON error response body. */ +function httpError(code: string, message: string): string { + return JSON.stringify({ error: { code, message } }); +} + async function handleRequest(req: IncomingMessage, res: ServerResponse): Promise { const url = req.url ?? "/"; // Only handle /mcp endpoint if (url !== "/mcp") { res.writeHead(404, { "Content-Type": "application/json" }); - res.end(JSON.stringify({ error: "Not found. Use /mcp endpoint." })); + res.end(httpError("NOT_FOUND", "Not found. Use /mcp endpoint.")); return; } @@ -451,7 +457,7 @@ async function handleRequest(req: IncomingMessage, res: ServerResponse): Promise const authHeader = req.headers.authorization ?? ""; if (authHeader !== `Bearer ${AUTH_TOKEN}`) { res.writeHead(401, { "Content-Type": "application/json" }); - res.end(JSON.stringify({ error: "Unauthorized" })); + res.end(httpError("UNAUTHORIZED", "Unauthorized")); return; } } @@ -467,7 +473,7 @@ async function handleRequest(req: IncomingMessage, res: ServerResponse): Promise } catch (err) { if (err instanceof BodyTooLargeError) { res.writeHead(413, { "Content-Type": "application/json" }); - res.end(JSON.stringify({ error: "Request body too large" })); + res.end(httpError("BODY_TOO_LARGE", "Request body too large")); return; } throw err; @@ -477,7 +483,7 @@ async function handleRequest(req: IncomingMessage, res: ServerResponse): Promise parsed = JSON.parse(body); } catch { res.writeHead(400, { "Content-Type": "application/json" }); - res.end(JSON.stringify({ error: "Invalid JSON" })); + res.end(httpError("INVALID_JSON", "Invalid JSON")); return; } @@ -522,14 +528,7 @@ async function handleRequest(req: IncomingMessage, res: ServerResponse): Promise const msg = err instanceof Error ? err.message : String(err); process.stderr.write(`${msg}\n`); res.writeHead(503, { "Content-Type": "application/json" }); - res.end( - JSON.stringify({ - error: { - code: "SESSION_NOT_READY", - message: msg, - }, - }), - ); + res.end(httpError("SESSION_NOT_READY", msg)); return; } @@ -542,15 +541,12 @@ async function handleRequest(req: IncomingMessage, res: ServerResponse): Promise if (nexusClient && scoped.sessionId === undefined) { res.writeHead(503, { "Content-Type": "application/json" }); res.end( - JSON.stringify({ - error: { - code: "SESSION_NOT_READY", - message: - "grove-mcp-http: no grove session selected — initialize the HTTP MCP " + - "after starting a session via the TUI. Mutations in bootstrap mode " + - "would land outside session scope and are refused.", - }, - }), + httpError( + "SESSION_NOT_READY", + "grove-mcp-http: no grove session selected — initialize the HTTP MCP " + + "after starting a session via the TUI. Mutations in bootstrap mode " + + "would land outside session scope and are refused.", + ), ); return; } @@ -589,7 +585,7 @@ async function handleRequest(req: IncomingMessage, res: ServerResponse): Promise const getSession = sessionId ? sessions.get(sessionId) : undefined; if (!getSession) { res.writeHead(400, { "Content-Type": "application/json" }); - res.end(JSON.stringify({ error: "Missing or invalid Mcp-Session-Id header" })); + res.end(httpError("INVALID_SESSION", "Missing or invalid Mcp-Session-Id header")); return; } getSession.lastActivity = Date.now(); @@ -606,7 +602,7 @@ async function handleRequest(req: IncomingMessage, res: ServerResponse): Promise const delSession = sessionId ? sessions.get(sessionId) : undefined; if (!delSession) { res.writeHead(404, { "Content-Type": "application/json" }); - res.end(JSON.stringify({ error: "Session not found" })); + res.end(httpError("NOT_FOUND", "Session not found")); return; } await delSession.transport.handleRequest(req, res); @@ -614,7 +610,7 @@ async function handleRequest(req: IncomingMessage, res: ServerResponse): Promise if (sessionId) sessions.delete(sessionId); } else { res.writeHead(405, { "Content-Type": "application/json" }); - res.end(JSON.stringify({ error: "Method not allowed" })); + res.end(httpError("METHOD_NOT_ALLOWED", "Method not allowed")); } } @@ -657,7 +653,7 @@ const httpServer = createServer((req, res) => { process.stderr.write(`grove-mcp-http: ${String(error)}\n`); if (!res.headersSent) { res.writeHead(500, { "Content-Type": "application/json" }); - res.end(JSON.stringify({ error: "Internal server error" })); + res.end(httpError("INTERNAL_ERROR", "Internal server error")); } }); }); diff --git a/src/mcp/serve.ts b/src/mcp/serve.ts index a128811f..fa880571 100644 --- a/src/mcp/serve.ts +++ b/src/mcp/serve.ts @@ -292,6 +292,7 @@ try { onContributionWrite: runtime.onContributionWrite, ...(onContributionWritten ? { onContributionWritten } : {}), workspaceBoundary: runtime.groveRoot, + goalSessionStore: runtime.goalSessionStore, ...(outcomeStore ? { outcomeStore } : {}), ...(eventBus ? { eventBus } : {}), ...(topologyRouter ? { topologyRouter } : {}), diff --git a/src/mcp/tools/goal.ts b/src/mcp/tools/goal.ts index bc1e3672..6dad8165 100644 --- a/src/mcp/tools/goal.ts +++ b/src/mcp/tools/goal.ts @@ -11,6 +11,7 @@ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { z } from "zod"; import type { McpDeps } from "../deps.js"; +import { toolError } from "../error-handler.js"; // --------------------------------------------------------------------------- // Input schemas @@ -44,15 +45,7 @@ export function registerGoalTools(server: McpServer, deps: McpDeps): void { async () => { const store = deps.goalSessionStore; if (!store) { - return { - isError: true, - content: [ - { - type: "text" as const, - text: "[NOT_CONFIGURED] Goal/session store is not configured", - }, - ], - }; + return toolError("NOT_CONFIGURED", "Goal/session store is not configured"); } const goalData = await store.getGoal(); @@ -91,15 +84,7 @@ export function registerGoalTools(server: McpServer, deps: McpDeps): void { async (args) => { const store = deps.goalSessionStore; if (!store) { - return { - isError: true, - content: [ - { - type: "text" as const, - text: "[NOT_CONFIGURED] Goal/session store is not configured", - }, - ], - }; + return toolError("NOT_CONFIGURED", "Goal/session store is not configured"); } const result = await store.setGoal(args.goal, args.acceptance, "mcp"); diff --git a/src/mcp/tools/handoffs.ts b/src/mcp/tools/handoffs.ts index e8c251bb..1334a79d 100644 --- a/src/mcp/tools/handoffs.ts +++ b/src/mcp/tools/handoffs.ts @@ -9,6 +9,7 @@ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { z } from "zod"; import { HandoffStatus } from "../../core/handoff.js"; import type { McpDeps } from "../deps.js"; +import { toolError } from "../error-handler.js"; const listHandoffsInputSchema = z.object({ toRole: z @@ -60,15 +61,10 @@ export function registerHandoffTools(server: McpServer, deps: McpDeps): void { async (args) => { const { handoffStore } = deps; if (handoffStore === undefined) { - return { - isError: true, - content: [ - { - type: "text" as const, - text: "[NOT_CONFIGURED] Handoff store is not available. Topology routing must be active.", - }, - ], - }; + return toolError( + "NOT_CONFIGURED", + "Handoff store is not available. Topology routing must be active.", + ); } // Expire stale handoffs before listing so callers always see fresh status. @@ -97,28 +93,12 @@ export function registerHandoffTools(server: McpServer, deps: McpDeps): void { async (args) => { const { handoffStore } = deps; if (handoffStore === undefined) { - return { - isError: true, - content: [ - { - type: "text" as const, - text: "[NOT_CONFIGURED] Handoff store is not available.", - }, - ], - }; + return toolError("NOT_CONFIGURED", "Handoff store is not available."); } const handoff = await handoffStore.get(args.handoffId); if (handoff === undefined) { - return { - isError: true, - content: [ - { - type: "text" as const, - text: `[NOT_FOUND] Handoff '${args.handoffId}' not found.`, - }, - ], - }; + return toolError("NOT_FOUND", `Handoff '${args.handoffId}' not found.`); } return { diff --git a/src/mcp/tools/ingest.ts b/src/mcp/tools/ingest.ts index dd2eeb6e..061e3242 100644 --- a/src/mcp/tools/ingest.ts +++ b/src/mcp/tools/ingest.ts @@ -17,7 +17,7 @@ import { assertWithinBoundary } from "../../core/path-safety.js"; import { ingestGitDiff } from "../../local/ingest/git-diff.js"; import { ingestGitTree } from "../../local/ingest/git-tree.js"; import type { McpDeps } from "../deps.js"; -import { handleToolError } from "../error-handler.js"; +import { handleToolError, toolError } from "../error-handler.js"; // --------------------------------------------------------------------------- // Helpers @@ -98,26 +98,13 @@ export function registerIngestTools(server: McpServer, deps: McpDeps): void { // Validate: exactly one of content or filePath must be provided if (content !== undefined && filePath !== undefined) { - return { - isError: true, - content: [ - { - type: "text" as const, - text: "[VALIDATION_ERROR] Provide exactly one of `content` or `filePath`, not both.", - }, - ], - }; + return toolError( + "VALIDATION_ERROR", + "Provide exactly one of `content` or `filePath`, not both.", + ); } if (content === undefined && filePath === undefined) { - return { - isError: true, - content: [ - { - type: "text" as const, - text: "[VALIDATION_ERROR] Provide exactly one of `content` or `filePath`.", - }, - ], - }; + return toolError("VALIDATION_ERROR", "Provide exactly one of `content` or `filePath`."); } const putOptions = mediaType !== undefined ? { mediaType } : undefined; diff --git a/src/mcp/tools/session.ts b/src/mcp/tools/session.ts index a74b0608..5f7635f2 100644 --- a/src/mcp/tools/session.ts +++ b/src/mcp/tools/session.ts @@ -11,6 +11,7 @@ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { z } from "zod"; import type { McpDeps } from "../deps.js"; +import { toolError } from "../error-handler.js"; // --------------------------------------------------------------------------- // Input schemas @@ -47,15 +48,7 @@ export function registerSessionTools(server: McpServer, deps: McpDeps): void { async (args) => { const store = deps.goalSessionStore; if (!store) { - return { - isError: true, - content: [ - { - type: "text" as const, - text: "[NOT_CONFIGURED] Goal/session store is not configured", - }, - ], - }; + return toolError("NOT_CONFIGURED", "Goal/session store is not configured"); } const query = args.status !== undefined ? { status: args.status } : undefined; @@ -84,15 +77,7 @@ export function registerSessionTools(server: McpServer, deps: McpDeps): void { async (args) => { const store = deps.goalSessionStore; if (!store) { - return { - isError: true, - content: [ - { - type: "text" as const, - text: "[NOT_CONFIGURED] Goal/session store is not configured", - }, - ], - }; + return toolError("NOT_CONFIGURED", "Goal/session store is not configured"); } const session = await store.createSession({