diff --git a/CHANGELOG.md b/CHANGELOG.md index fade38e..98912f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ Tagged releases are published to npm from GitHub Actions when a **GitHub Release ### Added +- Zod `toolErrorSchema` and exported types `ToolError` / `ToolErrorCode` for parsing MCP tool failures; all tools now return this JSON shape in the text content when `isError` is true. +- `validateMetadataFilterDetailed()` returns `{ message, field }` for invalid filters; `validateMetadataFilter()` remains a string-only wrapper for backward compatibility. - `.coderabbit.yaml` sets the pre-merge **docstring coverage** threshold to **79%** (default **80%**) so marginal documentation-only gaps do not block merges; adjust upward as coverage improves. - `registerBuiltinUrlGenerators()` for built-in URL generators; `setupServer()` invokes it so CLI/library parity stays default. - Discriminated result type for `listNamespacesFromKeywordIndex()` (`KeywordIndexNamespacesResult`). @@ -24,6 +26,8 @@ Tagged releases are published to npm from GitHub Actions when a **GitHub Release ### Changed +- **Breaking (MCP):** Tool error bodies no longer use `{ status: 'error', message }`. Failures are typed `ToolError` objects: `code` (`FLOW_GATE` | `VALIDATION` | `PINECONE_ERROR` | `TIMEOUT`), `message`, `recoverable`, optional `suggestion`, and optional `field` (required for `VALIDATION`). The outer MCP result still sets `isError: true`. +- **Breaking (types):** `QueryResponse` and exported `KeywordSearchResponse` no longer include `status: 'error'` / error-only fields; errors use `ToolError` only. - **Breaking (MCP):** `suggest_query_params` and in-process suggestion flow now emit `recommended_tool` as `count` | `fast` | `detailed` | `full` (aligned with the unified `query` tool `preset`), not legacy `query_fast` / `query_detailed` strings. - **Breaking (MCP):** Single hybrid `query` tool with `preset` (`fast` | `detailed` | `full`); removed separate `query_fast` / `query_detailed` tool registrations. - `resolveConfig()` throws if the Pinecone API key is missing (after trim); library callers must supply `apiKey` via overrides or set `PINECONE_API_KEY`. diff --git a/README.md b/README.md index 33a4947..d87ad6e 100644 --- a/README.md +++ b/README.md @@ -21,6 +21,20 @@ A Model Context Protocol (MCP) server that provides semantic search over Pinecon | [CONTRIBUTING.md](CONTRIBUTING.md) | How to contribute | | [SECURITY.md](SECURITY.md) | Vulnerability reporting | +## Error responses + +When a tool fails, the MCP tool result sets **`isError: true`**. The `text` content is JSON matching **`ToolError`** (parse with `toolErrorSchema` from `@will-cppa/pinecone-read-only-mcp`). + +| Field | Description | +| ----- | ----------- | +| `code` | `FLOW_GATE` — `suggest_query_params` was not run for this namespace (or context expired). `VALIDATION` — bad input or metadata filter. `PINECONE_ERROR` — SDK / network / server failure. `TIMEOUT` — outbound Pinecone call exceeded `--request-timeout-ms`. | +| `message` | Human-readable detail (`DEBUG` log level may surface raw SDK messages in the message for `PINECONE_ERROR` / `TIMEOUT`). | +| `recoverable` | Whether the client can plausibly fix the issue and retry (`true` for flow gate, validation, timeouts; typically `false` for generic Pinecone errors). | +| `suggestion` | Optional hint. **`FLOW_GATE`** always includes: `Call suggest_query_params for namespace '' first`. **`TIMEOUT`** suggests retrying or increasing the request timeout. | +| `field` | **Required when `code` is `VALIDATION`:** the input parameter name (e.g. `query_text`, `namespace`) or a dot-path into `metadata_filter` (e.g. `author.$in`). | + +Success payloads are unchanged and do **not** wrap `ToolError`. Clients that still expect `{ "status": "error", "message": "..." }` must migrate to the shape above. + ## Features - **Hybrid Search**: Combines dense and sparse embeddings for superior recall diff --git a/src/server.ts b/src/server.ts index ca24c86..7badaac 100644 --- a/src/server.ts +++ b/src/server.ts @@ -10,6 +10,7 @@ * - {@link resolveConfig} — merge CLI-style overrides with `process.env`. * - {@link setPineconeClient} — inject a client instance before `setupServer()`. * - {@link registerUrlGenerator} / {@link unregisterUrlGenerator} — extend URL synthesis. + * - {@link toolErrorSchema} / {@link ToolError} — parse MCP tool failures (`isError: true` JSON bodies). * - Built-in `mailing` / `slack-Cpplang` URL generators are registered from {@link setupServer} * via {@link registerBuiltinUrlGenerators}; call it yourself if you use the library without `setupServer`. * @@ -35,6 +36,12 @@ import { registerSuggestQueryParamsTool } from './server/tools/suggest-query-par export { setPineconeClient } from './server/client-context.js'; /** Validate user-supplied Pinecone metadata filter objects before querying. */ export { validateMetadataFilter } from './server/metadata-filter.js'; +/** Structured metadata filter validation (`field` dot-path); {@link validateMetadataFilter} remains a string-only wrapper. */ +export { validateMetadataFilterDetailed } from './server/metadata-filter.js'; +export type { MetadataFilterValidationError } from './server/metadata-filter.js'; +/** Zod schema and types for MCP tool error JSON bodies (`isError: true`). */ +export { toolErrorSchema } from './server/tool-error.js'; +export type { ToolError, ToolErrorCode } from './server/tool-error.js'; /** Heuristic field + tool suggestions from a namespace schema + user query. */ export { suggestQueryParams } from './server/query-suggestion.js'; export type { RecommendedTool, SuggestQueryParamsResult } from './server/query-suggestion.js'; diff --git a/src/server/metadata-filter.test.ts b/src/server/metadata-filter.test.ts new file mode 100644 index 0000000..aaa5ab7 --- /dev/null +++ b/src/server/metadata-filter.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it } from 'vitest'; +import { validateMetadataFilter, validateMetadataFilterDetailed } from './metadata-filter.js'; + +describe('validateMetadataFilterDetailed', () => { + it('returns null for a valid filter', () => { + expect( + validateMetadataFilterDetailed({ + year: { $gte: 2020, $lte: 2026 }, + tags: { $in: ['a', 'b'] }, + }) + ).toBeNull(); + }); + + it('returns message and dot-path field for unknown nested operator', () => { + const d = validateMetadataFilterDetailed({ + year: { $regex: '^202' }, + }); + expect(d).not.toBeNull(); + expect(d!.message).toContain('Unknown filter operator'); + expect(d!.field).toBe('year.$regex'); + expect(validateMetadataFilter({ year: { $regex: '^202' } })).toBe(d!.message); + }); + + it('returns field for invalid $in value', () => { + const d = validateMetadataFilterDetailed({ + tags: { $in: 'not-an-array' }, + }); + expect(d!.field).toBe('tags.$in'); + expect(d!.message).toContain('primitive values'); + }); + + it('returns field for null metadata value', () => { + const d = validateMetadataFilterDetailed({ + author: null as unknown as Record, + }); + expect(d!.field).toBe('author'); + expect(d!.message).toContain('null'); + }); + + it('returns field when nested $and value is not an array', () => { + const d = validateMetadataFilterDetailed({ + tags: { $and: { $eq: 'x' } }, + }); + expect(d!.field).toBe('tags.$and'); + }); + + it('returns field when nested $or array element is an array, not a filter object', () => { + const d = validateMetadataFilterDetailed({ + tags: { $or: [[1]] }, + }); + expect(d!.field).toBe('tags.$or.0'); + }); +}); diff --git a/src/server/metadata-filter.ts b/src/server/metadata-filter.ts index cfcf281..4bed734 100644 --- a/src/server/metadata-filter.ts +++ b/src/server/metadata-filter.ts @@ -27,6 +27,12 @@ const ALLOWED_FILTER_OPERATORS = new Set([ '$or', ]); +export type MetadataFilterValidationError = { + message: string; + /** Dot-path to the failing segment (metadata key and/or operator chain). */ + field: string; +}; + /** True if value is a string, number, or boolean (allowed for $eq, $gt, etc.). */ function isPrimitiveFilterValue(value: unknown): boolean { return typeof value === 'string' || typeof value === 'number' || typeof value === 'boolean'; @@ -40,10 +46,17 @@ function isPrimitiveArray(value: unknown): boolean { ); } -/** Recursively validate a filter value; returns an error string or null if valid. */ -function validateMetadataFilterValue(value: unknown, path: string[]): string | null { +function err(message: string, path: string[]): MetadataFilterValidationError { + return { message, field: path.join('.') }; +} + +/** Recursively validate a filter value; returns an error or null if valid. */ +function validateMetadataFilterValue( + value: unknown, + path: string[] +): MetadataFilterValidationError | null { if (value === null || value === undefined) { - return `Invalid null/undefined at "${path.join('.')}".`; + return err(`Invalid null/undefined at "${path.join('.')}".`, path); } if (isPrimitiveFilterValue(value) || isPrimitiveArray(value)) { @@ -54,27 +67,39 @@ function validateMetadataFilterValue(value: unknown, path: string[]): string | n for (let i = 0; i < value.length; i++) { const item = value[i]; if (typeof item !== 'object' || item === null || Array.isArray(item)) { - return `Operator "${path[path.length - 1]}" at "${[...path, String(i)].join('.')}" must use an array of filter objects.`; + return err( + `Operator "${path[path.length - 1]}" at "${[...path, String(i)].join('.')}" must use an array of filter objects.`, + [...path, String(i)] + ); } - const nestedError = validateMetadataFilter(item as Record); + const nestedError = validateMetadataFilterRecord(item as Record); if (nestedError) return nestedError; } return null; } if (typeof value !== 'object') { - return `Unsupported filter value at "${path.join('.')}".`; + return err(`Unsupported filter value at "${path.join('.')}".`, path); } for (const [key, nestedValue] of Object.entries(value)) { if (!key.startsWith('$')) { - return `Nested metadata filters must use operator keys starting with "$" at "${path.join('.')}"; got "${key}".`; + return err( + `Nested metadata filters must use operator keys starting with "$" at "${path.join('.')}"; got "${key}".`, + [...path, key] + ); } if (!ALLOWED_FILTER_OPERATORS.has(key)) { - return `Unknown filter operator "${key}" at "${path.join('.')}". Allowed operators: ${[...ALLOWED_FILTER_OPERATORS].join(', ')}.`; + return err( + `Unknown filter operator "${key}" at "${path.join('.')}". Allowed operators: ${[...ALLOWED_FILTER_OPERATORS].join(', ')}.`, + [...path, key] + ); } if ((key === '$in' || key === '$nin') && !isPrimitiveArray(nestedValue)) { - return `Operator "${key}" at "${path.join('.')}" must use an array of primitive values.`; + return err( + `Operator "${key}" at "${path.join('.')}" must use an array of primitive values.`, + [...path, key] + ); } if ( (key === '$eq' || @@ -85,10 +110,16 @@ function validateMetadataFilterValue(value: unknown, path: string[]): string | n key === '$lte') && !isPrimitiveFilterValue(nestedValue) ) { - return `Operator "${key}" at "${path.join('.')}" must use a primitive value.`; + return err(`Operator "${key}" at "${path.join('.')}" must use a primitive value.`, [ + ...path, + key, + ]); } if ((key === '$and' || key === '$or') && !Array.isArray(nestedValue)) { - return `Operator "${key}" at "${path.join('.')}" must use an array of filter objects.`; + return err(`Operator "${key}" at "${path.join('.')}" must use an array of filter objects.`, [ + ...path, + key, + ]); } const nestedError = validateMetadataFilterValue(nestedValue, [...path, key]); @@ -100,11 +131,27 @@ function validateMetadataFilterValue(value: unknown, path: string[]): string | n return null; } -/** Validate a Pinecone metadata filter object; returns an error message or null if valid. */ -export function validateMetadataFilter(filter: Record): string | null { +function validateMetadataFilterRecord( + filter: Record +): MetadataFilterValidationError | null { for (const [field, value] of Object.entries(filter)) { const error = validateMetadataFilterValue(value, [field]); if (error) return error; } return null; } + +/** + * Validate a Pinecone metadata filter object; returns structured error or null if valid. + */ +export function validateMetadataFilterDetailed( + filter: Record +): MetadataFilterValidationError | null { + return validateMetadataFilterRecord(filter); +} + +/** Validate a Pinecone metadata filter object; returns an error message or null if valid. */ +export function validateMetadataFilter(filter: Record): string | null { + const detailed = validateMetadataFilterDetailed(filter); + return detailed?.message ?? null; +} diff --git a/src/server/tool-error.test.ts b/src/server/tool-error.test.ts new file mode 100644 index 0000000..87b8000 --- /dev/null +++ b/src/server/tool-error.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it } from 'vitest'; +import { + classifyToolCatchError, + flowGateToolError, + toolErrorSchema, + validationToolError, +} from './tool-error.js'; + +describe('ToolError schema and builders', () => { + it('FLOW_GATE: includes required suggestion template', () => { + const err = flowGateToolError('wg21', 'Flow requires suggest_query_params first.'); + const parsed = toolErrorSchema.parse(err); + expect(parsed.code).toBe('FLOW_GATE'); + expect(parsed.recoverable).toBe(true); + expect(parsed.suggestion).toBe("Call suggest_query_params for namespace 'wg21' first"); + expect(parsed.message).toContain('Flow requires'); + }); + + it('VALIDATION: requires field and parses', () => { + const err = validationToolError('Unknown filter operator', 'author.$badop'); + const parsed = toolErrorSchema.parse(err); + expect(parsed.code).toBe('VALIDATION'); + expect(parsed.field).toBe('author.$badop'); + expect(parsed.recoverable).toBe(true); + }); + + it('VALIDATION: schema rejects payload missing field', () => { + const bad = { code: 'VALIDATION' as const, message: 'x', recoverable: true as const }; + const result = toolErrorSchema.safeParse(bad); + expect(result.success).toBe(false); + }); + + it('PINECONE_ERROR: classifyToolCatchError maps generic Error', () => { + const err = classifyToolCatchError(new Error('SDK boom'), 'fallback'); + expect(err.code).toBe('PINECONE_ERROR'); + expect(err.recoverable).toBe(false); + expect(toolErrorSchema.parse(err).code).toBe('PINECONE_ERROR'); + }); + + it('TIMEOUT: classifyToolCatchError matches withTimeout message prefix', () => { + const err = classifyToolCatchError( + new Error('Timeout after 100ms while waiting for query'), + 'fallback' + ); + expect(err.code).toBe('TIMEOUT'); + expect(err.recoverable).toBe(true); + expect(err.suggestion).toMatch(/retry|timeout/i); + toolErrorSchema.parse(err); + }); +}); diff --git a/src/server/tool-error.ts b/src/server/tool-error.ts index ec2274a..d506f96 100644 --- a/src/server/tool-error.ts +++ b/src/server/tool-error.ts @@ -1,7 +1,9 @@ /** - * Shared error handling for MCP tools: consistent logging and user-facing messages. + * Shared error handling for MCP tools: consistent logging, user-facing messages, + * and typed {@link ToolError} payloads for MCP `isError` responses. */ +import { z } from 'zod'; import { getLogLevel, error as logError } from '../logger.js'; /** User-facing error message: detailed in DEBUG, generic otherwise. */ @@ -14,3 +16,110 @@ export function getToolErrorMessage(error: unknown, fallbackMessage: string): st export function logToolError(toolName: string, error: unknown): void { logError(`Error in ${toolName} tool`, error); } + +export const toolErrorCodeSchema = z.enum(['FLOW_GATE', 'VALIDATION', 'PINECONE_ERROR', 'TIMEOUT']); +export type ToolErrorCode = z.infer; + +const flowGateToolErrorSchema = z.object({ + code: z.literal('FLOW_GATE'), + message: z.string(), + recoverable: z.literal(true), + suggestion: z.string(), +}); + +const validationToolErrorSchema = z.object({ + code: z.literal('VALIDATION'), + message: z.string(), + recoverable: z.literal(true), + field: z.string(), + suggestion: z.string().optional(), +}); + +const pineconeToolErrorSchema = z.object({ + code: z.literal('PINECONE_ERROR'), + message: z.string(), + recoverable: z.boolean(), + suggestion: z.string().optional(), +}); + +const timeoutToolErrorSchema = z.object({ + code: z.literal('TIMEOUT'), + message: z.string(), + recoverable: z.literal(true), + suggestion: z.string().optional(), +}); + +export const toolErrorSchema = z.discriminatedUnion('code', [ + flowGateToolErrorSchema, + validationToolErrorSchema, + pineconeToolErrorSchema, + timeoutToolErrorSchema, +]); + +export type ToolError = z.infer; + +const DEFAULT_TIMEOUT_SUGGESTION = 'Retry the request, or increase --request-timeout-ms.'; + +/** Matches {@link withTimeout} rejection message prefix in `retry.ts`. */ +const TIMEOUT_MESSAGE_PATTERN = /^Timeout after \d+ms while waiting for /i; + +export function flowGateToolError(namespace: string, message: string): ToolError { + return { + code: 'FLOW_GATE', + message, + recoverable: true, + suggestion: `Call suggest_query_params for namespace '${namespace}' first`, + }; +} + +export function validationToolError( + message: string, + field: string, + options?: { suggestion?: string } +): ToolError { + return { + code: 'VALIDATION', + message, + recoverable: true, + field, + ...(options?.suggestion ? { suggestion: options.suggestion } : {}), + }; +} + +export function pineconeToolError( + message: string, + options?: { recoverable?: boolean; suggestion?: string } +): ToolError { + return { + code: 'PINECONE_ERROR', + message, + recoverable: options?.recoverable ?? false, + ...(options?.suggestion ? { suggestion: options.suggestion } : {}), + }; +} + +export function timeoutToolError(message: string, options?: { suggestion?: string }): ToolError { + return { + code: 'TIMEOUT', + message, + recoverable: true, + suggestion: options?.suggestion ?? DEFAULT_TIMEOUT_SUGGESTION, + }; +} + +function rawErrorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + +/** + * Map an unexpected thrown error to {@link ToolError} for MCP responses. + * Uses raw `Error#message` for timeout detection (DEBUG mode replaces the user message). + */ +export function classifyToolCatchError(error: unknown, fallbackMessage: string): ToolError { + const raw = rawErrorMessage(error); + const message = getToolErrorMessage(error, fallbackMessage); + if (TIMEOUT_MESSAGE_PATTERN.test(raw)) { + return timeoutToolError(message); + } + return pineconeToolError(message); +} diff --git a/src/server/tool-response.ts b/src/server/tool-response.ts index bf56f29..0c247a6 100644 --- a/src/server/tool-response.ts +++ b/src/server/tool-response.ts @@ -1,3 +1,6 @@ +import type { ToolError } from './tool-error.js'; +import { toolErrorSchema } from './tool-error.js'; + export type TextPayload = { content: Array<{ type: 'text'; text: string }>; isError?: boolean; @@ -15,14 +18,15 @@ export function jsonResponse(payload: unknown): TextPayload { }; } -/** Build an MCP tool error payload with JSON-stringified content and isError: true. */ -export function jsonErrorResponse(payload: unknown): TextPayload { +/** Build an MCP tool error payload with JSON-stringified {@link ToolError} and isError: true. */ +export function jsonErrorResponse(err: ToolError): TextPayload { + const validated = toolErrorSchema.parse(err); return { isError: true, content: [ { type: 'text', - text: JSON.stringify(payload, null, 2), + text: JSON.stringify(validated, null, 2), }, ], }; diff --git a/src/server/tools/count-tool.test.ts b/src/server/tools/count-tool.test.ts new file mode 100644 index 0000000..c899d2b --- /dev/null +++ b/src/server/tools/count-tool.test.ts @@ -0,0 +1,87 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { getPineconeClient } from '../client-context.js'; +import * as suggestionFlow from '../suggestion-flow.js'; +import { registerCountTool } from './count-tool.js'; +import { assertToolErrorCode, createMockServer } from './test-helpers.js'; + +vi.mock('../client-context.js', () => ({ + getPineconeClient: vi.fn(), +})); + +const mockedGetClient = vi.mocked(getPineconeClient); + +describe('count tool handler', () => { + const flowOk = { + ok: true as const, + flow: { + updatedAt: Date.now(), + recommended_tool: 'count' as const, + suggested_fields: [], + user_query: 'q', + }, + }; + + beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(suggestionFlow, 'requireSuggested').mockReturnValue(flowOk); + mockedGetClient.mockReturnValue({ + count: vi.fn().mockResolvedValue({ count: 3, truncated: false }), + } as never); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('returns VALIDATION when query_text is empty', async () => { + const server = createMockServer(); + registerCountTool(server as never); + const raw = await server.getHandler('count')!({ + namespace: 'wg21', + query_text: ' ', + }); + const err = assertToolErrorCode(raw, 'VALIDATION'); + expect(err.field).toBe('query_text'); + expect(mockedGetClient().count).not.toHaveBeenCalled(); + }); + + it('returns VALIDATION when metadata_filter is invalid', async () => { + const server = createMockServer(); + registerCountTool(server as never); + const raw = await server.getHandler('count')!({ + namespace: 'wg21', + query_text: 'doc', + metadata_filter: { year: { $unknown: 1 } }, + }); + const err = assertToolErrorCode(raw, 'VALIDATION'); + expect(err.field).toBe('year.$unknown'); + }); + + it('returns FLOW_GATE when suggestion flow is not satisfied', async () => { + vi.spyOn(suggestionFlow, 'requireSuggested').mockReturnValue({ + ok: false, + message: 'Call suggest_query_params first.', + }); + const server = createMockServer(); + registerCountTool(server as never); + const raw = await server.getHandler('count')!({ + namespace: 'wg21', + query_text: 'x', + }); + const err = assertToolErrorCode(raw, 'FLOW_GATE'); + expect(err.suggestion).toBe("Call suggest_query_params for namespace 'wg21' first"); + }); + + it('returns PINECONE_ERROR when client.count throws', async () => { + mockedGetClient.mockReturnValue({ + count: vi.fn().mockRejectedValue(new Error('pinecone down')), + } as never); + const server = createMockServer(); + registerCountTool(server as never); + const raw = await server.getHandler('count')!({ + namespace: 'wg21', + query_text: 'x', + }); + expect(assertToolErrorCode(raw, 'PINECONE_ERROR').code).toBe('PINECONE_ERROR'); + }); +}); diff --git a/src/server/tools/count-tool.ts b/src/server/tools/count-tool.ts index 22c7182..f9214ff 100644 --- a/src/server/tools/count-tool.ts +++ b/src/server/tools/count-tool.ts @@ -1,21 +1,24 @@ import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import { z } from 'zod'; import { getPineconeClient } from '../client-context.js'; -import { metadataFilterSchema, validateMetadataFilter } from '../metadata-filter.js'; +import { metadataFilterSchema, validateMetadataFilterDetailed } from '../metadata-filter.js'; import { requireSuggested } from '../suggestion-flow.js'; -import { getToolErrorMessage, logToolError } from '../tool-error.js'; +import { + classifyToolCatchError, + flowGateToolError, + logToolError, + validationToolError, +} from '../tool-error.js'; import { jsonErrorResponse, jsonResponse } from '../tool-response.js'; const COUNT_RESPONSE_STATUS = 'success' as const; -type CountResponse = - | { - status: 'success'; - count: number; - truncated: boolean; - namespace: string; - metadata_filter?: Record; - } - | { status: 'error'; message: string }; +type CountResponse = { + status: 'success'; + count: number; + truncated: boolean; + namespace: string; + metadata_filter?: Record; +}; /** Register the count tool on the MCP server. */ export function registerCountTool(server: McpServer): void { @@ -51,21 +54,17 @@ export function registerCountTool(server: McpServer): void { try { const { namespace, query_text, metadata_filter } = params; if (!query_text.trim()) { - const response: CountResponse = { - status: 'error', - message: 'query_text cannot be empty', - }; - return jsonErrorResponse(response); + return jsonErrorResponse(validationToolError('query_text cannot be empty', 'query_text')); } if (metadata_filter) { - const err = validateMetadataFilter(metadata_filter); + const err = validateMetadataFilterDetailed(metadata_filter); if (err) { - return jsonErrorResponse({ status: 'error', message: err }); + return jsonErrorResponse(validationToolError(err.message, err.field)); } } const flowCheck = requireSuggested(namespace); if (!flowCheck.ok) { - return jsonErrorResponse({ status: 'error', message: flowCheck.message }); + return jsonErrorResponse(flowGateToolError(namespace, flowCheck.message)); } const client = getPineconeClient(); const { count, truncated } = await client.count({ @@ -83,11 +82,7 @@ export function registerCountTool(server: McpServer): void { return jsonResponse(response); } catch (error) { logToolError('count', error); - const response: CountResponse = { - status: 'error', - message: getToolErrorMessage(error, 'Failed to get count'), - }; - return jsonErrorResponse(response); + return jsonErrorResponse(classifyToolCatchError(error, 'Failed to get count')); } } ); diff --git a/src/server/tools/generate-urls-tool.test.ts b/src/server/tools/generate-urls-tool.test.ts new file mode 100644 index 0000000..96a57a8 --- /dev/null +++ b/src/server/tools/generate-urls-tool.test.ts @@ -0,0 +1,23 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import * as urlGeneration from '../url-generation.js'; +import { registerGenerateUrlsTool } from './generate-urls-tool.js'; +import { assertToolErrorCode, createMockServer } from './test-helpers.js'; + +describe('generate_urls tool handler', () => { + beforeEach(() => { + vi.restoreAllMocks(); + }); + + it('returns PINECONE_ERROR when generateUrlForNamespace throws', async () => { + vi.spyOn(urlGeneration, 'generateUrlForNamespace').mockImplementation(() => { + throw new Error('generator boom'); + }); + const server = createMockServer(); + registerGenerateUrlsTool(server as never); + const raw = await server.getHandler('generate_urls')!({ + namespace: 'mailing', + records: [{ document_number: 'P1234' }], + }); + expect(assertToolErrorCode(raw, 'PINECONE_ERROR').code).toBe('PINECONE_ERROR'); + }); +}); diff --git a/src/server/tools/generate-urls-tool.ts b/src/server/tools/generate-urls-tool.ts index ea3adc2..c8b6941 100644 --- a/src/server/tools/generate-urls-tool.ts +++ b/src/server/tools/generate-urls-tool.ts @@ -1,7 +1,7 @@ import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import { z } from 'zod'; import { generateUrlForNamespace } from '../url-generation.js'; -import { getToolErrorMessage, logToolError } from '../tool-error.js'; +import { classifyToolCatchError, logToolError } from '../tool-error.js'; import { jsonErrorResponse, jsonResponse } from '../tool-response.js'; /** Get metadata from a record (either record.metadata or the record itself). */ @@ -58,10 +58,7 @@ export function registerGenerateUrlsTool(server: McpServer): void { }); } catch (error) { logToolError('generate_urls', error); - return jsonErrorResponse({ - status: 'error', - message: getToolErrorMessage(error, 'Failed to generate URLs'), - }); + return jsonErrorResponse(classifyToolCatchError(error, 'Failed to generate URLs')); } } ); diff --git a/src/server/tools/guided-query-tool.test.ts b/src/server/tools/guided-query-tool.test.ts index 05dcd08..a748f74 100644 --- a/src/server/tools/guided-query-tool.test.ts +++ b/src/server/tools/guided-query-tool.test.ts @@ -3,6 +3,7 @@ import { getPineconeClient } from '../client-context.js'; import { getNamespacesWithCache } from '../namespaces-cache.js'; import { registerGuidedQueryTool } from './guided-query-tool.js'; import { + assertToolErrorCode, createMockServer, makeNamespaceCacheEntry, makeSearchResult, @@ -111,7 +112,9 @@ describe('guided_query tool handler', () => { }); expect((raw as { isError?: boolean }).isError).toBe(true); - expect(parseToolJson(raw).message).toBe('user_query cannot be empty'); + const err = assertToolErrorCode(raw, 'VALIDATION'); + expect(err.field).toBe('user_query'); + expect(err.message).toBe('user_query cannot be empty'); }); it('returns error when no namespace can be resolved', async () => { @@ -124,13 +127,24 @@ describe('guided_query tool handler', () => { const server = createMockServer(); registerGuidedQueryTool(server as never); - const body = parseToolJson( - await server.getHandler('guided_query')!({ - user_query: 'hello world', - }) - ); + const raw = await server.getHandler('guided_query')!({ + user_query: 'hello world', + }); - expect(body.status).toBe('error'); - expect(String(body.message)).toContain('No namespace available'); + const err = assertToolErrorCode(raw, 'PINECONE_ERROR'); + expect(err.recoverable).toBe(true); + expect(err.message).toContain('No namespace available'); + }); + + it('returns VALIDATION when explicit namespace is not in cached namespaces', async () => { + const server = createMockServer(); + registerGuidedQueryTool(server as never); + const raw = await server.getHandler('guided_query')!({ + user_query: 'hello', + namespace: 'not-in-cache', + }); + const err = assertToolErrorCode(raw, 'VALIDATION'); + expect(err.field).toBe('namespace'); + expect(err.message).toContain('not-in-cache'); }); }); diff --git a/src/server/tools/guided-query-tool.ts b/src/server/tools/guided-query-tool.ts index fa31a3e..a745122 100644 --- a/src/server/tools/guided-query-tool.ts +++ b/src/server/tools/guided-query-tool.ts @@ -4,12 +4,17 @@ import { FAST_QUERY_FIELDS, MAX_TOP_K, MIN_TOP_K } from '../../constants.js'; import type { QueryResponse } from '../../types.js'; import { getPineconeClient } from '../client-context.js'; import { formatQueryResultRows } from '../format-query-result.js'; -import { metadataFilterSchema, validateMetadataFilter } from '../metadata-filter.js'; +import { metadataFilterSchema, validateMetadataFilterDetailed } from '../metadata-filter.js'; import { rankNamespacesByQuery } from '../namespace-router.js'; import { getNamespacesWithCache } from '../namespaces-cache.js'; import { suggestQueryParams } from '../query-suggestion.js'; import { markSuggested } from '../suggestion-flow.js'; -import { getToolErrorMessage, logToolError } from '../tool-error.js'; +import { + classifyToolCatchError, + logToolError, + pineconeToolError, + validationToolError, +} from '../tool-error.js'; import { jsonErrorResponse, jsonResponse } from '../tool-response.js'; type GuidedToolName = 'count' | 'fast' | 'detailed' | 'full'; @@ -78,13 +83,13 @@ export function registerGuidedQueryTool(server: McpServer): void { } = params; if (!user_query?.trim()) { - return jsonErrorResponse({ status: 'error', message: 'user_query cannot be empty' }); + return jsonErrorResponse(validationToolError('user_query cannot be empty', 'user_query')); } if (metadata_filter) { - const err = validateMetadataFilter(metadata_filter); + const err = validateMetadataFilterDetailed(metadata_filter); if (err) { - return jsonErrorResponse({ status: 'error', message: err }); + return jsonErrorResponse(validationToolError(err.message, err.field)); } } @@ -92,21 +97,41 @@ export function registerGuidedQueryTool(server: McpServer): void { const { data: namespaces, cache_hit } = await getNamespacesWithCache(); const ranked = rankNamespacesByQuery(queryText, namespaces, 3); - const namespace = inputNamespace ?? ranked[0]?.namespace; + const explicitNamespace = inputNamespace?.trim(); + if (inputNamespace !== undefined && !explicitNamespace) { + return jsonErrorResponse(validationToolError('namespace cannot be empty', 'namespace')); + } + const namespace = explicitNamespace ?? ranked[0]?.namespace; + /* + * ToolError mapping: empty index / no routable namespace is backend/data state + * (PINECONE_ERROR, recoverable). Explicit namespace missing from cache is user/input + * (VALIDATION, field namespace). + */ if (!namespace) { - return jsonErrorResponse({ - status: 'error', - message: 'No namespace available. Please run list_namespaces and verify index data.', - }); + return jsonErrorResponse( + pineconeToolError( + 'No namespace available. Please run list_namespaces and verify index data.', + { + recoverable: true, + suggestion: 'Call list_namespaces to confirm the index has namespaces, then retry.', + } + ) + ); } const ns = namespaces.find((n) => n.namespace === namespace); const suggestion = suggestQueryParams(ns?.metadata ?? null, queryText); if (!suggestion.namespace_found) { - return jsonErrorResponse({ - status: 'error', - message: `Namespace "${namespace}" not found in cached namespaces. Call list_namespaces and retry.`, - }); + return jsonErrorResponse( + validationToolError( + `Namespace "${namespace}" not found in cached namespaces. Call list_namespaces and retry.`, + 'namespace', + { + suggestion: + 'Use a namespace name returned by list_namespaces, then call list_namespaces again if the cache is stale.', + } + ) + ); } const selectedTool: GuidedToolName = resolveGuidedToolName(preferred_tool, suggestion); @@ -192,10 +217,7 @@ export function registerGuidedQueryTool(server: McpServer): void { }); } catch (error) { logToolError('guided_query', error); - return jsonErrorResponse({ - status: 'error', - message: getToolErrorMessage(error, 'Failed to execute guided query'), - }); + return jsonErrorResponse(classifyToolCatchError(error, 'Failed to execute guided query')); } } ); diff --git a/src/server/tools/keyword-search-tool.test.ts b/src/server/tools/keyword-search-tool.test.ts new file mode 100644 index 0000000..96e6f21 --- /dev/null +++ b/src/server/tools/keyword-search-tool.test.ts @@ -0,0 +1,91 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { getPineconeClient } from '../client-context.js'; +import { registerKeywordSearchTool } from './keyword-search-tool.js'; +import { assertToolErrorCode, createMockServer, makeSearchResult } from './test-helpers.js'; + +vi.mock('../client-context.js', () => ({ + getPineconeClient: vi.fn(), +})); + +const mockedGetClient = vi.mocked(getPineconeClient); + +describe('keyword_search tool handler', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockedGetClient.mockReturnValue({ + keywordSearch: vi.fn().mockResolvedValue([makeSearchResult()]), + getSparseIndexName: () => 'rag-hybrid-sparse', + } as never); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('returns VALIDATION when query_text is empty', async () => { + const server = createMockServer(); + registerKeywordSearchTool(server as never); + const raw = await server.getHandler('keyword_search')!({ + query_text: ' ', + namespace: 'ns', + top_k: 5, + }); + const err = assertToolErrorCode(raw, 'VALIDATION'); + expect(err.field).toBe('query_text'); + }); + + it('returns VALIDATION when namespace is empty', async () => { + const server = createMockServer(); + registerKeywordSearchTool(server as never); + const raw = await server.getHandler('keyword_search')!({ + query_text: 'hello', + namespace: ' ', + top_k: 5, + }); + const err = assertToolErrorCode(raw, 'VALIDATION'); + expect(err.field).toBe('namespace'); + }); + + it('returns VALIDATION when metadata_filter is invalid', async () => { + const server = createMockServer(); + registerKeywordSearchTool(server as never); + const raw = await server.getHandler('keyword_search')!({ + query_text: 'q', + namespace: 'ns', + top_k: 5, + metadata_filter: { bad: { $nope: true } }, + }); + const err = assertToolErrorCode(raw, 'VALIDATION'); + expect(err.field).toBe('bad.$nope'); + }); + + it('happy path returns success', async () => { + const server = createMockServer(); + registerKeywordSearchTool(server as never); + const raw = await server.getHandler('keyword_search')!({ + query_text: 'contracts', + namespace: 'wg21', + top_k: 3, + }); + expect((raw as { isError?: boolean }).isError).toBeFalsy(); + const text = (raw as { content: Array<{ text: string }> }).content[0].text; + const body = JSON.parse(text) as { status: string; result_count?: number }; + expect(body.status).toBe('success'); + expect(body.result_count).toBe(1); + }); + + it('returns PINECONE_ERROR when keywordSearch throws', async () => { + mockedGetClient.mockReturnValue({ + keywordSearch: vi.fn().mockRejectedValue(new Error('sparse error')), + getSparseIndexName: () => 'rag-hybrid-sparse', + } as never); + const server = createMockServer(); + registerKeywordSearchTool(server as never); + const raw = await server.getHandler('keyword_search')!({ + query_text: 'q', + namespace: 'ns', + top_k: 5, + }); + expect(assertToolErrorCode(raw, 'PINECONE_ERROR').code).toBe('PINECONE_ERROR'); + }); +}); diff --git a/src/server/tools/keyword-search-tool.ts b/src/server/tools/keyword-search-tool.ts index 88cfc04..7931e06 100644 --- a/src/server/tools/keyword-search-tool.ts +++ b/src/server/tools/keyword-search-tool.ts @@ -3,13 +3,14 @@ import { z } from 'zod'; import { MAX_TOP_K, MIN_TOP_K } from '../../constants.js'; import { getPineconeClient } from '../client-context.js'; import { formatQueryResultRows } from '../format-query-result.js'; -import { metadataFilterSchema, validateMetadataFilter } from '../metadata-filter.js'; -import { getToolErrorMessage, logToolError } from '../tool-error.js'; +import { metadataFilterSchema, validateMetadataFilterDetailed } from '../metadata-filter.js'; +import type { ToolError } from '../tool-error.js'; +import { classifyToolCatchError, logToolError, validationToolError } from '../tool-error.js'; import { jsonErrorResponse, jsonResponse } from '../tool-response.js'; -/** Response shape for keyword_search: same as query tool for consistency. */ +/** Success response shape for keyword_search (aligned with query tool fields). */ export interface KeywordSearchResponse { - status: 'success' | 'error'; + status: 'success'; query?: string; namespace?: string; index?: string; @@ -29,16 +30,19 @@ export interface KeywordSearchResponse { metadata?: Record; }>; fields?: string[]; - message?: string; } +type KeywordSearchExecResult = + | { ok: true; body: KeywordSearchResponse } + | { ok: false; error: ToolError }; + async function executeKeywordSearch(params: { query_text: string; namespace: string; top_k: number; metadata_filter?: Record; fields?: string[]; -}): Promise { +}): Promise { const { query_text, namespace, top_k, metadata_filter, fields } = params; const normalizedQuery = query_text.trim(); @@ -46,22 +50,25 @@ async function executeKeywordSearch(params: { if (!normalizedQuery) { return { - status: 'error', - message: 'Query text cannot be empty', + ok: false, + error: validationToolError('Query text cannot be empty', 'query_text'), }; } if (!normalizedNamespace) { return { - status: 'error', - message: 'Namespace cannot be empty', + ok: false, + error: validationToolError('Namespace cannot be empty', 'namespace'), }; } if (metadata_filter) { - const filterError = validateMetadataFilter(metadata_filter); + const filterError = validateMetadataFilterDetailed(metadata_filter); if (filterError) { - return { status: 'error', message: filterError }; + return { + ok: false, + error: validationToolError(filterError.message, filterError.field), + }; } } @@ -90,7 +97,7 @@ async function executeKeywordSearch(params: { if (fields?.length) { response.fields = fields; } - return response; + return { ok: true, body: response }; } /** Register the keyword_search tool on the MCP server. */ @@ -127,24 +134,20 @@ export function registerKeywordSearchTool(server: McpServer): void { }, async (params) => { try { - const response = await executeKeywordSearch({ + const result = await executeKeywordSearch({ query_text: params.query_text, namespace: params.namespace, top_k: params.top_k, metadata_filter: params.metadata_filter, fields: params.fields, }); - if (response.status === 'error') { - return jsonErrorResponse(response); + if (!result.ok) { + return jsonErrorResponse(result.error); } - return jsonResponse(response); + return jsonResponse(result.body); } catch (error) { logToolError('keyword_search', error); - const response: KeywordSearchResponse = { - status: 'error', - message: getToolErrorMessage(error, 'Keyword search failed'), - }; - return jsonErrorResponse(response); + return jsonErrorResponse(classifyToolCatchError(error, 'Keyword search failed')); } } ); diff --git a/src/server/tools/list-namespaces-tool.test.ts b/src/server/tools/list-namespaces-tool.test.ts index 639f8eb..e77d3a2 100644 --- a/src/server/tools/list-namespaces-tool.test.ts +++ b/src/server/tools/list-namespaces-tool.test.ts @@ -1,7 +1,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { getNamespacesWithCache } from '../namespaces-cache.js'; import { registerListNamespacesTool } from './list-namespaces-tool.js'; -import { createMockServer, parseToolJson } from './test-helpers.js'; +import { createMockServer, parseToolJson, assertToolErrorCode } from './test-helpers.js'; vi.mock('../namespaces-cache.js', () => ({ getNamespacesWithCache: vi.fn(), @@ -65,8 +65,7 @@ describe('list_namespaces tool handler', () => { const payload = raw as { isError?: boolean }; expect(payload.isError).toBe(true); - const body = parseToolJson(raw); - expect(body.status).toBe('error'); - expect(String(body.message)).toBe('Failed to list namespaces'); + const err = assertToolErrorCode(raw, 'PINECONE_ERROR'); + expect(err.message).toBe('Failed to list namespaces'); }); }); diff --git a/src/server/tools/list-namespaces-tool.ts b/src/server/tools/list-namespaces-tool.ts index 153ef1c..f7ae9fa 100644 --- a/src/server/tools/list-namespaces-tool.ts +++ b/src/server/tools/list-namespaces-tool.ts @@ -1,6 +1,6 @@ import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import { getNamespacesWithCache } from '../namespaces-cache.js'; -import { getToolErrorMessage, logToolError } from '../tool-error.js'; +import { classifyToolCatchError, logToolError } from '../tool-error.js'; import { jsonErrorResponse, jsonResponse } from '../tool-response.js'; /** Register the list_namespaces tool on the MCP server. */ @@ -37,11 +37,7 @@ export function registerListNamespacesTool(server: McpServer): void { return jsonResponse(response); } catch (error) { logToolError('list_namespaces', error); - const response = { - status: 'error', - message: getToolErrorMessage(error, 'Failed to list namespaces'), - }; - return jsonErrorResponse(response); + return jsonErrorResponse(classifyToolCatchError(error, 'Failed to list namespaces')); } } ); diff --git a/src/server/tools/namespace-router-tool.test.ts b/src/server/tools/namespace-router-tool.test.ts new file mode 100644 index 0000000..d1d9d27 --- /dev/null +++ b/src/server/tools/namespace-router-tool.test.ts @@ -0,0 +1,43 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { getNamespacesWithCache } from '../namespaces-cache.js'; +import { registerNamespaceRouterTool } from './namespace-router-tool.js'; +import { assertToolErrorCode, createMockServer, makeNamespaceCacheEntry } from './test-helpers.js'; + +vi.mock('../namespaces-cache.js', () => ({ + getNamespacesWithCache: vi.fn(), +})); + +const mockedGetNamespaces = vi.mocked(getNamespacesWithCache); + +describe('namespace_router tool handler', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockedGetNamespaces.mockResolvedValue({ + data: [makeNamespaceCacheEntry('papers')], + cache_hit: false, + expires_at: Date.now() + 1_800_000, + }); + }); + + it('returns VALIDATION when user_query is empty', async () => { + const server = createMockServer(); + registerNamespaceRouterTool(server as never); + const raw = await server.getHandler('namespace_router')!({ + user_query: ' ', + top_n: 3, + }); + const err = assertToolErrorCode(raw, 'VALIDATION'); + expect(err.field).toBe('user_query'); + }); + + it('returns PINECONE_ERROR when getNamespacesWithCache throws', async () => { + mockedGetNamespaces.mockRejectedValue(new Error('cache failure')); + const server = createMockServer(); + registerNamespaceRouterTool(server as never); + const raw = await server.getHandler('namespace_router')!({ + user_query: 'find cpp papers', + top_n: 2, + }); + expect(assertToolErrorCode(raw, 'PINECONE_ERROR').code).toBe('PINECONE_ERROR'); + }); +}); diff --git a/src/server/tools/namespace-router-tool.ts b/src/server/tools/namespace-router-tool.ts index 02e70d9..319b0c7 100644 --- a/src/server/tools/namespace-router-tool.ts +++ b/src/server/tools/namespace-router-tool.ts @@ -2,7 +2,7 @@ import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import { z } from 'zod'; import { getNamespacesWithCache } from '../namespaces-cache.js'; import { rankNamespacesByQuery } from '../namespace-router.js'; -import { getToolErrorMessage, logToolError } from '../tool-error.js'; +import { classifyToolCatchError, logToolError, validationToolError } from '../tool-error.js'; import { jsonErrorResponse, jsonResponse } from '../tool-response.js'; /** Register the namespace_router tool on the MCP server. */ @@ -30,7 +30,7 @@ export function registerNamespaceRouterTool(server: McpServer): void { try { const { user_query, top_n } = params; if (!user_query?.trim()) { - return jsonErrorResponse({ status: 'error', message: 'user_query cannot be empty' }); + return jsonErrorResponse(validationToolError('user_query cannot be empty', 'user_query')); } const { data, cache_hit } = await getNamespacesWithCache(); const ranked = rankNamespacesByQuery(user_query.trim(), data, top_n); @@ -45,10 +45,7 @@ export function registerNamespaceRouterTool(server: McpServer): void { return jsonResponse(response); } catch (error) { logToolError('namespace_router', error); - return jsonErrorResponse({ - status: 'error', - message: getToolErrorMessage(error, 'Failed to route namespace'), - }); + return jsonErrorResponse(classifyToolCatchError(error, 'Failed to route namespace')); } } ); diff --git a/src/server/tools/query-documents-tool.test.ts b/src/server/tools/query-documents-tool.test.ts index c68b452..b41768e 100644 --- a/src/server/tools/query-documents-tool.test.ts +++ b/src/server/tools/query-documents-tool.test.ts @@ -4,7 +4,12 @@ import { getPineconeClient } from '../client-context.js'; import { reassembleByDocument } from '../reassemble-documents.js'; import * as suggestionFlow from '../suggestion-flow.js'; import { registerQueryDocumentsTool } from './query-documents-tool.js'; -import { createMockServer, makeSearchResult, parseToolJson } from './test-helpers.js'; +import { + assertToolErrorCode, + createMockServer, + makeSearchResult, + parseToolJson, +} from './test-helpers.js'; vi.mock('../client-context.js', () => ({ getPineconeClient: vi.fn(), @@ -90,7 +95,9 @@ describe('query_documents tool handler', () => { expect((raw as { isError?: boolean }).isError).toBe(true); expect(query).not.toHaveBeenCalled(); - expect(parseToolJson(raw).message).toBe('query_text cannot be empty'); + const err = assertToolErrorCode(raw, 'VALIDATION'); + expect(err.field).toBe('query_text'); + expect(err.message).toBe('query_text cannot be empty'); }); it('returns flow error when suggest_query_params gate fails', async () => { @@ -104,14 +111,27 @@ describe('query_documents tool handler', () => { registerQueryDocumentsTool(server as never); const query = mockedGetClient().query as ReturnType; - const body = parseToolJson( - await server.getHandler('query_documents')!({ - query_text: 'ok', - namespace: 'wg21', - }) - ); + const raw = await server.getHandler('query_documents')!({ + query_text: 'ok', + namespace: 'wg21', + }); - expect(body.status).toBe('error'); + const err = assertToolErrorCode(raw, 'FLOW_GATE'); + expect(err.suggestion).toBe("Call suggest_query_params for namespace 'wg21' first"); + expect(query).not.toHaveBeenCalled(); + }); + + it('returns VALIDATION for invalid metadata_filter', async () => { + const server = createMockServer(); + registerQueryDocumentsTool(server as never); + const query = mockedGetClient().query as ReturnType; + const raw = await server.getHandler('query_documents')!({ + query_text: 'ok', + namespace: 'wg21', + metadata_filter: { x: { $in: {} } }, + }); + const err = assertToolErrorCode(raw, 'VALIDATION'); + expect(err.field).toBe('x.$in'); expect(query).not.toHaveBeenCalled(); }); @@ -125,13 +145,13 @@ describe('query_documents tool handler', () => { const server = createMockServer(); registerQueryDocumentsTool(server as never); - const body = parseToolJson( - await server.getHandler('query_documents')!({ - query_text: 'ok', - namespace: 'wg21', - }) - ); + const raw = await server.getHandler('query_documents')!({ + query_text: 'ok', + namespace: 'wg21', + }); - expect(body.message).toContain('expired'); + const err = assertToolErrorCode(raw, 'FLOW_GATE'); + expect(err.message).toContain('expired'); + expect(err.suggestion).toBe("Call suggest_query_params for namespace 'wg21' first"); }); }); diff --git a/src/server/tools/query-documents-tool.ts b/src/server/tools/query-documents-tool.ts index 61abe76..95acaf3 100644 --- a/src/server/tools/query-documents-tool.ts +++ b/src/server/tools/query-documents-tool.ts @@ -6,10 +6,15 @@ import { QUERY_DOCUMENTS_MAX_CHUNKS, } from '../../constants.js'; import { getPineconeClient } from '../client-context.js'; -import { metadataFilterSchema, validateMetadataFilter } from '../metadata-filter.js'; +import { metadataFilterSchema, validateMetadataFilterDetailed } from '../metadata-filter.js'; import { reassembleByDocument } from '../reassemble-documents.js'; import { requireSuggested } from '../suggestion-flow.js'; -import { getToolErrorMessage, logToolError } from '../tool-error.js'; +import { + classifyToolCatchError, + flowGateToolError, + logToolError, + validationToolError, +} from '../tool-error.js'; import { jsonErrorResponse, jsonResponse } from '../tool-response.js'; /** @@ -73,20 +78,19 @@ export function registerQueryDocumentsTool(server: McpServer): void { } = params; if (!query_text?.trim()) { - return jsonErrorResponse({ - status: 'error', - message: 'query_text cannot be empty', - }); + return jsonErrorResponse(validationToolError('query_text cannot be empty', 'query_text')); } if (metadata_filter) { - const err = validateMetadataFilter(metadata_filter); - if (err) return jsonErrorResponse({ status: 'error', message: err }); + const err = validateMetadataFilterDetailed(metadata_filter); + if (err) { + return jsonErrorResponse(validationToolError(err.message, err.field)); + } } const flowCheck = requireSuggested(namespace); if (!flowCheck.ok) { - return jsonErrorResponse({ status: 'error', message: flowCheck.message }); + return jsonErrorResponse(flowGateToolError(namespace, flowCheck.message)); } const chunkLimit = Math.min(QUERY_DOCUMENTS_MAX_CHUNKS, top_k * CHUNKS_PER_DOCUMENT); @@ -124,10 +128,9 @@ export function registerQueryDocumentsTool(server: McpServer): void { }); } catch (error) { logToolError('query_documents', error); - return jsonErrorResponse({ - status: 'error', - message: getToolErrorMessage(error, 'Failed to query and reassemble documents'), - }); + return jsonErrorResponse( + classifyToolCatchError(error, 'Failed to query and reassemble documents') + ); } } ); diff --git a/src/server/tools/query-tool.test.ts b/src/server/tools/query-tool.test.ts index fe40079..1b004c0 100644 --- a/src/server/tools/query-tool.test.ts +++ b/src/server/tools/query-tool.test.ts @@ -3,7 +3,12 @@ import { FAST_QUERY_FIELDS } from '../../constants.js'; import { getPineconeClient } from '../client-context.js'; import * as suggestionFlow from '../suggestion-flow.js'; import { registerQueryTool } from './query-tool.js'; -import { createMockServer, makeSearchResult, parseToolJson } from './test-helpers.js'; +import { + assertToolErrorCode, + createMockServer, + makeSearchResult, + parseToolJson, +} from './test-helpers.js'; vi.mock('../client-context.js', () => ({ getPineconeClient: vi.fn(), @@ -102,8 +107,9 @@ describe('query tool handler (preset-driven)', () => { expect((raw as { isError?: boolean }).isError).toBe(true); expect(query).not.toHaveBeenCalled(); - const body = parseToolJson(raw); - expect(body.message).toBe('Query text cannot be empty'); + const err = assertToolErrorCode(raw, 'VALIDATION'); + expect(err.field).toBe('query_text'); + expect(err.message).toBe('Query text cannot be empty'); }); it('query: returns flow error when suggest_query_params was not called first', async () => { @@ -126,8 +132,9 @@ describe('query tool handler (preset-driven)', () => { expect((raw as { isError?: boolean }).isError).toBe(true); expect(query).not.toHaveBeenCalled(); - const body = parseToolJson(raw); - expect(body.message).toBe( + const err = assertToolErrorCode(raw, 'FLOW_GATE'); + expect(err.suggestion).toBe("Call suggest_query_params for namespace 'wg21' first"); + expect(err.message).toBe( 'Flow requires suggest_query_params first. Call suggest_query_params with namespace and user_query before query/count tools.' ); }); @@ -143,17 +150,33 @@ describe('query tool handler (preset-driven)', () => { const server = createMockServer(); registerQueryTool(server as never); - const body = parseToolJson( - await server.getHandler('query')!({ - query_text: 'hello', - namespace: 'wg21', - top_k: 10, - preset: 'full', - }) - ); + const raw = await server.getHandler('query')!({ + query_text: 'hello', + namespace: 'wg21', + top_k: 10, + preset: 'full', + }); + + const err = assertToolErrorCode(raw, 'FLOW_GATE'); + expect(err.message).toBe(expiredMsg); + expect(err.suggestion).toBe("Call suggest_query_params for namespace 'wg21' first"); + }); - expect(body.status).toBe('error'); - expect(body.message).toBe(expiredMsg); + it('query: returns VALIDATION for invalid metadata_filter', async () => { + const server = createMockServer(); + registerQueryTool(server as never); + const query = mockedGetClient().query as ReturnType; + const raw = await server.getHandler('query')!({ + query_text: 'hello', + namespace: 'wg21', + top_k: 5, + preset: 'full', + metadata_filter: { year: { $badop: 1 } }, + }); + expect((raw as { isError?: boolean }).isError).toBe(true); + const err = assertToolErrorCode(raw, 'VALIDATION'); + expect(err.field).toBe('year.$badop'); + expect(query).not.toHaveBeenCalled(); }); it('query: surfaces unreranked hits when client returns reranked:false (rerank fallback shape)', async () => { diff --git a/src/server/tools/query-tool.ts b/src/server/tools/query-tool.ts index d83276c..e4269c2 100644 --- a/src/server/tools/query-tool.ts +++ b/src/server/tools/query-tool.ts @@ -4,9 +4,14 @@ import { FAST_QUERY_FIELDS, MAX_TOP_K, MIN_TOP_K } from '../../constants.js'; import type { QueryResponse } from '../../types.js'; import { getPineconeClient } from '../client-context.js'; import { formatQueryResultRows } from '../format-query-result.js'; -import { metadataFilterSchema, validateMetadataFilter } from '../metadata-filter.js'; +import { metadataFilterSchema, validateMetadataFilterDetailed } from '../metadata-filter.js'; import { requireSuggested } from '../suggestion-flow.js'; -import { getToolErrorMessage, logToolError } from '../tool-error.js'; +import { + classifyToolCatchError, + flowGateToolError, + logToolError, + validationToolError, +} from '../tool-error.js'; import { jsonErrorResponse, jsonResponse } from '../tool-response.js'; type QueryMode = 'query' | 'query_fast' | 'query_detailed'; @@ -26,27 +31,21 @@ async function executeQuery(params: QueryExecParams) { const { query_text, namespace, top_k, use_reranking, metadata_filter, fields, mode } = params; try { if (!query_text.trim()) { - const response: QueryResponse = { - status: 'error', - message: 'Query text cannot be empty', - }; - return jsonErrorResponse(response); + return jsonErrorResponse(validationToolError('Query text cannot be empty', 'query_text')); } if (metadata_filter) { - const filterValidationError = validateMetadataFilter(metadata_filter); + const filterValidationError = validateMetadataFilterDetailed(metadata_filter); if (filterValidationError) { - const response: QueryResponse = { - status: 'error', - message: filterValidationError, - }; - return jsonErrorResponse(response); + return jsonErrorResponse( + validationToolError(filterValidationError.message, filterValidationError.field) + ); } } const flowCheck = requireSuggested(namespace); if (!flowCheck.ok) { - return jsonErrorResponse({ status: 'error', message: flowCheck.message }); + return jsonErrorResponse(flowGateToolError(namespace, flowCheck.message)); } const client = getPineconeClient(); @@ -74,11 +73,9 @@ async function executeQuery(params: QueryExecParams) { return jsonResponse(response); } catch (error) { logToolError(mode, error); - const response: QueryResponse = { - status: 'error', - message: getToolErrorMessage(error, 'An error occurred while processing your query'), - }; - return jsonErrorResponse(response); + return jsonErrorResponse( + classifyToolCatchError(error, 'An error occurred while processing your query') + ); } } diff --git a/src/server/tools/suggest-query-params-tool.test.ts b/src/server/tools/suggest-query-params-tool.test.ts index 3c6e317..472a573 100644 --- a/src/server/tools/suggest-query-params-tool.test.ts +++ b/src/server/tools/suggest-query-params-tool.test.ts @@ -2,7 +2,12 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { getNamespacesWithCache } from '../namespaces-cache.js'; import { markSuggested } from '../suggestion-flow.js'; import { registerSuggestQueryParamsTool } from './suggest-query-params-tool.js'; -import { createMockServer, makeNamespaceCacheEntry, parseToolJson } from './test-helpers.js'; +import { + createMockServer, + makeNamespaceCacheEntry, + parseToolJson, + assertToolErrorCode, +} from './test-helpers.js'; vi.mock('../namespaces-cache.js', () => ({ getNamespacesWithCache: vi.fn(), @@ -83,9 +88,9 @@ describe('suggest_query_params tool handler', () => { }); expect((raw as { isError?: boolean }).isError).toBe(true); - const body = parseToolJson(raw); - expect(body.status).toBe('error'); - expect(body.message).toBe('user_query cannot be empty'); + const err = assertToolErrorCode(raw, 'VALIDATION'); + expect(err.field).toBe('user_query'); + expect(err.message).toBe('user_query cannot be empty'); }); it('returns error when namespace cache fails', async () => { @@ -99,8 +104,7 @@ describe('suggest_query_params tool handler', () => { }); expect((raw as { isError?: boolean }).isError).toBe(true); - const body = parseToolJson(raw); - expect(body.status).toBe('error'); - expect(String(body.message)).toBe('Failed to suggest query params'); + const err = assertToolErrorCode(raw, 'PINECONE_ERROR'); + expect(err.message).toBe('Failed to suggest query params'); }); }); diff --git a/src/server/tools/suggest-query-params-tool.ts b/src/server/tools/suggest-query-params-tool.ts index 515dae5..10f929b 100644 --- a/src/server/tools/suggest-query-params-tool.ts +++ b/src/server/tools/suggest-query-params-tool.ts @@ -3,7 +3,7 @@ import { z } from 'zod'; import { getNamespacesWithCache } from '../namespaces-cache.js'; import { suggestQueryParams } from '../query-suggestion.js'; import { markSuggested } from '../suggestion-flow.js'; -import { getToolErrorMessage, logToolError } from '../tool-error.js'; +import { classifyToolCatchError, logToolError, validationToolError } from '../tool-error.js'; import { jsonErrorResponse, jsonResponse } from '../tool-response.js'; /** Register the suggest_query_params tool on the MCP server. */ @@ -33,7 +33,7 @@ export function registerSuggestQueryParamsTool(server: McpServer): void { try { const { namespace, user_query } = params; if (!user_query?.trim()) { - return jsonErrorResponse({ status: 'error', message: 'user_query cannot be empty' }); + return jsonErrorResponse(validationToolError('user_query cannot be empty', 'user_query')); } const { data: namespacesInfo, cache_hit } = await getNamespacesWithCache(); const ns = namespacesInfo.find((n) => n.namespace === namespace); @@ -54,10 +54,7 @@ export function registerSuggestQueryParamsTool(server: McpServer): void { return jsonResponse(response); } catch (error) { logToolError('suggest_query_params', error); - return jsonErrorResponse({ - status: 'error', - message: getToolErrorMessage(error, 'Failed to suggest query params'), - }); + return jsonErrorResponse(classifyToolCatchError(error, 'Failed to suggest query params')); } } ); diff --git a/src/server/tools/test-helpers.ts b/src/server/tools/test-helpers.ts index f0db291..7dcd118 100644 --- a/src/server/tools/test-helpers.ts +++ b/src/server/tools/test-helpers.ts @@ -1,4 +1,6 @@ import type { SearchResult } from '../../types.js'; +import type { ToolError, ToolErrorCode } from '../tool-error.js'; +import { toolErrorSchema } from '../tool-error.js'; /** Handler invoked by MCP tool registration (params shape varies by tool). */ export type ToolHandler = (params: Record) => Promise; @@ -38,6 +40,31 @@ export function parseToolJson(payload: unknown): Record { return JSON.parse(text) as Record; } +/** Parse MCP tool error JSON from a handler return value (expects `isError: true` on the envelope). */ +export function assertToolError(payload: unknown): ToolError { + const envelope = payload as { isError?: unknown }; + if (envelope?.isError !== true) { + throw new Error('Expected MCP tool response with isError: true'); + } + const raw = parseToolJson(payload); + return toolErrorSchema.parse(raw); +} + +/** + * Like {@link assertToolError}, but asserts `code` and narrows the union so + * `field` / `suggestion` are type-safe per variant. + */ +export function assertToolErrorCode( + payload: unknown, + code: C +): Extract { + const err = assertToolError(payload); + if (err.code !== code) { + throw new Error(`Expected tool error code ${code}, got ${err.code}`); + } + return err as Extract; +} + export function makeSearchResult(overrides?: Partial): SearchResult { return { id: 'hit-1', diff --git a/src/types.ts b/src/types.ts index 81ec651..965da6d 100644 --- a/src/types.ts +++ b/src/types.ts @@ -123,7 +123,7 @@ export type KeywordIndexNamespacesResult = | { ok: false; error: string }; export interface QueryResponse { - status: 'success' | 'error'; + status: 'success'; mode?: 'query' | 'query_fast' | 'query_detailed'; query?: string; namespace?: string; @@ -132,7 +132,6 @@ export interface QueryResponse { /** Present when the query requested specific fields. */ fields?: string[]; results?: QueryResultRowShape[]; - message?: string; } /** Internal merged hit shape before rerank (dense + sparse deduped). */