fix(mcp): classify tool failures, hide write tools from read-only keys, agents send feedback

apps/ai/src/mcp/app.test.ts

@@ -1,5 +1,6 @@
 import { afterEach, describe, expect, it } from "@effect/vitest"
 import { OrgId, UserId } from "@maple/domain/http"
+import { INVESTIGATION_READ_FAMILIES } from "@maple/domain/http/v2"
 import {
 	ConfigProvider,
 	Context,
@@ -661,6 +662,77 @@ describe("MCP HTTP authorization", () => {
 	})
 })
 
+describe("MCP tools/list per credential", () => {
+	it("hides write tools from a read-only key, in the same isolate that shows them to a full key", async () => {
+		const db = createTestDb(createdDbs)
+		const base = Layer.mergeAll(db.layer, Env.layer.pipe(Layer.provide(testConfig())))
+		const services = Layer.mergeAll(
+			ApiKeysService.layer,
+			ErrorActorsService.layer,
+			AuthService.layer,
+			makeMcpToolExecutorStubLayer(),
+			makeRateLimiterStubLayer(),
+		).pipe(Layer.provideMerge(base))
+		const orgId = Schema.decodeSync(OrgId)("org_test")
+		const userId = Schema.decodeSync(UserId)("user_test")
+		const [full, readOnly] = await Effect.runPromise(
+			Effect.gen(function* () {
+				const apiKeys = yield* ApiKeysService
+				return [
+					yield* apiKeys.create(orgId, userId, { name: "Full key", kind: "mcp" }),
+					yield* apiKeys.create(orgId, userId, {
+						name: "Read-only key",
+						scopes: Arr.map(INVESTIGATION_READ_FAMILIES, (family) => `${family}:read`),
+					}),
+				] as const
+			}).pipe(Effect.provide(services)),
+		)
+		const { handler, dispose } = HttpRouter.toWebHandler(McpLive.pipe(Layer.provideMerge(services)), {
+			disableLogger: true,
+		})
+		const listFor = async (secret: string) => {
+			const response = await handler(
+				new Request("https://api.example.com/mcp", {
+					method: "POST",
+					headers: {
+						authorization: `Bearer ${secret}`,
+						accept: "application/json, text/event-stream",
+						"content-type": "application/json",
+						host: "api.example.com",
+						"x-forwarded-proto": "https",
+						"mcp-protocol-version": "2025-06-18",
+					},
+					body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "tools/list" }),
+				}),
+				Context.empty(),
+			)
+			const names = Arr.map((await readBody(response)).result?.tools ?? [], (tool) => tool.name)
+			return {
+				status: response.status,
+				write: names.includes("create_alert_rule"),
+				read: names.includes("inspect_trace"),
+				feedback: names.includes("send_maple_feedback"),
+			}
+		}
+		try {
+			expect(await listFor(full.secret)).toEqual({
+				status: 200,
+				write: true,
+				read: true,
+				feedback: true,
+			})
+			expect(await listFor(readOnly.secret)).toEqual({
+				status: 200,
+				write: false,
+				read: true,
+				feedback: true,
+			})
+		} finally {
+			await dispose()
+		}
+	})
+})
+
 const ToolCallBody = Schema.Struct({
 	id: Schema.Number,
 	result: Schema.optionalKey(

apps/ai/src/mcp/dispatcher.test.ts

@@ -237,6 +237,39 @@ describe("MCP dispatcher", () => {
 			)
 			expect(error._tag).toBe("@maple/mcp/ToolNotFoundError")
 			expect(error.toolName).toBe("not_a_maple_tool")
+			expect(error.message).toContain("tools/list")
+		}),
+	)
+
+	it.effect("names the tools an unknown name most likely meant", () =>
+		Effect.gen(function* () {
+			const executor = yield* makeValidationExecutor
+			const error = yield* Effect.flip(
+				executor.execute(TENANT, "uptime_monitors", {}, "mcp") as Effect.Effect<
+					never,
+					McpToolNotFoundError
+				>,
+			)
+			expect(error.message).toContain("Did you mean `list_uptime_monitors`")
+		}),
+	)
+
+	// On /mcp the visibility gate hides these tools first; this is the registry's own refusal.
+	it.effect("answers a read-only key's write call as a permission problem, not a bug", () =>
+		Effect.gen(function* () {
+			const executor = yield* makeValidationExecutor
+			const result = yield* executor.execute(
+				{ ...TENANT, readOnly: true },
+				"create_alert_rule",
+				{},
+				"mcp",
+			)
+			expect({
+				isError: result.isError,
+				category: result.failureCategory,
+				lead: Str.startsWith("Permission denied:")(result.content[0]?.text ?? ""),
+				bugHint: Str.includes("send_maple_feedback")(result.content[0]?.text ?? ""),
+			}).toEqual({ isError: true, category: "permission", lead: true, bugHint: false })
 		}),
 	)
 
@@ -410,6 +443,8 @@ describe("MCP dispatcher", () => {
 				if (dispatchSpan.status._tag === "Ended")
 					expect(dispatchSpan.status.exit._tag).toBe("Failure")
 				expect(dispatchSpan.attributes.get("maple.mcp.error.category")).toBe("query")
+				expect(dispatchSpan.attributes.get("maple.mcp.error.message")).toBe("boom")
+				expect(result.content[0]?.text).toContain("report it now with `send_maple_feedback`")
 			}),
 		)
 

