fix(connectors): return validation failures as MCP errors, not success results (#154)

The GitHub and Google Drive connectors returned { error: '...' } for
user-facing validation failures, which normalized to a success McpToolResult,
so an agent could not detect failure via the MCP isError flag. Return
McpResponse.error(...) instead (isError: true).

@gemstack/connectors now re-exports McpResponse (+ the McpToolResult type) so a
connector's handle can signal these without importing @gemstack/mcp directly.

Closes #147

Author: suleimansh

.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.

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' }))

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')

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)
 })
 

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<Record<string, any>>(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<string, unknown> = { role: input.role ?? 'reader', type }
         if (input.emailAddress) payload.emailAddress = input.emailAddress
         if (input.domain) payload.domain = input.domain

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
 

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: () => '' }] })

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,