From 0930e483ecf98ef363fddb51e47bf0475d34b629 Mon Sep 17 00:00:00 2001 From: Dudu-0223 Date: Thu, 25 Jun 2026 16:03:55 +0800 Subject: [PATCH] fix: address codex review round 2 - Preserve abort errors while parsing search responses: when the caller's AbortSignal fires after headers but during response.json() (both the success and HTTP-error body parses), surface WEB_ABORTED instead of wrapping it as WEB_PROVIDER_ERROR, so agent cancel/dispose is not misreported as a provider failure. Applied to both the Exa and Perplexity providers. - Report a malformed baseURL as misconfigured in status() (URL.canParse), so selection diagnostics and execution agree (configured-unavailable up front rather than a late WEB_PROVIDER_ERROR). WebProviderStatus already had the reason. --- packages/web/web-search-exa/README.md | 2 +- packages/web/web-search-exa/src/provider.ts | 20 ++++++++++++++----- packages/web/web-search-exa/tests/exa.spec.ts | 19 ++++++++++++++++++ packages/web/web-search-perplexity/README.md | 2 +- .../web/web-search-perplexity/src/provider.ts | 15 +++++++++----- .../tests/perplexity.spec.ts | 19 ++++++++++++++++++ 6 files changed, 65 insertions(+), 12 deletions(-) diff --git a/packages/web/web-search-exa/README.md b/packages/web/web-search-exa/README.md index 7f39356e81..61da605df7 100644 --- a/packages/web/web-search-exa/README.md +++ b/packages/web/web-search-exa/README.md @@ -9,7 +9,7 @@ This is an **implementation** package: it registers a provider into `ctx.web`, i | Key | Default | Meaning | |---|---|---| | `apiKey` | `$EXA_API_KEY` | Exa API key. Empty/absent → provider `status()` reports `missing-credential` (the seam reports `configured-unavailable`/`none`). | -| `baseURL` | `https://api.exa.ai` | Endpoint base; `/search` is appended. | +| `baseURL` | `https://api.exa.ai` | Endpoint base; `/search` is appended. An unparseable value makes `status()` report `misconfigured`. | ```yaml - id: web-search-exa diff --git a/packages/web/web-search-exa/src/provider.ts b/packages/web/web-search-exa/src/provider.ts index d7c464059a..70e14cbf25 100644 --- a/packages/web/web-search-exa/src/provider.ts +++ b/packages/web/web-search-exa/src/provider.ts @@ -72,6 +72,7 @@ export class ExaSearchProvider implements WebSearchProvider { status(): WebProviderStatus { if (this.options.apiKey.length === 0) return { available: false, reason: 'missing-credential' } + if (!isValidBaseUrl(this.options.baseURL)) return { available: false, reason: 'misconfigured' } return { available: true } } @@ -105,11 +106,14 @@ export class ExaSearchProvider implements WebSearchProvider { const parsed = await response.json() as ExaError const detail = parsed.error ?? parsed.message if (detail !== undefined && detail.length > 0) message = detail - } catch { - // The HTTP status is already captured in `message` above; a malformed or - // non-JSON error body (normal for gateway 5xx/429s) can only cost a - // richer provider message, never the real error. `response.json()` is - // the sole statement and nothing else of consequence reaches here. + } catch (error: unknown) { + // An abort fired mid-body must surface as WEB_ABORTED, not be swallowed + // into a generic HTTP-error message — cancellation is not a provider + // error (the seam's cancellation contract). + if (isAbortError(error)) throw new WebError('Exa search aborted', 'WEB_ABORTED', { cause: error }) + // Otherwise: the HTTP status is already captured in `message` above; a + // malformed/non-JSON error body (normal for gateway 5xx/429s) can only + // cost a richer provider message, never the real error. } throw new WebError(message, 'WEB_PROVIDER_ERROR') } @@ -118,12 +122,18 @@ export class ExaSearchProvider implements WebSearchProvider { try { payload = await response.json() as ExaSearchResponse } catch (error: unknown) { + if (isAbortError(error)) throw new WebError('Exa search aborted', 'WEB_ABORTED', { cause: error }) throw new WebError(`Exa returned an unparseable response body: ${String(error)}`, 'WEB_PROVIDER_ERROR', { cause: error }) } return mapExaResponse(request.query, payload) } } +/** True when `baseURL` parses as an absolute URL (a cheap local config check). */ +function isValidBaseUrl(baseURL: string): boolean { + return URL.canParse(baseURL) +} + /** True for a fetch/`AbortSignal` abort, surfaced as `WEB_ABORTED`. */ function isAbortError(error: unknown): boolean { return error instanceof DOMException && error.name === 'AbortError' diff --git a/packages/web/web-search-exa/tests/exa.spec.ts b/packages/web/web-search-exa/tests/exa.spec.ts index 3fdd878180..403198401e 100644 --- a/packages/web/web-search-exa/tests/exa.spec.ts +++ b/packages/web/web-search-exa/tests/exa.spec.ts @@ -71,6 +71,11 @@ describe('ExaSearchProvider status', () => { it('is available with a key', () => { expect(new ExaSearchProvider(options).status()).toEqual({ available: true }) }) + + it('is misconfigured when the base URL is unparseable', () => { + expect(new ExaSearchProvider({ apiKey: 'exa-key', baseURL: 'not a url' }).status()) + .toEqual({ available: false, reason: 'misconfigured' }) + }) }) describe('ExaSearchProvider request mapping', () => { @@ -142,6 +147,20 @@ describe('ExaSearchProvider error handling', () => { await expect(new ExaSearchProvider(options).search({ query: 'q' })) .rejects.toThrow(expect.objectContaining({ code: 'WEB_PROVIDER_ERROR' })) }) + + it('surfaces an abort during success-body parse as WEB_ABORTED, not provider error', async () => { + const body = { json: () => Promise.reject(new DOMException('aborted', 'AbortError')), ok: true, status: 200 } + vi.stubGlobal('fetch', vi.fn(async () => body as unknown as Response)) + await expect(new ExaSearchProvider(options).search({ query: 'q' })) + .rejects.toThrow(expect.objectContaining({ code: 'WEB_ABORTED' })) + }) + + it('surfaces an abort during error-body parse as WEB_ABORTED', async () => { + const body = { json: () => Promise.reject(new DOMException('aborted', 'AbortError')), ok: false, status: 500 } + vi.stubGlobal('fetch', vi.fn(async () => body as unknown as Response)) + await expect(new ExaSearchProvider(options).search({ query: 'q' })) + .rejects.toThrow(expect.objectContaining({ code: 'WEB_ABORTED' })) + }) }) describe('web-search-exa plugin registration', () => { diff --git a/packages/web/web-search-perplexity/README.md b/packages/web/web-search-perplexity/README.md index 3850e5e9c1..e7093a1133 100644 --- a/packages/web/web-search-perplexity/README.md +++ b/packages/web/web-search-perplexity/README.md @@ -9,7 +9,7 @@ This is an **implementation** package: it registers a provider into `ctx.web`, i | Key | Default | Meaning | |---|---|---| | `apiKey` | `$PERPLEXITY_API_KEY` | Perplexity API key. Empty/absent → provider `status()` reports `missing-credential`. | -| `baseURL` | `https://api.perplexity.ai` | Endpoint base; `/chat/completions` is appended. | +| `baseURL` | `https://api.perplexity.ai` | Endpoint base; `/chat/completions` is appended. An unparseable value makes `status()` report `misconfigured`. | | `model` | `sonar` | Search model name. | ```yaml diff --git a/packages/web/web-search-perplexity/src/provider.ts b/packages/web/web-search-perplexity/src/provider.ts index 2b596414dd..69b4f794dd 100644 --- a/packages/web/web-search-perplexity/src/provider.ts +++ b/packages/web/web-search-perplexity/src/provider.ts @@ -81,6 +81,7 @@ export class PerplexitySearchProvider implements WebSearchProvider { status(): WebProviderStatus { if (this.options.apiKey.length === 0) return { available: false, reason: 'missing-credential' } + if (!URL.canParse(this.options.baseURL)) return { available: false, reason: 'misconfigured' } return { available: true } } @@ -113,11 +114,14 @@ export class PerplexitySearchProvider implements WebSearchProvider { const parsed = await response.json() as PerplexityError const detail = typeof parsed.error === 'string' ? parsed.error : parsed.error?.message ?? parsed.message if (detail !== undefined && detail.length > 0) message = detail - } catch { - // The HTTP status is already captured in `message` above; a malformed or - // non-JSON error body (normal for gateway 5xx/429s) can only cost a - // richer provider message, never the real error. `response.json()` is - // the sole statement and nothing else of consequence reaches here. + } catch (error: unknown) { + // An abort fired mid-body must surface as WEB_ABORTED, not be swallowed + // into a generic HTTP-error message — cancellation is not a provider + // error (the seam's cancellation contract). + if (isAbortError(error)) throw new WebError('Perplexity search aborted', 'WEB_ABORTED', { cause: error }) + // Otherwise: the HTTP status is already captured in `message` above; a + // malformed/non-JSON error body (normal for gateway 5xx/429s) can only + // cost a richer provider message, never the real error. } throw new WebError(message, 'WEB_PROVIDER_ERROR') } @@ -126,6 +130,7 @@ export class PerplexitySearchProvider implements WebSearchProvider { try { payload = await response.json() as PerplexityResponse } catch (error: unknown) { + if (isAbortError(error)) throw new WebError('Perplexity search aborted', 'WEB_ABORTED', { cause: error }) throw new WebError(`Perplexity returned an unparseable response body: ${String(error)}`, 'WEB_PROVIDER_ERROR', { cause: error }) } return mapPerplexityResponse(request.query, payload) diff --git a/packages/web/web-search-perplexity/tests/perplexity.spec.ts b/packages/web/web-search-perplexity/tests/perplexity.spec.ts index 16557af888..c1f76a63fb 100644 --- a/packages/web/web-search-perplexity/tests/perplexity.spec.ts +++ b/packages/web/web-search-perplexity/tests/perplexity.spec.ts @@ -75,6 +75,11 @@ describe('PerplexitySearchProvider status', () => { it('is available with a key', () => { expect(new PerplexitySearchProvider(options).status()).toEqual({ available: true }) }) + + it('is misconfigured when the base URL is unparseable', () => { + expect(new PerplexitySearchProvider({ ...options, baseURL: 'not a url' }).status()) + .toEqual({ available: false, reason: 'misconfigured' }) + }) }) describe('PerplexitySearchProvider request mapping', () => { @@ -135,6 +140,20 @@ describe('PerplexitySearchProvider error handling', () => { .rejects.toThrow(expect.objectContaining({ code: 'WEB_PROVIDER_ERROR' })) }) + it('surfaces an abort during success-body parse as WEB_ABORTED, not provider error', async () => { + const body = { json: () => Promise.reject(new DOMException('aborted', 'AbortError')), ok: true, status: 200 } + vi.stubGlobal('fetch', vi.fn(async () => body as unknown as Response)) + await expect(new PerplexitySearchProvider(options).search({ query: 'q' })) + .rejects.toThrow(expect.objectContaining({ code: 'WEB_ABORTED' })) + }) + + it('surfaces an abort during error-body parse as WEB_ABORTED', async () => { + const body = { json: () => Promise.reject(new DOMException('aborted', 'AbortError')), ok: false, status: 500 } + vi.stubGlobal('fetch', vi.fn(async () => body as unknown as Response)) + await expect(new PerplexitySearchProvider(options).search({ query: 'q' })) + .rejects.toThrow(expect.objectContaining({ code: 'WEB_ABORTED' })) + }) + it('maps a network failure to WEB_PROVIDER_ERROR', async () => { vi.stubGlobal('fetch', vi.fn(() => Promise.reject(new TypeError('connection refused')))) await expect(new PerplexitySearchProvider(options).search({ query: 'q' }))