smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge PR #5182: fix(coding-agent): load web search fallbacks lazily (@wolfiesch)

Browse Source
This commit is contained in:
can1357
2026-07-14 18:39:53 +02:00
parent 72b1ddf04c d34ae9b79d
commit 3a92c920fd
5 changed files with 127 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/CHANGELOG.md
+1
View File
@@ -128,6 +128,7 @@
- Fixed the Model Hub role-assignment strip hiding the selected chip once the row overflowed; the strip now scrolls horizontally, truncating passed chips behind a leading ellipsis so the selection (plus one chip of lookahead) stays visible.
- Fixed mouse hover and clicks in the /models Roles view landing one row above the pointer (the row mapping subtracted the status row twice).
- Fixed model search keeping the most-recently-used model on top of the results: match quality now ranks first (an exact `gpt-5.5` beats the active `gpt-5.6-sol`), with MRU order only breaking ties between equally good matches.
- 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.5] - 2026-07-11
packages/coding-agent/src/web/search/index.ts
+39 -22
View File
@@ -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<SearchProvider, "id" | "label">; error: unknown }> = [];
let availableProviderCount = 0;
let lastProvider: Pick<SearchProvider, "id" | "label"> | 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,
},
};
}
packages/coding-agent/src/web/search/provider.ts
+33 -16
View File
@@ -210,10 +210,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,
@@ -221,19 +245,12 @@ export async function resolveProviderChain(
): Promise<SearchProvider[]> {
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;
packages/coding-agent/test/web/search/abort-and-timeout.test.ts
+33 -6
View File
@@ -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();
});
});
packages/coding-agent/test/web/search/provider-chain.test.ts
+21
View File
@@ -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();