fix(mcp-common): classify upstream 4xx errors correctly instead of returning 500 (#294)

* fix(mcp-common): classify upstream errors correctly instead of returning 500

Several error paths in mcp-common unconditionally throw McpError with
status 500 regardless of the actual upstream HTTP status. This makes it
impossible for clients to distinguish retriable server errors from
actionable client errors like expired tokens or invalid grants.

- Pass upstream 4xx status codes through with reportToSentry=false
- Normalize upstream 5xx to 502 Bad Gateway with reportToSentry=true
- Add safeStatusCode() helper to clamp status codes safely
- Return OAuth 2.1 spec-compliant JSON error responses
- Strip raw upstream error bodies from client-facing messages
- Check response.ok before Zod parsing in getUserAndAccounts
- Add 51 tests across 5 new spec files

* refactor(mcp-common): align error handling with cloudflare-mcp patterns

Extract shared helpers to DRY up repeated error classification:
- throwUpstreamApiError() in mcp-error.ts for API failures
- throwUpstreamTokenError() in cloudflare-auth.ts for token endpoints
- throwCombinedApiError() for priority-based dual-endpoint failures

Make getUserAndAccounts resilient:
- Wrap Promise.all in try/catch for network errors (→ 502)
- Use safeParse with graceful fallback instead of throwing .parse()
- Surface accounts error when user returns unparseable 200

Fix OAuth 429 mapping to temporarily_unavailable (RFC 6749).
Truncate raw upstream bodies in internalMessage to 500 chars.
Add 12 new tests for combined failures, mixed-status priority,
and malformed JSON edge cases (88 total).

* style: fix prettier formatting

* fix(mcp-common): update test config for node:fs and remove env mocks

- Update compatibilityDate to 2026-03-05 for node:fs ReadStream support
- Add DEV_DISABLE_OAUTH binding to vitest config instead of vi.mock
- Remove vi.mock('cloudflare:workers') from cloudflare-api.spec.ts and sentry.spec.ts

* fix(mcp-common): surface accounts errors, granular refresh mapping, truncate rawBody

- Throw when /accounts fails even if /user succeeds (empty accounts is unusable)
- Map refresh 429→temporarily_unavailable, 401→invalid_client instead of all-4xx→invalid_grant
- Truncate rawBody to 500 chars in throwUpstreamApiError matching throwUpstreamTokenError

* fix(mcp-common): mock cloudflare SDK in workerd tests, update vitest compat date

The cloudflare SDK internally imports ReadStream from node:fs, which
isn't available in workerd's vitest environment. Mock the SDK in the
three affected test files. Update vitest compat date to 2026-03-09.

* fix(mcp-common): use resolve.alias to mock cloudflare SDK in workerd tests

vi.mock() doesn't prevent workerd from resolving the cloudflare SDK's
transitive node:fs ReadStream import during module graph analysis.
Use vitest resolve.alias to redirect 'cloudflare' to a lightweight
mock module, avoiding the node:fs incompatibility entirely.

* ci: use Node.js 24 only, drop matrix

No longer publishing npm packages from this repo, so testing
against multiple Node versions is unnecessary.

* Apply suggestion from @mattzcarey

* chore: add changeset for mcp-common error classification fix

* fix(mcp-common): use GrantType enum and add requestedScope for updated oauth provider

---------

Co-authored-by: Matt Carey <mcarey@cloudflare.com>
Co-authored-by: Matt <77928207+mattzcarey@users.noreply.github.com>

Author: irvinebroque

.changeset/fix-5xx-error-classification.md

@@ -0,0 +1,5 @@
+---
+'@repo/mcp-common': patch
+---
+
+Classify upstream 4xx errors correctly instead of returning 500, and set reportToSentry flag to avoid alerting on expected client errors

packages/mcp-common/src/__mocks__/cloudflare.ts

@@ -0,0 +1,18 @@
+/**
+ * Mock for the 'cloudflare' SDK package.
+ * The real SDK transitively imports ReadStream from node:fs which is unavailable in workerd.
+ * This mock is wired via resolve.alias in vitest.config.ts so the real module is never loaded.
+ */
+
+export class Cloudflare {
+	constructor(_opts?: Record<string, unknown>) {}
+}
+
+export class APIError extends Error {
+	status: number
+	constructor(status: number, message?: string) {
+		super(message)
+		this.status = status
+		this.name = 'APIError'
+	}
+}

packages/mcp-common/src/cloudflare-api.spec.ts

@@ -0,0 +1,159 @@
+import { fetchMock } from 'cloudflare:test'
+import { beforeAll, describe, expect, it } from 'vitest'
+
+import { fetchCloudflareApi } from './cloudflare-api'
+import { McpError } from './mcp-error'
+
+beforeAll(() => {
+	fetchMock.activate()
+	fetchMock.disableNetConnect()
+})
+
+describe('fetchCloudflareApi', () => {
+	const baseParams = {
+		endpoint: '/workers/scripts',
+		accountId: 'test-account-id',
+		apiToken: 'test-api-token',
+	}
+
+	it('returns parsed data on success', async () => {
+		const responseData = { result: { id: 'test-script' } }
+		fetchMock
+			.get('https://api.cloudflare.com')
+			.intercept({
+				path: '/client/v4/accounts/test-account-id/workers/scripts',
+				method: 'GET',
+			})
+			.reply(200, responseData)
+
+		const result = await fetchCloudflareApi(baseParams)
+		expect(result).toEqual(responseData)
+	})
+
+	it('throws McpError with status 404 for not found', async () => {
+		fetchMock
+			.get('https://api.cloudflare.com')
+			.intercept({
+				path: '/client/v4/accounts/test-account-id/workers/scripts',
+				method: 'GET',
+			})
+			.reply(404, JSON.stringify({ errors: [{ message: 'Script not found' }] }))
+
+		try {
+			await fetchCloudflareApi(baseParams)
+			expect.unreachable()
+		} catch (e) {
+			expect(e).toBeInstanceOf(McpError)
+			const err = e as McpError
+			expect(err.code).toBe(404)
+			expect(err.reportToSentry).toBe(false)
+			expect(err.message).toBe('Cloudflare API request failed')
+			expect(err.internalMessage).toContain('Script not found')
+		}
+	})
+
+	it('throws McpError with status 403 for forbidden', async () => {
+		fetchMock
+			.get('https://api.cloudflare.com')
+			.intercept({
+				path: '/client/v4/accounts/test-account-id/workers/scripts',
+				method: 'GET',
+			})
+			.reply(403, JSON.stringify({ errors: [{ message: 'Forbidden' }] }))
+
+		try {
+			await fetchCloudflareApi(baseParams)
+			expect.unreachable()
+		} catch (e) {
+			expect(e).toBeInstanceOf(McpError)
+			const err = e as McpError
+			expect(err.code).toBe(403)
+			expect(err.reportToSentry).toBe(false)
+		}
+	})
+
+	it('throws McpError with status 429 for rate limiting', async () => {
+		fetchMock
+			.get('https://api.cloudflare.com')
+			.intercept({
+				path: '/client/v4/accounts/test-account-id/workers/scripts',
+				method: 'GET',
+			})
+			.reply(429, JSON.stringify({ errors: [{ message: 'Rate limited' }] }))
+
+		try {
+			await fetchCloudflareApi(baseParams)
+			expect.unreachable()
+		} catch (e) {
+			expect(e).toBeInstanceOf(McpError)
+			const err = e as McpError
+			expect(err.code).toBe(429)
+			expect(err.reportToSentry).toBe(false)
+		}
+	})
+
+	it('throws McpError with status 502 for upstream 500 (bad gateway)', async () => {
+		fetchMock
+			.get('https://api.cloudflare.com')
+			.intercept({
+				path: '/client/v4/accounts/test-account-id/workers/scripts',
+				method: 'GET',
+			})
+			.reply(500, 'Internal Server Error')
+
+		try {
+			await fetchCloudflareApi(baseParams)
+			expect.unreachable()
+		} catch (e) {
+			expect(e).toBeInstanceOf(McpError)
+			const err = e as McpError
+			expect(err.code).toBe(502)
+			expect(err.message).toBe('Upstream Cloudflare API unavailable')
+			expect(err.reportToSentry).toBe(true)
+			expect(err.internalMessage).toContain('Cloudflare API 500')
+		}
+	})
+
+	it('throws McpError with status 502 for upstream 502', async () => {
+		fetchMock
+			.get('https://api.cloudflare.com')
+			.intercept({
+				path: '/client/v4/accounts/test-account-id/workers/scripts',
+				method: 'GET',
+			})
+			.reply(502, 'Bad Gateway')
+
+		try {
+			await fetchCloudflareApi(baseParams)
+			expect.unreachable()
+		} catch (e) {
+			expect(e).toBeInstanceOf(McpError)
+			const err = e as McpError
+			expect(err.code).toBe(502)
+			expect(err.message).toBe('Upstream Cloudflare API unavailable')
+			expect(err.reportToSentry).toBe(true)
+			expect(err.internalMessage).toContain('Cloudflare API 502')
+		}
+	})
+
+	it('preserves error text in internalMessage (not user-facing message)', async () => {
+		const errorBody = '{"errors":[{"message":"Worker not found","code":10007}]}'
+		fetchMock
+			.get('https://api.cloudflare.com')
+			.intercept({
+				path: '/client/v4/accounts/test-account-id/workers/scripts',
+				method: 'GET',
+			})
+			.reply(404, errorBody)
+
+		try {
+			await fetchCloudflareApi(baseParams)
+			expect.unreachable()
+		} catch (e) {
+			expect(e).toBeInstanceOf(McpError)
+			const err = e as McpError
+			expect(err.message).toBe('Cloudflare API request failed')
+			expect(err.internalMessage).toContain('Worker not found')
+		}
+	})
+})

packages/mcp-common/src/cloudflare-api.ts

@@ -1,6 +1,8 @@
 import { Cloudflare } from 'cloudflare'
 import { env } from 'cloudflare:workers'
 
+import { throwUpstreamApiError } from './mcp-error'
+
 import type { z } from 'zod'
 
 export function getCloudflareClient(apiToken: string) {
@@ -55,8 +57,7 @@ export async function fetchCloudflareApi<T>({
 	})
 
 	if (!response.ok) {
-		const error = await response.text()
-		throw new Error(`Cloudflare API request failed: ${error}`)
+		throwUpstreamApiError(response.status, 'Cloudflare API', await response.text())
 	}
 
 	const data = await response.json()