diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index f2e6e06f6..fb89893ce 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -31,6 +31,7 @@ - Fixed discarded `Settings` instances keeping debounced save timers and chained background saves armed; discarding an instance now cancels its pending writes so they cannot race a successor's file locks. - Fixed startup status messages (warnings, errors, extension/tool errors, status lines) keeping the dark-mode color after auto-theme detection later switched the active theme to light — e.g. `dark-catppuccin`/`light-catppuccin` warnings rendered in Mocha yellow on the Latte background. Transient status presenters now resolve their color lazily at render time so a theme swap re-shapes them ([#6337](https://github.com/can1357/oh-my-pi/issues/6337)). +- Fixed MCP OAuth endpoint discovery and Smithery browser-login polling hanging indefinitely against an endpoint that accepts the TCP connection but never responds. `discoverOAuthEndpoints`/`fetchResourceMetadataScopes` now bound every metadata/well-known/authorization-server fetch with a per-request `AbortSignal.timeout`, and `pollSmitheryCliAuthSession` bounds each poll so the loop reaches its 5-minute deadline instead of stalling ([#4103](https://github.com/can1357/oh-my-pi/issues/4103)). - Fixed `error.notify` raising a "Stopped with error" toast for provider failures while an auto-retry or async-delivery continuation was pending; the toast now waits for the true terminal settle. - Fixed concurrent MCP config mutations losing updates and racing on a shared temp path: every `mcp.json` read-modify-write (add/update/remove server, disabled/force-enabled lists) is now serialized under a per-file lock, and each atomic write uses a unique temp file so overlapping writers no longer rename each other's `.tmp` out from under them (ENOENT or clobbered config) — reachable in-process via the fire-and-forget extensions-dashboard toggle and across processes on a shared `~/.omp/mcp.json` ([#4104](https://github.com/can1357/oh-my-pi/issues/4104)). - Fixed terminal `yield` results racing post-turn maintenance, which could trigger an unnecessary automatic handoff or compaction. diff --git a/packages/coding-agent/src/mcp/oauth-discovery.ts b/packages/coding-agent/src/mcp/oauth-discovery.ts index c47830823..ab9927379 100644 --- a/packages/coding-agent/src/mcp/oauth-discovery.ts +++ b/packages/coding-agent/src/mcp/oauth-discovery.ts @@ -6,6 +6,10 @@ */ import * as AIError from "@oh-my-pi/pi-ai/error"; import type { FetchImpl } from "@oh-my-pi/pi-ai/types"; +import { withTimeoutSignal } from "../utils/fetch-timeout"; + +/** Per-request abort deadline for each OAuth discovery metadata fetch. */ +const DISCOVERY_FETCH_TIMEOUT_MS = 10_000; export interface OAuthEndpoints { authorizationUrl: string; @@ -346,7 +350,7 @@ function readMetadataScopes(metadata: Record): string | undefin */ export async function fetchResourceMetadataScopes( resourceMetadataUrl: string, - opts?: { fetch?: FetchImpl }, + opts?: { fetch?: FetchImpl; signal?: AbortSignal }, ): Promise { const fetchImpl: FetchImpl = opts?.fetch ?? fetch; try { @@ -354,6 +358,7 @@ export async function fetchResourceMetadataScopes( method: "GET", headers: { Accept: "application/json" }, redirect: "follow", + signal: withTimeoutSignal(DISCOVERY_FETCH_TIMEOUT_MS, opts?.signal), }); if (!resp.ok) return undefined; const meta = (await resp.json()) as Record; @@ -371,7 +376,7 @@ export async function discoverOAuthEndpoints( serverUrl: string, authServerUrl?: string, resourceMetadataUrl?: string, - opts?: { fetch?: FetchImpl; protectedResource?: string; protectedScopes?: string }, + opts?: { fetch?: FetchImpl; protectedResource?: string; protectedScopes?: string; signal?: AbortSignal }, ): Promise { const fetchImpl: FetchImpl = opts?.fetch ?? fetch; const wellKnownPaths = [ @@ -402,6 +407,7 @@ export async function discoverOAuthEndpoints( method: "GET", headers: { Accept: "application/json" }, redirect: "follow", + signal: withTimeoutSignal(DISCOVERY_FETCH_TIMEOUT_MS, opts?.signal), }); if (metaResp.ok) { const meta = (await metaResp.json()) as Record; @@ -486,6 +492,7 @@ export async function discoverOAuthEndpoints( method: "GET", headers: { Accept: "application/json" }, redirect: "follow", + signal: withTimeoutSignal(DISCOVERY_FETCH_TIMEOUT_MS, opts?.signal), }); if (response.ok) { @@ -521,6 +528,7 @@ export async function discoverOAuthEndpoints( fetch: fetchImpl, protectedResource: discoveredProtectedResource, protectedScopes: readMetadataScopes(metadata) ?? protectedScopes, + signal: opts?.signal, }); if (discovered) return discovered; } diff --git a/packages/coding-agent/src/mcp/smithery-auth.test.ts b/packages/coding-agent/src/mcp/smithery-auth.test.ts new file mode 100644 index 000000000..ccee396b2 --- /dev/null +++ b/packages/coding-agent/src/mcp/smithery-auth.test.ts @@ -0,0 +1,29 @@ +import { afterEach, describe, expect, it, vi } from "bun:test"; +import { pollSmitheryCliAuthSession } from "./smithery-auth"; + +type FetchInput = string | URL | Request; +type FetchInit = RequestInit | BunFetchRequestInit; + +describe("pollSmitheryCliAuthSession fetch cancellation", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("bounds each poll request with a timeout abort signal even without a caller signal", async () => { + const signals: AbortSignal[] = []; + const fetchStub = Object.assign( + async (_input: FetchInput, init?: FetchInit) => { + if (init?.signal instanceof AbortSignal) signals.push(init.signal); + return Response.json({ status: "pending" }); + }, + { preconnect: globalThis.fetch.preconnect }, + ); + vi.spyOn(globalThis, "fetch").mockImplementation(fetchStub); + + const result = await pollSmitheryCliAuthSession("sess-123"); + + expect(result.status).toBe("pending"); + expect(signals).toHaveLength(1); + expect(signals[0]?.aborted).toBe(false); + }); +}); diff --git a/packages/coding-agent/src/mcp/smithery-auth.ts b/packages/coding-agent/src/mcp/smithery-auth.ts index 4aa03ae0e..fd3a578b5 100644 --- a/packages/coding-agent/src/mcp/smithery-auth.ts +++ b/packages/coding-agent/src/mcp/smithery-auth.ts @@ -7,6 +7,7 @@ import { withTimeoutSignal } from "../utils/fetch-timeout"; const SMITHERY_AUTH_FILENAME = "smithery.json"; const SMITHERY_URL = process.env.SMITHERY_URL || "https://smithery.ai"; const SMITHERY_AUTH_TIMEOUT_MS = 10_000; +const SMITHERY_POLL_TIMEOUT_MS = 30_000; type SmitheryCliAuthSession = { sessionId: string; @@ -53,7 +54,7 @@ export async function pollSmitheryCliAuthSession( signal?: AbortSignal, ): Promise { const response = await fetch(`${SMITHERY_URL}/api/auth/cli/poll/${sessionId}`, { - signal, + signal: withTimeoutSignal(SMITHERY_POLL_TIMEOUT_MS, signal), }); if (!response.ok) { if (response.status === 404 || response.status === 410) { diff --git a/packages/coding-agent/test/oauth-discovery.test.ts b/packages/coding-agent/test/oauth-discovery.test.ts index 5b77c2b2e..8a5023ab5 100644 --- a/packages/coding-agent/test/oauth-discovery.test.ts +++ b/packages/coding-agent/test/oauth-discovery.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from "bun:test"; +import type { FetchImpl } from "@oh-my-pi/pi-ai/types"; import { analyzeAuthError, discoverOAuthEndpoints, @@ -641,3 +642,34 @@ describe("RFC 8414 §3.3 issuer validation", () => { }); }); }); + +describe("bounded discovery fetches", () => { + // A fetch that never resolves on its own; it settles only when its + // AbortSignal fires. Pre-fix, discovery passed no signal, so this hung forever. + const hangingFetch: FetchImpl = (_input, init) => + new Promise((_resolve, reject) => { + const signal = init?.signal; + const abort = () => reject(new DOMException("aborted", "AbortError")); + if (signal?.aborted) { + abort(); + return; + } + signal?.addEventListener("abort", abort, { once: true }); + }); + + it("aborts hanging well-known discovery fetches instead of stalling", async () => { + const oauth = await discoverOAuthEndpoints("https://mcp.example.test/mcp", undefined, undefined, { + fetch: hangingFetch, + signal: AbortSignal.timeout(50), + }); + expect(oauth).toBeNull(); + }); + + it("aborts a hanging resource_metadata fetch and returns undefined", async () => { + const scopes = await fetchResourceMetadataScopes( + "https://mcp.example.test/.well-known/oauth-protected-resource", + { fetch: hangingFetch, signal: AbortSignal.timeout(50) }, + ); + expect(scopes).toBeUndefined(); + }); +});