[2593 by GPT-5.6 Sol] MCP: introduce a structured error envelope (code, message, stage, retryable) with secret redaction for every tool call

package.json

@@ -98,7 +98,7 @@
     "test:hosted-tunnel:server": "PROPR_DEMO_MODE=true npx tsx --test test/orchestratorConfig.test.mjs packages/cli/src/commands/setup/engine.test.ts packages/cli/src/commands/setup/sequential.test.ts packages/cli/src/commands/setupCommand.test.ts packages/cli/src/tui/SetupApp.test.tsx packages/cli/src/commands/tunnelCommand.test.ts test/orchestratorTunnelLifecycle.test.mjs test/orchestratorTunnelStatus.test.mjs test/orchestratorProprUrlsDrift.test.ts test/routingWebSocketProtocol.test.ts test/routingWebSocketIntakeService.test.ts packages/api/test/corsValidation.test.ts",
     "test:hosted-tunnel:ui": "npm --workspace propr-ui test -- runtimeConfig.test.ts compatibility.test.ts",
     "test:hosted-tunnel": "npm run build -w @propr/shared && npm run build -w @propr/local-setup && npm run test:hosted-tunnel:server && npm run test:hosted-tunnel:ui",
-    "test:mcp": "NODE_ENV=test npx tsx --experimental-test-module-mocks --test packages/api/test/mcpOAuth.test.ts packages/api/test/mcpOperations.test.ts packages/api/test/mcpTaskSubmissions.test.ts packages/api/test/mcpPresentation.test.ts packages/api/test/mcpConnectedApps.test.ts packages/api/test/mcpIntegration.test.ts packages/api/test/mcpAgentActivity.test.ts packages/api/test/mcpActivity.test.ts packages/api/test/mcpRecentActivity.test.ts packages/api/test/mcpGoalTaskDepth.test.ts packages/api/test/mcpAccessLog.test.ts packages/api/test/mcpAccessLogDispatch.test.ts packages/api/test/mcpDelegation.test.ts packages/api/test/mcpWorkflows.test.ts packages/api/test/mcpPullRequests.test.ts packages/api/test/mcpOperatorSurface.test.ts packages/api/test/mcpReviewerConcurrency.test.ts test/mcpConnectHarness.test.mjs",
+    "test:mcp": "NODE_ENV=test npx tsx --experimental-test-module-mocks --test packages/api/test/mcpErrorEnvelope.test.ts packages/api/test/mcpOAuth.test.ts packages/api/test/mcpOperations.test.ts packages/api/test/mcpTaskSubmissions.test.ts packages/api/test/mcpPresentation.test.ts packages/api/test/mcpConnectedApps.test.ts packages/api/test/mcpIntegration.test.ts packages/api/test/mcpAgentActivity.test.ts packages/api/test/mcpActivity.test.ts packages/api/test/mcpRecentActivity.test.ts packages/api/test/mcpGoalTaskDepth.test.ts packages/api/test/mcpAccessLog.test.ts packages/api/test/mcpAccessLogDispatch.test.ts packages/api/test/mcpDelegation.test.ts packages/api/test/mcpWorkflows.test.ts packages/api/test/mcpPullRequests.test.ts packages/api/test/mcpOperatorSurface.test.ts packages/api/test/mcpReviewerConcurrency.test.ts test/mcpConnectHarness.test.mjs",
     "test:mcp:browser": "NODE_ENV=test npx tsx --test packages/api/test/mcpBrowser.test.ts",
     "test:mcp:connect": "node scripts/test-mcp-connect.mjs",
     "mcp:connect:register": "tsx scripts/mcp-connect-register.ts",

packages/api/mcp/accessLog.ts

@@ -1,7 +1,6 @@
 import { AsyncLocalStorage } from 'node:async_hooks';
 import type { Knex } from 'knex';
-import { z } from 'zod';
-import { McpError } from './config.js';
+import { classifyError } from './errorEnvelope.js';
 import type { McpPrincipal } from './policy.js';
 
 /**
@@ -172,12 +171,9 @@ export function accessPrincipal(principal?: Pick<McpPrincipal, 'user' | 'grant'>
  * scope, forbidden repository, invalid arguments) are denials; anything the
  * instance itself could not complete is an error.
  */
