fix(mcp): review quick-wins M1/M5/M9/M10/M11/M12

- M1: fix 3 phantom event types in get_session_summary sets
  (performance_collector_stopped, agent_metrics_collector_stopped,
  agent_trace); export EXCLUDED/KEY/PHASE_EVENT_TYPES + hoist
  PHASE_EVENT_TYPES; add subset drift test (sets ⊆ ALL_EVENT_TYPES)
- M5: get_metrics uses allSettled — rethrows when both endpoints fail,
  attaches partialErrors on single failure (was silent null success);
  get_session parallelizes analysis fetch + swallows only 404
- M9: error-handler timeout branch matches error.name (real
  AbortSignal.timeout DOMException); drop exceptionType (CLR) leak from
  4xx; add 401 branch; new error-handler.test.ts
- M10: central config.ts (API_BASE_URL) — kills 3x hard-coded prod URL
  fallback, fail-fast in production, logs effective URL at boot
- M11: cveId regex-validated + normalized; search_sessions date params
  parseable-ISO refined
- M12: get_software_inventory GA-without-tenantId guard routed through
  toolError (was success-shaped envelope)

tsc clean, vitest 248 passed / 41 skipped

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

src/McpServer/autopilot-monitor-mcp/src/__tests__/error-handler.test.ts

@@ -0,0 +1,60 @@
+import { describe, it, expect } from 'vitest';
+import { toolError } from '../tools/error-handler.js';
+import { ApiError } from '../client.js';
+
+describe('toolError', () => {
+  it('detects a real AbortSignal.timeout DOMException (name TimeoutError, non-matching message)', () => {
+    // This is exactly what fetch rejects with under AbortSignal.timeout(): the
+    // message does NOT contain "TimeoutError", only the name does — so a
+    // message-only match would miss it.
+    const reason = new DOMException('The operation was aborted due to timeout', 'TimeoutError');
+    const res = toolError('get_session', {}, reason);
+    expect(res.isError).toBe(true);
+    expect(res.content[0].text).toContain('Timeout in get_session');
+    expect(res.content[0].text).toContain('narrowing the query');
+  });
+
+  it('detects an AbortError DOMException', () => {
+    const reason = new DOMException('The operation was aborted', 'AbortError');
+    const res = toolError('query_table', {}, reason);
+    expect(res.content[0].text).toContain('Timeout in query_table');
+  });
+
+  it('does NOT leak exceptionType (CLR type) from a structured 4xx body', () => {
+    const err = new ApiError(400, JSON.stringify({
+      error: 'Bad request',
+      exceptionType: 'System.ArgumentException',
+      correlationId: 'abc-123',
+      errorCode: 'E42',
+    }));
+    const res = toolError('search_sessions', {}, err);
+    const text = res.content[0].text;
+    expect(text).toContain('Bad request');
+    expect(text).toContain('abc-123');
+    expect(text).toContain('E42');
+    expect(text).not.toContain('System.ArgumentException');
+    expect(text).not.toContain('Exception type');
+  });
+
+  it('gives a friendly 401 message for an unstructured auth failure', () => {
+    const err = new ApiError(401, 'Unauthorized');
+    const res = toolError('get_metrics', {}, err);
+    expect(res.content[0].text).toContain('Authentication required in get_metrics');
+    expect(res.content[0].text).toContain('Re-authenticate');
+  });
+
+  it('sanitizes 5xx to a generic message but keeps operational handles', () => {
+    const err = new ApiError(500, JSON.stringify({
+      error: 'boom',
+      exceptionType: 'System.NullReferenceException',
+      correlationId: 'cid-9',
+      errorCode: 'E500',
+    }));
+    const res = toolError('get_session', {}, err);
+    const text = res.content[0].text;
+    expect(text).not.toContain('System.NullReferenceException');
+    expect(text).not.toContain('boom');
+    expect(text).toContain('cid-9');
+    expect(text).toContain('E500');
+  });
+});

src/McpServer/autopilot-monitor-mcp/src/__tests__/event-types-drift.test.ts

@@ -13,6 +13,7 @@ import { fileURLToPath } from 'node:url';
 import { dirname, join } from 'node:path';
 import { existsSync, readFileSync } from 'node:fs';
 import { EVENT_TYPES_CATALOG, INTERNAL_EVENT_TYPES, ALL_EVENT_TYPES } from '../resource-catalog.js';
