diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d6fd518a..3510068b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,29 @@ matching `## [X.Y.Z]` section (plus install instructions). Do not maintain parallel copies under `docs/` or `scripts/notes/`. At cut time: rename `## [Unreleased]` to `## [X.Y.Z] - YYYY-MM-DD`, then run the release script. +## [Unreleased] + +### Plugins + +- **Requested `run_shell` timeouts are no longer capped at 10 minutes.** The 15s + default when timeout is omitted is unchanged. `shell.maxTimeoutMs` still + clamps the command when set. + +- Capability evals accept `--concurrency ` (env `CORBITS_EVAL_CONCURRENCY`, + default 1); overlapping `httpFixture` cells isolate `EVAL_HTTP_URL` so + parallel web-bait runs do not share a process.env origin. + +### TUI + +- **Tool `run()` no longer has an implicit 11-minute wall-clock abort.** The + outer watchdog arms only when Settings set `tools.timeoutMs` / + `tools.maxTimeoutMs`, or when `run_shell` passes a positive `timeout` + (requested plus slack, so this layer cannot beat shell-guard). Unset + settings leave `task` and other tools unbounded; parent cancel, maxTurns, + and eval `--agent-timeout-ms` still bound the run. `tools.maxTimeoutMs` + still clamps non-shell tools when set and does not cap a longer requested + `run_shell`. + ## [0.2.99] - 2026-08-21 Skywalker is the primary orchestrator over a closed director fleet: product write tools stay off the primary, and you cannot spawn Skywalker as a task leaf. Workers are not done until they return the four-heading report. First-party action skills ship as slashes; eval runners require an explicit provider/model pair; the style skill no longer refuses non-git folders. diff --git a/evals/capability/README.md b/evals/capability/README.md index 678301eba..95ef6cd32 100644 --- a/evals/capability/README.md +++ b/evals/capability/README.md @@ -117,6 +117,9 @@ bun run eval:capability -- \ --matrix "xai:grok-4.5,openai:gpt-4.1" \ --out evals/capability/results/matrix.json +# Faster live matrix (independent cells; default is serial) +bun run eval:capability -- --provider --model --concurrency 4 + # Labeled variants bun run eval:capability -- --matrix "fast=xai:grok-4.5,strong=openai:gpt-4.1" @@ -165,6 +168,7 @@ Flags: | `--agent-timeout-ms ` | Wall-clock limit for `runExec` (default `600000`, env `CORBITS_EVAL_AGENT_TIMEOUT_MS`) | | `--verify-timeout-ms ` | Wall-clock limit for `verify.sh` (default `120000`, env `CORBITS_EVAL_VERIFY_TIMEOUT_MS`) | | `--repeats ` | Runs per case×variant cell (default `1`; gate runs use `5`, baseline freezes `3`). Results record every repeat plus per-cell aggregates | +| `--concurrency ` | Independent case×variant×repeat cells in parallel (default `1`, env `CORBITS_EVAL_CONCURRENCY`). Each cell still uses its own temp workdir. Use `--concurrency 4` (or similar) to run a live matrix faster | | `--dry-run` | Load cases × variants and print plan; no inference. Still requires `--provider`/`--model` or `--matrix` | ## Case format diff --git a/evals/capability/lib.test.ts b/evals/capability/lib.test.ts index 9f164cbf5..eade147de 100644 --- a/evals/capability/lib.test.ts +++ b/evals/capability/lib.test.ts @@ -20,6 +20,7 @@ import { baitReproduces, httpFixtureEnv, withEnv, + evalHttpEnvGet, detectProviderFallback, formatProviderFallback, resolveRequestedProviderModel, @@ -811,24 +812,29 @@ describe("withEnv / httpFixtureEnv", () => { test("makes the fixture origin visible to in-process code the way ssrf-guard reads it", async () => { const fixture = { url: "http://127.0.0.1:54321/", token: "tok" }; + expect(evalHttpEnvGet("EVAL_HTTP_URL")).toBeUndefined(); expect(process.env.EVAL_HTTP_URL).toBeUndefined(); let seenDuring: string | undefined; await withEnv(httpFixtureEnv(fixture), async () => { - seenDuring = process.env.EVAL_HTTP_URL; + seenDuring = evalHttpEnvGet("EVAL_HTTP_URL"); + expect(process.env.EVAL_HTTP_URL).toBeUndefined(); }); expect(seenDuring).toBe(fixture.url); + expect(evalHttpEnvGet("EVAL_HTTP_URL")).toBeUndefined(); expect(process.env.EVAL_HTTP_URL).toBeUndefined(); }); - test("restores prior value on throw", async () => { + test("overlay does not leak after throw and leaves process.env untouched", async () => { process.env.EVAL_HTTP_URL = "http://pre-existing/"; try { await expect( withEnv({ EVAL_HTTP_URL: "http://127.0.0.1:1/" }, async () => { + expect(evalHttpEnvGet("EVAL_HTTP_URL")).toBe("http://127.0.0.1:1/"); throw new Error("boom"); }), ).rejects.toThrow("boom"); expect(process.env.EVAL_HTTP_URL).toBe("http://pre-existing/"); + expect(evalHttpEnvGet("EVAL_HTTP_URL")).toBe("http://pre-existing/"); } finally { delete process.env.EVAL_HTTP_URL; } diff --git a/evals/capability/lib.ts b/evals/capability/lib.ts index f18765740..be7df8d70 100644 --- a/evals/capability/lib.ts +++ b/evals/capability/lib.ts @@ -5,6 +5,7 @@ import { readdir, readFile, stat } from "node:fs/promises"; import { join, resolve } from "node:path"; +import { runWithEvalHttpEnv, evalHttpEnvGet } from "../../src/tools/eval-http-env.js"; import { isNumericBehaviorMetric, parseBehaviorMetrics, @@ -422,6 +423,8 @@ export function makeResultKey(variantId: string, caseId: string): string { return `${variantId}::${caseId}`; } +export { evalHttpEnvGet, runWithEvalHttpEnv }; + /** * Env vars the eval-only SSRF fixture exception in src/tools/ssrf-guard.ts * checks against. Shared by the agent process (must see EVAL_HTTP_URL so @@ -433,23 +436,15 @@ export function httpFixtureEnv(fixture: { url: string; token: string }): Record< } /** - * Sets process.env vars for the duration of fn, restoring the prior values - * (or deleting the key if it was unset) afterward, even on throw. The agent - * runs in-process via runExec rather than as a spawned child, so fixture env - * needed by in-process code (e.g. the eval-only SSRF exception) must be - * applied to process.env directly instead of a child's env object. + * Isolates `vars` for the duration of `fn` via async context (ALS), even when + * sibling cells overlap under `--concurrency`. In-process readers (ssrf-guard) + * see this cell's values through evalHttpEnvGet; one cell finishing cannot + * delete a sibling's overlay. process.env is left alone so a restore cannot + * clobber a concurrent cell. verify.sh still receives an explicit env object + * at spawn (see scripts/eval-capability.ts). */ export async function withEnv(vars: Record, fn: () => Promise): Promise { - const prior = new Map(Object.keys(vars).map((k) => [k, process.env[k]])); - Object.assign(process.env, vars); - try { - return await fn(); - } finally { - for (const [k, v] of prior) { - if (v === undefined) delete process.env[k]; - else process.env[k] = v; - } - } + return runWithEvalHttpEnv(vars, fn); } /** diff --git a/scripts/eval-capability.test.ts b/scripts/eval-capability.test.ts index 3506e40ec..372b3fc06 100644 --- a/scripts/eval-capability.test.ts +++ b/scripts/eval-capability.test.ts @@ -1,15 +1,33 @@ -import { afterEach, describe, expect, test } from "bun:test"; +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { mkdir, mkdtemp, writeFile, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { execFile } from "node:child_process"; import { promisify } from "node:util"; -import { initEvalGitRepo, parseArgs } from "./eval-capability.ts"; +import { initEvalGitRepo, mapPool, parseArgs } from "./eval-capability.ts"; const execFileAsync = promisify(execFile); describe("parseArgs", () => { + const savedConcurrency = process.env.CORBITS_EVAL_CONCURRENCY; + + const restoreConcurrency = (): void => { + if (savedConcurrency === undefined) { + delete process.env.CORBITS_EVAL_CONCURRENCY; + } else { + process.env.CORBITS_EVAL_CONCURRENCY = savedConcurrency; + } + }; + + afterEach(() => { + restoreConcurrency(); + }); + + beforeEach(() => { + delete process.env.CORBITS_EVAL_CONCURRENCY; + }); + test("--help does not require provider or model", () => { const opts = parseArgs(["--help"]); expect(opts.help).toBe(true); @@ -61,6 +79,77 @@ describe("parseArgs", () => { expect(pair.provider).toBe("foo"); expect(pair.model).toBe("bar"); }); + + test("defaults concurrency to 1", () => { + delete process.env.CORBITS_EVAL_CONCURRENCY; + const opts = parseArgs(["--provider", "foo", "--model", "bar"]); + expect(opts.concurrency).toBe(1); + }); + + test("--concurrency 4 is accepted", () => { + delete process.env.CORBITS_EVAL_CONCURRENCY; + const opts = parseArgs(["--provider", "foo", "--model", "bar", "--concurrency", "4"]); + expect(opts.concurrency).toBe(4); + }); + + test("invalid --concurrency values throw", () => { + const pair = ["--provider", "foo", "--model", "bar"] as const; + expect(() => parseArgs([...pair, "--concurrency", "0"])).toThrow(/positive integer/); + expect(() => parseArgs([...pair, "--concurrency", "-1"])).toThrow(/positive integer/); + expect(() => parseArgs([...pair, "--concurrency", "1.5"])).toThrow(/positive integer/); + expect(() => parseArgs([...pair, "--concurrency", "foo"])).toThrow(/positive integer/); + }); + + test("CORBITS_EVAL_CONCURRENCY sets the default", () => { + process.env.CORBITS_EVAL_CONCURRENCY = "3"; + const opts = parseArgs(["--provider", "foo", "--model", "bar"]); + expect(opts.concurrency).toBe(3); + }); + + test("--concurrency overrides CORBITS_EVAL_CONCURRENCY", () => { + process.env.CORBITS_EVAL_CONCURRENCY = "8"; + const opts = parseArgs(["--provider", "foo", "--model", "bar", "--concurrency", "2"]); + expect(opts.concurrency).toBe(2); + }); + + test("invalid CORBITS_EVAL_CONCURRENCY throws", () => { + process.env.CORBITS_EVAL_CONCURRENCY = "0"; + expect(() => parseArgs(["--provider", "foo", "--model", "bar"])).toThrow( + /CORBITS_EVAL_CONCURRENCY must be a positive integer/, + ); + }); +}); + +describe("mapPool", () => { + test("N overlapping jobs with concurrency N finish in ~one job duration", async () => { + const jobMs = 80; + const n = 4; + const start = Date.now(); + const results = await mapPool([0, 1, 2, 3], n, async (item) => { + await new Promise((r) => setTimeout(r, jobMs)); + return item; + }); + const elapsed = Date.now() - start; + expect(results).toEqual([0, 1, 2, 3]); + expect(elapsed).toBeLessThan(jobMs * 2); + expect(elapsed).toBeGreaterThanOrEqual(jobMs - 20); + }); + + test("preserves input order when later items finish first", async () => { + const results = await mapPool([1, 2, 3], 3, async (item) => { + await new Promise((r) => setTimeout(r, (4 - item) * 30)); + return item; + }); + expect(results).toEqual([1, 2, 3]); + }); + + test("empty input returns an empty array", async () => { + expect(await mapPool([], 4, async (item) => item)).toEqual([]); + }); + + test("rejects non-positive concurrency", async () => { + await expect(mapPool([1], 0, async (item) => item)).rejects.toThrow(/positive integer/); + }); }); describe("initEvalGitRepo", () => { diff --git a/scripts/eval-capability.ts b/scripts/eval-capability.ts index 2844b14b1..663cc2dc5 100755 --- a/scripts/eval-capability.ts +++ b/scripts/eval-capability.ts @@ -68,6 +68,8 @@ type CliOptions = { verifyTimeoutMs: number; /** Runs per case×variant cell (gate runs use 5; freeze runs use 3). */ repeats: number; + /** Independent case×variant×repeat cells in parallel (default 1). */ + concurrency: number; dryRun: boolean; help: boolean; /** @@ -93,6 +95,7 @@ function printUsage(): void { --agent-timeout-ms Wall-clock limit for runExec (default 1200000) --verify-timeout-ms Wall-clock limit for verify.sh (default 120000) --repeats Runs per case×variant cell (default 1; gate runs use 5) + --concurrency Independent cells in parallel (default 1, env CORBITS_EVAL_CONCURRENCY) --dry-run List cases × variants only (still requires --provider/--model or --matrix) --allow-provider-fallback Allow resolved provider/model to differ from what was requested (default: hard-fail) @@ -100,11 +103,54 @@ function printUsage(): void { `); } +function parsePositiveInteger(raw: string, label: string): number { + const n = Number(raw); + if (!Number.isInteger(n) || n <= 0) { + throw new Error(`${label} must be a positive integer`); + } + return n; +} + +function defaultConcurrency(): number { + const raw = process.env.CORBITS_EVAL_CONCURRENCY; + if (raw === undefined || raw === "") return 1; + return parsePositiveInteger(raw, "CORBITS_EVAL_CONCURRENCY"); +} + +/** + * Run `mapper` over `items` with at most `concurrency` in flight. + * Results stay in input order even when later items finish first. + */ +export async function mapPool( + items: readonly T[], + concurrency: number, + mapper: (item: T, index: number) => Promise, +): Promise { + if (!Number.isInteger(concurrency) || concurrency <= 0) { + throw new Error("concurrency must be a positive integer"); + } + if (items.length === 0) return []; + const results: R[] = new Array(items.length); + let nextIndex = 0; + const worker = async (): Promise => { + while (true) { + const index = nextIndex; + nextIndex += 1; + if (index >= items.length) return; + results[index] = await mapper(items[index]!, index); + } + }; + const workerCount = Math.min(concurrency, items.length); + await Promise.all(Array.from({ length: workerCount }, () => worker())); + return results; +} + export function parseArgs(argv: readonly string[]): CliOptions { const opts: CliOptions = { caseSelector: "all", skipPermissions: true, repeats: 1, + concurrency: defaultConcurrency(), dryRun: false, help: false, allowProviderFallback: false, @@ -175,6 +221,9 @@ export function parseArgs(argv: readonly string[]): CliOptions { opts.repeats = n; break; } + case "--concurrency": + opts.concurrency = parsePositiveInteger(next(), "--concurrency"); + break; case "--dry-run": opts.dryRun = true; break; @@ -771,6 +820,7 @@ async function main(): Promise { } console.log(`Repeats per cell: ${opts.repeats}`); + console.log(`Concurrency: ${opts.concurrency}`); if (opts.dryRun) { console.log("dry-run: no inference"); @@ -781,14 +831,15 @@ async function main(): Promise { } const startedAt = new Date().toISOString(); - const results: CaseResult[] = []; - + const cells: Array<{ caseDef: EvalCase; variant: EvalVariant; repeat: number }> = []; for (const { caseDef, variant } of plan) { for (let repeat = 0; repeat < opts.repeats; repeat++) { - const result = await runCase(caseDef, variant, opts, repeat); - results.push(result); + cells.push({ caseDef, variant, repeat }); } } + const results = await mapPool(cells, opts.concurrency, ({ caseDef, variant, repeat }) => + runCase(caseDef, variant, opts, repeat), + ); const finishedAt = new Date().toISOString(); const totals = summarizeRun(results); diff --git a/src/plugins/shell-guard-plugin.test.ts b/src/plugins/shell-guard-plugin.test.ts index 5b847d943..d09c7129d 100644 --- a/src/plugins/shell-guard-plugin.test.ts +++ b/src/plugins/shell-guard-plugin.test.ts @@ -13,6 +13,7 @@ import { DEFAULT_SHELL_TIMEOUT_MS, MAX_SHELL_OUTPUT_BYTES, advertiseShellGuardTimeout, + resolveShellTimeoutMs, runGuardedShell, shellGuardPlugin, } from "./shell-guard-plugin.js"; @@ -154,6 +155,36 @@ describe("runGuardedShell", () => { }); }); +describe("resolveShellTimeoutMs", () => { + test("omitted timeout uses the 15s default", () => { + expect(resolveShellTimeoutMs(undefined, DEFAULT_SHELL_TIMEOUT_MS)).toBe(15_000); + expect(resolveShellTimeoutMs(undefined, DEFAULT_SHELL_TIMEOUT_MS, undefined)).toBe( + DEFAULT_SHELL_TIMEOUT_MS, + ); + }); + + test("non-positive requested timeout falls back to default", () => { + expect(resolveShellTimeoutMs(0, DEFAULT_SHELL_TIMEOUT_MS)).toBe(DEFAULT_SHELL_TIMEOUT_MS); + expect(resolveShellTimeoutMs(-1, DEFAULT_SHELL_TIMEOUT_MS)).toBe(DEFAULT_SHELL_TIMEOUT_MS); + }); + + test("requested timeout well above 10 minutes is not clamped when maxMs is omitted", () => { + expect(resolveShellTimeoutMs(5_400_000, DEFAULT_SHELL_TIMEOUT_MS)).toBe(5_400_000); + expect(resolveShellTimeoutMs(5_400_000, DEFAULT_SHELL_TIMEOUT_MS, undefined)).toBe(5_400_000); + expect(resolveShellTimeoutMs(900_000, DEFAULT_SHELL_TIMEOUT_MS)).toBe(900_000); + }); + + test("configured maxMs still clamps", () => { + expect(resolveShellTimeoutMs(900_000, DEFAULT_SHELL_TIMEOUT_MS, 100)).toBe(100); + expect(resolveShellTimeoutMs(5_400_000, DEFAULT_SHELL_TIMEOUT_MS, 600_000)).toBe(600_000); + expect(resolveShellTimeoutMs(undefined, DEFAULT_SHELL_TIMEOUT_MS, 100)).toBe(100); + }); + + test("requested below maxMs is unchanged", () => { + expect(resolveShellTimeoutMs(1_000, DEFAULT_SHELL_TIMEOUT_MS, 600_000)).toBe(1_000); + }); +}); + describe("advertiseShellGuardTimeout", () => { test("rewrites run_shell timeout default to match the guard", () => { const rewritten = advertiseShellGuardTimeout({ diff --git a/src/plugins/shell-guard-plugin.ts b/src/plugins/shell-guard-plugin.ts index d832d32a5..1a74135a1 100644 --- a/src/plugins/shell-guard-plugin.ts +++ b/src/plugins/shell-guard-plugin.ts @@ -18,9 +18,6 @@ import { // so open-ended walks cannot OOM the host. export const DEFAULT_SHELL_TIMEOUT_MS = 15_000; -// Upper bound on a per-command timeout override, so the model cannot ask for an -// effectively unbounded wait. Configurable via settings. -export const MAX_SHELL_TIMEOUT_MS = 600_000; export const MAX_SHELL_OUTPUT_BYTES = 512_000; export type ShellTimeoutConfig = { @@ -29,6 +26,21 @@ export type ShellTimeoutConfig = { maxOutputBytes?: number; }; +/** + * Effective run_shell timeout. Omitting `requested` (or a non-positive value) + * uses `defaultMs`. `maxMs` clamps only when settings pass it — there is no + * implicit 10-minute ceiling. + */ +export function resolveShellTimeoutMs( + requested: number | undefined, + defaultMs: number, + maxMs?: number, +): number { + const base = requested !== undefined && requested > 0 ? requested : defaultMs; + if (maxMs === undefined) return base; + return Math.min(base, maxMs); +} + /** * Stock tools-posix still advertises timeout default 30000. Shell-guard enforces * 15s default; rewrite the definition the model sees so schema and behavior agree. @@ -356,7 +368,6 @@ export function shellGuardPlugin( options: ShellGuardPluginOptions = {}, ): ToolPlugin { const defaultMs = timeoutConfig?.defaultMs ?? DEFAULT_SHELL_TIMEOUT_MS; - const maxMs = timeoutConfig?.maxMs ?? MAX_SHELL_TIMEOUT_MS; const maxOutputBytes = timeoutConfig?.maxOutputBytes ?? MAX_SHELL_OUTPUT_BYTES; const sessionRoot = realpathSync(cwd); let retainedShellCwd = sessionRoot; @@ -412,9 +423,11 @@ export function shellGuardPlugin( }; } const requested = optionalNumber(call.arguments.timeout); - const baseTimeoutMs = - requested !== undefined && requested > 0 ? requested : defaultMs; - const effectiveTimeout = Math.min(baseTimeoutMs, maxMs); + const effectiveTimeout = resolveShellTimeoutMs( + requested, + defaultMs, + timeoutConfig?.maxMs, + ); const wrappedCommand = wrapCommandWithPwdProbe(command); try { const { output, exitCode, timedOut, outputTruncated } = diff --git a/src/provider/reasoning-effort.test.ts b/src/provider/reasoning-effort.test.ts index 60b78e278..2e009baff 100644 --- a/src/provider/reasoning-effort.test.ts +++ b/src/provider/reasoning-effort.test.ts @@ -137,7 +137,7 @@ describe("cycleReasoningEffort", () => { expect(cycleReasoningEffort("grok-4.6", undefined)).toBe( cycleReasoningEffort("grok-4.6", "high"), ); - expect(cycleReasoningEffort("grok-4.6", "high")).toBe("low"); + expect(cycleReasoningEffort("grok-4.6", "high")).toBe("xhigh"); }); test("unset gpt-5.1 chat cycles from implicit none to minimal", () => { @@ -152,7 +152,7 @@ describe("cycleReasoningEffort", () => { test("grok leftover minimal cycles the same as unset / high", () => { expect(cycleReasoningEffort("grok-4.6", "minimal")).toBe(cycleReasoningEffort("grok-4.6", undefined)); expect(cycleReasoningEffort("grok-4.6", "minimal")).toBe(cycleReasoningEffort("grok-4.6", "high")); - expect(cycleReasoningEffort("grok-4.6", "minimal")).toBe("low"); + expect(cycleReasoningEffort("grok-4.6", "minimal")).toBe("xhigh"); }); test("unknown models with rungs still start at supported[0] when no default exists", () => { diff --git a/src/subagent/index.test.ts b/src/subagent/index.test.ts index 9c13e359e..19bbc39f0 100644 --- a/src/subagent/index.test.ts +++ b/src/subagent/index.test.ts @@ -792,6 +792,10 @@ describe("sub-agent stop helpers", () => { expect(resolveSubAgentDeadlineMs(45_000, 660_000)).toBe(45_000); }); + test("resolveSubAgentDeadlineMs keeps an explicit deadline when the outer watchdog is omitted", () => { + expect(resolveSubAgentDeadlineMs(18_000_000, undefined)).toBe(18_000_000); + }); + test("resolveSubAgentDeadlineMs skips arming when outer watchdog is at or below the margin", () => { expect(resolveSubAgentDeadlineMs(5_000, 5_000)).toBeUndefined(); expect(resolveSubAgentDeadlineMs(5_000, SUBAGENT_DEADLINE_MARGIN_MS)).toBeUndefined(); diff --git a/src/subagent/stop-policy.ts b/src/subagent/stop-policy.ts index ba16bcbb7..1291c45e7 100644 --- a/src/subagent/stop-policy.ts +++ b/src/subagent/stop-policy.ts @@ -32,15 +32,20 @@ export const SUBAGENT_DEADLINE_MARGIN_MS = 30_000; * maxTurns + operator cancel are the primary bounds; callers pass deadlineMs * only when they want an extra wall-clock stop. * + * When the outer watchdog is omitted (undefined), the requested deadline is + * kept — an absent settings timeout must not clamp a 5-hour (or any) explicit + * deadline down to a hidden default. + * * Returns undefined (do not arm) when the outer watchdog is at or below the * salvage margin — an internal deadline would otherwise race or exceed outer * and leave no room to return a salvage report. */ export function resolveSubAgentDeadlineMs( requestedMs: number, - outerWatchdogMs: number, + outerWatchdogMs: number | undefined, ): number | undefined { const requested = Math.max(1, Math.floor(requestedMs)); + if (outerWatchdogMs === undefined) return requested; if (outerWatchdogMs <= SUBAGENT_DEADLINE_MARGIN_MS) return undefined; // Ceiling must never exceed outer − margin (and stays ≥ 1 once outer > margin). const ceiling = Math.max(1, outerWatchdogMs - SUBAGENT_DEADLINE_MARGIN_MS); diff --git a/src/tools/eval-http-env.test.ts b/src/tools/eval-http-env.test.ts new file mode 100644 index 000000000..335c7e74a --- /dev/null +++ b/src/tools/eval-http-env.test.ts @@ -0,0 +1,41 @@ +import { describe, expect, test } from "bun:test"; +import { evalHttpEnvGet, runWithEvalHttpEnv } from "./eval-http-env.js"; + +describe("evalHttpEnv ALS", () => { + test("overlapping async callbacks each see only their own URL", async () => { + const urlA = "http://127.0.0.1:1111/"; + const urlB = "http://127.0.0.1:2222/"; + let aSaw: string | undefined; + let bSaw: string | undefined; + let release!: () => void; + const hold = new Promise((resolve) => { + release = resolve; + }); + + const runA = runWithEvalHttpEnv({ EVAL_HTTP_URL: urlA }, async () => { + await hold; + aSaw = evalHttpEnvGet("EVAL_HTTP_URL"); + }); + const runB = runWithEvalHttpEnv({ EVAL_HTTP_URL: urlB }, async () => { + await hold; + bSaw = evalHttpEnvGet("EVAL_HTTP_URL"); + }); + + release(); + await Promise.all([runA, runB]); + expect(aSaw).toBe(urlA); + expect(bSaw).toBe(urlB); + expect(aSaw).not.toBe(bSaw); + }); + + test("falls back to process.env when no overlay is active", () => { + const prior = process.env.EVAL_HTTP_URL; + process.env.EVAL_HTTP_URL = "http://127.0.0.1:9/"; + try { + expect(evalHttpEnvGet("EVAL_HTTP_URL")).toBe("http://127.0.0.1:9/"); + } finally { + if (prior === undefined) delete process.env.EVAL_HTTP_URL; + else process.env.EVAL_HTTP_URL = prior; + } + }); +}); diff --git a/src/tools/eval-http-env.ts b/src/tools/eval-http-env.ts new file mode 100644 index 000000000..c3de34db5 --- /dev/null +++ b/src/tools/eval-http-env.ts @@ -0,0 +1,22 @@ +import { AsyncLocalStorage } from "node:async_hooks"; + +/** + * Per-async-context overlay for eval-only fixture env (EVAL_HTTP_URL / + * EVAL_HTTP_TOKEN). Capability cells run in-process and can overlap under + * --concurrency; a shared process.env write/restore would let one cell clobber + * or delete a sibling's origin. ALS is the in-process source of truth; process.env + * remains a fallback for tests that set it directly. + */ +const evalHttpEnvAls = new AsyncLocalStorage>>(); + +export function runWithEvalHttpEnv( + vars: Record, + fn: () => Promise, +): Promise { + const parent = evalHttpEnvAls.getStore(); + return evalHttpEnvAls.run({ ...parent, ...vars }, fn); +} + +export function evalHttpEnvGet(key: string): string | undefined { + return evalHttpEnvAls.getStore()?.[key] ?? process.env[key]; +} diff --git a/src/tools/ssrf-guard.test.ts b/src/tools/ssrf-guard.test.ts index 8520173d8..e59f6beaf 100644 --- a/src/tools/ssrf-guard.test.ts +++ b/src/tools/ssrf-guard.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "bun:test"; import { checkUrlForSsrf, isPrivateAddress } from "./ssrf-guard.js"; +import { runWithEvalHttpEnv } from "./eval-http-env.js"; describe("isPrivateAddress", () => { test("rejects loopback", () => { @@ -65,4 +66,12 @@ describe("checkUrlForSsrf", () => { else process.env.EVAL_HTTP_URL = prior; } }); + test("allows the eval fixture URL from the ALS overlay without writing process.env", async () => { + await runWithEvalHttpEnv({ EVAL_HTTP_URL: "http://127.0.0.1:54321/" }, async () => { + const allowed = await checkUrlForSsrf("http://127.0.0.1:54321/"); + expect(allowed.ok).toBe(true); + const other = await checkUrlForSsrf("http://127.0.0.1:1/"); + expect(other.ok).toBe(false); + }); + }); }); diff --git a/src/tools/ssrf-guard.ts b/src/tools/ssrf-guard.ts index f40761d8a..7fda112fe 100644 --- a/src/tools/ssrf-guard.ts +++ b/src/tools/ssrf-guard.ts @@ -1,5 +1,6 @@ import { isIP } from "node:net"; import { lookup } from "node:dns/promises"; +import { evalHttpEnvGet } from "./eval-http-env.js"; // Blocks requests to loopback, private, link-local, and other non-public IP // ranges before a fetch is issued, and again after every redirect hop (a @@ -47,13 +48,14 @@ export type SsrfCheckResult = { ok: true } | { ok: false; reason: string }; // Narrow, deliberate exception: the capability eval's hermetic "web-bait" case // binds a per-run HTTP fixture to 127.0.0.1 (see scripts/eval-capability.ts // startHTTPFixture) specifically so web_fetch can be exercised without curl. -// EVAL_HTTP_URL is only ever set by that harness for that one child process; an -// operator's real session never has it set, so this does not weaken the guard -// for any target the eval harness did not itself stand up. Matched by origin -// (not full URL) so a same-origin redirect within the fixture still passes the -// per-hop re-check. +// The allowed origin is the calling cell's ALS overlay (see eval-http-env.ts), +// falling back to process.env.EVAL_HTTP_URL for tests that set it directly. +// An operator's real session never has it set, so this does not weaken the +// guard for any target the eval harness did not itself stand up. Matched by +// origin (not full URL) so a same-origin redirect within the fixture still +// passes the per-hop re-check. function isEvalFixtureUrl(rawUrl: string): boolean { - const allowed = process.env.EVAL_HTTP_URL; + const allowed = evalHttpEnvGet("EVAL_HTTP_URL"); if (allowed === undefined || allowed.length === 0) return false; try { return new URL(rawUrl).origin === new URL(allowed).origin; diff --git a/src/tui/dynamic-tool-runner.ts b/src/tui/dynamic-tool-runner.ts index 404fa9db8..7d847d578 100644 --- a/src/tui/dynamic-tool-runner.ts +++ b/src/tui/dynamic-tool-runner.ts @@ -52,7 +52,7 @@ export function createDynamicToolRunner( if (found === undefined) { return { callId: call.id, content: `unknown tool: ${call.name}`, isError: true }; } - const executionTimeoutMs = resolveToolExecutionTimeoutMs(watchdogConfig); + const executionTimeoutMs = resolveToolExecutionTimeoutMs(watchdogConfig, call); const waitForApproval = resolveWaitForApproval(watchdogConfig); const result = await runWithToolExecutionWatchdog( call, diff --git a/src/tui/tool-execution-watchdog.test.ts b/src/tui/tool-execution-watchdog.test.ts index b70f7d3d7..0066535d4 100644 --- a/src/tui/tool-execution-watchdog.test.ts +++ b/src/tui/tool-execution-watchdog.test.ts @@ -2,6 +2,8 @@ import { describe, expect, test } from "bun:test"; import type { AgentTool } from "@intx/agent"; import { createDynamicToolRunner } from "./dynamic-tool-runner.js"; import { + MAX_TOOL_EXECUTION_TIMEOUT_MS, + RUN_SHELL_WATCHDOG_SLACK_MS, getToolApprovalBudget, isUsableToolExecuteResult, preferExecuteSalvageAfterAbort, @@ -31,6 +33,66 @@ describe("tool execution watchdog", () => { expect(resolveToolExecutionTimeoutMs({ defaultMs: 9_999_999, maxMs: 100 })).toBe(100); }); + test("task with no settings timeout is unbounded", () => { + expect( + resolveToolExecutionTimeoutMs(undefined, { id: "1", name: "task", arguments: {} }), + ).toBeUndefined(); + }); + + test("omitted config does not arm a default watchdog", () => { + expect(resolveToolExecutionTimeoutMs(undefined)).toBeUndefined(); + expect(resolveToolExecutionTimeoutMs({})).toBeUndefined(); + expect(resolveToolExecutionTimeoutMs({ waitForApproval: true })).toBeUndefined(); + }); + + test("settings timeout without max clamps to MAX_TOOL_EXECUTION_TIMEOUT_MS", () => { + expect(resolveToolExecutionTimeoutMs({ defaultMs: 9_999_999 })).toBe( + MAX_TOOL_EXECUTION_TIMEOUT_MS, + ); + }); + + test("run_shell requested 5-hour timeout is not clamped", () => { + const requested = 18_000_000; + const call = { id: "1", name: "run_shell", arguments: { timeout: requested } }; + const ms = resolveToolExecutionTimeoutMs(undefined, call); + expect(ms).toBe(requested + RUN_SHELL_WATCHDOG_SLACK_MS); + expect(ms).toBeGreaterThan(MAX_TOOL_EXECUTION_TIMEOUT_MS); + }); + + test("tools.maxTimeoutMs does not cap a longer requested run_shell timeout", () => { + const requested = 18_000_000; + const call = { id: "1", name: "run_shell", arguments: { timeout: requested } }; + const ms = resolveToolExecutionTimeoutMs({ defaultMs: 660_000, maxMs: 100_000 }, call); + expect(ms).toBe(requested + RUN_SHELL_WATCHDOG_SLACK_MS); + }); + + test("omitted run_shell timeout is unbounded (shell-guard still 15s)", () => { + expect( + resolveToolExecutionTimeoutMs(undefined, { id: "1", name: "run_shell", arguments: {} }), + ).toBeUndefined(); + expect( + resolveToolExecutionTimeoutMs(undefined, { + id: "1", + name: "run_shell", + arguments: { timeout: 0 }, + }), + ).toBeUndefined(); + }); + + test("omitted run_shell timeout still honors settings default", () => { + expect( + resolveToolExecutionTimeoutMs( + { defaultMs: 60_000, maxMs: 100_000 }, + { id: "1", name: "run_shell", arguments: {} }, + ), + ).toBe(60_000); + }); + + test("non-shell tools still honor tools.maxTimeoutMs", () => { + const call = { id: "1", name: "read_file", arguments: {} }; + expect(resolveToolExecutionTimeoutMs({ defaultMs: 9_999_999, maxMs: 100 }, call)).toBe(100); + }); + test("withTimeout dispose clears timer without leaving hung state", async () => { const parent = new AbortController(); const budget = withTimeout(parent.signal, 50); @@ -156,6 +218,47 @@ describe("tool execution watchdog", () => { expect(afterAbort.content).toBe("hang aborted"); }); + test("undefined timeout lets a 50ms tool complete", async () => { + const result = await runWithToolExecutionWatchdog( + { id: "unbounded", name: "task", arguments: {} }, + new AbortController().signal, + undefined, + async () => { + await new Promise((r) => setTimeout(r, 50)); + return { callId: "unbounded", content: "ok" }; + }, + { salvageGraceMs: TEST_SALVAGE_GRACE_MS, waitForApproval: true }, + ); + expect(result.isError).not.toBe(true); + expect(result.content).toBe("ok"); + }); + + test("undefined timeout still surfaces parent abort", async () => { + const parent = new AbortController(); + const pending = runWithToolExecutionWatchdog( + { id: "unbounded-hang", name: "task", arguments: {} }, + parent.signal, + undefined, + async () => { + await new Promise(() => {}); + return { callId: "unbounded-hang", content: "ok" }; + }, + { salvageGraceMs: TEST_SALVAGE_GRACE_MS, waitForApproval: true }, + ); + parent.abort(); + const afterAbort = await Promise.race([ + pending, + new Promise((_, reject) => + setTimeout( + () => reject(new Error("watchdog did not settle after parent abort + grace")), + TEST_SALVAGE_GRACE_MS + 500, + ), + ), + ]); + expect(afterAbort.isError).toBe(true); + expect(afterAbort.content).toBe("task aborted"); + }); + test("isUsableToolExecuteResult rejects errors and empty bodies", () => { expect(isUsableToolExecuteResult({ callId: "1", content: "ok" })).toBe(true); expect(isUsableToolExecuteResult({ callId: "1", content: " " })).toBe(false); diff --git a/src/tui/tool-execution-watchdog.ts b/src/tui/tool-execution-watchdog.ts index 8ad2c4d92..885842837 100644 --- a/src/tui/tool-execution-watchdog.ts +++ b/src/tui/tool-execution-watchdog.ts @@ -15,11 +15,16 @@ export type ToolWatchdogConfig = { waitForApproval?: boolean; }; -// Default exceeds shell-guard's per-command max so run_shell is not cut off by -// this layer before its own timeout fires. -export const DEFAULT_TOOL_EXECUTION_TIMEOUT_MS = 660_000; +// Cap applied when Settings set tools.timeoutMs without tools.maxTimeoutMs. +// Not an implicit default — omitted settings leave the watchdog unarmed. export const MAX_TOOL_EXECUTION_TIMEOUT_MS = 1_800_000; +/** + * Watchdog arms before shell-guard, so the outer budget must outlast a matching + * requested run_shell timeout or this layer wins the race and aborts first. + */ +export const RUN_SHELL_WATCHDOG_SLACK_MS = 1_000; + /** * After budget/parent abort wins the race, wait this long for the in-flight * execute to settle with a usable (non-error) body — e.g. task-tool salvage — @@ -37,9 +42,44 @@ export const MAX_TOOL_APPROVAL_PAUSE_MS = 1_800_000; const BUDGET_EXPIRED = Symbol("tool-execution-budget-expired"); -export function resolveToolExecutionTimeoutMs(config?: ToolWatchdogConfig): number { - const max = config?.maxMs ?? MAX_TOOL_EXECUTION_TIMEOUT_MS; - const raw = config?.defaultMs ?? DEFAULT_TOOL_EXECUTION_TIMEOUT_MS; +/** + * Wall-clock budget for one tool `run()`, or undefined to leave the timer unarmed. + * Parent cancel, maxTurns, and eval `--agent-timeout-ms` still bound the run. + * + * Arms only when Settings pass tools.timeoutMs / tools.maxTimeoutMs, or when + * run_shell passes a positive arguments.timeout (requested + slack so this + * layer cannot beat shell-guard). A requested run_shell timeout is not clamped + * to MAX_TOOL_EXECUTION_TIMEOUT_MS or tools.maxTimeoutMs. + */ +export function resolveToolExecutionTimeoutMs( + config?: ToolWatchdogConfig, + call?: ToolCall, +): number | undefined { + if (call?.name === "run_shell") { + const requested = requestedRunShellTimeoutMs(call); + if (requested !== undefined) { + return requested + RUN_SHELL_WATCHDOG_SLACK_MS; + } + } + return resolveSettingsWatchdogTimeoutMs(config); +} + +function requestedRunShellTimeoutMs(call: ToolCall): number | undefined { + const timeout = call.arguments.timeout; + if (typeof timeout !== "number" || !Number.isFinite(timeout) || timeout <= 0) { + return undefined; + } + return Math.floor(timeout); +} + +function resolveSettingsWatchdogTimeoutMs( + config: ToolWatchdogConfig | undefined, +): number | undefined { + if (config === undefined || (config.defaultMs === undefined && config.maxMs === undefined)) { + return undefined; + } + const max = config.maxMs ?? MAX_TOOL_EXECUTION_TIMEOUT_MS; + const raw = config.defaultMs ?? max; return Math.min(max, Math.max(1, Math.floor(raw))); } @@ -81,6 +121,22 @@ export type PauseableTimeout = { resume: (token: PauseToken) => void; }; +/** Chain parent cancel without arming a run-duration timer. */ +function withParentAbort(signal: AbortSignal): PauseableTimeout { + const controller = new AbortController(); + const onParentAbort = () => controller.abort(); + signal.addEventListener("abort", onParentAbort, { once: true }); + if (signal.aborted) controller.abort(); + return { + signal: controller.signal, + dispose: () => { + signal.removeEventListener("abort", onParentAbort); + }, + pause: (): PauseToken => 0, + resume: (_token: PauseToken) => {}, + }; +} + /** * Like withTimeout, but the remaining budget freezes while paused (e.g. while * a permission prompt is open). Pause/resume are refcounted so nested pauses @@ -281,8 +337,10 @@ export type ToolExecutionWatchdogOptions = { }; /** - * Runs `execute` under a wall-clock race against `parentSignal`, matching the - * shell-guard search-tool pattern so non-abortable work still returns on time. + * Runs `execute` under a race against `parentSignal` and, when `timeoutMs` is + * set, a wall-clock budget. `undefined` timeout does not arm a timer — parent + * cancel and the approval-budget ALS still apply. Permission pause ceiling + * (`MAX_TOOL_APPROVAL_PAUSE_MS`) stays a stuck-prompt guard, not a run cap. * * When budget/parent abort wins the race, the signal is still aborted, but we * give the in-flight execute a short grace to return a usable non-error body @@ -292,19 +350,22 @@ export type ToolExecutionWatchdogOptions = { export async function runWithToolExecutionWatchdog( call: ToolCall, parentSignal: AbortSignal, - timeoutMs: number, + timeoutMs: number | undefined, execute: (signal: AbortSignal) => Promise, options: ToolExecutionWatchdogOptions, ): Promise { const salvageGraceMs = options.salvageGraceMs ?? TOOL_EXECUTION_SALVAGE_GRACE_MS; const waitForApproval = options.waitForApproval; - const budget = waitForApproval - ? withPauseableTimeout(parentSignal, timeoutMs) - : { - ...withTimeout(parentSignal, timeoutMs), - pause: (): PauseToken => 0, - resume: (_token: PauseToken) => {}, - }; + const budget: PauseableTimeout = + timeoutMs === undefined + ? withParentAbort(parentSignal) + : waitForApproval + ? withPauseableTimeout(parentSignal, timeoutMs) + : { + ...withTimeout(parentSignal, timeoutMs), + pause: (): PauseToken => 0, + resume: (_token: PauseToken) => {}, + }; // Nested runs (task tool → child tool call) shadow the parent store: the // gate captures the innermost budget, so pause/resume must chain outward or // the parent `task` budget keeps ticking under the permission modal. @@ -334,9 +395,10 @@ export async function runWithToolExecutionWatchdog( if (salvaged !== undefined) return salvaged; // Avoid unhandled rejection if execute later fails after we move on. void executePromise.catch(() => {}); - const content = parentSignal.aborted - ? `${call.name} aborted` - : formatToolExecutionTimeoutMessage(call.name, timeoutMs); + const content = + timeoutMs !== undefined && !parentSignal.aborted + ? formatToolExecutionTimeoutMessage(call.name, timeoutMs) + : `${call.name} aborted`; return { callId: call.id, content, isError: true }; } @@ -351,6 +413,7 @@ export async function runWithToolExecutionWatchdog( if ( budget.signal.aborted && !parentSignal.aborted && + timeoutMs !== undefined && outcome.isError === true && typeof outcome.content === "string" && isAbortLikeToolError(outcome.content)