diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 9b021d8d6..82ceef5a1 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -54,6 +54,7 @@ - Fixed agents getting stuck waiting for messages from peers that have already stopped running. - Fixed compiled Linux binary extension loading when bundled web-search header generation cannot read `header-generator` data files from the build-time path. ([#5178](https://github.com/can1357/oh-my-pi/issues/5178)) - Fixed plugin custom tool loading to skip and report invalid feature entries instead of crashing startup when a plugin dependency tree leaves one feature unresolved. ([#5189](https://github.com/can1357/oh-my-pi/issues/5189)) +- Fixed preferred web search providers failing before execution when an unrelated fallback provider could not initialize. ([#5182](https://github.com/can1357/oh-my-pi/pull/5182) by [@wolfiesch](https://github.com/wolfiesch)) ## [16.4.4] - 2026-07-11 diff --git a/packages/coding-agent/src/web/search/index.ts b/packages/coding-agent/src/web/search/index.ts index b81eedacf..b4d3ae795 100644 --- a/packages/coding-agent/src/web/search/index.ts +++ b/packages/coding-agent/src/web/search/index.ts @@ -21,8 +21,10 @@ import { formatSearchProviderFailure, formatSearchProviderFailures, getSearchProvider, - resolveProviderChain, + getSearchProviderLabel, + resolveProviderCandidates, type SearchProvider, + type SearchProviderCandidate, } from "./provider"; import { renderSearchCall, renderSearchResult, type SearchRenderDetails } from "./render"; import type { SearchProviderId, SearchResponse } from "./types"; @@ -128,25 +130,18 @@ async function executeSearch( ): Promise<{ content: Array<{ type: "text"; text: string }>; details: SearchRenderDetails }> { const { authStorage, sessionId, signal } = options; const explicitProvider = params.provider; - let providers: SearchProvider[]; + let candidates: SearchProviderCandidate[]; if (explicitProvider && explicitProvider !== "auto") { const provider = await getSearchProvider(explicitProvider); - providers = (await provider.isExplicitlyAvailable(authStorage)) - ? [provider] - : await resolveProviderChain(authStorage, "auto"); + candidates = (await provider.isExplicitlyAvailable(authStorage)) + ? [{ id: explicitProvider, explicit: true }] + : resolveProviderCandidates("auto"); } else if (explicitProvider === "auto") { // Explicit `--provider auto` bypasses the configured preferred provider // for this invocation; exclusions still apply. - providers = await resolveProviderChain(authStorage, "auto"); + candidates = resolveProviderCandidates("auto"); } else { - providers = await resolveProviderChain(authStorage); - } - if (providers.length === 0) { - const message = "No web search provider configured."; - return { - content: [{ type: "text" as const, text: `Error: ${message}` }], - details: { response: { provider: "none", sources: [] }, error: message }, - }; + candidates = resolveProviderCandidates(); } // Invariant across providers; read once and tolerate an uninitialized @@ -166,11 +161,22 @@ async function executeSearch( geminiModel = undefined; } - const failures: Array<{ provider: SearchProvider; error: unknown }> = []; - let lastProvider = providers[0]; - for (const provider of providers) { - lastProvider = provider; + const failures: Array<{ provider: Pick; error: unknown }> = []; + let availableProviderCount = 0; + let lastProvider: Pick | undefined; + for (const candidate of candidates) { + let provider: SearchProvider | undefined; + const providerMeta = { id: candidate.id, label: getSearchProviderLabel(candidate.id) }; + lastProvider = providerMeta; try { + provider = await getSearchProvider(candidate.id); + const available = candidate.explicit + ? await provider.isExplicitlyAvailable(authStorage) + : await provider.isAvailable(authStorage); + if (!available) continue; + availableProviderCount++; + lastProvider = provider; + const response = await provider.search({ query: params.query, limit: params.limit, @@ -203,20 +209,31 @@ async function executeSearch( // failure and the loop falls through to the next provider (or to the // summary error), masking the cancellation. throwIfAborted(signal); - failures.push({ provider, error }); + failures.push({ provider: provider ?? providerMeta, error }); } } + if (availableProviderCount === 0 && failures.length === 0) { + const message = "No web search provider configured."; + return { + content: [{ type: "text" as const, text: `Error: ${message}` }], + details: { response: { provider: "none", sources: [] }, error: message }, + }; + } + const lastFailure = failures[failures.length - 1]; const baseMessage = lastFailure ? formatSearchProviderFailure(lastFailure.error, lastFailure.provider) - : `Unknown error from ${lastProvider.label}`; + : `Unknown error from ${lastProvider?.label ?? "web search provider"}`; const message = - providers.length > 1 ? `All web search providers failed: ${formatSearchProviderFailures(failures)}` : baseMessage; + failures.length > 1 ? `All web search providers failed: ${formatSearchProviderFailures(failures)}` : baseMessage; return { content: [{ type: "text" as const, text: `Error: ${message}` }], - details: { response: { provider: lastProvider.id, sources: [] }, error: message }, + details: { + response: { provider: lastFailure?.provider.id ?? lastProvider?.id ?? "none", sources: [] }, + error: message, + }, }; } diff --git a/packages/coding-agent/src/web/search/provider.ts b/packages/coding-agent/src/web/search/provider.ts index 4c2ee6f28..86e769292 100644 --- a/packages/coding-agent/src/web/search/provider.ts +++ b/packages/coding-agent/src/web/search/provider.ts @@ -220,10 +220,34 @@ export function isSearchProviderExcluded(id: SearchProviderId): boolean { return excludedProvIds.has(id); } +export interface SearchProviderCandidate { + id: SearchProviderId; + explicit: boolean; +} + +/** Return provider candidates in fallback order without loading their modules. */ +export function resolveProviderCandidates( + preferredProvider: SearchProviderId | "auto" = preferredProvId, +): SearchProviderCandidate[] { + const candidates: SearchProviderCandidate[] = []; + + if (preferredProvider !== "auto" && !isSearchProviderExcluded(preferredProvider)) { + candidates.push({ id: preferredProvider, explicit: true }); + } + + for (const id of SEARCH_PROVIDER_ORDER) { + if (id === preferredProvider || isSearchProviderExcluded(id)) continue; + candidates.push({ id, explicit: false }); + } + + return candidates; +} + /** - * Determine which providers are configured and currently available. - * Each candidate is loaded (and its `isAvailable()` called) only as the chain - * is walked, so unconfigured providers never pay the load cost. + * Resolve the complete available provider chain. + * + * This compatibility helper loads every candidate. Search execution should use + * {@link resolveProviderCandidates} so fallback modules load only when reached. */ export async function resolveProviderChain( authStorage: AuthStorage, @@ -231,19 +255,12 @@ export async function resolveProviderChain( ): Promise { const providers: SearchProvider[] = []; - if (preferredProvider !== "auto" && !isSearchProviderExcluded(preferredProvider)) { - const provider = await getSearchProvider(preferredProvider); - if (await provider.isExplicitlyAvailable(authStorage)) { - providers.push(provider); - } - } - - for (const id of SEARCH_PROVIDER_ORDER) { - if (id === preferredProvider || isSearchProviderExcluded(id)) continue; - const provider = await getSearchProvider(id); - if (await provider.isAvailable(authStorage)) { - providers.push(provider); - } + for (const candidate of resolveProviderCandidates(preferredProvider)) { + const provider = await getSearchProvider(candidate.id); + const available = candidate.explicit + ? await provider.isExplicitlyAvailable(authStorage) + : await provider.isAvailable(authStorage); + if (available) providers.push(provider); } return providers; diff --git a/packages/coding-agent/test/web/search/abort-and-timeout.test.ts b/packages/coding-agent/test/web/search/abort-and-timeout.test.ts index 3ca38a00c..7a55c646c 100644 --- a/packages/coding-agent/test/web/search/abort-and-timeout.test.ts +++ b/packages/coding-agent/test/web/search/abort-and-timeout.test.ts @@ -173,13 +173,24 @@ describe("executeSearch abort propagation", () => { }; } + function mockProviderChain(providers: provider.SearchProvider[]) { + vi.spyOn(provider, "resolveProviderCandidates").mockReturnValue( + providers.map(({ id }) => ({ id, explicit: false })), + ); + return vi.spyOn(provider, "getSearchProvider").mockImplementation(async id => { + const match = providers.find(candidate => candidate.id === id); + if (!match) throw new Error(`Unexpected provider: ${id}`); + return match; + }); + } + it("surfaces caller cancellation as ToolAbortError instead of falling through to the next provider", async () => { // Two providers: the first throws an AbortError after the caller aborted, // the second would happily return a value. Pre-fix, executeSearch would // fall through to provider B and report success; post-fix, the abort // re-throw stops the loop immediately. const secondProviderSearch = vi.fn(); - vi.spyOn(provider, "resolveProviderChain").mockResolvedValue([ + mockProviderChain([ fakeProvider("anthropic", async () => { throw new DOMException("aborted", "AbortError"); }), @@ -198,7 +209,7 @@ describe("executeSearch abort propagation", () => { // Defensive: the abort re-throw must NOT alter normal provider-error // flow. A genuine provider error should still produce an error result // rather than throwing. - vi.spyOn(provider, "resolveProviderChain").mockResolvedValue([ + mockProviderChain([ fakeProvider("anthropic", async () => { throw new Error("upstream 500"); }), @@ -225,10 +236,7 @@ describe("executeSearch abort propagation", () => { sources: [{ title: "Fallback result", url: "https://example.com/fallback", snippet: "fallback body" }], }), ); - vi.spyOn(provider, "resolveProviderChain").mockResolvedValue([ - fakeProvider("searxng", emptyProviderSearch), - fakeProvider("brave", sourceProviderSearch), - ]); + mockProviderChain([fakeProvider("searxng", emptyProviderSearch), fakeProvider("brave", sourceProviderSearch)]); const tool = new WebSearchTool(FAKE_SESSION); const result = await tool.execute("test-id", { query: "anything" }); @@ -240,4 +248,23 @@ describe("executeSearch abort propagation", () => { expect(block && "text" in block ? block.text : "").toContain("Fallback result"); expect(result.details?.response.provider).toBe("brave"); }); + + it("does not load fallback providers after the preferred provider succeeds", async () => { + const fallbackSearch = vi.fn(); + const getProvider = mockProviderChain([ + fakeProvider("exa", async () => ({ + provider: "exa", + sources: [{ title: "Preferred result", url: "https://example.com/preferred" }], + })), + fakeProvider("bing", fallbackSearch), + ]); + + const tool = new WebSearchTool(FAKE_SESSION); + const result = await tool.execute("test-id", { query: "anything" }); + + expect(result.details?.response.provider).toBe("exa"); + expect(getProvider).toHaveBeenCalledTimes(1); + expect(getProvider).toHaveBeenCalledWith("exa"); + expect(fallbackSearch).not.toHaveBeenCalled(); + }); }); diff --git a/packages/coding-agent/test/web/search/provider-chain.test.ts b/packages/coding-agent/test/web/search/provider-chain.test.ts index 2aa3d6248..cfd8228ef 100644 --- a/packages/coding-agent/test/web/search/provider-chain.test.ts +++ b/packages/coding-agent/test/web/search/provider-chain.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it } from "bun:test"; import type { AuthStorage } from "@oh-my-pi/pi-ai"; import { SelectorController } from "@oh-my-pi/pi-coding-agent/modes/controllers/selector-controller"; import { + resolveProviderCandidates, resolveProviderChain, setExcludedSearchProviders, setPreferredSearchProvider, @@ -37,6 +38,26 @@ afterEach(() => { restoreEnv(); }); +describe("resolveProviderCandidates", () => { + it("orders the preferred provider before unloaded fallbacks", () => { + const candidates = resolveProviderCandidates("exa"); + + expect(candidates[0]).toEqual({ id: "exa", explicit: true }); + expect(candidates.slice(1).map(candidate => candidate.id)).toEqual( + SEARCH_PROVIDER_ORDER.filter(id => id !== "exa"), + ); + }); + + it("omits excluded providers without resolving them", () => { + setExcludedSearchProviders(["bing", "google"]); + + const candidates = resolveProviderCandidates("exa"); + + expect(candidates.map(candidate => candidate.id)).not.toContain("bing"); + expect(candidates.map(candidate => candidate.id)).not.toContain("google"); + }); +}); + describe("resolveProviderChain", () => { it("omits excluded providers from the fallback chain", async () => { enableKeyBackedProviders();