+import { EXCLUDED_EVENT_TYPES, KEY_EVENT_TYPES, PHASE_EVENT_TYPES } from '../tools/sessions.js';
 
 /** Walk up from this test file to the repo root (marked by AutopilotMonitor.sln). */
 function findConstantsCs(): string | null {
@@ -77,3 +78,27 @@ describe('event-type catalog drift vs C# Constants.EventTypes', () => {
     expect(dupes, 'duplicate event types across catalog groups').toEqual([]);
   });
 });
+
+describe('get_session_summary event-type sets ⊆ canonical catalog', () => {
+  // These hand-curated sets in sessions.ts drive timeline filtering/ranking.
+  // A phantom member (a string with no matching event type) silently degrades
+  // get_session_summary — collector noise leaks in, or a real event never
+  // key-ranks. They bypass the C#-drift test above (which only checks the
+  // public catalog), so guard them directly against ALL_EVENT_TYPES.
+  const known = new Set(ALL_EVENT_TYPES);
+  const cases: Array<[string, Set<string>]> = [
+    ['EXCLUDED_EVENT_TYPES', EXCLUDED_EVENT_TYPES],
+    ['KEY_EVENT_TYPES', KEY_EVENT_TYPES],
+    ['PHASE_EVENT_TYPES', PHASE_EVENT_TYPES],
+  ];
+  for (const [name, set] of cases) {
+    it(`${name} contains only real event types`, () => {
+      const phantom = [...set].filter((t) => !known.has(t)).sort();
+      expect(
+        phantom,
+        `${name} (sessions.ts) lists event types absent from the catalog — ` +
+          'fix the typo or add the const; these never match real events:',
+      ).toEqual([]);
+    });
+  }
+});

src/McpServer/autopilot-monitor-mcp/src/__tests__/helpers.ts

@@ -8,7 +8,9 @@
  *   AUTOPILOT_API_TOKEN="eyJ..." npm test
  */
 
-const BASE_URL = process.env.AUTOPILOT_API_URL ?? 'https://autopilotmonitor-api.azurewebsites.net';
+import { API_BASE_URL } from '../config.js';
+
+const BASE_URL = API_BASE_URL;
 const TOKEN = process.env.AUTOPILOT_API_TOKEN ?? '';
 
 export function getToken(): string {

src/McpServer/autopilot-monitor-mcp/src/access-guard.ts

@@ -11,8 +11,9 @@ import type { Request, Response, NextFunction } from 'express';
 import crypto from 'node:crypto';
 import { extractTokenClaims, isTokenExpired } from './auth.js';
 import { runWithCaller } from './client.js';
+import { API_BASE_URL } from './config.js';
 
-const BASE_URL = process.env.AUTOPILOT_API_URL ?? 'https://autopilotmonitor-api.azurewebsites.net';
+const BASE_URL = API_BASE_URL;
 
 /**
  * Derives the public base URL for the WWW-Authenticate header.

src/McpServer/autopilot-monitor-mcp/src/client.ts

@@ -1,6 +1,7 @@
 import { AsyncLocalStorage } from 'node:async_hooks';
+import { API_BASE_URL } from './config.js';
 
-const BASE_URL = process.env.AUTOPILOT_API_URL ?? 'https://autopilotmonitor-api.azurewebsites.net';
+const BASE_URL = API_BASE_URL;
 
 /** Default timeout for backend API requests (30 seconds) */
 const API_TIMEOUT_MS = 30_000;

src/McpServer/autopilot-monitor-mcp/src/config.ts

@@ -0,0 +1,27 @@
+/**
+ * Central configuration for the MCP server.
+ *
+ * Single source of truth for the backend API base URL. Previously the
+ * hard-coded production host was copy-pasted into client.ts, access-guard.ts
+ * and the test helpers — three independent fallbacks that could drift and
+ * silently send real user Bearer tokens to the wrong host. This module makes
+ * the resolution one decision, logs the effective value at boot, and fails
+ * fast in production rather than defaulting (matching the fail-fast posture in
+ * oauth.ts).
+ */
+const DEFAULT_API_BASE_URL = 'https://autopilotmonitor-api.azurewebsites.net';
+
+function resolveApiBaseUrl(): string {
+  const fromEnv = process.env.AUTOPILOT_API_URL?.trim();
+  if (fromEnv) return fromEnv.replace(/\/+$/, '');
+  if (process.env.NODE_ENV === 'production') {
+    throw new Error(
+      'AUTOPILOT_API_URL must be set in production — refusing to default to ' +
+      `${DEFAULT_API_BASE_URL} and risk forwarding user tokens to the wrong host.`,
+    );
+  }
+  return DEFAULT_API_BASE_URL;
+}
+
+/** Resolved backend API base URL (no trailing slash). */
+export const API_BASE_URL = resolveApiBaseUrl();

src/McpServer/autopilot-monitor-mcp/src/index.ts

@@ -16,6 +16,7 @@ import { buildEventTypeSearchDocs } from './resource-catalog.js';
 import { createOAuthRouter } from './oauth.js';
 import { accessGuard } from './access-guard.js';
 import { isGlobalAdmin } from './client.js';
+import { API_BASE_URL } from './config.js';
 
 const __dirname = dirname(fileURLToPath(import.meta.url));
 
@@ -38,6 +39,11 @@ if (!process.env.MCP_PUBLIC_URL) {
   );
 }
 
