diff --git a/AGENTS.md b/AGENTS.md index a87e18bc..09b73d03 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -179,7 +179,7 @@ The TUI is clickable. Ink has no mouse layer, so this is built in `src/tui/mouse | `src/prompt/` | Prompt builder, stable prefix, token budget. See [PROMPT.md](PROMPT.md) for full anatomy of the stable prefix and variable tail. | | `src/session/` | Session state + sqlite persistence | | `src/agent/` | Agent loop + step executor + parallel batch executor (`batch-executor.ts`) + resource-class taxonomy (`tool-resource-class.ts`) + no-progress loop detector | -| `src/tools/` | Tool registry + individual tools. OS tools: `shell.run` (direct-exec by default; routes to a `sh -c` subshell when `needsShellInterpretation` sees shell metacharacters `\| & ; > < $ \`` or a pre-joined command line in `cmd` with empty `args` — the common ENOENT trap where the model puts a whole command line in `cmd`; the guard still inspects a tokenised view of the full line so hardline/dangerous rules match), `fs.read` (w/ `offset`/`limit`/`lineNumbers`), `fs.write`, `fs.list`, `fs.glob`, `fs.locate_project` (fuzzy project-name → directory over bounded sources, see §"Project path resolution"), `fs.grep` (bundled ripgrep), `fs.edit` (atomic string replace), `fs.read_document` (PDF/DOCX/XLSX/RTF/ODT/PPTX/legacy .doc → plain text via pure-JS), `fs.archive.list` / `fs.archive.read_entry` / `fs.archive.extract` (zip/tar/tar.gz/gz via pure-JS; zip-slip + bomb guards), `fs.hash` (md5/sha1/sha256/sha512 streaming), `fs.diff` (unified diff, jsdiff), `fs.patch` (dry-run default, all-or-nothing apply), `fs.watch` (chokidar one-shot, timeout-capped), `git.status` / `git.log` / `git.diff` / `git.show` / `git.blame` / `git.branch` (read-only shell-out with structured parse), `proc.list` / `proc.kill` (ps/tasklist + approval), `http.request` (curl + host allowlist + `config.http.approvalMode`), `web.search` (configured provider; keyless DuckDuckGo by default, SearXNG/Exa/Brave selectable via `web.search.*`; Exa uses `EXA_API_KEY` when present), `web.fetch` (read a known URL as markdown/text), `clipboard.*`, `window.*`, `notify`. | +| `src/tools/` | Tool registry + individual tools. OS tools: `shell.run` (direct-exec by default; routes to a `sh -c` subshell when `needsShellInterpretation` sees shell metacharacters `\| & ; > < $ \`` or a pre-joined command line in `cmd` with empty `args` — the common ENOENT trap where the model puts a whole command line in `cmd`; the guard still inspects a tokenised view of the full line so hardline/dangerous rules match), `fs.read` (w/ `offset`/`limit`/`lineNumbers`), `fs.write`, `fs.list`, `fs.glob`, `fs.locate_project` (fuzzy project-name → directory over bounded sources, see §"Project path resolution"), `fs.grep` (bundled ripgrep), `fs.edit` (atomic string replace), `fs.read_document` (PDF/DOCX/XLSX/RTF/ODT/PPTX/legacy .doc → plain text via pure-JS), `fs.archive.list` / `fs.archive.read_entry` / `fs.archive.extract` (zip/tar/tar.gz/gz via pure-JS; zip-slip + bomb guards), `fs.hash` (md5/sha1/sha256/sha512 streaming), `fs.diff` (unified diff, jsdiff), `fs.patch` (dry-run default, all-or-nothing apply), `fs.watch` (chokidar one-shot, timeout-capped), `git.status` / `git.log` / `git.diff` / `git.show` / `git.blame` / `git.branch` (read-only shell-out with structured parse), `proc.list` / `proc.kill` (ps/tasklist + approval), `http.request` (curl + host allowlist + `config.http.approvalMode`), `web.search` (configured provider; keyless Exa with a DuckDuckGo fallback by default, SearXNG/Brave selectable via `web.search.*`; Exa/Brave use an env API key when present, see §"Web search reliability"), `web.fetch` (read a known URL as markdown/text), `clipboard.*`, `window.*`, `notify`. | | `src/compressor/` | Result compressor, log summariser | | `src/sandbox/` | git worktree + sandboxed command runner | | `src/approval/` | Approval gate and event wiring | @@ -207,6 +207,48 @@ The startup read is defensive about transient locks (#59). A failing read of an There is currently **no per-tool env filtering**. `runCommand` in [src/sandbox/command-runner.ts](src/sandbox/command-runner.ts) inherits the full agent `process.env`, so every spawned subprocess (`os.shell.run`, `runSkillScript`, the managed `llama-server`, future MCP servers) sees every variable loaded from `.env`. Tightening this — per-skill `env_vars` whitelist + safe-baseline filtering (`PATH`, `HOME`, `USER`, `LANG`, `TERM`, `XDG_*`) — is tracked as a separate effort and pinned by no tests yet. Do not assume isolation when designing new skills that handle highly sensitive secrets; document the shared-env reality in the skill's `SKILL.md` instead. +## Web search reliability + +`os.web.search` defaults to `web.search.provider = "exa"` with a +`["duckduckgo"]` fallback. Exa's MCP endpoint answers **keyless** when +`EXA_API_KEY` is unset, and that keyless tier returns HTTP 429 under sustained +agent load — a GAIA validation campaign logged 1341 `Exa returned HTTP 429` +errors, 44% of all tool failures in the run (#179). Two mechanisms keep that +from silently deciding answer quality: + +1. **Retry before falling through.** [transport/retry-after.ts](src/tools/os/web-search/transport/retry-after.ts) + owns the schedule; `searchHttp` retries a 429 against the **same** provider + (default 2 retries, 500 ms doubling) before returning it. Without this, one + transient 429 permanently downgraded a session to the weakest provider in + the chain, because the orchestrator advances on any throw. A server + `Retry-After` wins over the local schedule; both are clamped to + `MAX_RETRY_AFTER_MS` (10 s) so one hostile header cannot stall a turn. The + header rides the existing `curl -w` meta line via `%header{retry-after}` + (curl >= 7.83; older curl emits the literal format string, which is read as + absent). Retries are spent, not skipped, when the limit is real — the + fallback chain remains the backstop. +2. **Name the degradation.** [tool/warn-missing-search-key.ts](src/tools/os/web-search/tool/warn-missing-search-key.ts) + emits one stderr line at tool construction when the primary provider reads + an `apiKeyEnv` that resolves to nothing. The fallback chain works as + designed, so nothing hard-fails; the run just produces weaker groundings + than configured. Warning **once at construction** (not per search) is + deliberate: a long autonomous run would drown in a per-query warning. + +`cacheTtlMinutes` stays at 15. The cache is per-process, in-memory, capped at +256 entries, and keyed on the exact query string, so a longer TTL neither +survives the per-task restarts a campaign does nor catches the near-miss +rephrasings that actually burn quota — while it would serve staler results for +time-sensitive lookups. A restart-surviving cache is the real fix and is not +built. + +Pinned by [retry-after.test.ts](src/tools/os/web-search/transport/retry-after.test.ts), +[search-http.test.ts](src/tools/os/web-search/transport/search-http.test.ts) +(retry-then-succeed, `Retry-After` precedence, give-up-after-maxRetries, +non-429 untouched, old-curl tolerance), +[warn-missing-search-key.test.ts](src/tools/os/web-search/tool/warn-missing-search-key.test.ts), +and [web-search-tool.test.ts](src/tools/os/web-search/tool/web-search-tool.test.ts) +("warns once at construction, not once per search"). + ## Build & test ```bash diff --git a/src/tools/os/web-search/tool/index.ts b/src/tools/os/web-search/tool/index.ts index d97e7b43..6c61e756 100644 --- a/src/tools/os/web-search/tool/index.ts +++ b/src/tools/os/web-search/tool/index.ts @@ -1,2 +1,4 @@ export { buildOsWebSearchTool } from "./web-search-tool.js"; export type { OsWebSearchOptions } from "./web-search-tool.js"; +export { checkMissingSearchKey } from "./warn-missing-search-key.js"; +export type { MissingSearchKeyWarning } from "./warn-missing-search-key.js"; diff --git a/src/tools/os/web-search/tool/warn-missing-search-key.test.ts b/src/tools/os/web-search/tool/warn-missing-search-key.test.ts new file mode 100644 index 00000000..fe270f62 --- /dev/null +++ b/src/tools/os/web-search/tool/warn-missing-search-key.test.ts @@ -0,0 +1,96 @@ +import { describe, expect, it } from "vitest"; + +import type { AtomicAgentConfig } from "../../../../config/index.js"; +import { checkMissingSearchKey } from "./warn-missing-search-key.js"; + +function makeConfig( + overrides: Partial = {}, +): Pick { + return { + web: { + search: { + enabled: true, + provider: "exa", + maxResults: 8, + timeoutMs: 15_000, + cacheTtlMinutes: 15, + fallback: ["duckduckgo"], + searxng: { instanceUrl: null }, + exa: { + endpoint: "https://mcp.exa.ai/mcp", + apiEndpoint: "https://api.exa.ai/search", + apiKeyEnv: "EXA_API_KEY", + }, + brave: { apiKeyEnv: "BRAVE_SEARCH_API_KEY" }, + ...overrides, + }, + }, + } as Pick; +} + +describe("checkMissingSearchKey", () => { + it("warns on the shipped default: exa primary with no EXA_API_KEY", () => { + const warning = checkMissingSearchKey({ config: makeConfig(), env: {} }); + + expect(warning).not.toBeNull(); + expect(warning!.provider).toBe("exa"); + expect(warning!.apiKeyEnv).toBe("EXA_API_KEY"); + // The message must name the silent consequence, not just the missing key. + expect(warning!.message).toContain("EXA_API_KEY"); + expect(warning!.message).toContain("duckduckgo"); + expect(warning!.message).toContain("429"); + }); + + it("stays silent when the key is present", () => { + expect( + checkMissingSearchKey({ config: makeConfig(), env: { EXA_API_KEY: "k" } }), + ).toBeNull(); + }); + + it("treats a whitespace-only key as missing", () => { + expect( + checkMissingSearchKey({ config: makeConfig(), env: { EXA_API_KEY: " " } }), + ).not.toBeNull(); + }); + + it("stays silent for keyless-by-design providers", () => { + for (const provider of ["duckduckgo", "searxng"] as const) { + expect( + checkMissingSearchKey({ config: makeConfig({ provider }), env: {} }), + ).toBeNull(); + } + }); + + it("warns for a brave primary against its own env var", () => { + const warning = checkMissingSearchKey({ + config: makeConfig({ provider: "brave" }), + env: {}, + }); + + expect(warning!.apiKeyEnv).toBe("BRAVE_SEARCH_API_KEY"); + }); + + it("stays silent when search is disabled outright", () => { + expect( + checkMissingSearchKey({ config: makeConfig({ enabled: false }), env: {} }), + ).toBeNull(); + }); + + it("says so when no fallback is configured", () => { + const warning = checkMissingSearchKey({ + config: makeConfig({ fallback: [] }), + env: {}, + }); + + expect(warning!.message).toContain("no fallback configured"); + }); + + it("dedupes the primary out of the reported fallback chain", () => { + const warning = checkMissingSearchKey({ + config: makeConfig({ fallback: ["exa", "duckduckgo"] }), + env: {}, + }); + + expect(warning!.fallback).toEqual(["duckduckgo"]); + }); +}); diff --git a/src/tools/os/web-search/tool/warn-missing-search-key.ts b/src/tools/os/web-search/tool/warn-missing-search-key.ts new file mode 100644 index 00000000..5fd56bf5 --- /dev/null +++ b/src/tools/os/web-search/tool/warn-missing-search-key.ts @@ -0,0 +1,72 @@ +import type { AtomicAgentConfig } from "../../../../config/index.js"; +import type { WebSearchProviderName } from "../web-search-provider.js"; + +/** + * Startup diagnostic for a keyless primary search provider. + * + * `web.search.provider` defaults to `exa` with a `duckduckgo` fallback, and + * Exa's keyless endpoint answers HTTP 429 under sustained agent load. The + * fallback chain then works exactly as designed, so nothing hard-fails — the + * run just quietly produces weaker groundings than the operator configured. + * That silent degradation is the failure mode this warning exists to break: + * it neither works well nor tells you why (#179). + */ + +/** Providers whose configured `apiKeyEnv` materially changes their quota. */ +const KEYED_PROVIDERS = new Set(["exa", "brave"]); + +export interface MissingSearchKeyWarning { + provider: WebSearchProviderName; + apiKeyEnv: string; + /** Providers that will actually serve traffic once the primary is limited. */ + fallback: WebSearchProviderName[]; + message: string; +} + +/** + * Returns a warning when the configured primary provider reads an API key + * from the environment and that variable resolves to nothing. Returns `null` + * for a keyed primary, a keyless-by-design primary (`duckduckgo`, `searxng`), + * or when search is disabled outright. + */ +export function checkMissingSearchKey(input: { + config: Pick; + env: NodeJS.ProcessEnv; +}): MissingSearchKeyWarning | null { + const search = input.config.web.search; + if (!search.enabled) return null; + + const provider = search.provider; + if (!KEYED_PROVIDERS.has(provider)) return null; + + const apiKeyEnv = + provider === "exa" ? search.exa.apiKeyEnv : search.brave.apiKeyEnv; + const key = input.env[apiKeyEnv]?.trim(); + if (typeof key === "string" && key.length > 0) return null; + + // Dedupe the primary out of the chain the same way the orchestrator does. + const fallback = search.fallback.filter((name) => name !== provider); + + return { + provider, + apiKeyEnv, + fallback, + message: buildMessage(provider, apiKeyEnv, fallback), + }; +} + +function buildMessage( + provider: WebSearchProviderName, + apiKeyEnv: string, + fallback: WebSearchProviderName[], +): string { + const consequence = + fallback.length > 0 + ? `expect HTTP 429 and silent degradation to ${fallback.join(", ")}` + : "expect HTTP 429 with no fallback configured"; + return ( + `web.search: provider "${provider}" is configured but ${apiKeyEnv} is not set; ` + + `running on the keyless tier — ${consequence}. ` + + `Set ${apiKeyEnv} for search-heavy autonomous work.` + ); +} diff --git a/src/tools/os/web-search/tool/web-search-tool.test.ts b/src/tools/os/web-search/tool/web-search-tool.test.ts index ccc992e2..6e710622 100644 --- a/src/tools/os/web-search/tool/web-search-tool.test.ts +++ b/src/tools/os/web-search/tool/web-search-tool.test.ts @@ -190,3 +190,42 @@ describe("os.web.search", () => { expect(result.details.provider).toBe("duckduckgo"); }); }); + +describe("buildOsWebSearchTool keyless-provider warning", () => { + it("warns once at construction, not once per search", async () => { + // Per-search warnings would flood a long autonomous run; the operator + // needs exactly one line telling them search is degraded (#179). + const warnings: string[] = []; + // Fail every curl immediately: this test is about warning cardinality, + // and a real network round-trip would make it slow and flaky. + const failingRunCommand = (async () => { + throw new Error("network disabled in test"); + }) as unknown as typeof RunCommandType; + const tool = buildOsWebSearchTool({ + config: makeConfig({ provider: "exa", fallback: ["duckduckgo"] }), + env: {}, + warn: (message) => warnings.push(message), + runCommand: failingRunCommand, + lookup: publicLookup, + }); + + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain("EXA_API_KEY"); + + await tool.run({ query: "a" }, makeCtx()).catch(() => undefined); + await tool.run({ query: "b" }, makeCtx()).catch(() => undefined); + + expect(warnings).toHaveLength(1); + }); + + it("stays silent when the provider key is set", () => { + const warnings: string[] = []; + buildOsWebSearchTool({ + config: makeConfig({ provider: "exa" }), + env: { EXA_API_KEY: "k" }, + warn: (message) => warnings.push(message), + }); + + expect(warnings).toEqual([]); + }); +}); diff --git a/src/tools/os/web-search/tool/web-search-tool.ts b/src/tools/os/web-search/tool/web-search-tool.ts index 4aa76469..2ca1a526 100644 --- a/src/tools/os/web-search/tool/web-search-tool.ts +++ b/src/tools/os/web-search/tool/web-search-tool.ts @@ -8,6 +8,7 @@ import type { HostLookup } from "../../web-fetch-ssrf-guard.js"; import { runWebSearchWithFallback } from "../providers/index.js"; import { createSearchCache } from "../transport/search-cache.js"; import type { WebSearchResult } from "../web-search-provider.js"; +import { checkMissingSearchKey } from "./warn-missing-search-key.js"; const TOOL_NAME = "os.web.search"; const MAX_RESULTS_CAP = 20; @@ -16,6 +17,10 @@ export interface OsWebSearchOptions { config: Pick; runCommand?: typeof defaultRunCommand; lookup?: HostLookup; + /** Process env source for the missing-key check; injectable for tests. */ + env?: NodeJS.ProcessEnv; + /** Warning sink; defaults to stderr. Injectable for tests. */ + warn?: (message: string) => void; } interface WebSearchArgs { @@ -29,6 +34,19 @@ export function buildOsWebSearchTool(options: OsWebSearchOptions): ToolDefinitio // HTTP round-trip — the primary defence against provider rate-limiting. const cfg0 = options.config.web.search; const cache = createSearchCache({ ttlMs: cfg0.cacheTtlMinutes * 60_000 }); + + // Emitted once at construction, not per search: a keyless primary provider + // degrades every subsequent query, and one line at startup is what turns + // that from invisible into diagnosable (#179). + const missingKey = checkMissingSearchKey({ + config: options.config, + env: options.env ?? process.env, + }); + if (missingKey) { + const warn = + options.warn ?? ((message: string) => process.stderr.write(`${message}\n`)); + warn(missingKey.message); + } return { name: TOOL_NAME, description: diff --git a/src/tools/os/web-search/transport/index.ts b/src/tools/os/web-search/transport/index.ts index a9a22a9d..910d2a70 100644 --- a/src/tools/os/web-search/transport/index.ts +++ b/src/tools/os/web-search/transport/index.ts @@ -7,6 +7,13 @@ export type { SearchHttpRequest, SearchHttpResponse, } from "./search-http.js"; +export { + computeRetryDelayMs, + DEFAULT_SEARCH_RETRY_POLICY, + MAX_RETRY_AFTER_MS, + parseRetryAfterMs, +} from "./retry-after.js"; +export type { SearchRetryPolicy } from "./retry-after.js"; export { buildSearchCacheKey, createSearchCache, diff --git a/src/tools/os/web-search/transport/retry-after.test.ts b/src/tools/os/web-search/transport/retry-after.test.ts new file mode 100644 index 00000000..13a72f21 --- /dev/null +++ b/src/tools/os/web-search/transport/retry-after.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it } from "vitest"; + +import { + computeRetryDelayMs, + DEFAULT_SEARCH_RETRY_POLICY, + MAX_RETRY_AFTER_MS, + parseRetryAfterMs, +} from "./retry-after.js"; + +const NOW = Date.parse("2026-08-20T12:00:00Z"); + +describe("parseRetryAfterMs", () => { + it("reads the delta-seconds form", () => { + expect(parseRetryAfterMs("2", NOW)).toBe(2000); + }); + + it("reads the HTTP-date form relative to now", () => { + expect(parseRetryAfterMs("Thu, 20 Aug 2026 12:00:03 GMT", NOW)).toBe(3000); + }); + + it("clamps a hostile far-future value to the ceiling", () => { + // One bad header must not stall an agent turn for minutes. + expect(parseRetryAfterMs("3600", NOW)).toBe(MAX_RETRY_AFTER_MS); + }); + + it("treats an already-elapsed date as no wait", () => { + expect(parseRetryAfterMs("Thu, 20 Aug 2026 11:59:00 GMT", NOW)).toBe(0); + }); + + it("returns null when absent or unparseable so backoff takes over", () => { + expect(parseRetryAfterMs(undefined, NOW)).toBeNull(); + expect(parseRetryAfterMs(null, NOW)).toBeNull(); + expect(parseRetryAfterMs("", NOW)).toBeNull(); + expect(parseRetryAfterMs("soon", NOW)).toBeNull(); + // Must not accept a partially-numeric value as 10 seconds. + expect(parseRetryAfterMs("10abc", NOW)).toBeNull(); + }); +}); + +describe("computeRetryDelayMs", () => { + it("doubles the base delay per attempt when the server gave no header", () => { + const policy = DEFAULT_SEARCH_RETRY_POLICY; + expect(computeRetryDelayMs({ attempt: 1, policy, retryAfterMs: null })).toBe(500); + expect(computeRetryDelayMs({ attempt: 2, policy, retryAfterMs: null })).toBe(1000); + expect(computeRetryDelayMs({ attempt: 3, policy, retryAfterMs: null })).toBe(2000); + }); + + it("prefers the server's Retry-After over its own schedule", () => { + expect( + computeRetryDelayMs({ + attempt: 1, + policy: DEFAULT_SEARCH_RETRY_POLICY, + retryAfterMs: 4000, + }), + ).toBe(4000); + }); + + it("clamps its own exponential schedule to the ceiling", () => { + expect( + computeRetryDelayMs({ + attempt: 20, + policy: DEFAULT_SEARCH_RETRY_POLICY, + retryAfterMs: null, + }), + ).toBe(MAX_RETRY_AFTER_MS); + }); +}); diff --git a/src/tools/os/web-search/transport/retry-after.ts b/src/tools/os/web-search/transport/retry-after.ts new file mode 100644 index 00000000..0e69f8c5 --- /dev/null +++ b/src/tools/os/web-search/transport/retry-after.ts @@ -0,0 +1,71 @@ +/** + * Retry scheduling for rate-limited (HTTP 429) search responses. + * + * The keyless tiers every default provider rides on (Exa's MCP endpoint, + * DuckDuckGo's HTML endpoint) answer 429 under sustained agent load. Before + * this module a single 429 threw straight out of the provider and the + * orchestrator advanced the chain, which permanently downgraded a + * search-heavy session to the weakest provider on the first transient limit. + * Retrying the primary a couple of times first keeps the configured provider + * in play; the fallback chain remains the backstop when the limit is real. + */ + +/** Ceiling on a server-advertised `Retry-After`, so one hostile header cannot stall a turn. */ +export const MAX_RETRY_AFTER_MS = 10_000; + +export interface SearchRetryPolicy { + /** Extra attempts after the initial request. `0` disables retrying. */ + maxRetries: number; + /** Delay for the first retry; each subsequent retry doubles it. */ + baseDelayMs: number; +} + +export const DEFAULT_SEARCH_RETRY_POLICY: SearchRetryPolicy = { + maxRetries: 2, + baseDelayMs: 500, +}; + +/** + * Parse a `Retry-After` header value. Supports both documented forms: + * delta-seconds (`120`) and an HTTP-date (`Wed, 21 Oct 2026 07:28:00 GMT`). + * Returns `null` when absent or unparseable so the caller falls back to its + * own backoff schedule. The result is clamped to `[0, MAX_RETRY_AFTER_MS]`. + */ +export function parseRetryAfterMs( + headerValue: string | null | undefined, + now: number, +): number | null { + if (typeof headerValue !== "string") return null; + const raw = headerValue.trim(); + if (raw.length === 0) return null; + + // delta-seconds. Guard against `Number.parseInt` accepting "10abc". + if (/^\d+$/.test(raw)) { + const seconds = Number.parseInt(raw, 10); + if (!Number.isFinite(seconds)) return null; + return clampDelay(seconds * 1000); + } + + const at = Date.parse(raw); + if (!Number.isFinite(at)) return null; + return clampDelay(at - now); +} + +/** + * Delay before retry number `attempt` (1-based): the server's `Retry-After` + * when it gave one, otherwise exponential backoff from `baseDelayMs`. + */ +export function computeRetryDelayMs(input: { + attempt: number; + policy: SearchRetryPolicy; + retryAfterMs: number | null; +}): number { + if (input.retryAfterMs !== null) return clampDelay(input.retryAfterMs); + const exponent = Math.max(0, input.attempt - 1); + return clampDelay(input.policy.baseDelayMs * 2 ** exponent); +} + +function clampDelay(ms: number): number { + if (!Number.isFinite(ms) || ms <= 0) return 0; + return Math.min(ms, MAX_RETRY_AFTER_MS); +} diff --git a/src/tools/os/web-search/transport/search-http.test.ts b/src/tools/os/web-search/transport/search-http.test.ts index fdeee981..06d3a930 100644 --- a/src/tools/os/web-search/transport/search-http.test.ts +++ b/src/tools/os/web-search/transport/search-http.test.ts @@ -17,6 +17,34 @@ function stubCurlStdout(body: string): string { return `${body}\n__ATOMIC_WEB_SEARCH_META__200|text/html||${body.length}`; } +/** Curl envelope for an arbitrary status, with an optional Retry-After header. */ +function stubCurlStatus(status: number, retryAfter = ""): string { + const header = `__ATOMIC_WEB_SEARCH_HEADERS__${retryAfter}`; + return `body\n__ATOMIC_WEB_SEARCH_META__${status}|text/html||4|${header}`; +} + +/** Replays the given stdout envelopes in order, one per curl invocation. */ +function scriptedRunCommand( + stdouts: string[], + calls: string[][], +): typeof RunCommandType { + return (async (_command: string, args: string[]) => { + const stdout = stdouts[calls.length] ?? stdouts.at(-1)!; + calls.push(args); + return { + command: "curl", + args, + exitCode: 0, + signal: null, + stdout, + stderr: "", + durationMs: 1, + timedOut: false, + truncated: false, + }; + }) as unknown as typeof RunCommandType; +} + function capturingRunCommand(calls: string[][]): typeof RunCommandType { return (async (_command: string, args: string[]) => { calls.push(args); @@ -56,3 +84,134 @@ describe("searchHttp curl argv", () => { expect(calls[0]![calls[0]!.length - 1]).toContain("[Dd]ewey"); }); }); + +describe("searchHttp 429 retry", () => { + /** Records requested backoff instead of spending real wall-clock. */ + function fakeSleep(slept: number[]) { + return async (ms: number) => { + slept.push(ms); + }; + } + + it("retries the SAME provider on 429 and returns the eventual success", async () => { + // The regression this guards: one transient 429 used to throw straight out + // of the provider, permanently downgrading the session to a weaker one. + const calls: string[][] = []; + const slept: number[] = []; + const response = await searchHttp({ + url: "https://search.example/q", + timeoutMs: 1000, + cwd: "/tmp", + signal: new AbortController().signal, + runCommand: scriptedRunCommand( + [stubCurlStatus(429), stubCurlStdout("ok")], + calls, + ), + lookup: publicLookup, + sleep: fakeSleep(slept), + }); + + expect(calls).toHaveLength(2); + expect(response.status).toBe(200); + expect(slept).toEqual([500]); + }); + + it("honours the server's Retry-After over its own backoff schedule", async () => { + const calls: string[][] = []; + const slept: number[] = []; + await searchHttp({ + url: "https://search.example/q", + timeoutMs: 1000, + cwd: "/tmp", + signal: new AbortController().signal, + runCommand: scriptedRunCommand( + [stubCurlStatus(429, "3"), stubCurlStdout("ok")], + calls, + ), + lookup: publicLookup, + sleep: fakeSleep(slept), + now: () => Date.parse("2026-08-20T12:00:00Z"), + }); + + expect(slept).toEqual([3000]); + }); + + it("gives up after maxRetries and returns the 429 so the chain advances", async () => { + // Retrying must not mask a real, standing rate limit: the fallback chain + // is still the backstop once the retries are spent. + const calls: string[][] = []; + const slept: number[] = []; + const response = await searchHttp({ + url: "https://search.example/q", + timeoutMs: 1000, + cwd: "/tmp", + signal: new AbortController().signal, + runCommand: scriptedRunCommand([stubCurlStatus(429)], calls), + lookup: publicLookup, + sleep: fakeSleep(slept), + }); + + expect(response.status).toBe(429); + expect(calls).toHaveLength(3); // initial + 2 retries + expect(slept).toEqual([500, 1000]); + }); + + it("does not retry a non-429 failure", async () => { + const calls: string[][] = []; + const slept: number[] = []; + const response = await searchHttp({ + url: "https://search.example/q", + timeoutMs: 1000, + cwd: "/tmp", + signal: new AbortController().signal, + runCommand: scriptedRunCommand([stubCurlStatus(503)], calls), + lookup: publicLookup, + sleep: fakeSleep(slept), + }); + + expect(response.status).toBe(503); + expect(calls).toHaveLength(1); + expect(slept).toEqual([]); + }); + + it("can be disabled with maxRetries: 0", async () => { + const calls: string[][] = []; + await searchHttp({ + url: "https://search.example/q", + timeoutMs: 1000, + cwd: "/tmp", + signal: new AbortController().signal, + runCommand: scriptedRunCommand([stubCurlStatus(429)], calls), + lookup: publicLookup, + retryPolicy: { maxRetries: 0, baseDelayMs: 500 }, + sleep: async () => {}, + }); + + expect(calls).toHaveLength(1); + }); + + it("tolerates a curl too old for %header{} and falls back to backoff", async () => { + // curl < 7.83 emits the literal format string; it must not be read as a + // Retry-After value. + const calls: string[][] = []; + const slept: number[] = []; + await searchHttp({ + url: "https://search.example/q", + timeoutMs: 1000, + cwd: "/tmp", + signal: new AbortController().signal, + runCommand: scriptedRunCommand( + [ + "body\n__ATOMIC_WEB_SEARCH_META__429|text/html||4|" + + "__ATOMIC_WEB_SEARCH_HEADERS__%header{retry-after}", + stubCurlStdout("ok"), + ], + calls, + ), + lookup: publicLookup, + sleep: fakeSleep(slept), + }); + + expect(slept).toEqual([500]); + }); +}); diff --git a/src/tools/os/web-search/transport/search-http.ts b/src/tools/os/web-search/transport/search-http.ts index 92ff5040..45258de1 100644 --- a/src/tools/os/web-search/transport/search-http.ts +++ b/src/tools/os/web-search/transport/search-http.ts @@ -8,8 +8,15 @@ import { type HostLookup, } from "../../web-fetch-ssrf-guard.js"; import { CurlUnavailableError, isCurlMissingError } from "../../ensure-curl.js"; +import { + computeRetryDelayMs, + DEFAULT_SEARCH_RETRY_POLICY, + parseRetryAfterMs, + type SearchRetryPolicy, +} from "./retry-after.js"; const CURL_META_MARKER = "__ATOMIC_WEB_SEARCH_META__"; +const CURL_HEADER_MARKER = "__ATOMIC_WEB_SEARCH_HEADERS__"; const DEFAULT_MAX_RESPONSE_BYTES = 1_000_000; const MAX_REDIRECTS = 3; const USER_AGENT = @@ -31,6 +38,12 @@ export interface SearchHttpRequest { maxResponseBytes?: number; runCommand?: typeof defaultRunCommand; lookup?: HostLookup; + /** Overrides the 429 retry schedule; `maxRetries: 0` disables retrying. */ + retryPolicy?: SearchRetryPolicy; + /** Injectable sleep so tests do not spend real wall-clock in backoff. */ + sleep?: (ms: number, signal: AbortSignal) => Promise; + /** Injectable clock for deterministic `Retry-After` HTTP-date parsing. */ + now?: () => number; } export interface SearchHttpResponse { @@ -46,6 +59,7 @@ interface CurlResponse { status: number; contentType: string; redirectUrl: string; + retryAfter: string; body: string; truncated: boolean; } @@ -53,6 +67,37 @@ interface CurlResponse { export async function searchHttp( request: SearchHttpRequest, ): Promise { + const policy = request.retryPolicy ?? DEFAULT_SEARCH_RETRY_POLICY; + const sleep = request.sleep ?? defaultSleep; + const now = request.now ?? Date.now; + + // Attempt 0 is the initial request; 1..maxRetries are 429 retries. A 429 is + // retried against the SAME provider before the orchestrator is allowed to + // advance the chain, so a transient limit cannot permanently downgrade the + // session to a weaker provider. + for (let attempt = 0; ; attempt++) { + const { retryAfter, ...response } = await sendOnce(request); + if (response.status !== 429 || attempt >= policy.maxRetries) { + return response; + } + const delayMs = computeRetryDelayMs({ + attempt: attempt + 1, + policy, + retryAfterMs: parseRetryAfterMs(retryAfter, now()), + }); + await sleep(delayMs, request.signal); + } +} + +/** Internal shape: the public response plus the header the retry loop reads. */ +interface SearchHttpAttempt extends SearchHttpResponse { + retryAfter: string; +} + +/** One full request/redirect walk. Retrying re-enters this from the top. */ +async function sendOnce( + request: SearchHttpRequest, +): Promise { const runCommand = request.runCommand ?? defaultRunCommand; const method = request.method ?? "GET"; let currentUrl = parseHttpUrl(request.url); @@ -106,10 +151,29 @@ export async function searchHttp( body: response.body, truncated: response.truncated, redirectChain: chain, + retryAfter: response.retryAfter, }; } } +/** + * Abort-aware sleep. A cancelled turn must not sit out the backoff: the + * pending timer is cleared and the wait resolves immediately so the caller + * observes the abort on its next checkpoint. + */ +function defaultSleep(ms: number, signal: AbortSignal): Promise { + if (ms <= 0 || signal.aborted) return Promise.resolve(); + return new Promise((resolve) => { + const timer = setTimeout(finish, ms); + function finish(): void { + clearTimeout(timer); + signal.removeEventListener("abort", finish); + resolve(); + } + signal.addEventListener("abort", finish, { once: true }); + }); +} + function buildCurlArgs(input: { url: URL; pinnedIp: string; @@ -147,7 +211,8 @@ function buildCurlArgs(input: { } args.push( "-w", - `\n${CURL_META_MARKER}%{http_code}|%{content_type}|%{redirect_url}|%{size_download}`, + `\n${CURL_META_MARKER}%{http_code}|%{content_type}|%{redirect_url}|` + + `%{size_download}|${CURL_HEADER_MARKER}%header{retry-after}`, "--", input.url.toString(), ); @@ -157,16 +222,32 @@ function buildCurlArgs(input: { export function parseCurlMeta(stdout: string): Omit { const markerIdx = stdout.lastIndexOf(CURL_META_MARKER); if (markerIdx === -1) { - return { status: 0, contentType: "", redirectUrl: "", body: stdout }; + return { + status: 0, + contentType: "", + redirectUrl: "", + retryAfter: "", + body: stdout, + }; } const body = stdout.slice(0, markerIdx).replace(/\n$/, ""); const meta = stdout.slice(markerIdx + CURL_META_MARKER.length).trim(); const [statusStr = "", contentType = "", redirectUrl = ""] = meta.split("|"); const status = Number.parseInt(statusStr, 10); + // Read the header block off the whole meta line rather than a fixed field: + // a `Retry-After` value may itself contain `|`, and the marker is the only + // reliable delimiter. `%header{}` is curl >= 7.83; older curl emits the + // literal format string, which must not be read as a value. + const headerIdx = meta.indexOf(CURL_HEADER_MARKER); + const retryAfter = + headerIdx === -1 + ? "" + : meta.slice(headerIdx + CURL_HEADER_MARKER.length).trim(); return { status: Number.isFinite(status) ? status : 0, contentType: contentType.trim(), redirectUrl: redirectUrl.trim(), + retryAfter: retryAfter.startsWith("%header{") ? "" : retryAfter, body, }; }