From 14240e1dafa6ea0eb3147afe8fc1dc7c2a34a207 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 29 Oct 2026 12:10:20 +0000 Subject: [PATCH 1/3] feat(mcp): Route Sentry API requests through an injected fetch Add `fetch` to McpServerOptions and ServerContext. SentryApiService sends every request through it, including @sentry/api operations, so a host can supply its authenticated transport. Without one, requests go through the shared @sentry/toolkit-core/api-transport with REQUEST_RETRY_POLICY. SentryApiService drops its own retry loop, so retries happen in one layer and do not stack. ApiRateLimitError still takes retryAfterMs from the final response's headers, and an abort during a retry wait keeps the caller's reason. The transport's credentials and timeouts become optional: without a CredentialProvider it keeps the request's Authorization header, never refreshes, and skips the cache. Implements #1669 task 6.1. Co-Authored-By: BYK --- docs/contributing/api-patterns.md | 13 +- .../src/server/lib/mcp-handler.test.ts | 11 +- .../mcp-core/src/api-client/client.test.ts | 97 ++++++++++++--- packages/mcp-core/src/api-client/client.ts | 111 ++++++------------ .../mcp-core/src/internal/tool-helpers/api.ts | 1 + packages/mcp-core/src/types.ts | 5 + packages/mcp-server/src/index.test.ts | 86 ++++++++++++++ packages/mcp-server/src/index.ts | 6 + packages/toolkit-core/README.md | 8 ++ .../toolkit-core/src/api-transport.test.ts | 29 +++++ packages/toolkit-core/src/api-transport.ts | 83 ++++++++----- 11 files changed, 320 insertions(+), 130 deletions(-) create mode 100644 packages/mcp-server/src/index.test.ts diff --git a/docs/contributing/api-patterns.md b/docs/contributing/api-patterns.md index e0db72ea5..d4dbb42ae 100644 --- a/docs/contributing/api-patterns.md +++ b/docs/contributing/api-patterns.md @@ -260,8 +260,13 @@ See error patterns in [common-patterns.md](common-patterns.md#error-handling). ### Automatic Retries -`SentryApiService.request()` transparently retries failed requests using the -shared policy in `@sentry/toolkit-core/api-retry`. This page describes the +`SentryApiService` sends every request, including `@sentry/api` operations, +through a fetch that retries failed requests using the shared policy in +`@sentry/toolkit-core/api-retry`. By default that is the shared transport in +`@sentry/toolkit-core/api-transport`; a host can pass its own authenticated +fetch instead (`fetch` in `runMcpServer` options or `ServerContext`), which +then owns the retries. `SentryApiService` never retries on top, so a request is +retried by one layer only. This page describes the behavior; the values live only in code, so read them there instead of copying them here: @@ -288,8 +293,8 @@ Retries are: are not retried. A rate-limited tool call returns a **Rate Limited** message with the retry time, not an input error. -- **Bypassed by `allowStatuses`** — a status the caller opts into is returned - as-is from `requestOnce`, never retried. +- **Before `allowStatuses`** — a status the caller opts into is returned + as-is, after the transport has finished retrying it. Callers and tools need no changes: a retried request either resolves normally or throws the same error it would have thrown without retries. diff --git a/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts b/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts index 237e12bd4..cda732f28 100644 --- a/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts +++ b/packages/mcp-cloudflare/src/server/lib/mcp-handler.test.ts @@ -634,13 +634,10 @@ describe("MCP Handler", () => { ]); expect(env.MCP_CACHE.put).not.toHaveBeenCalled(); expect(apiFetch).toHaveBeenCalledOnce(); - expect(apiFetch).toHaveBeenCalledWith( - profileUrl, - expect.objectContaining({ - headers: expect.objectContaining({ - Authorization: `Bearer ${ctx.props.accessToken}`, - }), - }), + const [url, init] = apiFetch.mock.calls[0] ?? []; + expect(url).toBe(profileUrl); + expect(new Headers(init?.headers).get("Authorization")).toBe( + `Bearer ${ctx.props.accessToken}`, ); } finally { apiFetch.mockRestore(); diff --git a/packages/mcp-core/src/api-client/client.test.ts b/packages/mcp-core/src/api-client/client.test.ts index 9e41f36fb..355a31023 100644 --- a/packages/mcp-core/src/api-client/client.test.ts +++ b/packages/mcp-core/src/api-client/client.test.ts @@ -1312,7 +1312,6 @@ describe("network error handling", () => { /Unable to connect to .* - Network request failed/, ); }); - it("should preserve the original error in the cause chain", async () => { const originalError = new Error("getaddrinfo EAI_AGAIN"); const fetchError = new Error("fetch failed"); @@ -1503,7 +1502,7 @@ describe("transient 5xx retry", () => { expect(fetchMock).toHaveBeenCalledTimes(1); }); - it("does not retry a 5xx that the caller opted into via allowStatuses", async () => { + it("returns an allowed 5xx as-is once the transport stops retrying", async () => { const fetchMock = vi.fn().mockResolvedValue(serviceUnavailable()); globalThis.fetch = fetchMock; @@ -1513,8 +1512,8 @@ describe("transient 5xx retry", () => { }); // getEventAttachment passes allowStatuses so the caller can inspect the raw - // response; such requests must be returned as-is, never retried. - const response = await ( + // response. Retries happen in the transport, before the status is checked. + const pending = ( apiService as unknown as { request: ( path: string, @@ -1523,9 +1522,10 @@ describe("transient 5xx retry", () => { ) => Promise; } ).request("/auth/", undefined, { allowStatuses: [503] }); + await vi.runAllTimersAsync(); - expect(response.status).toBe(503); - expect(fetchMock).toHaveBeenCalledTimes(1); + expect((await pending).status).toBe(503); + expect(fetchMock).toHaveBeenCalledTimes(3); }); }); @@ -1780,6 +1780,41 @@ describe("rate limit retry", () => { } }); + it("sends a still-limited GET at most maxRetries + 1 times", async () => { + const fetchMock = vi + .fn() + .mockImplementation(async () => rateLimited(nowSeconds + 1)); + globalThis.fetch = fetchMock; + + const promise = apiService().getAuthenticatedUser(); + promise.catch(() => {}); + await vi.runAllTimersAsync(); + + await expect(promise).rejects.toBeInstanceOf(ApiRateLimitError); + expect(fetchMock).toHaveBeenCalledTimes(3); + }); + + it("does not retry on top of a host-supplied fetch", async () => { + const hostFetch = vi + .fn() + .mockImplementation(async () => rateLimited(nowSeconds + 1)); + globalThis.fetch = vi.fn(); + + const promise = new SentryApiService({ + host: "sentry.io", + accessToken: "test-token", + fetch: hostFetch, + }).getAuthenticatedUser(); + promise.catch(() => {}); + await vi.runAllTimersAsync(); + + const error = await promise.catch((caught: unknown) => caught); + expect(error).toBeInstanceOf(ApiRateLimitError); + expect((error as ApiRateLimitError).retryAfterMs).toBe(700); + expect(hostFetch).toHaveBeenCalledOnce(); + expect(globalThis.fetch).not.toHaveBeenCalled(); + }); + it("stops waiting when the request is aborted", async () => { const fetchMock = vi.fn().mockResolvedValue(rateLimited(nowSeconds + 1)); globalThis.fetch = fetchMock; @@ -1837,12 +1872,18 @@ describe("request headers", () => { const [, requestInit] = (globalThis.fetch as ReturnType).mock .calls[0]; - expect(requestInit.headers["X-Sentry-MCP-Client-Id"]).toBe("abc123"); - expect(requestInit.headers["X-Sentry-MCP-Client-Name"]).toBe("Claude Code"); - expect(requestInit.headers["X-Sentry-MCP-Client-Family"]).toBe( - "claude-code", + expect(new Headers(requestInit.headers).get("X-Sentry-MCP-Client-Id")).toBe( + "abc123", ); - expect(requestInit.headers["X-Sentry-MCP-Utm-Source"]).toBe("plugin"); + expect( + new Headers(requestInit.headers).get("X-Sentry-MCP-Client-Name"), + ).toBe("Claude Code"); + expect( + new Headers(requestInit.headers).get("X-Sentry-MCP-Client-Family"), + ).toBe("claude-code"); + expect( + new Headers(requestInit.headers).get("X-Sentry-MCP-Utm-Source"), + ).toBe("plugin"); }); it("should not send MCP client headers when clientId and clientName are not set", async () => { @@ -1869,10 +1910,18 @@ describe("request headers", () => { const [, requestInit] = (globalThis.fetch as ReturnType).mock .calls[0]; - expect(requestInit.headers["X-Sentry-MCP-Client-Id"]).toBeUndefined(); - expect(requestInit.headers["X-Sentry-MCP-Client-Name"]).toBeUndefined(); - expect(requestInit.headers["X-Sentry-MCP-Client-Family"]).toBeUndefined(); - expect(requestInit.headers["X-Sentry-MCP-Utm-Source"]).toBeUndefined(); + expect( + new Headers(requestInit.headers).get("X-Sentry-MCP-Client-Id"), + ).toBeNull(); + expect( + new Headers(requestInit.headers).get("X-Sentry-MCP-Client-Name"), + ).toBeNull(); + expect( + new Headers(requestInit.headers).get("X-Sentry-MCP-Client-Family"), + ).toBeNull(); + expect( + new Headers(requestInit.headers).get("X-Sentry-MCP-Utm-Source"), + ).toBeNull(); }); }); @@ -2526,6 +2575,7 @@ describe("API query builders", () => { // Mock the API response globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, + status: 200, headers: { get: (key: string) => key === "content-type" ? "application/json" : null, @@ -2565,6 +2615,7 @@ describe("API query builders", () => { // Mock the API response globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, + status: 200, headers: { get: (key: string) => key === "content-type" ? "application/json" : null, @@ -2602,6 +2653,7 @@ describe("API query builders", () => { globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, + status: 200, headers: { get: (key: string) => key === "content-type" ? "application/json" : null, @@ -2640,6 +2692,7 @@ describe("API query builders", () => { globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, + status: 200, headers: { get: (key: string) => key === "content-type" ? "application/json" : null, @@ -2678,6 +2731,7 @@ describe("API query builders", () => { return Promise.resolve({ ok: true, + status: 200, headers: { get: (key: string) => key === "content-type" ? "application/json" : null, @@ -2773,6 +2827,7 @@ describe("API query builders", () => { return Promise.resolve({ ok: true, + status: 200, headers: { get: (key: string) => key === "content-type" ? "application/json" : null, @@ -3200,6 +3255,7 @@ describe("API query builders", () => { it("should call the repos endpoint without query", async () => { globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, + status: 200, headers: { get: (key: string) => key === "content-type" ? "application/json" : null, @@ -3236,6 +3292,7 @@ describe("API query builders", () => { .fn() .mockResolvedValueOnce({ ok: true, + status: 200, headers: new Headers({ "content-type": "application/json", link: '; rel="next"; results="true"; cursor="cursor-2"', @@ -3252,6 +3309,7 @@ describe("API query builders", () => { }) .mockResolvedValueOnce({ ok: true, + status: 200, headers: new Headers({ "content-type": "application/json", }), @@ -3283,6 +3341,7 @@ describe("API query builders", () => { it("should include query parameter when provided", async () => { globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, + status: 200, headers: { get: (key: string) => key === "content-type" ? "application/json" : null, @@ -3312,6 +3371,7 @@ describe("API query builders", () => { it("should return empty array when no repos exist", async () => { globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, + status: 200, headers: { get: (key: string) => key === "content-type" ? "application/json" : null, @@ -3329,6 +3389,7 @@ describe("API query builders", () => { it("should accept repos with null externalSlug and externalId", async () => { globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, + status: 200, headers: { get: (key: string) => key === "content-type" ? "application/json" : null, @@ -3373,6 +3434,7 @@ describe("API query builders", () => { it("should POST to the code mappings bulk endpoint", async () => { globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, + status: 200, headers: { get: (key: string) => key === "content-type" ? "application/json" : null, @@ -3423,6 +3485,7 @@ describe("API query builders", () => { it("should handle updated mappings response", async () => { globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, + status: 200, headers: { get: (key: string) => key === "content-type" ? "application/json" : null, @@ -3481,6 +3544,7 @@ describe("API query builders", () => { return Promise.resolve({ ok: true, + status: 200, headers: { get: (key: string) => { if (key === "content-type") { @@ -3526,6 +3590,7 @@ describe("API query builders", () => { requestUrl = url; return Promise.resolve({ ok: true, + status: 200, headers: { get: (key: string) => key === "content-type" ? "application/json" : null, @@ -3552,6 +3617,7 @@ describe("API query builders", () => { requestUrl = url; return Promise.resolve({ ok: true, + status: 200, headers: { get: (key: string) => key === "content-type" ? "application/json" : null, @@ -3578,6 +3644,7 @@ describe("API query builders", () => { requestUrl = url; return Promise.resolve({ ok: true, + status: 200, headers: { get: (key: string) => key === "content-type" ? "application/json" : null, diff --git a/packages/mcp-core/src/api-client/client.ts b/packages/mcp-core/src/api-client/client.ts index 0c2579b2f..b964021dc 100644 --- a/packages/mcp-core/src/api-client/client.ts +++ b/packages/mcp-core/src/api-client/client.ts @@ -22,11 +22,10 @@ import { import { buildSentryApiUrl } from "@sentry/toolkit-core/api-request"; import { sentryBearerHeader } from "@sentry/toolkit-core/auth-token"; import { - nextRetryDelayMs, parseRetryDelayMs, type RetryPolicy, - sleep, } from "@sentry/toolkit-core/api-retry"; +import { createApiTransport } from "@sentry/toolkit-core/api-transport"; import { z } from "zod"; import { DEFAULT_SEARCH_ISSUES_PERIOD } from "../constants"; import { ConfigurationError } from "../errors"; @@ -332,11 +331,13 @@ const REQUEST_RETRY_POLICY: RetryPolicy = { }; /** - * Errors thrown because `fetch` itself failed (DNS, connection refused or - * reset, socket timeout), as opposed to configuration checks before the - * request. The shared policy retries these for idempotent requests only. + * Requests without a host-supplied fetch go through the shared transport. + * `SentryApiService` sets its own headers, including Authorization, so the + * transport only retries; as before, it adds no timeout. */ -const fetchFailures = new WeakSet(); +const defaultTransport = createApiTransport({ retry: REQUEST_RETRY_POLICY }); +const defaultFetch: typeof fetch = (input, init) => + defaultTransport.request(input, init); const API_MAX_PER_PAGE = 100; const MAX_LIST_PAGES = 50; @@ -622,6 +623,7 @@ export class SentryApiService { protected host: string; protected protocol: SentryProtocol; protected apiPrefix: string; + private fetch: typeof fetch; /** * Creates a new Sentry API service instance. @@ -635,6 +637,9 @@ export class SentryApiService { * @param config.clientName DCR-registered OAuth client name * @param config.clientFamily Bucketed client family (e.g. "claude-code", "cursor") * @param config.utmSource Sanitized MCP request attribution source + * @param config.fetch Fetch every request goes through. A host passes its + * authenticated transport, which retries on its own; by default requests + * use the shared transport with `REQUEST_RETRY_POLICY`. */ constructor({ accessToken = null, @@ -644,6 +649,7 @@ export class SentryApiService { clientName = null, clientFamily = null, utmSource = null, + fetch = defaultFetch, }: { accessToken?: string | null; host?: string; @@ -652,7 +658,9 @@ export class SentryApiService { clientName?: string | null; clientFamily?: string | null; utmSource?: string | null; + fetch?: typeof globalThis.fetch; }) { + this.fetch = fetch; this.accessToken = accessToken; this.clientId = clientId; this.clientName = clientName; @@ -787,64 +795,7 @@ export class SentryApiService { } /** - * Makes an authenticated request to the Sentry API, transparently retrying - * rate-limited (429) requests, and GET and HEAD requests that hit a - * 500/502/503/504 or a network failure, under `REQUEST_RETRY_POLICY`. - * - * A 429 waits for its `Retry-After` or `X-Sentry-Rate-Limit-Reset` delay; - * everything else uses jittered backoff. Writes that hit a 5xx or a network - * failure, other 4xx errors, and configuration checks before the request - * fail fast. Callers that opt - * a status into `allowStatuses` receive the raw response from `requestOnce` - * and are never retried. Aborting `options.signal` ends any wait. - * - * @param path API endpoint path (without /api/0 prefix) - * @param options Fetch options - * @param requestOptions Additional request configuration - * @returns Promise resolving to Response object - * @throws {ApiError} Enhanced API errors with user-friendly messages - * @throws {Error} Network or parsing errors - */ - private async request( - path: string, - options: RequestInit = {}, - requestOptions: { host?: string; allowStatuses?: number[] } = {}, - ): Promise { - const method = options.method ?? "GET"; - let waitedMs = 0; - - for (let attempt = 0; ; attempt++) { - try { - return await this.requestOnce(path, options, requestOptions); - } catch (error) { - const status = error instanceof ApiError ? error.status : undefined; - const networkFailure = - error instanceof Error && - fetchFailures.has(error) && - !options.signal?.aborted; - if (status === undefined && !networkFailure) { - throw error; - } - const delayMs = nextRetryDelayMs({ - policy: REQUEST_RETRY_POLICY, - method, - status, - serverDelayMs: - error instanceof ApiRateLimitError ? error.retryAfterMs : undefined, - attempt, - waitedMs, - }); - if (delayMs === undefined) { - throw error; - } - await sleep(delayMs, options.signal); - waitedMs += delayMs; - } - } - } - - /** - * Internal method for making authenticated requests to Sentry API. + * Makes an authenticated request to the Sentry API through `this.fetch`. * * Handles: * - Bearer token authentication @@ -852,6 +803,13 @@ export class SentryApiService { * - Multi-region host overrides * - Fetch availability validation * + * Retries happen in the transport, never here, so they do not stack: a + * rate-limited (429) request, or a GET that hits a 500/502/503/504 or a + * network failure, is retried under `REQUEST_RETRY_POLICY` (or the host's + * policy), and the final failure is thrown here, with + * `ApiRateLimitError.retryAfterMs` from its headers. + * Callers that opt a status into `allowStatuses` receive the raw response. + * * @param path API endpoint path (without /api/0 prefix) * @param options Fetch options * @param requestOptions Additional request configuration @@ -859,7 +817,7 @@ export class SentryApiService { * @throws {ApiError} Enhanced API errors with user-friendly messages * @throws {Error} Network or parsing errors */ - private async requestOnce( + private async request( path: string, options: RequestInit = {}, { host, allowStatuses }: { host?: string; allowStatuses?: number[] } = {}, @@ -893,8 +851,11 @@ export class SentryApiService { headers["X-Sentry-MCP-Utm-Source"] = this.utmSource; } - // Check if fetch is available, otherwise provide a helpful error message - if (typeof globalThis.fetch === "undefined") { + // The default transport needs a global fetch; say so plainly without one. + if ( + this.fetch === defaultFetch && + typeof globalThis.fetch === "undefined" + ) { throw new ConfigurationError( "fetch is not available. Please use Node.js >= 18 or ensure fetch is available in your environment.", ); @@ -903,11 +864,15 @@ export class SentryApiService { // logger.info(logger.fmt`[sentryApi] ${options.method || "GET"} ${url}`); let response: Response; try { - response = await fetch(url, { + response = await this.fetch(url, { ...options, headers, }); } catch (error) { + // An abort, including one during a retry wait, keeps the caller's reason. + if (options.signal?.aborted) { + throw error; + } // Extract the root cause from the error chain let rootCause = error; while (rootCause instanceof Error && rootCause.cause) { @@ -938,16 +903,10 @@ export class SentryApiService { errorCode === "ETIMEDOUT" || errorMessage.includes("Connect Timeout Error") ) { - const configError = new ConfigurationError(friendlyMessage, { - cause: error, - }); - fetchFailures.add(configError); - throw configError; + throw new ConfigurationError(friendlyMessage, { cause: error }); } - const networkError = new Error(friendlyMessage, { cause: error }); - fetchFailures.add(networkError); - throw networkError; + throw new Error(friendlyMessage, { cause: error }); } // Handle error responses generically diff --git a/packages/mcp-core/src/internal/tool-helpers/api.ts b/packages/mcp-core/src/internal/tool-helpers/api.ts index 8b475a5a3..47203f4db 100644 --- a/packages/mcp-core/src/internal/tool-helpers/api.ts +++ b/packages/mcp-core/src/internal/tool-helpers/api.ts @@ -37,6 +37,7 @@ export function apiServiceFromContext( clientName: context.clientName, clientFamily: context.clientFamily, utmSource: context.utmSource, + fetch: context.fetch, }); } diff --git a/packages/mcp-core/src/types.ts b/packages/mcp-core/src/types.ts index 971ba827a..b05b0d47a 100644 --- a/packages/mcp-core/src/types.ts +++ b/packages/mcp-core/src/types.ts @@ -75,4 +75,9 @@ export type ServerContext = { * errors are swallowed. */ onUpstreamUnauthorized?: () => void | Promise; + /** + * Host transport every Sentry API request goes through, including + * `@sentry/api` operations. It owns retries, so the MCP adds none. + */ + fetch?: typeof fetch; }; diff --git a/packages/mcp-server/src/index.test.ts b/packages/mcp-server/src/index.test.ts new file mode 100644 index 000000000..3e8efbdcd --- /dev/null +++ b/packages/mcp-server/src/index.test.ts @@ -0,0 +1,86 @@ +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; +import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { runMcpServer } from "./index"; + +const started = vi.hoisted(() => ({ server: undefined as unknown })); + +vi.mock("./transports/stdio", () => ({ + startStdio: vi.fn(async (server: unknown) => { + started.server = server; + }), +})); + +/** + * A host transport answering the two endpoints the tools below call: one a + * plain `SentryApiService` request, the other an `@sentry/api` operation. + */ +function hostFetch() { + return vi.fn(async (input: RequestInfo | URL) => { + const { pathname } = new URL( + input instanceof Request ? input.url : String(input), + ); + if (pathname === "/api/0/auth/") { + return Response.json({ id: "1", name: "Test", email: "t@example.com" }); + } + if (pathname === "/api/0/organizations/") { + return Response.json([]); + } + return Response.json({ detail: "Not found" }, { status: 404 }); + }); +} + +type ToolCall = { name: string; arguments: Record }; + +async function callTools(server: McpServer, calls: ToolCall[]) { + const [clientTransport, serverTransport] = + InMemoryTransport.createLinkedPair(); + const client = new Client({ name: "test", version: "1.0.0" }); + await server.connect(serverTransport); + await client.connect(clientTransport); + try { + for (const call of calls) { + const result = await client.callTool(call); + expect(result.isError, JSON.stringify(result.content)).toBeFalsy(); + } + } finally { + await client.close(); + } +} + +describe("runMcpServer", () => { + const originalFetch = globalThis.fetch; + + afterEach(() => { + globalThis.fetch = originalFetch; + }); + + it("sends every tool request through the host's fetch", async () => { + const fetch = hostFetch(); + globalThis.fetch = vi.fn(async () => { + throw new Error("tool requests must use the host fetch"); + }); + + await runMcpServer([], { + environment: {}, + resolveAccessToken: async () => "host-token", + fetch, + throwOnError: true, + }); + await callTools(started.server as McpServer, [ + { + name: "execute_sentry_tool", + arguments: { name: "whoami", arguments: {} }, + }, + { name: "find_organizations", arguments: {} }, + ]); + + const paths = fetch.mock.calls.map( + ([input]) => + new URL(input instanceof Request ? input.url : String(input)).pathname, + ); + expect(paths).toEqual(["/api/0/auth/", "/api/0/organizations/"]); + expect(globalThis.fetch).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/mcp-server/src/index.ts b/packages/mcp-server/src/index.ts index cd8a1d608..66e9dc53f 100644 --- a/packages/mcp-server/src/index.ts +++ b/packages/mcp-server/src/index.ts @@ -61,6 +61,11 @@ export type McpServerOptions = { onUpstreamUnauthorized?: ( setAccessToken: (token: string) => void, ) => void | Promise; + /** + * Authenticated fetch for every Sentry API request the tools make. The host + * owns retries, caching, and auth refresh; the MCP does not retry on top. + */ + fetch?: typeof fetch; /** Command name used in usage output. */ packageName?: string; /** Environment used for server configuration. */ @@ -427,6 +432,7 @@ export async function runMcpServer( openaiBaseUrl: cfg.openaiBaseUrl, experimentalMode: cli.experimental, transport: "stdio" as const, + fetch: options.fetch, // Let the host refresh its credential on an upstream 401 and write it back // so subsequent tool calls use the new token. Tool handlers read // `context.accessToken` fresh per call, so mutating it here is enough. diff --git a/packages/toolkit-core/README.md b/packages/toolkit-core/README.md index 5b2ce5bb9..cc920f677 100644 --- a/packages/toolkit-core/README.md +++ b/packages/toolkit-core/README.md @@ -37,6 +37,14 @@ fatal fetch errors such as TLS failures, a logger, and a span hook. The CLI's `packages/cli/src/lib/sentry-client.ts` wires in its stored credentials, host-scope checks, disk cache, and telemetry. +Credentials and timeouts are optional. Without a `CredentialProvider`, the +transport keeps the request's own Authorization header, never refreshes, and +skips the cache, since entries are scoped to a credential. The MCP's +`SentryApiService` uses it that way by default, with retries only. A host can +instead pass its authenticated fetch through the `fetch` option of +`@sentry/mcp-server`'s `runMcpServer`, and the MCP then adds no retries of its +own. + ## Pagination `pagination` walks Sentry list endpoints up to a row limit. `paginate` splits diff --git a/packages/toolkit-core/src/api-transport.test.ts b/packages/toolkit-core/src/api-transport.test.ts index 1492ff57d..cb68c9956 100644 --- a/packages/toolkit-core/src/api-transport.test.ts +++ b/packages/toolkit-core/src/api-transport.test.ts @@ -406,6 +406,17 @@ describe("headers", () => { "user-agent": "test-agent", }); }); + + it("leaves Authorization to the request without credentials", async () => { + const { fetch, sent } = scriptedFetch([Response.json({})]); + const api = createApiTransport({ retry: policy, fetch }); + + await api.request(ISSUES_URL, { + headers: { Authorization: "Bearer from-request" }, + }); + + expect(sent[0]?.headers.get("Authorization")).toBe("Bearer from-request"); + }); }); describe("validated redirects", () => { @@ -567,6 +578,24 @@ describe("cache", () => { expect(sent).toHaveLength(2); }); + it("is not consulted without credentials", async () => { + const lookup = vi.fn(); + const store = vi.fn(); + const { fetch, sent } = scriptedFetch([Response.json({})]); + const api = createApiTransport({ + retry: policy, + fetch, + cache: { lookup, store, invalidate: vi.fn() }, + }); + + await (await api.request(ISSUES_URL)).json(); + await (await api.request(ISSUES_URL)).json(); + + expect(sent).toHaveLength(2); + expect(lookup).not.toHaveBeenCalled(); + expect(store).not.toHaveBeenCalled(); + }); + it("revalidates a stale entry with its ETag", async () => { const response = await transport( scriptedFetch([new Response(null, { status: 304 })]).fetch, diff --git a/packages/toolkit-core/src/api-transport.ts b/packages/toolkit-core/src/api-transport.ts index 5c0ac1388..55c726344 100644 --- a/packages/toolkit-core/src/api-transport.ts +++ b/packages/toolkit-core/src/api-transport.ts @@ -64,10 +64,15 @@ type TransportLogger = { }; export type ApiTransportOptions = { - credentials: CredentialProvider; + /** + * Without credentials the transport leaves Authorization to the request, + * does not refresh, and does not cache, since entries are scoped to the + * credential's identity. + */ + credentials?: CredentialProvider; retry: RetryPolicy; - /** Per-attempt timeout, or a function of the request URL. */ - timeoutMs: number | ((url: string) => number); + /** Per-attempt timeout, or a function of the request URL. Default: none. */ + timeoutMs?: number | ((url: string) => number); /** Underlying fetch, read on each call. Defaults to `globalThis.fetch`. */ fetch?: (input: FetchInput, init?: RequestInit) => Promise; /** Sent when the request sets no `User-Agent`. */ @@ -164,6 +169,9 @@ export class ApiTimeoutError extends Error { const MAX_REDIRECTS = 20; +/** Stands in for the credential when the host sets Authorization itself. */ +const ANONYMOUS: TransportCredential = { token: "", identity: "" }; + /** Where a response fetched with `validateRedirects` came from. */ export type ValidatedResponseSource = { origin: string; identity: string }; @@ -202,8 +210,10 @@ class Rejection { constructor(readonly error: unknown) {} } -/** One attempt: a response, or a thrown error and whether our timeout caused it. */ -type Outcome = { response: Response } | { failed: unknown; timedOut: boolean }; +/** One attempt: a response, or a thrown error and our timeout if it caused it. */ +type Outcome = + | { response: Response } + | { failed: unknown; timedOutAfterMs?: number }; type AttemptResult = | { action: "done"; response: Response } @@ -226,12 +236,16 @@ type Send = { export function createApiTransport( config: ApiTransportOptions, ): ApiTransport { - const { credentials, retry, cache, logger } = config; + const { credentials, retry, logger } = config; + const cache = credentials && config.cache; const baseFetch = (input: FetchInput, init?: RequestInit) => (config.fetch ?? globalThis.fetch)(input, init); - function authorization(credential: C): string { + function authorization(credential: C): string | undefined { + if (!credentials) { + return; + } if (credentials.authorization) { return credentials.authorization(credential); } @@ -242,10 +256,10 @@ export function createApiTransport( return header; } - function timeoutFor(url: string): number { - return typeof config.timeoutMs === "number" - ? config.timeoutMs - : config.timeoutMs(url); + function timeoutFor(url: string): number | undefined { + return typeof config.timeoutMs === "function" + ? config.timeoutMs(url) + : config.timeoutMs; } /** @@ -263,7 +277,7 @@ export function createApiTransport( init?.headers ?? (input instanceof Request ? input.headers : undefined), ); config.prepareHeaders?.(headers, { input, credential }); - headers.set("Authorization", authorization(credential)); + setAuthorization(headers, authorization(credential)); if (config.userAgent && !headers.has("User-Agent")) { headers.set("User-Agent", config.userAgent); } @@ -292,15 +306,18 @@ export function createApiTransport( input: FetchInput, init: RequestInit, signal: AbortSignal | null | undefined, - timeoutMs: number, + timeoutMs: number | undefined, mode: ResponseBodyMode, ): Promise { const controller = new AbortController(); let timedOut = false; - const timer = setTimeout(() => { - timedOut = true; - controller.abort(); - }, timeoutMs); + const timer = + timeoutMs === undefined + ? undefined + : setTimeout(() => { + timedOut = true; + controller.abort(); + }, timeoutMs); const onAbort = () => controller.abort(); if (signal?.aborted) { controller.abort(); @@ -318,7 +335,7 @@ export function createApiTransport( return { response }; } catch (error) { if (timedOut) { - return { failed: error, timedOut: true }; + return { failed: error, timedOutAfterMs: timeoutMs }; } throw error; } finally { @@ -386,7 +403,7 @@ export function createApiTransport( init: RequestInit | undefined, headers: Headers, credential: C, - timeoutMs: number, + timeoutMs: number | undefined, mode: ResponseBodyMode, ): Promise { const original = new Request(input, init); @@ -438,7 +455,7 @@ export function createApiTransport( /** Send with refresh, timeouts, and retries; the cache is handled by the caller. */ async function send(sendArgs: Send): Promise { const { input, init, method, url, path, options } = sendArgs; - let credential = credentials.prepare + let credential = credentials?.prepare ? await credentials.prepare(sendArgs.credential) : sendArgs.credential; const headers = buildHeaders(input, init, credential); @@ -483,7 +500,7 @@ export function createApiTransport( ): Promise { if (response.status === 401 && attempt < retry.maxRetries) { const next = - refreshed || !credentials.refresh + refreshed || !credentials?.refresh ? undefined : await credentials.refresh(credential); if (!next) { @@ -491,7 +508,7 @@ export function createApiTransport( } refreshed = true; credential = next; - headers.set("Authorization", authorization(next)); + setAuthorization(headers, authorization(next)); if (config.refreshedRequestHeader) { headers.set(config.refreshedRequestHeader, "1"); } @@ -521,7 +538,7 @@ export function createApiTransport( } function onFailure( - { failed, timedOut }: Extract, + { failed, timedOutAfterMs }: Extract, attempt: number, ): AttemptResult { if (failed instanceof Rejection) { @@ -538,7 +555,7 @@ export function createApiTransport( if (delayMs !== undefined) { return { action: "retry", delayMs }; } - if (!timedOut) { + if (timedOutAfterMs === undefined) { return { action: "throw", error: failed }; } // Whether the endpoint may have run, for the host's error wording. @@ -546,8 +563,10 @@ export function createApiTransport( return { action: "throw", error: - config.errors?.timeout?.({ timeoutMs, idempotent: replayable }) ?? - new ApiTimeoutError(timeoutMs, replayable), + config.errors?.timeout?.({ + timeoutMs: timedOutAfterMs, + idempotent: replayable, + }) ?? new ApiTimeoutError(timedOutAfterMs, replayable), }; } @@ -572,7 +591,7 @@ export function createApiTransport( mode, ); } catch (error) { - outcome = { failed: error, timedOut: false }; + outcome = { failed: error }; } // Outside the try: handling a response is not a fetch failure. const result = @@ -665,7 +684,9 @@ export function createApiTransport( const run = async (): Promise => { const startTime = performance.now(); const elapsed = () => (performance.now() - startTime).toFixed(0); - const credential = await credentials.current(); + const credential = credentials + ? await credentials.current() + : (ANONYMOUS as C); // A cached response has no validated origin, so discovery always // goes to the network. const lookup = @@ -758,6 +779,12 @@ async function replayableInput( return () => ({ input, init }); } +function setAuthorization(headers: Headers, value: string | undefined): void { + if (value !== undefined) { + headers.set("Authorization", value); + } +} + /** The origin of a credential-free HTTP(S) URL, or undefined. */ function httpOrigin(raw: string): string | undefined { const url = new URL(raw); From 2f8203a0683693705080651260091adee8e9fbbd Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 29 Oct 2026 13:58:42 +0000 Subject: [PATCH 2/3] fix(toolkit-core): Keep the caller's abort reason for in-flight requests The transport aborted its per-attempt controller without the caller's reason, so an in-flight fetch rejected with a generic AbortError. Forward signal.reason, and treat a rejection with that reason as the caller's abort rather than a retryable network failure. Co-Authored-By: BYK --- .../mcp-core/src/api-client/client.test.ts | 24 +++++++++++++++++++ .../toolkit-core/src/api-transport.test.ts | 23 ++++++++++++++++++ packages/toolkit-core/src/api-transport.ts | 12 ++++++---- 3 files changed, 54 insertions(+), 5 deletions(-) diff --git a/packages/mcp-core/src/api-client/client.test.ts b/packages/mcp-core/src/api-client/client.test.ts index 355a31023..9fa082830 100644 --- a/packages/mcp-core/src/api-client/client.test.ts +++ b/packages/mcp-core/src/api-client/client.test.ts @@ -1831,6 +1831,30 @@ describe("rate limit retry", () => { await expect(promise).rejects.toBe(reason); expect(fetchMock).toHaveBeenCalledTimes(1); }); + + it("keeps the caller's abort reason for an in-flight request", async () => { + const fetchMock = vi.fn( + (_input: Request | string | URL, init?: RequestInit) => + new Promise((_, reject) => { + init?.signal?.addEventListener("abort", () => + reject(init.signal?.reason), + ); + }), + ); + globalThis.fetch = fetchMock; + const controller = new AbortController(); + const reason = new Error("tool call cancelled"); + + const promise = rawRequest(apiService())("/auth/", { + signal: controller.signal, + }); + promise.catch(() => {}); + await vi.waitFor(() => expect(fetchMock).toHaveBeenCalledOnce()); + controller.abort(reason); + + await expect(promise).rejects.toBe(reason); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); }); describe("request headers", () => { diff --git a/packages/toolkit-core/src/api-transport.test.ts b/packages/toolkit-core/src/api-transport.test.ts index cb68c9956..471bce4e3 100644 --- a/packages/toolkit-core/src/api-transport.test.ts +++ b/packages/toolkit-core/src/api-transport.test.ts @@ -331,6 +331,29 @@ describe("timeouts and fetch errors", () => { ); }); + it("rejects an in-flight request with the caller's abort reason", async () => { + // Like fetch, reject with the signal's reason once it aborts. + const fetch = vi.fn( + (_input: Request | string | URL, init?: RequestInit) => + new Promise((_, reject) => { + init?.signal?.addEventListener("abort", () => + reject(init.signal?.reason), + ); + }), + ); + const controller = new AbortController(); + const reason = new Error("tool call cancelled"); + + const pending = transport(fetch) + .request(ISSUES_URL, { signal: controller.signal }) + .catch((error: unknown) => error); + await vi.waitFor(() => expect(fetch).toHaveBeenCalledOnce()); + controller.abort(reason); + + expect(await pending).toBe(reason); + expect(fetch).toHaveBeenCalledOnce(); + }); + it("throws the host timeout error for a POST without retrying", async () => { const fetch = vi.fn(hang); diff --git a/packages/toolkit-core/src/api-transport.ts b/packages/toolkit-core/src/api-transport.ts index 55c726344..6be6f5171 100644 --- a/packages/toolkit-core/src/api-transport.ts +++ b/packages/toolkit-core/src/api-transport.ts @@ -318,9 +318,10 @@ export function createApiTransport( timedOut = true; controller.abort(); }, timeoutMs); - const onAbort = () => controller.abort(); + // Forward the caller's reason, so an aborted fetch rejects with it. + const onAbort = () => controller.abort(signal?.reason); if (signal?.aborted) { - controller.abort(); + onAbort(); } else { signal?.addEventListener("abort", onAbort, { once: true }); } @@ -736,11 +737,12 @@ export function createApiTransport( return { request }; } +/** Whether `error` is the caller's abort, with the default or its own reason. */ function isAbort(error: unknown, signal: AbortSignal | null | undefined) { return ( - error instanceof DOMException && - error.name === "AbortError" && - Boolean(signal?.aborted) + Boolean(signal?.aborted) && + (error === signal?.reason || + (error instanceof DOMException && error.name === "AbortError")) ); } From 0a3ca3634d71740e4c2a44155387c392542007bb Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 29 Oct 2026 16:36:14 +0000 Subject: [PATCH 3/3] ref(mcp): Drop the host-supplied fetch option The shared API layer moves into toolkit-core and then @sentry/api, so the MCP no longer needs a host fetch hook. SentryApiService keeps sending every request through the shared transport with no retry loop of its own. Co-Authored-By: BYK --- docs/contributing/api-patterns.md | 10 +-- .../mcp-core/src/api-client/client.test.ts | 22 +---- packages/mcp-core/src/api-client/client.ts | 32 +++---- .../mcp-core/src/internal/tool-helpers/api.ts | 1 - packages/mcp-core/src/types.ts | 5 -- packages/mcp-server/src/index.test.ts | 86 ------------------- packages/mcp-server/src/index.ts | 6 -- packages/toolkit-core/README.md | 5 +- 8 files changed, 16 insertions(+), 151 deletions(-) delete mode 100644 packages/mcp-server/src/index.test.ts diff --git a/docs/contributing/api-patterns.md b/docs/contributing/api-patterns.md index d4dbb42ae..34eee1a65 100644 --- a/docs/contributing/api-patterns.md +++ b/docs/contributing/api-patterns.md @@ -261,12 +261,10 @@ See error patterns in [common-patterns.md](common-patterns.md#error-handling). ### Automatic Retries `SentryApiService` sends every request, including `@sentry/api` operations, -through a fetch that retries failed requests using the shared policy in -`@sentry/toolkit-core/api-retry`. By default that is the shared transport in -`@sentry/toolkit-core/api-transport`; a host can pass its own authenticated -fetch instead (`fetch` in `runMcpServer` options or `ServerContext`), which -then owns the retries. `SentryApiService` never retries on top, so a request is -retried by one layer only. This page describes the +through the shared transport in `@sentry/toolkit-core/api-transport`, which +retries failed requests using the shared policy in +`@sentry/toolkit-core/api-retry`. `SentryApiService` never retries on top, so +a request is retried by one layer only. This page describes the behavior; the values live only in code, so read them there instead of copying them here: diff --git a/packages/mcp-core/src/api-client/client.test.ts b/packages/mcp-core/src/api-client/client.test.ts index 9fa082830..976f4914f 100644 --- a/packages/mcp-core/src/api-client/client.test.ts +++ b/packages/mcp-core/src/api-client/client.test.ts @@ -1312,6 +1312,7 @@ describe("network error handling", () => { /Unable to connect to .* - Network request failed/, ); }); + it("should preserve the original error in the cause chain", async () => { const originalError = new Error("getaddrinfo EAI_AGAIN"); const fetchError = new Error("fetch failed"); @@ -1794,27 +1795,6 @@ describe("rate limit retry", () => { expect(fetchMock).toHaveBeenCalledTimes(3); }); - it("does not retry on top of a host-supplied fetch", async () => { - const hostFetch = vi - .fn() - .mockImplementation(async () => rateLimited(nowSeconds + 1)); - globalThis.fetch = vi.fn(); - - const promise = new SentryApiService({ - host: "sentry.io", - accessToken: "test-token", - fetch: hostFetch, - }).getAuthenticatedUser(); - promise.catch(() => {}); - await vi.runAllTimersAsync(); - - const error = await promise.catch((caught: unknown) => caught); - expect(error).toBeInstanceOf(ApiRateLimitError); - expect((error as ApiRateLimitError).retryAfterMs).toBe(700); - expect(hostFetch).toHaveBeenCalledOnce(); - expect(globalThis.fetch).not.toHaveBeenCalled(); - }); - it("stops waiting when the request is aborted", async () => { const fetchMock = vi.fn().mockResolvedValue(rateLimited(nowSeconds + 1)); globalThis.fetch = fetchMock; diff --git a/packages/mcp-core/src/api-client/client.ts b/packages/mcp-core/src/api-client/client.ts index b964021dc..b099a9d7b 100644 --- a/packages/mcp-core/src/api-client/client.ts +++ b/packages/mcp-core/src/api-client/client.ts @@ -331,13 +331,11 @@ const REQUEST_RETRY_POLICY: RetryPolicy = { }; /** - * Requests without a host-supplied fetch go through the shared transport. - * `SentryApiService` sets its own headers, including Authorization, so the - * transport only retries; as before, it adds no timeout. + * Every request goes through the shared transport. `SentryApiService` sets its + * own headers, including Authorization, so the transport only retries; as + * before, it adds no timeout. */ -const defaultTransport = createApiTransport({ retry: REQUEST_RETRY_POLICY }); -const defaultFetch: typeof fetch = (input, init) => - defaultTransport.request(input, init); +const transport = createApiTransport({ retry: REQUEST_RETRY_POLICY }); const API_MAX_PER_PAGE = 100; const MAX_LIST_PAGES = 50; @@ -623,7 +621,6 @@ export class SentryApiService { protected host: string; protected protocol: SentryProtocol; protected apiPrefix: string; - private fetch: typeof fetch; /** * Creates a new Sentry API service instance. @@ -637,9 +634,6 @@ export class SentryApiService { * @param config.clientName DCR-registered OAuth client name * @param config.clientFamily Bucketed client family (e.g. "claude-code", "cursor") * @param config.utmSource Sanitized MCP request attribution source - * @param config.fetch Fetch every request goes through. A host passes its - * authenticated transport, which retries on its own; by default requests - * use the shared transport with `REQUEST_RETRY_POLICY`. */ constructor({ accessToken = null, @@ -649,7 +643,6 @@ export class SentryApiService { clientName = null, clientFamily = null, utmSource = null, - fetch = defaultFetch, }: { accessToken?: string | null; host?: string; @@ -658,9 +651,7 @@ export class SentryApiService { clientName?: string | null; clientFamily?: string | null; utmSource?: string | null; - fetch?: typeof globalThis.fetch; }) { - this.fetch = fetch; this.accessToken = accessToken; this.clientId = clientId; this.clientName = clientName; @@ -795,7 +786,7 @@ export class SentryApiService { } /** - * Makes an authenticated request to the Sentry API through `this.fetch`. + * Makes an authenticated request to the Sentry API through the shared transport. * * Handles: * - Bearer token authentication @@ -805,8 +796,8 @@ export class SentryApiService { * * Retries happen in the transport, never here, so they do not stack: a * rate-limited (429) request, or a GET that hits a 500/502/503/504 or a - * network failure, is retried under `REQUEST_RETRY_POLICY` (or the host's - * policy), and the final failure is thrown here, with + * network failure, is retried under `REQUEST_RETRY_POLICY`, and the final + * failure is thrown here, with * `ApiRateLimitError.retryAfterMs` from its headers. * Callers that opt a status into `allowStatuses` receive the raw response. * @@ -851,11 +842,8 @@ export class SentryApiService { headers["X-Sentry-MCP-Utm-Source"] = this.utmSource; } - // The default transport needs a global fetch; say so plainly without one. - if ( - this.fetch === defaultFetch && - typeof globalThis.fetch === "undefined" - ) { + // Check if fetch is available, otherwise provide a helpful error message + if (typeof globalThis.fetch === "undefined") { throw new ConfigurationError( "fetch is not available. Please use Node.js >= 18 or ensure fetch is available in your environment.", ); @@ -864,7 +852,7 @@ export class SentryApiService { // logger.info(logger.fmt`[sentryApi] ${options.method || "GET"} ${url}`); let response: Response; try { - response = await this.fetch(url, { + response = await transport.request(url, { ...options, headers, }); diff --git a/packages/mcp-core/src/internal/tool-helpers/api.ts b/packages/mcp-core/src/internal/tool-helpers/api.ts index 47203f4db..8b475a5a3 100644 --- a/packages/mcp-core/src/internal/tool-helpers/api.ts +++ b/packages/mcp-core/src/internal/tool-helpers/api.ts @@ -37,7 +37,6 @@ export function apiServiceFromContext( clientName: context.clientName, clientFamily: context.clientFamily, utmSource: context.utmSource, - fetch: context.fetch, }); } diff --git a/packages/mcp-core/src/types.ts b/packages/mcp-core/src/types.ts index b05b0d47a..971ba827a 100644 --- a/packages/mcp-core/src/types.ts +++ b/packages/mcp-core/src/types.ts @@ -75,9 +75,4 @@ export type ServerContext = { * errors are swallowed. */ onUpstreamUnauthorized?: () => void | Promise; - /** - * Host transport every Sentry API request goes through, including - * `@sentry/api` operations. It owns retries, so the MCP adds none. - */ - fetch?: typeof fetch; }; diff --git a/packages/mcp-server/src/index.test.ts b/packages/mcp-server/src/index.test.ts deleted file mode 100644 index 3e8efbdcd..000000000 --- a/packages/mcp-server/src/index.test.ts +++ /dev/null @@ -1,86 +0,0 @@ -import { Client } from "@modelcontextprotocol/sdk/client/index.js"; -import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; -import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; -import { afterEach, describe, expect, it, vi } from "vitest"; -import { runMcpServer } from "./index"; - -const started = vi.hoisted(() => ({ server: undefined as unknown })); - -vi.mock("./transports/stdio", () => ({ - startStdio: vi.fn(async (server: unknown) => { - started.server = server; - }), -})); - -/** - * A host transport answering the two endpoints the tools below call: one a - * plain `SentryApiService` request, the other an `@sentry/api` operation. - */ -function hostFetch() { - return vi.fn(async (input: RequestInfo | URL) => { - const { pathname } = new URL( - input instanceof Request ? input.url : String(input), - ); - if (pathname === "/api/0/auth/") { - return Response.json({ id: "1", name: "Test", email: "t@example.com" }); - } - if (pathname === "/api/0/organizations/") { - return Response.json([]); - } - return Response.json({ detail: "Not found" }, { status: 404 }); - }); -} - -type ToolCall = { name: string; arguments: Record }; - -async function callTools(server: McpServer, calls: ToolCall[]) { - const [clientTransport, serverTransport] = - InMemoryTransport.createLinkedPair(); - const client = new Client({ name: "test", version: "1.0.0" }); - await server.connect(serverTransport); - await client.connect(clientTransport); - try { - for (const call of calls) { - const result = await client.callTool(call); - expect(result.isError, JSON.stringify(result.content)).toBeFalsy(); - } - } finally { - await client.close(); - } -} - -describe("runMcpServer", () => { - const originalFetch = globalThis.fetch; - - afterEach(() => { - globalThis.fetch = originalFetch; - }); - - it("sends every tool request through the host's fetch", async () => { - const fetch = hostFetch(); - globalThis.fetch = vi.fn(async () => { - throw new Error("tool requests must use the host fetch"); - }); - - await runMcpServer([], { - environment: {}, - resolveAccessToken: async () => "host-token", - fetch, - throwOnError: true, - }); - await callTools(started.server as McpServer, [ - { - name: "execute_sentry_tool", - arguments: { name: "whoami", arguments: {} }, - }, - { name: "find_organizations", arguments: {} }, - ]); - - const paths = fetch.mock.calls.map( - ([input]) => - new URL(input instanceof Request ? input.url : String(input)).pathname, - ); - expect(paths).toEqual(["/api/0/auth/", "/api/0/organizations/"]); - expect(globalThis.fetch).not.toHaveBeenCalled(); - }); -}); diff --git a/packages/mcp-server/src/index.ts b/packages/mcp-server/src/index.ts index 66e9dc53f..cd8a1d608 100644 --- a/packages/mcp-server/src/index.ts +++ b/packages/mcp-server/src/index.ts @@ -61,11 +61,6 @@ export type McpServerOptions = { onUpstreamUnauthorized?: ( setAccessToken: (token: string) => void, ) => void | Promise; - /** - * Authenticated fetch for every Sentry API request the tools make. The host - * owns retries, caching, and auth refresh; the MCP does not retry on top. - */ - fetch?: typeof fetch; /** Command name used in usage output. */ packageName?: string; /** Environment used for server configuration. */ @@ -432,7 +427,6 @@ export async function runMcpServer( openaiBaseUrl: cfg.openaiBaseUrl, experimentalMode: cli.experimental, transport: "stdio" as const, - fetch: options.fetch, // Let the host refresh its credential on an upstream 401 and write it back // so subsequent tool calls use the new token. Tool handlers read // `context.accessToken` fresh per call, so mutating it here is enough. diff --git a/packages/toolkit-core/README.md b/packages/toolkit-core/README.md index cc920f677..f3f94b631 100644 --- a/packages/toolkit-core/README.md +++ b/packages/toolkit-core/README.md @@ -40,10 +40,7 @@ host-scope checks, disk cache, and telemetry. Credentials and timeouts are optional. Without a `CredentialProvider`, the transport keeps the request's own Authorization header, never refreshes, and skips the cache, since entries are scoped to a credential. The MCP's -`SentryApiService` uses it that way by default, with retries only. A host can -instead pass its authenticated fetch through the `fetch` option of -`@sentry/mcp-server`'s `runMcpServer`, and the MCP then adds no retries of its -own. +`SentryApiService` uses it that way, with retries only. ## Pagination