+// Make the resolved backend host visible in container logs — a misconfigured
+// AUTOPILOT_API_URL silently sends user tokens to the wrong place, so surface
+// the effective value at boot rather than leaving it implicit.
+console.error(`[startup] Backend API base URL: ${API_BASE_URL}`);
+
 // --- Load shared knowledge base (reused across all sessions) ---
 
 console.error('Loading knowledge base documents…');

src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts

@@ -1045,11 +1045,13 @@ export function registerAdminTools(server: McpServer, ga: boolean): void {
         // The GA inventory endpoint requires a tenantId and 400s without one — guard up front
         // with actionable guidance instead of surfacing a raw 400 from a no-arg call.
         if (ga && !tenantId) {
-          return toolResultText({
-            error: "tenantId is required for scope='inventory' as a Global Admin. " +
-              'Pass a tenantId (use list_tenants to discover IDs), or use scope="unmatched" for the ' +
-              'cross-tenant view of software with no CPE mapping yet.',
-          }, MAX_RESULT_SIZE_CHARS.small);
+          // Route through toolError so the response carries isError:true — a
+          // success-shaped {error:...} envelope would be mistaken for data.
+          return toolError('get_software_inventory', args, new Error(
+            "tenantId is required for scope='inventory' as a Global Admin. " +
+            'Pass a tenantId (use list_tenants to discover IDs), or use scope="unmatched" for the ' +
+            'cross-tenant view of software with no CPE mapping yet.',
+          ));
         }
         // Endpoint returns the whole inventory in one shot, so page it client-side.
         const path = pickGlobalOrTenantPath('/api/vulnerability/software-inventory', '/api/metrics/software-inventory');

src/McpServer/autopilot-monitor-mcp/src/tools/error-handler.ts

@@ -41,11 +41,16 @@ export function toolError(
     parts.push(`**Error in ${toolName}**: ${p.error ?? error.message}`);
     if (p.hint) parts.push(`**Suggestion**: ${p.hint}`);
     if (p.correlationId) parts.push(`**Correlation ID**: ${p.correlationId}`);
-    if (p.exceptionType) parts.push(`**Exception type**: ${p.exceptionType}`);
+    // exceptionType (CLR type names) is an internal fingerprint the model has no
+    // legitimate reason to act on — deliberately NOT surfaced here, mirroring the
+    // 5xx sanitization above. correlationId + errorCode remain as operator handles.
     if (p.errorCode) parts.push(`**Error code**: ${p.errorCode}`);
   } else if (error instanceof ApiError) {
     // API error but non-JSON body
-    if (error.status === 403) {
+    if (error.status === 401) {
+      parts.push(`**Authentication required in ${toolName}**: your session is not authenticated or has expired.`);
+      parts.push('**Suggestion**: Re-authenticate and retry.');
+    } else if (error.status === 403) {
       parts.push(`**Access denied in ${toolName}**: you do not have permission to perform this operation.`);
     } else if (error.status === 404) {
       parts.push(`**Not found in ${toolName}**: The requested resource does not exist. Verify IDs, table names, or filters.`);
@@ -57,11 +62,20 @@ export function toolError(
       parts.push(`**Error in ${toolName}** (HTTP ${error.status}): ${truncated}`);
     }
   } else {
+    const name = error instanceof Error ? error.name : '';
     const message = error instanceof Error ? error.message : String(error);
+    // `AbortSignal.timeout()` rejects fetch with a DOMException whose NAME is
+    // 'TimeoutError' but whose MESSAGE is "The operation was aborted due to
+    // timeout" — so match on name first; the message checks are belt-and-braces
+    // for other abort/timeout shapes.
     if (message.includes('No authentication token')) {
       parts.push(`**Authentication error in ${toolName}**: ${message}`);
       parts.push('**Suggestion**: The MCP session may have expired. Re-authenticate.');
-    } else if (message.includes('TimeoutError') || message.includes('AbortError') || message.includes('timed out')) {
+    } else if (
+      name === 'TimeoutError' || name === 'AbortError' ||
+      message.includes('TimeoutError') || message.includes('AbortError') ||
+      message.includes('timed out') || message.includes('aborted due to timeout')
+    ) {
       parts.push(`**Timeout in ${toolName}**: The backend did not respond in time.`);
       parts.push('**Suggestion**: Try narrowing the query (smaller date range, fewer results, more specific filters).');
     } else {

src/McpServer/autopilot-monitor-mcp/src/tools/sessions.ts

@@ -1,6 +1,6 @@
 import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
 import { z } from 'zod';
-import { apiFetch, buildQuery, followNextLink, pickGlobalOrTenantPath, scanUntilMatch } from '../client.js';
+import { apiFetch, ApiError, buildQuery, followNextLink, pickGlobalOrTenantPath, scanUntilMatch } from '../client.js';
 import { withToolTelemetry } from '../telemetry.js';
 import { READ_ONLY, MAX_RESULT_SIZE_CHARS, toolResultText, SessionIdSchema, isBenignHealthDetectionReport } from './shared.js';
 import { toolError } from './error-handler.js';
@@ -9,22 +9,38 @@ import { interpolateAnalysisResults } from '../interpolate-rule-template.js';
 
 // ── Session summary constants ───────────────────────────────────────────
 
-const EXCLUDED_EVENT_TYPES = new Set([
+// Exported so a drift test (event-types-drift.test.ts) can assert every member
+// is a real Constants.EventTypes value — a phantom here silently degrades
+// get_session_summary (noise leaks in, real events never key-rank).
+export const EXCLUDED_EVENT_TYPES = new Set([
   'performance_snapshot', 'agent_metrics_snapshot',
-  'performance_snapshot_stopped', 'agent_metrics_snapshot_stopped',
+  'performance_collector_stopped', 'agent_metrics_collector_stopped',
   'gather_result', 'gather_rules_collection_completed',
   'software_inventory_analysis', 'security_audit',
   'device_location', 'ntp_time_check', 'ime_agent_version',
 ]);
-const KEY_EVENT_TYPES = new Set([
+export const KEY_EVENT_TYPES = new Set([
   'phase_transition', 'esp_phase_changed', 'enrollment_type_detected',
   'app_install_started', 'app_install_completed', 'app_install_failed', 'app_install_skipped',
   'app_tracking_summary', 'error_detected',
   'enrollment_complete', 'enrollment_failed', 'completion_check',
   'desktop_arrived', 'hello_policy_detected', 'waiting_for_hello', 'hello_completion_timeout',
-  'agent_started', 'agent_shutdown', 'agent_shutting_down', 'trace_event',
+  'agent_started', 'agent_shutdown', 'agent_shutting_down', 'agent_trace',
   'script_started', 'script_completed', 'script_failed', 'vulnerability_report',
 ]);
+// Phase-defining events promoted to the top of the triage timeline. Module-level
+// (not handler-local) so the same drift test can validate it.
+export const PHASE_EVENT_TYPES = new Set([
+  'phase_transition', 'esp_phase_changed', 'enrollment_type_detected',
+  'enrollment_complete', 'enrollment_failed', 'desktop_arrived',
+]);
+// Permissive ISO-8601 guard: rejects unparseable junk (which the backend would
+// silently treat as no filter) while still accepting the date-only and
+// timezone-offset forms the backend honors.
+const IsoDateString = z.string().refine(
+  (s) => !Number.isNaN(Date.parse(s)),
+  { message: 'Must be a parseable ISO 8601 date/datetime, e.g. "2024-01-15" or "2024-01-15T00:00:00Z".' },
+);
 const SEVERITY_RANK: Record<string, number> = { Trace: -1, Debug: 0, Info: 1, Warning: 2, Error: 3, Critical: 4 };
 // Phase labels MUST mirror the backend EnrollmentPhase enum and the web's
 // phaseConstants.ts (the product's source of truth). The only per-enrollment
@@ -84,8 +100,8 @@ export function registerSessionTools(server: McpServer, ga: boolean): void {
         isPreProvisioned: z.boolean().optional().describe('Filter by White Glove / pre-provisioned enrollment'),
         isHybridJoin: z.boolean().optional().describe('Filter by Hybrid Azure AD Join'),
         geoCountry: z.string().optional().describe('Country of enrollment (2-letter ISO code, e.g. "DE", "US")'),
-        startedAfter: z.string().optional().describe('ISO 8601 datetime — only sessions started after this'),
-        startedBefore: z.string().optional().describe('ISO 8601 datetime — only sessions started before this'),
+        startedAfter: IsoDateString.optional().describe('ISO 8601 datetime — only sessions started after this'),
+        startedBefore: IsoDateString.optional().describe('ISO 8601 datetime — only sessions started before this'),
         agentVersion: z.string().optional().describe('Monitor Agent version (exact match, e.g. "2.0.626")'),
         agentVersionPrefix: z.string().optional()
           .describe('Monitor Agent version prefix (e.g. "2.0." matches every 2.0.x build). Mutually exclusive with agentVersion (exact wins).'),
@@ -209,17 +225,21 @@ export function registerSessionTools(server: McpServer, ga: boolean): void {
       try {
         const { sessionId, tenantId, includeAnalysis } = args;
         const q = buildQuery({ tenantId } as Record<string, string | undefined>);
-        const sessionData = await apiFetch(`/api/sessions/${sessionId}${q}`);
-        let analysisData: unknown = null;
-        if (includeAnalysis) {
-          try {
-            // Substitute {{token}} placeholders from matchedConditions so the raw
-            // explanation/remediation text never leaks literal {{reason}}/{{appName}}/...
-            analysisData = interpolateAnalysisResults(await apiFetch(`/api/sessions/${sessionId}/analysis${q}`));
-          } catch {
-            // analysis may not exist yet
-          }
+        const sessionPromise = apiFetch(`/api/sessions/${sessionId}${q}`);
+        if (!includeAnalysis) {
+          return toolResultText({ session: await sessionPromise, analysis: null }, MAX_RESULT_SIZE_CHARS.small);
         }
+        // Fetch session + analysis in parallel (was sequential). Substitute
+        // {{token}} placeholders so the raw explanation/remediation text never
+        // leaks literal {{reason}}/{{appName}}/…. Swallow ONLY 404 (analysis may
+        // not exist yet) — a 403/500 is a real failure that must surface.
+        const analysisPromise = apiFetch(`/api/sessions/${sessionId}/analysis${q}`)
+          .then(interpolateAnalysisResults)
+          .catch((err: unknown) => {
+            if (err instanceof ApiError && err.status === 404) return null;
+            throw err;
+          });
+        const [sessionData, analysisData] = await Promise.all([sessionPromise, analysisPromise]);
         return toolResultText({ session: sessionData, analysis: analysisData }, MAX_RESULT_SIZE_CHARS.small);
       } catch (error: unknown) {
         return toolError('get_session', args, error);
@@ -376,10 +396,6 @@ export function registerSessionTools(server: McpServer, ga: boolean): void {
         // root cause of the previous 80 KB+ responses; callers needing full payloads
         // pull them via get_session_events with the same sessionId.
         const KEY_EVENTS_CAP = 50;
-        const PHASE_EVENT_TYPES = new Set([
-          'phase_transition', 'esp_phase_changed', 'enrollment_type_detected',
-          'enrollment_complete', 'enrollment_failed', 'desktop_arrived',
-        ]);
         const allKey = allEvents.filter((e) => {
           const et = String(e.eventType ?? '');
           if (EXCLUDED_EVENT_TYPES.has(et)) return false;
@@ -496,11 +512,29 @@ export function registerSessionTools(server: McpServer, ga: boolean): void {
         if (tenantId) params.tenantId = tenantId;
         const q = buildQuery(params);
         const prefix = pickGlobalOrTenantPath('/api/global/metrics', '/api/metrics');
-        const [summary, apps] = await Promise.all([
-          apiFetch(`${prefix}/summary${q}`).catch(() => null),
-          apiFetch(`${prefix}/app${q}`).catch(() => null),
+        const [summaryRes, appsRes] = await Promise.allSettled([
+          apiFetch(`${prefix}/summary${q}`),
+          apiFetch(`${prefix}/app${q}`),
         ]);
-        return toolResultText({ summary, apps }, MAX_RESULT_SIZE_CHARS.small);
+        // Swallowing both failures as {summary:null, apps:null} reports a 403/500/
+        // timeout as success. Only tolerate a partial failure (one endpoint down);
+        // when BOTH fail, surface the error so the caller doesn't read it as "0".
+        if (summaryRes.status === 'rejected' && appsRes.status === 'rejected') {
+          throw summaryRes.reason;
+        }
+        const summary = summaryRes.status === 'fulfilled' ? summaryRes.value : null;
+        const apps = appsRes.status === 'fulfilled' ? appsRes.value : null;
+        const partialErrors: Record<string, string> = {};
+        if (summaryRes.status === 'rejected') {
+          partialErrors.summary = summaryRes.reason instanceof Error ? summaryRes.reason.message : String(summaryRes.reason);
+        }
+        if (appsRes.status === 'rejected') {
+          partialErrors.apps = appsRes.reason instanceof Error ? appsRes.reason.message : String(appsRes.reason);
+        }
+        return toolResultText(
+          Object.keys(partialErrors).length ? { summary, apps, partialErrors } : { summary, apps },
+          MAX_RESULT_SIZE_CHARS.small,
+        );
       } catch (error: unknown) {
         return toolError('get_metrics', args, error);
       }
@@ -522,7 +556,9 @@ export function registerSessionTools(server: McpServer, ga: boolean): void {
         "use pageSize=1000 and follow nextLink repeatedly until absent. Pass the whole nextLink string as " +
         "\"continuation\" so all backend-echoed query params (cveId, minCvssScore, overallRisk) round-trip correctly.",
       inputSchema: {
-        cveId: z.string().describe('CVE identifier (e.g. "CVE-2024-21447")'),
+        cveId: z.string()
+          .regex(/^CVE-\d{4}-\d{4,}$/i, 'Must be a CVE identifier like "CVE-2024-21447" (CVE-YYYY-NNNN+).')
+          .describe('CVE identifier (e.g. "CVE-2024-21447"). Validated — a non-CVE string is rejected, not silently empty.'),
         tenantId: z.string().optional().describe(ga ? 'Tenant ID. Omit for cross-tenant search (Global Admin only).' : 'Optional tenant ID. Defaults to your tenant.'),
         minCvssScore: z.coerce.number().min(0).max(10).optional().describe('Minimum CVSS score filter (e.g. 7.0 for high+critical)'),
         overallRisk: z.enum(['low', 'medium', 'high', 'critical']).optional(),
@@ -536,10 +572,12 @@ export function registerSessionTools(server: McpServer, ga: boolean): void {
     async (args) => withToolTelemetry('search_sessions_by_cve', async () => {
       try {
         const { cveId, tenantId, minCvssScore, overallRisk, pageSize, continuation } = args;
+        // Normalize to canonical upper-case form (schema accepts case-insensitive).
+        const normalizedCve = cveId.toUpperCase();
         const basePath = pickGlobalOrTenantPath('/api/global/search/sessions-by-cve', '/api/search/sessions-by-cve');
         const path = followNextLink(
           basePath,
-          { cveId, tenantId, minCvssScore, overallRisk, pageSize },
+          { cveId: normalizedCve, tenantId, minCvssScore, overallRisk, pageSize },
           continuation,
         );
         const data = await apiFetch(path);