From fa4e6d2b53f68d53bc3814d00cfec82b0dd05f30 Mon Sep 17 00:00:00 2001 From: Josh Owens Date: Wed, 30 Sep 2026 21:55:40 +0000 Subject: [PATCH] fix: accept "tools": "" in scenario files like --tools "" The scenario loader parsed `tools` with the generic non-empty string helper, so `"tools": ""` failed with "string value must be a non-empty string" while `--tools ""` was accepted. Parse `tools` at both the top level and per case with a helper that accepts "" and names the field in its error. An explicit "" already flowed through effectiveTools, the runner, and the cache key unchanged, so it now means no tools from every source. Install delivery still rejects it. Fixes #37 Co-Authored-By: Claude Opus 5.5 --- README.md | 7 ++++ SPEC.md | 5 +++ src/engine/config.ts | 13 +++++- test/config.test.ts | 96 ++++++++++++++++++++++++++++++++++++++++++- test/delivery.test.ts | 14 ++++++- 5 files changed, 130 insertions(+), 5 deletions(-) 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(