diff --git a/.changeset/connectors-error-results.md b/.changeset/connectors-error-results.md new file mode 100644 index 0000000..3c1b65e --- /dev/null +++ b/.changeset/connectors-error-results.md @@ -0,0 +1,11 @@ +--- +'@gemstack/connectors': minor +'@gemstack/connector-github': patch +'@gemstack/connector-google-drive': patch +--- + +Return validation failures as MCP errors instead of success results. + +The GitHub and Google Drive connectors returned `{ error: '...' }` for user-facing validation failures (get-file on a directory, get-file-content on a folder, share-file missing an email/domain). These normalized to a **success** `McpToolResult`, so an agent could not tell failure from data via the MCP `isError` flag. They now return `McpResponse.error(...)`, which sets `isError: true`. + +`@gemstack/connectors` now re-exports `McpResponse` (and the `McpToolResult` type) so a connector's `handle` can signal these errors without depending on `@gemstack/mcp` directly. diff --git a/packages/connector-github/src/index.test.ts b/packages/connector-github/src/index.test.ts index 315b45c..7a5fcf1 100644 --- a/packages/connector-github/src/index.test.ts +++ b/packages/connector-github/src/index.test.ts @@ -90,6 +90,15 @@ test('get-file base64-decodes content', async () => { assert.equal(res.path, 'README.md') }) +test('get-file on a directory returns an MCP error, not a success result', async () => { + // The contents API returns an array for a directory path. + mockFetch(() => ({ body: [{ path: 'src/a.ts' }, { path: 'src/b.ts' }] })) + const res = await client().callTool('github_get-file', { owner: 'o', repo: 'r', path: 'src' }) + assert.equal(res.isError, true) + const first = res.content[0] + assert.match(first && first.type === 'text' ? first.text : '', /is a directory/) +}) + test('comment-on-issue POSTs the body and returns the comment ref', async () => { mockFetch(() => ({ body: { id: 99, html_url: 'comment-url' } })) const res = json(await client().callTool('github_comment-on-issue', { owner: 'o', repo: 'r', number: 7, body: 'hi' })) diff --git a/packages/connector-github/src/index.ts b/packages/connector-github/src/index.ts index 99a6e6b..6ffd163 100644 --- a/packages/connector-github/src/index.ts +++ b/packages/connector-github/src/index.ts @@ -1,4 +1,4 @@ -import { defineConnector } from '@gemstack/connectors' +import { defineConnector, McpResponse } from '@gemstack/connectors' import { z } from 'zod' import { gh } from './client.js' @@ -145,7 +145,7 @@ export default defineConnector({ 'GET', `/repos/${enc(input.owner)}/${enc(input.repo)}/contents/${input.path.split('/').map(enc).join('/')}${q}`, ) - if (Array.isArray(f)) return { error: `path "${input.path}" is a directory, not a file` } + if (Array.isArray(f)) return McpResponse.error(`path "${input.path}" is a directory, not a file`) const content = f.encoding === 'base64' && typeof f.content === 'string' ? Buffer.from(f.content, 'base64').toString('utf8') diff --git a/packages/connector-google-drive/src/index.test.ts b/packages/connector-google-drive/src/index.test.ts index be86005..b758212 100644 --- a/packages/connector-google-drive/src/index.test.ts +++ b/packages/connector-google-drive/src/index.test.ts @@ -40,6 +40,12 @@ function json(result: McpToolResult): any { return first && first.type === 'text' ? JSON.parse(first.text) : undefined } +/** The plain error text of an `isError` result. */ +function errText(result: McpToolResult): string { + const first = result.content[0] + return first && first.type === 'text' ? first.text : '' +} + /** Decode a captured request URL, restoring `+`-encoded spaces for readable matching. */ function urlOf(i: number): string { return decodeURIComponent(calls[i]!.url).replace(/\+/g, ' ') @@ -132,8 +138,9 @@ test('get-file-content downloads a regular file via alt=media', async () => { test('get-file-content refuses a folder', async () => { mockFetch(() => ({ body: { id: 'x', name: 'Stuff', mimeType: 'application/vnd.google-apps.folder' } })) - const res = json(await client().callTool('google-drive_get-file-content', { fileId: 'x' })) - assert.match(res.error, /folder/) + const res = await client().callTool('google-drive_get-file-content', { fileId: 'x' }) + assert.equal(res.isError, true) + assert.match(errText(res), /folder/) assert.equal(calls.length, 1) }) @@ -162,8 +169,9 @@ test('share-file posts a permission and validates the email requirement', async assert.deepEqual(calls[0]!.body, { role: 'writer', type: 'user', emailAddress: 'b@x.com' }) calls = [] - const bad = json(await client().callTool('google-drive_share-file', { fileId: 'F' })) - assert.match(bad.error, /requires an emailAddress/) + const bad = await client().callTool('google-drive_share-file', { fileId: 'F' }) + assert.equal(bad.isError, true) + assert.match(errText(bad), /requires an emailAddress/) assert.equal(calls.length, 0) }) diff --git a/packages/connector-google-drive/src/index.ts b/packages/connector-google-drive/src/index.ts index 1c59f48..97e1e85 100644 --- a/packages/connector-google-drive/src/index.ts +++ b/packages/connector-google-drive/src/index.ts @@ -1,4 +1,4 @@ -import { defineConnector } from '@gemstack/connectors' +import { defineConnector, McpResponse } from '@gemstack/connectors' import { z } from 'zod' import { gd, gdText } from './client.js' @@ -148,11 +148,11 @@ export default defineConnector({ const id = enc(input.fileId) const meta = await gd>(ctx, 'GET', `/files/${id}?fields=id,name,mimeType`) const mime: string = meta.mimeType ?? '' - if (mime === FOLDER_MIME) return { error: `"${meta.name}" is a folder, not a file` } + if (mime === FOLDER_MIME) return McpResponse.error(`"${meta.name}" is a folder, not a file`) let content: string if (mime.startsWith('application/vnd.google-apps.')) { const exportMime = exportMimeFor(mime) - if (!exportMime) return { error: `cannot export ${mime} as text` } + if (!exportMime) return McpResponse.error(`cannot export ${mime} as text`) content = await gdText(ctx, `/files/${id}/export?mimeType=${enc(exportMime)}`) } else { content = await gdText(ctx, `/files/${id}?alt=media`) @@ -209,9 +209,9 @@ export default defineConnector({ ) => { const type = input.type ?? 'user' if ((type === 'user' || type === 'group') && !input.emailAddress) { - return { error: `type "${type}" requires an emailAddress` } + return McpResponse.error(`type "${type}" requires an emailAddress`) } - if (type === 'domain' && !input.domain) return { error: 'type "domain" requires a domain' } + if (type === 'domain' && !input.domain) return McpResponse.error('type "domain" requires a domain') const payload: Record = { role: input.role ?? 'reader', type } if (input.emailAddress) payload.emailAddress = input.emailAddress if (input.domain) payload.domain = input.domain diff --git a/packages/connectors/README.md b/packages/connectors/README.md index 9aad55b..a57e582 100644 --- a/packages/connectors/README.md +++ b/packages/connectors/README.md @@ -44,7 +44,7 @@ export default defineConnector({ }) ``` -A tool's `handle` may return a `string` (wrapped as text), any JSON-serializable value (wrapped as pretty JSON), or a full `McpToolResult` (use `McpResponse` from `@gemstack/mcp` for errors / images). +A tool's `handle` may return a `string` (wrapped as text), any JSON-serializable value (wrapped as pretty JSON), or a full `McpToolResult`. For an expected, user-facing failure (validation, not-found), return `McpResponse.error(...)` (re-exported from this package) so the client sees a failed tool call (`isError: true`) it can detect, rather than a success result that merely contains an `error` field. Reserve throwing for unexpected faults. ## Mount connectors into a server diff --git a/packages/connectors/src/index.test.ts b/packages/connectors/src/index.test.ts index c9132c0..a4e0e79 100644 --- a/packages/connectors/src/index.test.ts +++ b/packages/connectors/src/index.test.ts @@ -3,7 +3,7 @@ import { test } from 'node:test' import assert from 'node:assert/strict' import { z } from 'zod' import { McpTestClient } from '@gemstack/mcp/testing' -import { defineConnector, mountConnectors } from './index.js' +import { defineConnector, mountConnectors, McpResponse } from './index.js' import type { ConnectorContext } from './index.js' import type { McpToolResult } from '@gemstack/mcp' @@ -110,6 +110,22 @@ test('annotations are advertised to clients', async () => { assert.equal(list?.annotations?.readOnlyHint, true) }) +test('a handler returning McpResponse.error surfaces as an MCP error result', async () => { + const c = defineConnector({ + id: 'guard', + tools: [ + { + name: 'check', + schema: z.object({}), + handle: () => McpResponse.error('not allowed'), + }, + ], + }) + const res = await new McpTestClient(mountConnectors([c])).callTool('guard_check') + assert.equal(res.isError, true) + assert.equal(txt(res), 'Error: not allowed') +}) + test('mountConnectors rejects cross-connector name collisions without namespacing', () => { const a = defineConnector({ id: 'a', tools: [{ name: 'x', schema: z.object({}), handle: () => '' }] }) const b = defineConnector({ id: 'b', tools: [{ name: 'x', schema: z.object({}), handle: () => '' }] }) diff --git a/packages/connectors/src/index.ts b/packages/connectors/src/index.ts index 65210f4..4f46a2d 100644 --- a/packages/connectors/src/index.ts +++ b/packages/connectors/src/index.ts @@ -1,6 +1,11 @@ export { defineConnector } from './defineConnector.js' export { mountConnectors } from './mountConnectors.js' export type { ConnectorServerClass, MountOptions } from './mountConnectors.js' +// Re-exported so a connector's `handle` can signal an expected, user-facing +// failure as an MCP error (`isError: true`) via `McpResponse.error(...)` +// without depending on `@gemstack/mcp` directly. +export { McpResponse } from '@gemstack/mcp' +export type { McpToolResult } from '@gemstack/mcp' export type { Connector, ConnectorDefinition,