-export function classifyMcpFailure(error: unknown): { status: number; outcome: McpAccessOutcome; errorCode: string } {
-  if (error instanceof McpError) {
-    return { status: error.status, outcome: error.status >= 500 ? 'error' : 'denied', errorCode: error.code };
-  }
-  if (error instanceof z.ZodError) return { status: 400, outcome: 'denied', errorCode: 'INVALID_INPUT' };
-  return { status: 500, outcome: 'error', errorCode: 'INTERNAL_ERROR' };
+export function classifyMcpFailure(error: unknown, options: { sideEffectsPossible?: boolean } = {}): { status: number; outcome: McpAccessOutcome; errorCode: string } {
+  const envelope = classifyError(error, { sideEffectsPossible: options.sideEffectsPossible ?? false });
+  return { status: envelope.status, outcome: envelope.code === 'OUTCOME_UNKNOWN' || envelope.status >= 500 ? 'error' : 'denied', errorCode: envelope.code };
 }
 
 /** JSON-RPC request identifiers are scalars; anything else is not correlatable. */

packages/api/mcp/config.ts

@@ -1,3 +1,5 @@
+import { redactDetails, redactSecrets, type McpErrorEnvelope, type McpErrorStage } from './errorEnvelope.js';
+
 export const MCP_SCOPES = ['read', 'plan', 'publish', 'execute', 'review', 'merge', 'deploy', 'manage'] as const;
 export type McpScope = typeof MCP_SCOPES[number];
 
@@ -35,7 +37,33 @@ export function loadMcpConfig(env: NodeJS.ProcessEnv = process.env): McpConfig |
 }
 
 export class McpError extends Error {
-  constructor(public readonly code: string, message: string, public readonly status = 400) { super(message); }
+  readonly stage: McpErrorStage | null;
+  readonly retryable: boolean;
+  readonly details?: Record<string, unknown>;
+
+  constructor(
+    public readonly code: string,
+    message: string,
+    public readonly status = 400,
+    options: { stage?: McpErrorStage | null; retryable?: boolean; details?: Record<string, unknown> } = {},
+  ) {
+    super(message);
+    this.name = 'McpError';
+    this.stage = options.stage ?? null;
+    this.retryable = options.retryable ?? false;
+    this.details = options.details;
+  }
+
+  toEnvelope(): McpErrorEnvelope {
+    return {
+      code: this.code,
+      message: redactSecrets(this.message),
+      stage: this.stage,
+      retryable: this.retryable,
+      status: this.status,
+      ...(this.details ? { details: redactDetails(this.details) } : {}),
+    };
+  }
 }
 
 function loadConnectConfig(env: NodeJS.ProcessEnv, instanceId: string): McpConfig['connect'] {

packages/api/mcp/errorEnvelope.ts

@@ -0,0 +1,235 @@
+import { z } from 'zod';
+import type { McpError } from './config.js';
+
+/** Stable locations a tool may expose without leaking implementation detail. */
+export type McpErrorStage = 'validation' | 'authorization' | 'precondition' | 'github' | 'transport' | 'database' | 'queue' | 'internal';
+
+/** The one public and durable representation of an MCP tool failure. */
+export interface McpErrorEnvelope {
+  code: string;
+  message: string;
+  stage: McpErrorStage | null;
+  retryable: boolean;
+  status: number;
+  details?: Record<string, unknown>;
+  cause?: { code: string; message: string };
+}
+
+const SENSITIVE_DETAIL_KEY = /token|secret|password|authorization|cookie|private.?key|credential/i;
+const GITHUB_TOKEN = /\b(?:gh[pousr]_[A-Za-z0-9_]+|github_pat_[A-Za-z0-9_]+)\b/g;
+const MCP_TOKEN = /\b(?:pia|propr)_mcp_[A-Za-z0-9._~-]+\b/g;
+const BEARER_VALUE = /\bBearer\s+[^\s,;"']+/gi;
+const CREDENTIALED_GIT_URL = /(https:\/\/x-access-token:)[^@\s/]+(@[^\s]+)/gi;
+const SIGNED_QUERY_VALUE = /([?&](?:X-Amz-Signature|token|access_token|signature)=)[^&#\s]*/gi;
+
+/** Remove credentials that can occur in upstream messages and safe detail values. */
+export function redactSecrets(value: string): string {
+  return value
+    .replace(GITHUB_TOKEN, '[REDACTED]')
+    .replace(MCP_TOKEN, '[REDACTED]')
+    .replace(BEARER_VALUE, 'Bearer [REDACTED]')
+    .replace(CREDENTIALED_GIT_URL, '$1[REDACTED]$2')
+    .replace(SIGNED_QUERY_VALUE, '$1[REDACTED]');
+}
+
+function basename(value: string): string {
+  const withoutTrailingSeparator = value.replace(/[\\/]+$/, '');
+  return withoutTrailingSeparator.split(/[\\/]/).pop() || '[REDACTED_PATH]';
+}
+
+function stripAbsolutePaths(value: string): string {
+  const posix = value.replace(/(^|[\s("'=])\/(?:[^/\s"'<>]+\/)+([^/\s"'<>]+)/g, (_match, prefix: string, filename: string) => `${prefix}${filename}`);
+  return posix.replace(/(^|[\s("'=])(?:[A-Za-z]:[\\/]|\\\\)(?:[^\\/\s"'<>]+[\\/])+([^\\/\s"'<>]+)/g,
+    (_match, prefix: string, filename: string) => `${prefix}${filename}`);
+}
+
+function redactDetailValue(value: unknown, seen: WeakSet<object>): unknown {
+  if (typeof value === 'string') {
+    const redacted = redactSecrets(value);
+    // Detail fields often carry paths directly or inside diagnostic prose.
+    // Keep only filenames for POSIX, Windows-drive and UNC absolute paths.
+    if (redacted.startsWith('/') || /^[A-Za-z]:[\\/]/.test(redacted)) return basename(redacted);
+    return stripAbsolutePaths(redacted);
+  }
+  if (Array.isArray(value)) {
+    if (seen.has(value)) return '[REDACTED]';
+    seen.add(value);
+    return value.map(item => redactDetailValue(item, seen));
+  }
+  if (value && typeof value === 'object') {
+    if (seen.has(value)) return '[REDACTED]';
+    seen.add(value);
+    const result: Record<string, unknown> = {};
+    for (const [key, item] of Object.entries(value)) {
+      if (SENSITIVE_DETAIL_KEY.test(key)) continue;
+      result[redactSecrets(key)] = redactDetailValue(item, seen);
+    }
+    return result;
+  }
+  if (typeof value === 'bigint') return String(value);
+  return value;
+}
+
+/** Recursively sanitize optional structured error detail. Sensitive keys are omitted. */
+export function redactDetails(details: Record<string, unknown>): Record<string, unknown> {
+  return redactDetailValue(details, new WeakSet()) as Record<string, unknown>;
+}
+
+type ErrorLike = {
+  name?: unknown;
+  code?: unknown;
+  message?: unknown;
+  status?: unknown;
+  request?: unknown;
+  response?: unknown;
+  cause?: unknown;
+};
+
+function errorLike(value: unknown): ErrorLike | undefined {
+  return value !== null && typeof value === 'object' ? value as ErrorLike : undefined;
+}
+
+function isMcpError(value: unknown): value is McpError {
+  return value instanceof Error
+    && value.name === 'McpError'
+    && typeof (value as Partial<McpError>).code === 'string'
+    && typeof (value as Partial<McpError>).status === 'number'
+    && typeof (value as Partial<McpError>).toEnvelope === 'function';
+}
+
+function record(value: unknown): Record<string, unknown> | undefined {
+  return value !== null && typeof value === 'object' ? value as Record<string, unknown> : undefined;
+}
+
+function finiteStatus(value: unknown): number | undefined {
+  return typeof value === 'number' && Number.isFinite(value) ? value : undefined;
+}
+
+function githubError(error: unknown): { value: ErrorLike; status: number; response?: Record<string, unknown> } | undefined {
+  const value = errorLike(error);
+  if (!value) return undefined;
+  const response = record(value.response);
+  const status = finiteStatus(value.status) ?? finiteStatus(response?.status);
+  if (!status) return undefined;
+  // RequestError uses name=HttpError and carries request/response metadata.
+  // Accept response-bearing test doubles too, without mistaking every domain
+  // error with an HTTP-like status for a GitHub failure.
+  if (value.name !== 'HttpError' && value.request === undefined && !response) return undefined;
+  return { value, status, response };
+}
+
+function githubHeaders(response: Record<string, unknown> | undefined): Record<string, unknown> {
+  const source = record(response?.headers);
+  if (!source) return {};
+  return Object.fromEntries(Object.entries(source).map(([key, value]) => [key.toLowerCase(), value]));
+}
+
+function githubMessage(value: ErrorLike, response: Record<string, unknown> | undefined): string {
+  const data = record(response?.data);
+  const primary = typeof data?.message === 'string' ? data.message
+    : typeof value.message === 'string' ? value.message : 'GitHub rejected the request.';
+  const errors = Array.isArray(data?.errors) ? data.errors.flatMap(item => {
+    if (typeof item === 'string') return [item];
+    const message = record(item)?.message;
+    return typeof message === 'string' ? [message] : [];
+  }) : [];
+  const additions = errors.filter(message => message && message !== primary);
+  return redactSecrets([primary, ...additions].join(': '));
+}
+
+function classifyGithub(error: unknown): McpErrorEnvelope | undefined {
+  const github = githubError(error);
+  if (!github) return undefined;
+  const { value, status, response } = github;
+  const headers = githubHeaders(response);
+  const rateLimited = status === 429 || (status === 403 && (
+    String(headers['x-ratelimit-remaining'] ?? '') === '0'
+    || headers['retry-after'] !== undefined
+  ));
+  const common = { stage: 'github' as const, status, message: githubMessage(value, response) };
+  if (rateLimited) return { code: 'GITHUB_RATE_LIMITED', retryable: true, ...common };
+  if (status === 401) return { code: 'GITHUB_AUTH_FAILED', retryable: false, ...common };
+  if (status === 403) return { code: 'GITHUB_FORBIDDEN', retryable: false, ...common };
+  if (status === 404) return { code: 'GITHUB_NOT_FOUND', retryable: false, ...common };
+  if (status >= 500) return { code: 'GITHUB_UNAVAILABLE', retryable: true, ...common };
+  if (status === 409 || status === 422 || (status >= 400 && status < 500)) {
+    return { code: 'GITHUB_REJECTED', retryable: false, ...common };
+  }
+  return undefined;
+}
+
+function errorChain(error: unknown): ErrorLike[] {
+  const chain: ErrorLike[] = [];
+  const seen = new Set<unknown>();
+  let current: unknown = error;
+  while (current && !seen.has(current) && chain.length < 5) {
+    seen.add(current);
+    const value = errorLike(current);
+    if (!value) break;
+    chain.push(value);
+    current = value.cause;
+  }
+  return chain;
+}
+
+function classifyKnown(error: unknown): McpErrorEnvelope {
+  if (isMcpError(error)) return error.toEnvelope();
+
+  if (error instanceof z.ZodError) {
+    return {
+      code: 'INVALID_INPUT',
+      message: 'Invalid or missing tool arguments.',
+      stage: 'validation',
+      retryable: false,
+      status: 400,
+      details: redactDetails({ issues: error.issues.map(issue => ({
+        path: issue.path.map(part => typeof part === 'symbol' ? part.description ?? 'symbol' : part),
+        message: issue.message,
+      })) }),
+    };
+  }
+
+  const github = classifyGithub(error);
+  if (github) return github;
+
+  const chain = errorChain(error);
+  if (chain.some(item => item.name === 'AbortError' || item.name === 'TimeoutError' || item.code === 'ETIMEDOUT')) {
+    return { code: 'UPSTREAM_TIMEOUT', message: 'The upstream request timed out.', stage: 'transport', retryable: true, status: 504 };
+  }
+  if (chain.some(item => ['ECONNRESET', 'ECONNREFUSED'].includes(String(item.code)))) {
+    return { code: 'UPSTREAM_UNREACHABLE', message: 'The upstream service could not be reached.', stage: 'transport', retryable: true, status: 503 };
+  }
+  if (chain.some(item => ['SQLITE_BUSY', 'SQLITE_LOCKED'].includes(String(item.code)))) {
+    return { code: 'DATABASE_BUSY', message: 'The database is temporarily busy.', stage: 'database', retryable: true, status: 503 };
+  }
+  return { code: 'INTERNAL_ERROR', message: 'The request could not be completed.', stage: 'internal', retryable: false, status: 500 };
+}
+
+/** Classify any thrown value, retaining mutation uncertainty when effects may have occurred. */
+export function classifyError(error: unknown, options: { sideEffectsPossible: boolean }): McpErrorEnvelope {
+  const classified = classifyKnown(error);
+  if (!options.sideEffectsPossible || (isMcpError(error) && classified.status < 500)) return classified;
+  return {
+    code: 'OUTCOME_UNKNOWN',
+    message: 'Outcome uncertain. Inspect the target before issuing a new action.',
+    stage: classified.stage,
+    retryable: false,
+    status: classified.status,
+    cause: { code: classified.code, message: redactSecrets(classified.message) },
+  };
+}
+
+/** Produce the protocol result understood by both text-only and structured clients. */
+export function toToolErrorResult(envelope: McpErrorEnvelope): {
+  isError: true;
+  content: [{ type: 'text'; text: string }];
+  structuredContent: { error: McpErrorEnvelope };
+} {
+  const safe: McpErrorEnvelope = {
+    ...envelope,
+    message: redactSecrets(envelope.message),
+    ...(envelope.details ? { details: redactDetails(envelope.details) } : {}),
+    ...(envelope.cause ? { cause: { code: envelope.cause.code, message: redactSecrets(envelope.cause.message) } } : {}),
+  };
+  return { isError: true, content: [{ type: 'text', text: JSON.stringify({ error: safe }) }], structuredContent: { error: safe } };
+}

packages/api/mcp/operations.ts

@@ -1,6 +1,7 @@
 import { randomUUID } from 'node:crypto';
 import type { Knex } from 'knex';
 import { McpError } from './config.js';
+import { classifyError } from './errorEnvelope.js';
 import { digest } from './store.js';
 import type { McpPrincipal } from './policy.js';
 
@@ -47,9 +48,9 @@ export class McpOperations {
     } catch (error) {
       // A transport failure can follow an external side effect. Never replay it
       // automatically or claim it was rolled back. The handle remains durable.
-      const code = error instanceof McpError && error.status < 500 ? error.code : 'OUTCOME_UNKNOWN';
-      await this.db('mcp_operations').where({ id }).update({ state: code === 'OUTCOME_UNKNOWN' ? 'unknown' : 'failed',
-        result: JSON.stringify({ error: { code, message: error instanceof McpError ? error.message : 'Outcome uncertain. Inspect the target before issuing a new action.' } }), updated_at: Date.now() });
+      const envelope = classifyError(error, { sideEffectsPossible: true });
+      await this.db('mcp_operations').where({ id }).update({ state: envelope.code === 'OUTCOME_UNKNOWN' ? 'unknown' : 'failed',
+        result: JSON.stringify({ error: envelope }), updated_at: Date.now() });
     }
     return this.project((await this.db<Operation>('mcp_operations').where({ id }).first())!);
   }

packages/api/mcp/server.ts

@@ -17,6 +17,7 @@ import { createToolCatalog, executeTool, type McpTool, type ToolDeps } from './t
 import { accessPrincipal, mcpRequestId, recordMcpAccess, withMcpDispatch, withMcpRequestContext, withMcpSurface } from './accessLog.js';
 import { presentResultText } from './presentation.js';
 import { resolveMcpConfig, isMcpEnabledSync, getMcpScopeCeilingSync } from './configResolver.js';
+import { classifyError, toToolErrorResult } from './errorEnvelope.js';
 
 const prompts: Record<string, string> = {
   plan_change: 'Resolve the repository and inspect indexed context. Create a draft plan, generate or refine it, and show it to the user. Publishing and implementation are separate explicit actions.',
@@ -43,9 +44,7 @@ export function buildMcpServer(principal: McpPrincipal, deps: ToolDeps, catalog:
         const result = await call(tool.name, args);
         return { content: [{ type: 'text', text: presentResultText(result) }], structuredContent: result };
       } catch (error) {
-        const code = error instanceof McpError ? error.code : error instanceof z.ZodError ? 'INVALID_INPUT' : 'INTERNAL_ERROR';
-        const message = error instanceof McpError ? error.message : error instanceof z.ZodError ? 'Invalid or missing tool arguments.' : 'The request could not be completed.';
-        return { isError: true, content: [{ type: 'text', text: `${code}: ${message}` }], structuredContent: { error: { code, message } } };
+        return toToolErrorResult(classifyError(error, { sideEffectsPossible: false }));
       }
     });
   }

packages/api/mcp/toolExecution.ts

@@ -96,7 +96,7 @@ async function runTool({ tool, raw, principal, deps, access }: ToolInvocation):
     // instead of throwing, so the classification is captured here, before that
     // projection consumes it.
     : await new McpOperations(deps.db).run(principal, { tool: tool.name, args, repository: operationRepository },
-      operationId => tool.run({ principal, args, operationId }).catch(error => { access.failure = classifyMcpFailure(error); throw error; })));
+      operationId => tool.run({ principal, args, operationId }).catch(error => { access.failure = classifyMcpFailure(error, { sideEffectsPossible: true }); throw error; })));
   const data = redact(result) as Record<string, unknown>;
   noteToolOutcome(tool, access, data);
   if (access.resultBytes > 256 * 1024) throw new McpError('RESULT_TOO_LARGE', 'Request a smaller page or narrower target.');