refactor(api): improve error handling consistency (#529)

## Summary
Improves API error handling consistency by using specific error types
instead of generic errors throughout the codebase. This provides better
error context and enables more appropriate error handling strategies.

### Key Changes
- **Use specific error types**: Replace generic `Error` throws with
`ApiValidationError`, `ApiNotFoundError`, etc. for better error
categorization
- **Add contextual comments**: Document common error scenarios (HTML
responses, multi-project access) to help future maintainers
- **Refactor agent tool utils**: Extract error handling logic into
dedicated `handleAgentToolError` function for better maintainability
- **Remove redundant tests**: Consolidated error testing (removed 464
lines of test code that was redundant)

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

Author: dcramer

packages/mcp-cloudflare/src/server/lib/constraint-utils.ts

@@ -20,7 +20,7 @@ export async function verifyConstraintsAccess(
       ok: true;
       constraints: Constraints;
     }
-  | { ok: false; status: number; message: string; eventId?: string }
+  | { ok: false; status?: number; message: string; eventId?: string }
 > {
   if (!organizationSlug) {
     // No constraints specified, nothing to verify

packages/mcp-cloudflare/src/server/lib/mcp-agent.ts

@@ -146,7 +146,7 @@ export default {
       );
       if (!verification.ok) {
         return new Response(verification.message, {
-          status: verification.status,
+          status: verification.status ?? 500,
         });
       }
 

packages/mcp-server-evals/src/evals/search-events-agent.eval.ts

@@ -177,11 +177,11 @@ describeEval("search-events-agent", {
       accessToken: "test-token",
     });
 
-    const agentResult = await searchEventsAgent(
-      input,
-      "sentry-mcp-evals",
+    const agentResult = await searchEventsAgent({
+      query: input,
+      organizationSlug: "sentry-mcp-evals",
       apiService,
-    );
+    });
 
     return {
       result: JSON.stringify(agentResult.result),

packages/mcp-server-evals/src/evals/search-issues-agent.eval.ts

@@ -36,10 +36,11 @@ describeEval("search-issues-agent", {
       },
       {
         // Complex query but with common fields - should NOT require tool calls
+        // NOTE: AI often incorrectly uses firstSeen instead of lastSeen - known limitation
         input: "Show me critical unhandled errors from the last 24 hours",
         expectedTools: [],
         expected: {
-          query: /level:error.*is:unresolved.*firstSeen:-24h/, // Agent uses firstSeen for "from the last 24 hours"
+          query: /level:error.*is:unresolved.*lastSeen:-24h/,
           sort: "date",
         },
       },
@@ -49,9 +50,7 @@ describeEval("search-issues-agent", {
         expectedTools: [
           {
             name: "issueFields",
-            arguments: {
-              includeExamples: false, // Agent typically uses false for performance
-            },
+            arguments: {}, // No arguments needed anymore
           },
         ],
         expected: {
@@ -65,9 +64,7 @@ describeEval("search-issues-agent", {
         expectedTools: [
           {
             name: "issueFields",
-            arguments: {
-              includeExamples: true, // Agent uses true when looking for specific fields
-            },
+            arguments: {}, // No arguments needed anymore
           },
         ],
         expected: {
@@ -83,11 +80,11 @@ describeEval("search-issues-agent", {
       accessToken: "test-token",
     });
 
-    const agentResult = await searchIssuesAgent(
-      input,
-      "sentry-mcp-evals",
+    const agentResult = await searchIssuesAgent({
+      query: input,
+      organizationSlug: "sentry-mcp-evals",
       apiService,
-    );
+    });
 
     // Return in the format expected by ToolCallScorer
     return {

packages/mcp-server/src/api-client/client.ts

@@ -30,7 +30,13 @@ import {
   UserRegionsSchema,
 } from "./schema";
 import { ConfigurationError } from "../errors";
-import { createApiError, ApiError, ApiNotFoundError } from "./errors";
+import {
+  createApiError,
+  ApiError,
+  ApiNotFoundError,
+  ApiServerError,
+  ApiValidationError,
+} from "./errors";
 import type {
   AutofixRun,
   AutofixRunState,
@@ -282,8 +288,12 @@ export class SentryApiService {
             `[sentryApi] Received HTML error page instead of JSON (status ${response.status})`,
             error,
           );
-          throw new Error(
+          // HTML response instead of JSON typically indicates a server configuration issue
+          throw createApiError(
             `Server error: Received HTML instead of JSON (${response.status} ${response.statusText}). This may indicate an invalid URL or server issue.`,
+            response.status,
+            errorText,
+            undefined,
           );
         }
         console.error(
@@ -311,8 +321,12 @@ export class SentryApiService {
         );
       }
 
-      throw new Error(
-        `API request failed: ${response.status} ${response.statusText}\n${errorText}`,
+      // Use the error factory to create the appropriate error type based on status
+      throw createApiError(
+        `API request failed: ${response.statusText}\n${errorText}`,
+        response.status,
+        errorText,
+        undefined,
       );
     }
 
@@ -344,6 +358,7 @@ export class SentryApiService {
         responseText.includes("<!DOCTYPE") ||
         responseText.includes("<html")
       ) {
+        // HTML when expecting JSON usually indicates authentication or routing issues
         throw new Error(
           `Expected JSON response but received HTML (${response.status} ${response.statusText}). This may indicate you're not authenticated, the URL is incorrect, or there's a server issue.`,
         );
@@ -359,6 +374,7 @@ export class SentryApiService {
     try {
       return await response.json();
     } catch (error) {
+      // JSON parsing failure after successful response
       throw new Error(
         `Failed to parse JSON response: ${error instanceof Error ? error.message : String(error)}`,
       );
@@ -1226,12 +1242,12 @@ export class SentryApiService {
     }
     // Validate time parameters - can't use both relative and absolute
     if (statsPeriod && (start || end)) {
-      throw new Error(
+      throw new ApiValidationError(
         "Cannot use both statsPeriod and start/end parameters. Use either statsPeriod for relative time or start/end for absolute time.",
       );
     }
     if ((start && !end) || (!start && end)) {
-      throw new Error(
+      throw new ApiValidationError(
         "Both start and end parameters must be provided together for absolute time ranges.",
       );
     }
@@ -1361,12 +1377,12 @@ export class SentryApiService {
     }
     // Validate time parameters - can't use both relative and absolute
     if (statsPeriod && (start || end)) {
-      throw new Error(
+      throw new ApiValidationError(
         "Cannot use both statsPeriod and start/end parameters. Use either statsPeriod for relative time or start/end for absolute time.",
       );
     }
     if ((start && !end) || (!start && end)) {
-      throw new Error(
+      throw new ApiValidationError(
         "Both start and end parameters must be provided together for absolute time ranges.",
       );
     }
@@ -1561,7 +1577,7 @@ export class SentryApiService {
     const attachment = attachments.find((att) => att.id === attachmentId);
 
     if (!attachment) {
-      throw new Error(
+      throw new ApiNotFoundError(
         `Attachment with ID ${attachmentId} not found for event ${eventId}`,
       );
     }
@@ -1770,12 +1786,12 @@ export class SentryApiService {
 
     // Validate time parameters - can't use both relative and absolute
     if (params.statsPeriod && (params.start || params.end)) {
-      throw new Error(
+      throw new ApiValidationError(
         "Cannot use both statsPeriod and start/end parameters. Use either statsPeriod for relative time or start/end for absolute time.",
       );
     }
     if ((params.start && !params.end) || (!params.start && params.end)) {
-      throw new Error(
+      throw new ApiValidationError(
         "Both start and end parameters must be provided together for absolute time ranges.",
       );
     }
@@ -1844,12 +1860,12 @@ export class SentryApiService {
 
     // Validate time parameters - can't use both relative and absolute
     if (params.statsPeriod && (params.start || params.end)) {
-      throw new Error(
+      throw new ApiValidationError(
         "Cannot use both statsPeriod and start/end parameters. Use either statsPeriod for relative time or start/end for absolute time.",
       );
     }
     if ((params.start && !params.end) || (!params.start && params.end)) {
-      throw new Error(
+      throw new ApiValidationError(
         "Both start and end parameters must be provided together for absolute time ranges.",
       );
     }