apps/ai/src/mcp/dispatcher.ts

@@ -54,7 +54,7 @@ import * as Cloudflare from "@alchemy.run/cloudflare"
 // BOUNDARY: This module owns unparsed external values and narrows them before domain use.
 import type { McpToolNotFoundError } from "@maple/domain/mcp-tool-contract"
 import type { McpToolSurface } from "@maple/domain/mcp-manifest"
-import { Context, Effect, Layer, Array as Arr } from "effect"
+import { Context, Effect, Layer, Array as Arr, String as Str } from "effect"
 import { FetchHttpClient } from "effect/http"
 import type { McpToolResult } from "./tools/types"
 import type { McpToolRuntimeRequirements } from "./tools/runtime-requirements"
@@ -76,12 +76,13 @@ const failureResult = (text: string, category: string): McpToolResult => ({
 
 /**
  * Appended to failures that are likely Maple's fault, on the public transport only: an outside
- * agent has the motive and the details to report it right when the call fails. Maple's own
+ * agent has the motive and the details to report it right when the call fails. "Offer the user"
+ * was read literally and no report was ever sent, so the agent sends it itself. Maple's own
  * agents (chat, bot) are not asked to report on Maple.
  */
 export const withFeedbackHint = (text: string, name: string, surface: McpToolSurface): string =>
 	surface === "mcp" && name !== "send_maple_feedback"
-		? `${text}\nIf this looks like a bug in Maple rather than in your call, offer the user to report it with \`send_maple_feedback\`.`
+		? `${text}\nIf this looks like a bug in Maple rather than in your call, report it now with \`send_maple_feedback\` (kind "bug", related_to "${name}", what you called and what came back, no user data), then tell the user in one line that you did.`
 		: text
 
 /**
@@ -95,6 +96,9 @@ const REAL_FAILURE_CATEGORY = new Map<string, string>([
 	["@maple/mcp/errors/McpInvalidTenantError", "tenant"],
 ])
 
+/** Matches `expected-failures.ts`, so both kinds of failure carry the same message bound. */
+const MAX_SPAN_MESSAGE_CHARS = 500
+
 const dispatchInSpan = Effect.fn("McpToolDispatcher.call")(function* (
 	name: string,
 	input: unknown,
@@ -160,9 +164,14 @@ const dispatchInSpan = Effect.fn("McpToolDispatcher.call")(function* (
 				recordExpectedMcpFailure(error, "MCP authentication failed").pipe(
 					Effect.as(failureResult(`Authentication required: ${error.message}`, "auth")),
 				),
+			// A credential that is valid but may not do this is a permission answer, not a bug.
 			"@maple/mcp/errors/McpAuthInvalidError": (error) =>
 				recordExpectedMcpFailure(error, "MCP authentication failed").pipe(
-					Effect.as(failureResult(`Authentication failed: ${error.message}`, "auth")),
+					Effect.as(
+						error.reason === "insufficient_scope"
+							? failureResult(`Permission denied: ${error.message}`, "permission")
+							: failureResult(`Authentication failed: ${error.message}`, "auth"),
+					),
 				),
 		}),
 		// After the catchTags above, so a failure they converted into an in-band
@@ -187,7 +196,12 @@ const dispatchInSpan = Effect.fn("McpToolDispatcher.call")(function* (
 			const category = REAL_FAILURE_CATEGORY.get(error._tag)
 			return category === undefined
 				? Effect.void
-				: Effect.annotateCurrentSpan({ "result.isError": true, "maple.mcp.error.category": category })
+				: Effect.annotateCurrentSpan({
+						"result.isError": true,
+						"maple.mcp.error.category": category,
+						// As expected failures record it: a tool's failure reasons are one query, not a log join.
+						"maple.mcp.error.message": Str.slice(0, MAX_SPAN_MESSAGE_CHARS)(error.message),
+					})
 		}),
 		Effect.annotateLogs({ "maple.mcp.tool": name }),
 	)

apps/ai/src/mcp/lib/map-http-error.test.ts

@@ -0,0 +1,40 @@
+import { describe, expect, it } from "@effect/vitest"
+import { AlertForbiddenError, AlertPersistenceError, AlertValidationError } from "@maple/domain/http"
+import { WarehouseQueryError } from "@maple/domain"
+import { toMcpHttpError } from "./map-http-error"
+
+describe("toMcpHttpError", () => {
+	it("reads a 403 as a permission problem in public words, never the internal tag", () => {
+		const mapped = toMcpHttpError("create_alert_rule")(
+			new AlertForbiddenError({ message: "Only admins can create alert rules" }),
+		)
+		expect({ tag: mapped._tag, reason: "reason" in mapped ? mapped.reason : undefined }).toEqual({
+			tag: "@maple/mcp/errors/McpAuthInvalidError",
+			reason: "insufficient_scope",
+		})
+		expect(mapped.message).not.toContain("@maple/")
+	})
+
+	it("reads a 400 as input the model fixes", () => {
+		const mapped = toMcpHttpError("create_alert_rule")(
+			new AlertValidationError({ message: "threshold must be positive", details: [] }),
+		)
+		expect(mapped._tag).toBe("@maple/mcp/errors/McpInvalidInputError")
+	})
+
+	it("keeps a retryable 5xx a real failure, saying a retry may work", () => {
+		const mapped = toMcpHttpError("create_alert_rule")(
+			new AlertPersistenceError({ message: "connection reset" }),
+		)
+		expect(mapped._tag).toBe("@maple/mcp/errors/McpQueryError")
+		expect(mapped.message).toContain("Retrying in about 5s may work.")
+	})
+
+	it("keeps a non-retryable 5xx a query failure, without the tag in its text", () => {
+		const mapped = toMcpHttpError("list_users")(
+			new WarehouseQueryError({ message: "boom", pipeName: "list_users" }),
+		)
+		expect(mapped._tag).toBe("@maple/mcp/errors/McpQueryError")
+		expect(mapped.message).not.toContain("@maple/")
+	})
+})

apps/ai/src/mcp/lib/map-http-error.ts

@@ -1,12 +1,59 @@
 import type { SelfDescribingHttpError } from "@maple/domain/http"
-import { McpQueryError } from "../tools/types"
+import {
+	McpAuthInvalidError,
+	McpInvalidInputError,
+	McpNotReadyError,
+	McpQueryError,
+	McpUnavailableError,
+} from "../tools/types"
 
-/** Adapt an HTTP-domain failure at the MCP protocol boundary without reclassifying its tag. */
+export type McpHttpError =
+	| McpAuthInvalidError
+	| McpInvalidInputError
+	| McpNotReadyError
+	| McpQueryError
+	| McpUnavailableError
+
+/** When a retryable error names no delay. */
+const DEFAULT_RETRY_SECONDS = 5
+
+/**
+ * Adapt an HTTP-domain failure at the MCP boundary by its public category, so a 403 reads as a
+ * permission problem and a 400 as a fix to the call, not as a Maple bug. The text is the error's
+ * public message: internal tags never reach the model. Only 5xx stay `McpQueryError`.
+ */
 export const toMcpHttpError =
 	(pipeName: string) =>
-	(error: SelfDescribingHttpError): McpQueryError =>
-		new McpQueryError({
-			message: `${error._tag}: ${error.error.message}`,
-			pipeName,
-			cause: error,
-		})
+	(error: SelfDescribingHttpError): McpHttpError => {
+		const body = error.error
+		const message = body.message
+		switch (body.type) {
+			case "invalid_request_error":
+			case "not_found_error":
+			case "conflict_error":
+				return new McpInvalidInputError({
+					message,
+					...(body.param === undefined ? undefined : { parameter: body.param }),
+				})
+			case "authentication_error":
+				return new McpAuthInvalidError({ message, reason: "invalid_credentials" })
+			case "permission_error":
+				return new McpAuthInvalidError({ message, reason: "insufficient_scope" })
+			case "payment_error":
+				return new McpUnavailableError({ message, capability: "plan" })
+			case "rate_limit_error":
+				return new McpNotReadyError({
+					message,
+					retryAfterSeconds: body.retry_after_seconds ?? DEFAULT_RETRY_SECONDS,
+				})
+			// A 5xx stays a real failure (Error span, cause kept) even when a retry may help.
+			case "api_error":
+				return new McpQueryError({
+					message: body.retryable
+						? `${message} Retrying in about ${body.retry_after_seconds ?? DEFAULT_RETRY_SECONDS}s may work.`
+						: message,
+					pipeName,
+					cause: error,
+				})
+		}
+	}

apps/ai/src/mcp/lib/map-warehouse-error.test.ts

@@ -1,6 +1,11 @@
 import { describe, expect, it } from "vitest"
-import { WarehouseQueryError, WarehouseResultDecodeError, WarehouseSchemaDriftError } from "@maple/domain"
-import { toMcpQueryError } from "./map-warehouse-error"
+import {
+	WarehouseConfigError,
+	WarehouseQueryError,
+	WarehouseResultDecodeError,
+	WarehouseSchemaDriftError,
+} from "@maple/domain"
+import { toMcpQueryError, toMcpReadError } from "./map-warehouse-error"
 
 describe("toMcpQueryError", () => {
 	it("forwards plain query errors verbatim with the pipe label", () => {
@@ -38,3 +43,23 @@ describe("toMcpQueryError", () => {
 		expect(mcp.message).not.toContain("schema apply")
 	})
 })
+
+describe("toMcpReadError", () => {
+	it("reads one of Maple's tables missing for the org as not ready", () => {
+		const err = new WarehouseConfigError({
+			message: "Resource 'user_activity_hourly' not found",
+			pipeName: "list_users",
+		})
+		const mcp = toMcpReadError("list_users")(err)
+		expect(mcp._tag).toBe("@maple/mcp/errors/McpNotReadyError")
+		expect(mcp.message).toContain("user_activity_hourly")
+	})
+
+	it("leaves a table Maple does not define as a query failure", () => {
+		const err = new WarehouseConfigError({
+			message: "Resource 'spans' not found",
+			pipeName: "list_users",
+		})
+		expect(toMcpReadError("list_users")(err)._tag).toBe("@maple/mcp/errors/McpQueryError")
+	})
+})