diff --git a/README.md b/README.md index 85c7ced..277332e 100644 --- a/README.md +++ b/README.md @@ -126,6 +126,13 @@ runs need headroom. Set `maxBudgetUsd` to `3` or more for artifact scenarios. A run that hits the cap fails with an explicit `claude hit the $N max budget` error rather than a silent bad sample. +**Tools:** `--tools ` (or `"tools"` in a scenario file, with +a per-case override) sets the tools each run gets: a comma-separated list, +`default`, or `""` for none. `"tools": ""` in a scenario file means the same +as `--tools ""`. When unset, text mode gets no tools and artifact mode gets +`default`. A per-case `"tools"` wins over the top-level value and over +`--tools`. + **Turn cap:** `--max-turns ` (or `"maxTurns"` in a scenario file, with a per-case override) caps agentic turns per run on the claude-p runner. Unlike a budget abort, a turn-capped run is a *measured outcome*: it is recorded and diff --git a/SPEC.md b/SPEC.md index 893e18c..d2b0762 100644 --- a/SPEC.md +++ b/SPEC.md @@ -69,6 +69,11 @@ used as a substitute for sandboxing. The runner captures final output text, `total_cost_usd`, turn count, duration, and model usage keys. +The `--tools` value comes from the per-case `tools` field, then `--tools` or +the top-level `tools` field, then the mode default (`""` for text mode, +`default` for artifact mode or install delivery). An explicit `""` at any +level means no tools and is never treated as unset. + A `maxTurns` cap (scenario field, per-case override, or `--max-turns`) is passed through as `--max-turns`. A run that stops at the cap (result subtype `error_max_turns`, on any exit code) is returned as a normal result flagged diff --git a/src/engine/config.ts b/src/engine/config.ts index 3eb5151..c04850d 100644 --- a/src/engine/config.ts +++ b/src/engine/config.ts @@ -210,7 +210,7 @@ export function loadCompareConfig( maxBudgetUsd: overrides.maxBudgetUsd ?? numberValue(raw.maxBudgetUsd, 1), maxTurns: overrides.maxTurns ?? optionalNumber(raw.maxTurns, "maxTurns"), mode: overrides.mode ?? modeValue(raw.mode, undefined), - tools: overrides.tools ?? stringValue(raw.tools, undefined), + tools: overrides.tools ?? toolsValue(raw.tools, "tools"), addDirs: (overrides.addDirs ?? stringArray(raw.addDirs, "addDirs")).map((dir) => resolveFrom(baseDir, dir)), sandboxRoot, sandboxSeed, @@ -238,7 +238,7 @@ function normalizeCase(baseDir: string, raw: RawCase, index: number): EvalCaseCo seed: optionalPath(baseDir, stringValue(raw.seed, undefined)), addDirs: stringArray(raw.addDirs, `${name}.addDirs`).map((dir) => resolveFrom(baseDir, dir)), mode: modeValue(raw.mode, undefined), - tools: stringValue(raw.tools, undefined), + tools: toolsValue(raw.tools, `${name}.tools`), maxTurns: optionalNumber(raw.maxTurns, `${name}.maxTurns`), }; } @@ -462,6 +462,15 @@ function modeValue(value: unknown, fallback: RunMode | undefined): RunMode | und throw new Error(`mode must be "text" or "artifact"`); } +/** Accepts "" like the --tools "" flag: an explicit empty list disables tools. */ +function toolsValue(value: unknown, label: string): string | undefined { + if (value === undefined) return undefined; + if (typeof value !== "string") { + throw new Error(`${label} must be a string: a comma-separated tool list, "default", or "" for no tools`); + } + return value; +} + function stringValue(value: unknown, fallback: string): string; function stringValue(value: unknown, fallback: string | undefined): string | undefined; function stringValue(value: unknown, fallback: string | undefined): string | undefined { diff --git a/test/config.test.ts b/test/config.test.ts index 8435215..6d34930 100644 --- a/test/config.test.ts +++ b/test/config.test.ts @@ -2,7 +2,9 @@ import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { expect, test } from "bun:test"; -import { loadCompareConfig } from "../src/engine/config"; +import { runCompare } from "../src/engine/compare"; +import { loadCompareConfig, type CompareConfig } from "../src/engine/config"; +import type { Runner, RunnerRunOptions } from "../src/types"; test("loadCompareConfig normalizes paths and rejects zero case runs", () => { const dir = mkdtempSync(join(tmpdir(), "promptdiff-config-test-")); @@ -322,3 +324,95 @@ test("loadCompareConfig accepts an explicit empty baselineSkills but still requi rmSync(dir, { recursive: true, force: true }); } }); + +function toolsRecordingRunner(sandboxTools: boolean, toolsSeen: string[]): Runner { + return { + name: sandboxTools ? "claude-p" : "openai", + capabilities: { sandboxTools, skillRegistry: sandboxTools, images: false, streamEvents: false }, + async run(options: RunnerRunOptions) { + toolsSeen.push(options.tools); + return { output: "ok", costUsd: 0, turns: 1, durationMs: 1, models: [options.model], raw: {} }; + }, + }; +} + +async function toolsSeenBy(config: CompareConfig, sandboxTools: boolean): Promise { + const toolsSeen: string[] = []; + const runner = toolsRecordingRunner(sandboxTools, toolsSeen); + await runCompare({ config, runners: { baseline: runner, proposed: runner } }); + return toolsSeen; +} + +test('scenario "tools": "" means no tools, the same as --tools ""', async () => { + const dir = mkdtempSync(join(tmpdir(), "promptdiff-config-empty-tools-")); + try { + writeFileSync(join(dir, "agent.md"), "Agent", "utf8"); + writeFileSync(join(dir, "baseline.md"), "Baseline", "utf8"); + writeFileSync(join(dir, "proposed.md"), "Proposed", "utf8"); + const base = { + agent: "./agent.md", + baselineSkills: ["./baseline.md"], + proposedSkills: ["./proposed.md"], + model: "gpt-4o-mini", + runner: "openai", + runs: 1, + sandbox: { root: "./runs" }, + }; + const textCase = { name: "t", prompt: "p", grader: { type: "text", contains: ["ok"] } }; + + // The shape from issue #37: top-level text mode with an explicit empty tools list. + writeFileSync(join(dir, "top-level.json"), JSON.stringify({ ...base, mode: "text", tools: "", scenarios: [textCase] }), "utf8"); + writeFileSync(join(dir, "no-tools-key.json"), JSON.stringify({ ...base, mode: "text", scenarios: [textCase] }), "utf8"); + const fromJson = loadCompareConfig(join(dir, "top-level.json")); + const fromFlag = loadCompareConfig(join(dir, "no-tools-key.json"), { tools: "" }); + expect(fromJson.tools).toBe(""); + expect(fromJson.tools).toBe(fromFlag.tools!); + // Runs on a text-only runner with no tools, exactly as the flag does. + expect(await toolsSeenBy(fromJson, false)).toEqual(["", ""]); + expect(await toolsSeenBy(fromFlag, false)).toEqual(["", ""]); + + // "" must not read as unset: artifact mode would otherwise fall back to "default". + writeFileSync( + join(dir, "artifact.json"), + JSON.stringify({ ...base, runner: "claude-p", tools: "", scenarios: [{ ...textCase, mode: "artifact" }] }), + "utf8", + ); + expect(await toolsSeenBy(loadCompareConfig(join(dir, "artifact.json")), true)).toEqual(["", ""]); + + // A per-scenario "" overrides a top-level tools list, so the text-only runner accepts it. + writeFileSync( + join(dir, "per-case.json"), + JSON.stringify({ ...base, tools: "Bash,Read", scenarios: [{ ...textCase, tools: "" }] }), + "utf8", + ); + const perCase = loadCompareConfig(join(dir, "per-case.json")); + expect(perCase.cases[0]?.tools).toBe(""); + expect(await toolsSeenBy(perCase, false)).toEqual(["", ""]); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("a non-string tools value fails with a message naming the field", () => { + const dir = mkdtempSync(join(tmpdir(), "promptdiff-config-bad-tools-")); + try { + writeFileSync(join(dir, "agent.md"), "Agent", "utf8"); + writeFileSync(join(dir, "baseline.md"), "Baseline", "utf8"); + writeFileSync(join(dir, "proposed.md"), "Proposed", "utf8"); + const base = { + agent: "./agent.md", + baselineSkills: ["./baseline.md"], + proposedSkills: ["./proposed.md"], + model: "sonnet", + }; + const textCase = { name: "t", prompt: "p", grader: { type: "text", contains: ["ok"] } }; + + writeFileSync(join(dir, "top-level.json"), JSON.stringify({ ...base, tools: ["Bash"], scenarios: [textCase] }), "utf8"); + expect(() => loadCompareConfig(join(dir, "top-level.json"))).toThrow(/^tools must be a string/); + + writeFileSync(join(dir, "per-case.json"), JSON.stringify({ ...base, scenarios: [{ ...textCase, tools: false }] }), "utf8"); + expect(() => loadCompareConfig(join(dir, "per-case.json"))).toThrow(/^t\.tools must be a string/); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); diff --git a/test/delivery.test.ts b/test/delivery.test.ts index 40715e8..4e900a1 100644 --- a/test/delivery.test.ts +++ b/test/delivery.test.ts @@ -163,9 +163,19 @@ test("scenario files parse delivery and reject install with disabled tools", () writeFileSync(defaultPath, JSON.stringify(base), "utf8"); expect(loadCompareConfig(defaultPath).delivery).toBe("inline"); - // The JSON parser already rejects a literal empty tools string; the CLI - // --tools "" override is the path that can smuggle it in. + // tools "" disables tools whether it comes from the scenario file or the + // --tools "" override, and install delivery rejects both. expect(() => loadCompareConfig(okPath, { tools: "" })).toThrow(/tools enabled/); + const noToolsPath = join(dir, "no-tools.json"); + writeFileSync(noToolsPath, JSON.stringify({ ...base, delivery: "install", tools: "" }), "utf8"); + expect(() => loadCompareConfig(noToolsPath)).toThrow(/tools enabled/); + const caseNoToolsPath = join(dir, "case-no-tools.json"); + writeFileSync( + caseNoToolsPath, + JSON.stringify({ ...base, delivery: "install", scenarios: [{ ...base.scenarios[0], tools: "" }] }), + "utf8", + ); + expect(() => loadCompareConfig(caseNoToolsPath)).toThrow(/tools enabled/); const badRegexPath = join(dir, "bad-regex.json"); writeFileSync(