diff --git a/src/resources/extensions/search-the-web/index.ts b/src/resources/extensions/search-the-web/index.ts index da809375f..793c1ea15 100644 --- a/src/resources/extensions/search-the-web/index.ts +++ b/src/resources/extensions/search-the-web/index.ts @@ -10,15 +10,21 @@ import { registerSearchProviderCommand } from "./command-search-provider.js"; import { registerNativeSearchHooks } from "./native-search.js"; let toolsPromise: Promise | null = null; +let resetSearchLoopGuardStateRef: (() => void) | null = null; async function registerSearchTools(pi: ExtensionAPI): Promise { if (!toolsPromise) { toolsPromise = (async () => { - const [{ registerSearchTool }, { registerFetchPageTool }, { registerLLMContextTool }] = await Promise.all([ + const [ + { registerSearchTool, resetSearchLoopGuardState }, + { registerFetchPageTool }, + { registerLLMContextTool }, + ] = await Promise.all([ importExtensionModule(import.meta.url, "./tool-search.js"), importExtensionModule(import.meta.url, "./tool-fetch-page.js"), importExtensionModule(import.meta.url, "./tool-llm-context.js"), ]); + resetSearchLoopGuardStateRef = resetSearchLoopGuardState; registerSearchTool(pi); registerFetchPageTool(pi); registerLLMContextTool(pi); @@ -36,13 +42,23 @@ export default function (pi: ExtensionAPI) { registerNativeSearchHooks(pi); pi.on("session_start", async (_event, ctx) => { + const resetLoopGuardState = () => { + resetSearchLoopGuardStateRef?.(); + }; + if (ctx.hasUI) { - void registerSearchTools(pi).catch((error) => { - ctx.ui.notify(`search-the-web failed to load: ${error instanceof Error ? error.message : String(error)}`, "warning"); - }); + resetLoopGuardState(); + void registerSearchTools(pi) + .then(() => { + resetLoopGuardState(); + }) + .catch((error) => { + ctx.ui.notify(`search-the-web failed to load: ${error instanceof Error ? error.message : String(error)}`, "warning"); + }); return; } await registerSearchTools(pi); + resetLoopGuardState(); }); } diff --git a/src/resources/extensions/search-the-web/tool-search.ts b/src/resources/extensions/search-the-web/tool-search.ts index ba39b1332..54dab89b0 100644 --- a/src/resources/extensions/search-the-web/tool-search.ts +++ b/src/resources/extensions/search-the-web/tool-search.ts @@ -110,6 +110,12 @@ const MAX_CONSECUTIVE_DUPES = 3; let lastSearchKey = ""; let consecutiveDupeCount = 0; +/** Reset session-scoped duplicate-search guard state. */ +export function resetSearchLoopGuardState(): void { + lastSearchKey = ""; + consecutiveDupeCount = 0; +} + // Summarizer responses: max 50 entries, 15-minute TTL const summarizerCache = new LRUTTLCache({ max: 50, ttlMs: 900_000 }); @@ -383,16 +389,18 @@ export function registerSearchTool(pi: ExtensionAPI) { // ------------------------------------------------------------------ const cacheKey = normalizeQuery(effectiveQuery) + `|f:${freshness || ""}|s:${wantSummary}|p:${provider}`; - // ── Consecutive duplicate search guard (#949) ────────────────────── + // ── Consecutive duplicate search guard (#949, #1671) ───────────────── // If the LLM keeps calling the same search query, break the loop // with an explicit warning instead of returning the same results. + // After the threshold is hit, do NOT reset the state — this keeps the + // guard armed so every subsequent duplicate immediately re-triggers it, + // preventing the "sawtooth" pattern where resetting allowed infinite loops + // with brief interruptions every MAX_CONSECUTIVE_DUPES+1 calls. if (cacheKey === lastSearchKey) { consecutiveDupeCount++; if (consecutiveDupeCount >= MAX_CONSECUTIVE_DUPES) { - consecutiveDupeCount = 0; - lastSearchKey = ""; return { - content: [{ type: "text" as const, text: `⚠️ Search loop detected: the query "${params.query}" has been searched ${MAX_CONSECUTIVE_DUPES + 1} times consecutively with identical results. The information you need is already in the previous search results above. Stop searching and use those results to proceed with your task.` }], + content: [{ type: "text" as const, text: `⚠️ Search loop detected: the query "${params.query}" has been searched ${consecutiveDupeCount + 1} times consecutively with identical results. The information you need is already in the previous search results above. Stop searching and use those results to proceed with your task.` }], isError: true, details: { errorKind: "search_loop", error: "Consecutive duplicate search detected" } satisfies Partial, }; diff --git a/src/tests/search-loop-guard.test.ts b/src/tests/search-loop-guard.test.ts new file mode 100644 index 000000000..266b5155a --- /dev/null +++ b/src/tests/search-loop-guard.test.ts @@ -0,0 +1,244 @@ +/** + * Regression tests for the consecutive duplicate search loop guard. + * + * Covers: + * - Guard fires after MAX_CONSECUTIVE_DUPES identical calls (#949) + * - Guard stays armed after firing — subsequent duplicates immediately + * re-trigger the error (#1671: the original fix reset state on trigger, + * allowing the loop to restart) + * - Guard resets cleanly when a different query is issued + */ + +import test from "node:test"; +import assert from "node:assert/strict"; +import { registerSearchTool } from "../resources/extensions/search-the-web/tool-search.ts"; +import searchExtension from "../resources/extensions/search-the-web/index.ts"; + +// ============================================================================= +// Mock helpers +// ============================================================================= + +/** Minimal Brave search API response fixture. */ +function makeBraveResponse() { + return { + query: { original: "test query", more_results_available: false }, + web: { + results: [ + { + title: "Result One", + url: "https://example.com/one", + description: "First result description.", + }, + ], + }, + }; +} + +/** Install a mock global fetch that always returns the given body. */ +function mockFetch(body: unknown, status = 200) { + const original = global.fetch; + (global as any).fetch = async () => ({ + ok: status === 200, + status, + headers: { get: () => null }, + json: async () => body, + text: async () => JSON.stringify(body), + }); + return () => { + global.fetch = original; + }; +} + +/** Create a minimal mock PI that captures the registered search tool. */ +function createMockPI() { + const handlers: Array<{ event: string; handler: (...args: any[]) => unknown }> = []; + const toolsByName = new Map(); + let registeredTool: any = null; + + const pi = { + on(event: string, handler: (...args: any[]) => unknown) { + handlers.push({ event, handler }); + }, + registerCommand(_name: string, _command: unknown) {}, + registerTool(tool: any) { + if (typeof tool?.name === "string") { + toolsByName.set(tool.name, tool); + } + registeredTool = tool; + }, + async fire(event: string, eventData: unknown, ctx: unknown) { + for (const h of handlers) { + if (h.event === event) await h.handler(eventData, ctx); + } + }, + getRegisteredTool(name = "search-the-web") { + return toolsByName.get(name) ?? registeredTool; + }, + writeTempFile: async (_content: string, _opts?: unknown) => "/tmp/search-out.txt", + }; + + return pi; +} + +/** Call the search tool execute function with the given query. */ +async function callSearch( + execute: (...args: any[]) => Promise, + query: string, + callId = "call-1" +) { + const mockCtx = { ui: { notify() {} } }; + return execute(callId, { query }, new AbortController().signal, () => {}, mockCtx); +} + +// ============================================================================= +// Tests +// ============================================================================= + +/** + * Each test file gets its own module registry, so the module-level loop guard + * state (lastSearchKey, consecutiveDupeCount) starts fresh here. + */ + +test("search loop guard fires after MAX_CONSECUTIVE_DUPES duplicates", async () => { + process.env.BRAVE_API_KEY = "test-key-loop-guard"; + const restoreFetch = mockFetch(makeBraveResponse()); + + try { + const pi = createMockPI(); + registerSearchTool(pi as any); + const tool = pi.getRegisteredTool(); + assert.ok(tool, "search tool should be registered"); + + const execute = tool.execute.bind(tool); + + // Calls 1–3: below threshold, should return search results (not an error) + for (let i = 1; i <= 3; i++) { + const result = await callSearch(execute, "loop test query", `call-${i}`); + assert.notEqual(result.isError, true, `call ${i} should not trigger loop guard`); + } + + // Call 4: hits the threshold — guard fires + const result4 = await callSearch(execute, "loop test query", "call-4"); + assert.equal(result4.isError, true, "call 4 should trigger the loop guard"); + assert.equal(result4.details?.errorKind, "search_loop"); + assert.ok( + result4.content[0].text.includes("Search loop detected"), + "error message should mention search loop" + ); + } finally { + restoreFetch(); + delete process.env.BRAVE_API_KEY; + } +}); + +test("search loop guard resets at session_start boundary", async () => { + process.env.BRAVE_API_KEY = "test-key-loop-guard-session"; + const restoreFetch = mockFetch(makeBraveResponse()); + const query = "session boundary query"; + + try { + const pi = createMockPI(); + const mockCtx = { + hasUI: false, + ui: { notify() {} }, + }; + searchExtension(pi as any); + await pi.fire("session_start", {}, mockCtx); + + const tool = pi.getRegisteredTool(); + assert.ok(tool, "search tool should be registered"); + const execute = tool.execute.bind(tool); + + // Trigger guard in session 1 + for (let i = 1; i <= 4; i++) { + await callSearch(execute, query, `s1-call-${i}`); + } + const guardResult = await callSearch(execute, query, "s1-call-5"); + assert.equal(guardResult.isError, true, "session 1 should be guarded"); + assert.equal(guardResult.details?.errorKind, "search_loop"); + + // New session should clear guard state + await pi.fire("session_start", {}, mockCtx); + const firstCallSession2 = await callSearch(execute, query, "s2-call-1"); + assert.notEqual( + firstCallSession2.isError, + true, + "first identical query in a new session should not be blocked by prior session state", + ); + } finally { + restoreFetch(); + delete process.env.BRAVE_API_KEY; + } +}); + +test("search loop guard stays armed after firing — subsequent duplicates immediately re-trigger (#1671)", async () => { + process.env.BRAVE_API_KEY = "test-key-loop-guard-2"; + const restoreFetch = mockFetch(makeBraveResponse()); + + // Use a unique query so module-level state from previous test doesn't interfere + const query = "persistent loop query"; + + try { + const pi = createMockPI(); + registerSearchTool(pi as any); + const tool = pi.getRegisteredTool(); + const execute = tool.execute.bind(tool); + + // Exhaust the initial window (calls 1–3 succeed, call 4 fires guard) + for (let i = 1; i <= 3; i++) { + await callSearch(execute, query, `call-${i}`); + } + const guardFirst = await callSearch(execute, query, "call-4"); + assert.equal(guardFirst.isError, true, "call 4 should trigger the loop guard"); + + // Key regression test: call 5 (and beyond) must ALSO trigger the guard. + // The original bug reset state on trigger, so call 5 was treated as a fresh + // first search and the loop restarted. + const guardSecond = await callSearch(execute, query, "call-5"); + assert.equal( + guardSecond.isError, true, + "call 5 should STILL trigger the loop guard (guard must stay armed after firing)" + ); + assert.equal(guardSecond.details?.errorKind, "search_loop"); + + // Call 6 as well — guard should keep firing + const guardThird = await callSearch(execute, query, "call-6"); + assert.equal( + guardThird.isError, true, + "call 6 should STILL trigger the loop guard" + ); + } finally { + restoreFetch(); + delete process.env.BRAVE_API_KEY; + } +}); + +test("search loop guard resets cleanly when a different query is issued", async () => { + process.env.BRAVE_API_KEY = "test-key-loop-guard-3"; + const restoreFetch = mockFetch(makeBraveResponse()); + + const queryA = "query alpha reset test"; + const queryB = "query beta reset test"; + + try { + const pi = createMockPI(); + registerSearchTool(pi as any); + const tool = pi.getRegisteredTool(); + const execute = tool.execute.bind(tool); + + // Trigger guard for queryA + for (let i = 1; i <= 4; i++) { + await callSearch(execute, queryA, `call-a-${i}`); + } + + // Issue a different query — should succeed (resets the duplicate counter) + const resultB = await callSearch(execute, queryB, "call-b-1"); + assert.notEqual( + resultB.isError, true, + "a different query after guard should not be treated as a loop" + ); + } finally { + restoreFetch(); + delete process.env.BRAVE_API_KEY; + } +});