From 378cebcaff235e6d56ac545d06b75fd7c3935eea Mon Sep 17 00:00:00 2001 From: zhao Date: Wed, 22 Jul 2026 02:15:56 +0800 Subject: [PATCH 1/4] redact credentials at tool-error construction and in source_errors --- docs/SECURITY.md | 9 +++- src/core/pinecone/rerank.ts | 4 +- src/core/server/redaction.test.ts | 22 ++++++++-- src/core/server/retry.ts | 4 +- src/core/server/source-registry.test.ts | 37 +++++++++++++--- src/core/server/source-registry.ts | 5 +-- src/core/server/tool-error.ts | 10 ++--- .../list-namespaces-tool.context.test.ts | 43 +++++++++++++------ src/core/server/tools/test-helpers.ts | 35 +++++++++++++++ src/index.ts | 5 ++- src/logger.ts | 24 +++++++++++ 11 files changed, 159 insertions(+), 39 deletions(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 5330067..7c2cb32 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -19,9 +19,14 @@ Logs go to **stderr**; use `PINECONE_READ_ONLY_MCP_LOG_FORMAT=json` for pipeline ## MCP response redaction -Tool responses returned to MCP clients (and LLM consumers) are sanitized in `src/core/server/tool-response.ts` via `redactSensitiveFields()` before JSON serialization. Only known sensitive keys are masked (`message`, `suggestion`, `degradation_reason`); document metadata UUIDs and other non-sensitive fields are preserved. +Tool responses are sanitized at construction and at the serialization boundary: -This covers tool error payloads, hybrid degradation reasons, and SDK error text surfaced in DEBUG log mode — not only stderr logs. +- `src/core/server/tool-error.ts`: `pineconeToolError` and `timeoutToolError` apply `redactApiKey` to `message` and `suggestion` before the `ToolError` object is built, so DEBUG-mode SDK error text is masked at the tool-error layer. +- `src/logger.ts`: `redactErrorMessage` redacts credential-shaped substrings from error values; `redactSensitiveFields()` masks known sensitive keys (`message`, `suggestion`, `degradation_reason`) and every string value nested directly under `source_errors` (keyed by source name, not a sensitive field name). +- `src/core/server/source-registry.ts`: multi-source `source_errors` rejection messages are redacted via `redactErrorMessage` when the per-source error map is built. +- `src/core/server/tool-response.ts`: `jsonResponse` / `jsonErrorResponse` call `redactSensitiveFields()` before JSON serialization (boundary layer). + +Document metadata UUIDs and other non-sensitive fields are preserved at every layer. ## Private config content (descriptions and schemas) diff --git a/src/core/pinecone/rerank.ts b/src/core/pinecone/rerank.ts index 28d7624..71b71be 100644 --- a/src/core/pinecone/rerank.ts +++ b/src/core/pinecone/rerank.ts @@ -3,7 +3,7 @@ */ import type { Pinecone } from '@pinecone-database/pinecone'; -import { error as logError, redactApiKey } from '../../logger.js'; +import { error as logError, redactErrorMessage } from '../../logger.js'; import { DEFAULT_REQUEST_TIMEOUT_MS } from '../config.js'; import { runWithPolicy } from '../server/retry.js'; import type { MergedHit, SearchResult } from '../../types.js'; @@ -66,7 +66,7 @@ export async function rerankResults( return { results: reranked, degraded: false }; } catch (error) { logError('Error reranking results', error); - const msg = redactApiKey(error instanceof Error ? error.message : String(error)); + const msg = redactErrorMessage(error); return { results: results.slice(0, topN).map((result) => ({ id: result._id || '', diff --git a/src/core/server/redaction.test.ts b/src/core/server/redaction.test.ts index d97e616..00c6d6b 100644 --- a/src/core/server/redaction.test.ts +++ b/src/core/server/redaction.test.ts @@ -15,6 +15,8 @@ import { makeNamespaceCacheEntry, makeSearchResult, parseToolJson, + PCSK_KEY, + UUID_KEY, } from './tools/test-helpers.js'; vi.mock('./client-context.js', () => ({ @@ -33,9 +35,6 @@ vi.mock('./suggestion-flow.js', async (importOriginal) => { }; }); -const PCSK_KEY = 'pcsk_abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ'; -const UUID_KEY = '12345678-1234-1234-1234-123456789abc'; - const flowOk = { ok: true as const, flow: { @@ -78,6 +77,19 @@ describe('redactSensitiveFields', () => { expect(out.message).not.toContain(PCSK_KEY); expect(out.results[0].metadata.document_id).toBe(UUID_KEY); }); + + it('redacts string values nested under source_errors regardless of key name', () => { + const input = { + source_errors: { + api_key_2: `unreachable: ${PCSK_KEY}`, + api_key_3: 'benign message with no token', + }, + }; + const out = redactSensitiveFields(input) as typeof input; + expect(out.source_errors.api_key_2).not.toContain(PCSK_KEY); + expect(out.source_errors.api_key_2).toContain('***'); + expect(out.source_errors.api_key_3).toBe('benign message with no token'); + }); }); describe('MCP response redaction', () => { @@ -99,6 +111,8 @@ describe('MCP response redaction', () => { const err = pineconeToolError(`PINECONE_ERROR with ${PCSK_KEY}`, { suggestion: `retry with ${PCSK_KEY}`, }); + expect(err.message).not.toContain(PCSK_KEY); + expect(err.suggestion).not.toContain(PCSK_KEY); const text = jsonErrorResponse(err).content[0]!.text; expect(text).not.toContain(PCSK_KEY); expect(text).toContain('***'); @@ -107,6 +121,8 @@ describe('MCP response redaction', () => { it('redacts classifyToolCatchError output in DEBUG mode', () => { setLogLevel('DEBUG'); const err = classifyToolCatchError(new Error(`SDK auth failed ${PCSK_KEY}`), 'fallback'); + expect(err.message).not.toContain(PCSK_KEY); + expect(err.message).toContain('***'); const text = jsonErrorResponse(err).content[0]!.text; expect(text).not.toContain(PCSK_KEY); }); diff --git a/src/core/server/retry.ts b/src/core/server/retry.ts index a8a84db..b8a9c90 100644 --- a/src/core/server/retry.ts +++ b/src/core/server/retry.ts @@ -7,7 +7,7 @@ * waiter still rejects immediately on timeout. */ -import { warn, redactApiKey } from '../../logger.js'; +import { warn, redactErrorMessage } from '../../logger.js'; /** Matches {@link withTimeout} rejection message prefix; used by tool-error and callers. */ export const APP_TIMEOUT_PATTERN = /^Timeout after \d+ms while waiting for /i; @@ -140,7 +140,7 @@ export function runWithPolicy( backoffMs: options.backoffMs, shouldRetry: transientShouldRetry, onRetry: (attempt, error) => { - const msg = redactApiKey(error instanceof Error ? error.message : String(error)); + const msg = redactErrorMessage(error); warn(`Retrying ${options.label} (attempt ${attempt})`, msg); }, }); diff --git a/src/core/server/source-registry.test.ts b/src/core/server/source-registry.test.ts index e34aef9..c80e021 100644 --- a/src/core/server/source-registry.test.ts +++ b/src/core/server/source-registry.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi } from 'vitest'; import { buildSourceRegistry } from './source-registry.js'; import type { SourceDefinition } from './source-config.js'; +import { PCSK_KEY, makeFailingSourceClient } from './tools/test-helpers.js'; const sources: SourceDefinition[] = [ { name: 'api_key_1', apiKey: 'k1', indexName: 'idx-a', sparseIndexName: 'idx-a-sparse' }, @@ -67,14 +68,14 @@ describe('SourceRegistry', () => { it('returns partial namespaces and source_errors when one source fails', async () => { const client1 = mockClient('api_key_1'); - const client2 = { - listNamespacesWithMetadata: vi.fn().mockRejectedValue(new Error('api_key_2 unreachable')), - checkIndexes: vi.fn().mockResolvedValue({ ok: true, errors: [] }), - getSparseIndexName: () => 'api_key_2-sparse', - }; const clients = new Map([ ['api_key_1', client1 as never], - ['api_key_2', client2 as never], + [ + 'api_key_2', + makeFailingSourceClient('api_key_2 unreachable', { + sparseIndexName: 'api_key_2-sparse', + }) as never, + ], ]); const registry = buildSourceRegistry({ sources, @@ -91,6 +92,30 @@ describe('SourceRegistry', () => { expect(result.cache_hit).toBe(false); }); + it('redacts credential-shaped tokens in source_errors at construction time', async () => { + const client1 = mockClient('api_key_1'); + const clients = new Map([ + ['api_key_1', client1 as never], + [ + 'api_key_2', + makeFailingSourceClient(`api_key_2 unreachable: auth failed ${PCSK_KEY}`, { + sparseIndexName: 'api_key_2-sparse', + }) as never, + ], + ]); + const registry = buildSourceRegistry({ + sources, + defaultSource: 'api_key_1', + cacheTtlMs: 60_000, + defaultTopK: 10, + requestTimeoutMs: 15_000, + clients, + }); + const result = await registry.getAllNamespacesWithCache(); + expect(result.source_errors?.['api_key_2']).not.toContain(PCSK_KEY); + expect(result.source_errors?.['api_key_2']).toContain('***'); + }); + it('aggregates warnings across sources in getAllNamespacesWithCache', async () => { const client1 = { listNamespacesWithMetadata: vi.fn().mockResolvedValue({ diff --git a/src/core/server/source-registry.ts b/src/core/server/source-registry.ts index 9a05a08..a046fd4 100644 --- a/src/core/server/source-registry.ts +++ b/src/core/server/source-registry.ts @@ -3,6 +3,7 @@ */ import { PineconeClient } from '../pinecone-client.js'; +import { redactErrorMessage } from '../../logger.js'; import type { NamespaceInfo } from './server-context.js'; import { fetchNamespacesWithDeclaredConfig, type NamespacesCacheEntry } from './namespace-cache.js'; import type { SourceDefinition } from './source-config.js'; @@ -151,9 +152,7 @@ export class SourceRegistry { maxExpires = Math.max(maxExpires, outcome.value.result.expires_at); } else { cache_hit = false; - const message = - outcome.reason instanceof Error ? outcome.reason.message : String(outcome.reason); - source_errors[name] = message; + source_errors[name] = redactErrorMessage(outcome.reason); } } const expires_at = maxExpires > 0 ? maxExpires : Date.now() + this.cacheTtlMs; diff --git a/src/core/server/tool-error.ts b/src/core/server/tool-error.ts index f299ce4..189e279 100644 --- a/src/core/server/tool-error.ts +++ b/src/core/server/tool-error.ts @@ -4,7 +4,7 @@ */ import { z } from 'zod'; -import { getLogLevel, error as logError, info } from '../../logger.js'; +import { getLogLevel, redactApiKey, error as logError, info } from '../../logger.js'; import { isAppTimeoutError } from './retry.js'; /** User-facing error message: detailed in DEBUG, generic otherwise. */ @@ -111,18 +111,18 @@ export function pineconeToolError( ): ToolError { return { code: 'PINECONE_ERROR', - message, + message: redactApiKey(message), recoverable: options?.recoverable ?? false, - ...(options?.suggestion ? { suggestion: options.suggestion } : {}), + ...(options?.suggestion ? { suggestion: redactApiKey(options.suggestion) } : {}), }; } export function timeoutToolError(message: string, options?: { suggestion?: string }): ToolError { return { code: 'TIMEOUT', - message, + message: redactApiKey(message), recoverable: true, - suggestion: options?.suggestion ?? DEFAULT_TIMEOUT_SUGGESTION, + suggestion: redactApiKey(options?.suggestion ?? DEFAULT_TIMEOUT_SUGGESTION), }; } diff --git a/src/core/server/tools/list-namespaces-tool.context.test.ts b/src/core/server/tools/list-namespaces-tool.context.test.ts index f5c25ca..cf7d5e4 100644 --- a/src/core/server/tools/list-namespaces-tool.context.test.ts +++ b/src/core/server/tools/list-namespaces-tool.context.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it, vi } from 'vitest'; +import { setLogLevel } from '../../../logger.js'; import { registerListNamespacesTool } from './list-namespaces-tool.js'; import { listNamespacesResponseSchema } from '../response-schemas.js'; import { @@ -6,9 +7,10 @@ import { createMultiSourceTestContext, createTestServerContext, expectMatchesResponseSchema, - makeMockPineconeClient, + makePartialFailureMultiSourceClients, mockNamespacesWithMetadataResult, parseToolJson, + PCSK_KEY, } from './test-helpers.js'; describe('list_namespaces tool handler (ServerContext instance path)', () => { @@ -73,19 +75,7 @@ describe('list_namespaces tool handler (ServerContext instance path)', () => { describe('list_namespaces tool handler (multi-source)', () => { it('tags namespaces with source and propagates source_errors on partial failure', async () => { - const client1 = makeMockPineconeClient(['wg21']); - const client2 = { - listNamespacesWithMetadata: vi.fn().mockRejectedValue(new Error('api_key_2 unreachable')), - query: vi.fn(), - count: vi.fn(), - keywordSearch: vi.fn(), - checkIndexes: vi.fn().mockResolvedValue({ ok: true, errors: [] }), - getSparseIndexName: () => 'sparse', - }; - const clients = new Map([ - ['api_key_1', client1], - ['api_key_2', client2], - ]); + const clients = makePartialFailureMultiSourceClients('api_key_2 unreachable'); const { ctx } = createMultiSourceTestContext({ clients }); const server = createMockServer(); @@ -99,6 +89,31 @@ describe('list_namespaces tool handler (multi-source)', () => { expect(body['cache_hit']).toBe(false); }); + it('redacts a credential-shaped token embedded in a source_errors rejection message at DEBUG log level', async () => { + setLogLevel('DEBUG'); + try { + const clients = makePartialFailureMultiSourceClients( + `api_key_2 unreachable: auth failed ${PCSK_KEY}` + ); + const { ctx } = createMultiSourceTestContext({ clients }); + + const server = createMockServer(); + registerListNamespacesTool(server as never, ctx); + const raw = await server.getHandler('list_namespaces')!({}); + const text = raw.content[0]!.text; + expect(text).not.toContain(PCSK_KEY); + expect(text).toContain('***'); + + const body = parseToolJson(raw); + expectMatchesResponseSchema(listNamespacesResponseSchema, body); + expect(body['source_errors']).toMatchObject({ + api_key_2: expect.not.stringContaining(PCSK_KEY), + }); + } finally { + setLogLevel('INFO'); + } + }); + it('surfaces schema_source and config_warnings when declarations mismatch live data', async () => { const client1 = { listNamespacesWithMetadata: vi.fn().mockResolvedValue({ diff --git a/src/core/server/tools/test-helpers.ts b/src/core/server/tools/test-helpers.ts index be00f1a..3bfd693 100644 --- a/src/core/server/tools/test-helpers.ts +++ b/src/core/server/tools/test-helpers.ts @@ -15,6 +15,12 @@ import { import type { ToolError, ToolErrorCode } from '../tool-error.js'; import { toolErrorSchema } from '../tool-error.js'; +/** Credential-shaped test token for redaction assertions. Not a real key. */ +export const PCSK_KEY = 'pcsk_abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ'; + +/** Non-sensitive UUID for regression tests (must not be masked). */ +export const UUID_KEY = '12345678-1234-1234-1234-123456789abc'; + /** Handler invoked by MCP tool registration (params shape varies by tool). */ export type ToolHandler = (params: Record) => Promise; @@ -216,6 +222,35 @@ export function makeMockPineconeClient( }; } +/** Mock client that rejects namespace listing with the given error message. */ +export function makeFailingSourceClient( + rejectionMessage: string, + options?: { sparseIndexName?: string } +) { + return { + listNamespacesWithMetadata: vi.fn().mockRejectedValue(new Error(rejectionMessage)), + query: vi.fn(), + count: vi.fn(), + keywordSearch: vi.fn(), + checkIndexes: vi.fn().mockResolvedValue({ ok: true, errors: [] }), + getSparseIndexName: () => options?.sparseIndexName ?? 'sparse', + }; +} + +/** Two-source client map: api_key_1 succeeds; api_key_2 fails with rejectionMessage. */ +export function makePartialFailureMultiSourceClients( + rejectionMessage: string, + options?: { successNamespaces?: string[] } +) { + return new Map([ + ['api_key_1', makeMockPineconeClient(options?.successNamespaces ?? ['wg21'])], + [ + 'api_key_2', + makeFailingSourceClient(rejectionMessage, { sparseIndexName: 'api_key_2-sparse' }), + ], + ]); +} + export type MultiSourceTestContext = { ctx: CoreServerContext; clients: Map>; diff --git a/src/index.ts b/src/index.ts index 875732a..2be3a2d 100644 --- a/src/index.ts +++ b/src/index.ts @@ -25,6 +25,7 @@ import { error as logError, info as logInfo, redactApiKey, + redactErrorMessage, setLogFormat, setLogLevel, warn as logWarn, @@ -50,7 +51,7 @@ function buildConfigOrExit(): AllianceServerConfig { try { return resolveAllianceConfig(parsed.overrides); } catch (err) { - const message = redactApiKey(err instanceof Error ? err.message : String(err)); + const message = redactErrorMessage(err); process.stderr.write(`Error: ${message}\n`); process.exit(1); } @@ -199,7 +200,7 @@ async function main(): Promise { process.exit(0); }); } catch (error) { - const summary = redactApiKey(error instanceof Error ? error.message : String(error)); + const summary = redactErrorMessage(error); logError(`Fatal error in main(): ${summary}`); process.exit(1); } diff --git a/src/logger.ts b/src/logger.ts index d041785..bfd73bb 100644 --- a/src/logger.ts +++ b/src/logger.ts @@ -65,12 +65,21 @@ export function redactApiKey(s: string): string { return out; } +/** Redact credential-shaped substrings from an error or string value. */ +export function redactErrorMessage(error: unknown): string { + return redactApiKey(error instanceof Error ? error.message : String(error)); +} + /** MCP response keys whose string values may carry SDK errors or credentials. */ const SENSITIVE_RESPONSE_KEYS = new Set(['message', 'suggestion', 'degradation_reason']); +/** Response key whose nested object values (keyed by source name) still need redaction. */ +const SOURCE_ERRORS_KEY = 'source_errors'; + /** * Recursively redact sensitive string fields in MCP tool payloads. * Only keys in {@link SENSITIVE_RESPONSE_KEYS} are masked; other strings (e.g. document UUIDs in metadata) are preserved. + * `source_errors` is a special case: its values are keyed by source name, so every string value directly under it is redacted. */ export function redactSensitiveFields( value: unknown, @@ -90,6 +99,21 @@ export function redactSensitiveFields( for (const [key, nested] of Object.entries(value as Record)) { if (typeof nested === 'string' && SENSITIVE_RESPONSE_KEYS.has(key)) { out[key] = redactApiKey(nested); + } else if ( + key === SOURCE_ERRORS_KEY && + nested !== null && + typeof nested === 'object' && + !Array.isArray(nested) + ) { + seen.add(nested as object); + const sourceErrors: Record = {}; + for (const [sourceName, sourceValue] of Object.entries(nested as Record)) { + sourceErrors[sourceName] = + typeof sourceValue === 'string' + ? redactApiKey(sourceValue) + : redactSensitiveFields(sourceValue, seen); + } + out[key] = sourceErrors; } else { out[key] = redactSensitiveFields(nested, seen); } From 12dd5dd9c10d9fefff0c7ce4e3039afbd1bd1782 Mon Sep 17 00:00:00 2001 From: zhao Date: Wed, 22 Jul 2026 02:35:40 +0800 Subject: [PATCH 2/4] addressed ai review --- docs/SECURITY.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 7c2cb32..275fb89 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -26,7 +26,7 @@ Tool responses are sanitized at construction and at the serialization boundary: - `src/core/server/source-registry.ts`: multi-source `source_errors` rejection messages are redacted via `redactErrorMessage` when the per-source error map is built. - `src/core/server/tool-response.ts`: `jsonResponse` / `jsonErrorResponse` call `redactSensitiveFields()` before JSON serialization (boundary layer). -Document metadata UUIDs and other non-sensitive fields are preserved at every layer. +Document metadata UUIDs and other non-sensitive fields are preserved throughout MCP response construction, aggregation, and serialization. ## Private config content (descriptions and schemas) From 5c916cd20531d2530d8ed04489b47a9d4f5f72ae Mon Sep 17 00:00:00 2001 From: zhao Date: Wed, 22 Jul 2026 19:44:04 +0800 Subject: [PATCH 3/4] addressed timon's review --- docs/SECURITY.md | 3 ++- src/core/server/tool-error.test.ts | 8 ++++++++ src/core/server/tool-error.ts | 7 ++++--- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 275fb89..935d1df 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -11,6 +11,7 @@ `src/logger.ts` implements `redactApiKey` and recursive redaction for structured log data: - UUID-shaped tokens (`xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx`) → `***` + The UUID mask is intentional: legacy (pre-`pcsk_`) Pinecone API keys are UUID-formatted, so this pattern is a credential mask, not just an incidental document-ID match. Tool-error `message`/`suggestion` therefore use the full `redactApiKey` (including UUID masking), not a document-ID-preserving variant. - Modern Pinecone keys (`pcsk_…`) → `***` - Substrings after `apiKey` / `api_key` / similar patterns → masked - `Authorization: Bearer …` tokens → masked @@ -21,7 +22,7 @@ Logs go to **stderr**; use `PINECONE_READ_ONLY_MCP_LOG_FORMAT=json` for pipeline Tool responses are sanitized at construction and at the serialization boundary: -- `src/core/server/tool-error.ts`: `pineconeToolError` and `timeoutToolError` apply `redactApiKey` to `message` and `suggestion` before the `ToolError` object is built, so DEBUG-mode SDK error text is masked at the tool-error layer. +- `src/core/server/tool-error.ts`: `pineconeToolError` and `timeoutToolError` apply `redactApiKey` to `message` before the `ToolError` object is built; `timeoutToolError` redacts caller-supplied `suggestion` only (the default suggestion is a static string with no secrets). - `src/logger.ts`: `redactErrorMessage` redacts credential-shaped substrings from error values; `redactSensitiveFields()` masks known sensitive keys (`message`, `suggestion`, `degradation_reason`) and every string value nested directly under `source_errors` (keyed by source name, not a sensitive field name). - `src/core/server/source-registry.ts`: multi-source `source_errors` rejection messages are redacted via `redactErrorMessage` when the per-source error map is built. - `src/core/server/tool-response.ts`: `jsonResponse` / `jsonErrorResponse` call `redactSensitiveFields()` before JSON serialization (boundary layer). diff --git a/src/core/server/tool-error.test.ts b/src/core/server/tool-error.test.ts index 73821bb..e9ffe3c 100644 --- a/src/core/server/tool-error.test.ts +++ b/src/core/server/tool-error.test.ts @@ -1,8 +1,10 @@ import { describe, expect, it } from 'vitest'; import { classifyToolCatchError, + DEFAULT_TIMEOUT_SUGGESTION, flowGateToolError, lifecycleToolError, + timeoutToolError, toolErrorSchema, validationToolError, } from './tool-error.js'; @@ -56,4 +58,10 @@ describe('ToolError schema and builders', () => { expect(err.suggestion).toMatch(/retry|timeout/i); toolErrorSchema.parse(err); }); + + it('TIMEOUT: default suggestion is unredacted literal', () => { + const err = timeoutToolError('Timeout after 100ms'); + expect(err.suggestion).toBe(DEFAULT_TIMEOUT_SUGGESTION); + toolErrorSchema.parse(err); + }); }); diff --git a/src/core/server/tool-error.ts b/src/core/server/tool-error.ts index 189e279..4fa1375 100644 --- a/src/core/server/tool-error.ts +++ b/src/core/server/tool-error.ts @@ -60,7 +60,7 @@ const timeoutToolErrorSchema = z.object({ code: z.literal('TIMEOUT'), message: z.string(), recoverable: z.literal(true), - suggestion: z.string().optional(), + suggestion: z.string(), }); const lifecycleToolErrorSchema = z.object({ @@ -80,7 +80,8 @@ export const toolErrorSchema = z.discriminatedUnion('code', [ export type ToolError = z.infer; -const DEFAULT_TIMEOUT_SUGGESTION = 'Retry the request, or increase --request-timeout-ms.'; +/** Default TIMEOUT suggestion when the caller does not supply one. */ +export const DEFAULT_TIMEOUT_SUGGESTION = 'Retry the request, or increase --request-timeout-ms.'; export function flowGateToolError(namespace: string, message: string): ToolError { return { @@ -122,7 +123,7 @@ export function timeoutToolError(message: string, options?: { suggestion?: strin code: 'TIMEOUT', message: redactApiKey(message), recoverable: true, - suggestion: redactApiKey(options?.suggestion ?? DEFAULT_TIMEOUT_SUGGESTION), + suggestion: options?.suggestion ? redactApiKey(options.suggestion) : DEFAULT_TIMEOUT_SUGGESTION, }; } From cc2f327ffe365b3e9879758028df765b6f14b6c6 Mon Sep 17 00:00:00 2001 From: zhao Date: Wed, 22 Jul 2026 22:10:41 +0800 Subject: [PATCH 4/4] addressed ai review: Use an explicit undefined check --- src/core/server/tool-error.test.ts | 6 ++++++ src/core/server/tool-error.ts | 5 ++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/core/server/tool-error.test.ts b/src/core/server/tool-error.test.ts index e9ffe3c..13c9a7a 100644 --- a/src/core/server/tool-error.test.ts +++ b/src/core/server/tool-error.test.ts @@ -64,4 +64,10 @@ describe('ToolError schema and builders', () => { expect(err.suggestion).toBe(DEFAULT_TIMEOUT_SUGGESTION); toolErrorSchema.parse(err); }); + + it('TIMEOUT: explicit empty suggestion is preserved, not replaced by default', () => { + const err = timeoutToolError('Timeout after 100ms', { suggestion: '' }); + expect(err.suggestion).toBe(''); + toolErrorSchema.parse(err); + }); }); diff --git a/src/core/server/tool-error.ts b/src/core/server/tool-error.ts index 4fa1375..ebca5b8 100644 --- a/src/core/server/tool-error.ts +++ b/src/core/server/tool-error.ts @@ -123,7 +123,10 @@ export function timeoutToolError(message: string, options?: { suggestion?: strin code: 'TIMEOUT', message: redactApiKey(message), recoverable: true, - suggestion: options?.suggestion ? redactApiKey(options.suggestion) : DEFAULT_TIMEOUT_SUGGESTION, + suggestion: + options?.suggestion !== undefined + ? redactApiKey(options.suggestion) + : DEFAULT_TIMEOUT_SUGGESTION, }; }