diff --git a/README.md b/README.md index bd4eaa9..941b2fd 100644 --- a/README.md +++ b/README.md @@ -293,6 +293,13 @@ Create a scenario file: Paths inside a scenario file are resolved relative to that scenario file. +`baselineSkills` may be `[]`, which compares "no skill" against a proposed +skill: the question of whether adding the skill changes anything at all. This +is preferable to pointing baseline at a placeholder skill file, which +contaminates the baseline arm: its text lands in the system prompt under +inline delivery, and its description is visible in the registry under install +delivery. + Run it: ```bash diff --git a/SPEC.md b/SPEC.md index b74d0e5..7330732 100644 --- a/SPEC.md +++ b/SPEC.md @@ -158,7 +158,7 @@ exactly one arm is openai). A compare file defines: - agent file -- baseline skill files (or a shared skill set both arms inherit) +- baseline skill files, empty to compare against no skill, or a shared skill set both arms inherit - proposed skill files - model (shared, or per arm for model comparisons) - run count diff --git a/src/engine/compare.ts b/src/engine/compare.ts index bec1851..160838d 100644 --- a/src/engine/compare.ts +++ b/src/engine/compare.ts @@ -495,7 +495,11 @@ async function runArm( if (config.delivery === "install") { const { installed, warnings } = installSkills(armSkills, sandbox.dir); if (index === 0) { - onProgress?.(` ${label} installed skills: ${installed.map((skill) => skill.name).join(", ")}`); + onProgress?.( + installed.length === 0 + ? ` ${label} installed skills: (none)` + : ` ${label} installed skills: ${installed.map((skill) => skill.name).join(", ")}`, + ); for (const warning of warnings) { onProgress?.(` WARNING: ${warning}`); } diff --git a/src/engine/config.ts b/src/engine/config.ts index f5793ac..1f5cea0 100644 --- a/src/engine/config.ts +++ b/src/engine/config.ts @@ -217,7 +217,7 @@ export function loadCompareConfig( cases: rawCases.map((rawCase, index) => normalizeCase(baseDir, recordValue(rawCase, `scenarios[${index}]`), index)), }; - validateCompareConfig(config); + validateCompareConfig(config, options); return config; } @@ -242,11 +242,16 @@ function normalizeCase(baseDir: string, raw: RawCase, index: number): EvalCaseCo }; } -function validateCompareConfig(config: CompareConfig): void { - if (config.baselineSkills.length === 0) { - throw new Error("compare requires at least one baseline skill"); - } - if (config.proposedSkills.length === 0) { +function validateCompareConfig(config: CompareConfig, options: LoadOptions = {}): void { + // An explicit empty baselineSkills is a valid comparison: "does adding this + // skill change anything at all" needs a no-skill baseline arm. normalizeSkills + // already rejects an omitted or misspelled key, so a typo still fails loudly. + // + // In single-arm (measure) mode the proposed arm is a mirror of baseline that + // no run ever exercises, so an empty mirror is not a missing proposed set. + // Requiring content there would reject "characterize the agent with no + // skills," the measure-side version of the question this baseline change enables. + if (!options.singleArm && config.proposedSkills.length === 0) { throw new Error("compare requires at least one proposed skill"); } if (config.runs < 1) { diff --git a/test/config.test.ts b/test/config.test.ts index 8e8ff5c..8435215 100644 --- a/test/config.test.ts +++ b/test/config.test.ts @@ -249,3 +249,76 @@ test("loadCompareConfig parses maxTurns at both levels and rejects bad caps", () rmSync(dir, { recursive: true, force: true }); } }); + +test("loadCompareConfig accepts an explicit empty baselineSkills but still requires proposedSkills", () => { + const dir = mkdtempSync(join(tmpdir(), "promptdiff-config-empty-baseline-")); + try { + writeFileSync(join(dir, "agent.md"), "Agent", "utf8"); + writeFileSync(join(dir, "proposed.md"), "Proposed", "utf8"); + + // An explicit `[]` answers "does adding this skill change anything at all" + // and loads with an empty baseline arm. + writeFileSync( + join(dir, "empty-baseline.json"), + JSON.stringify({ + agent: "./agent.md", + baselineSkills: [], + proposedSkills: ["./proposed.md"], + model: "sonnet", + scenarios: [{ name: "t", prompt: "p", grader: { type: "text", contains: ["ok"] } }], + }), + "utf8", + ); + const config = loadCompareConfig(join(dir, "empty-baseline.json")); + expect(config.baselineSkills).toEqual([]); + expect(config.proposedSkills).toEqual([join(dir, "proposed.md")]); + + // A shared top-level `skills` set must not backfill an explicit `[]`: + // normalizeSkills returns on the Array.isArray branch before it ever + // reaches the shared-skills fallback, so pin that here. + writeFileSync(join(dir, "shared.md"), "Shared", "utf8"); + writeFileSync( + join(dir, "empty-baseline-with-shared.json"), + JSON.stringify({ + agent: "./agent.md", + skills: ["./shared.md"], + baselineSkills: [], + proposedSkills: ["./proposed.md"], + model: "sonnet", + scenarios: [{ name: "t", prompt: "p", grader: { type: "text", contains: ["ok"] } }], + }), + "utf8", + ); + expect(loadCompareConfig(join(dir, "empty-baseline-with-shared.json")).baselineSkills).toEqual([]); + + // Omitting the key entirely is a different failure than supplying `[]`: + // a typo'd or forgotten key must not silently become a no-skill baseline. + writeFileSync( + join(dir, "omitted-baseline.json"), + JSON.stringify({ + agent: "./agent.md", + proposedSkills: ["./proposed.md"], + model: "sonnet", + scenarios: [{ name: "t", prompt: "p", grader: { type: "text", contains: ["ok"] } }], + }), + "utf8", + ); + expect(() => loadCompareConfig(join(dir, "omitted-baseline.json"))).toThrow(/baseline skill paths/); + + // A compare with nothing proposed is meaningless even when the baseline is empty. + writeFileSync( + join(dir, "empty-proposed.json"), + JSON.stringify({ + agent: "./agent.md", + baselineSkills: [], + proposedSkills: [], + model: "sonnet", + scenarios: [{ name: "t", prompt: "p", grader: { type: "text", contains: ["ok"] } }], + }), + "utf8", + ); + expect(() => loadCompareConfig(join(dir, "empty-proposed.json"))).toThrow("compare requires at least one proposed skill"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); diff --git a/test/delivery.test.ts b/test/delivery.test.ts index 9260320..40715e8 100644 --- a/test/delivery.test.ts +++ b/test/delivery.test.ts @@ -81,6 +81,66 @@ test("install delivery puts arm skills in each sandbox registry and keeps them o } }); +test("inline delivery with an empty baseline sends the agent body alone, with no SKILL marker", async () => { + const dir = mkdtempSync(join(tmpdir(), "promptdiff-delivery-empty-baseline-")); + try { + const agent = join(dir, "agent.md"); + writeFileSync(agent, "Agent persona", "utf8"); + // Inline delivery reads skill paths as files directly (assembleSystemPrompt + // maps over them and reads each one), unlike install delivery's directories. + const proposed = join(dir, "proposed-skill.md"); + writeFileSync(proposed, "PROPOSED_MARKER", "utf8"); + + const seenPrompts: { arm: string; prompt: string }[] = []; + const runner: Runner = { + name: "mock", + capabilities: { sandboxTools: true, skillRegistry: true, images: false, streamEvents: false }, + async run(options: RunnerRunOptions) { + const arm = options.systemPrompt.includes("PROPOSED_MARKER") ? "proposed" : "baseline"; + seenPrompts.push({ arm, prompt: options.systemPrompt }); + return { output: "ok", costUsd: 0.1, turns: 1, durationMs: 10, models: ["sonnet"], raw: {} }; + }, + }; + + const config: CompareConfig = { + name: "inline delivery, empty baseline", + agent, + baselineSkills: [], + proposedSkills: [proposed], + delivery: "inline", + arms: { + baseline: { model: "sonnet", runner: "claude-p" }, + proposed: { model: "sonnet", runner: "claude-p" }, + }, + runs: 1, + timeoutMs: 1_000, + maxBudgetUsd: 1, + addDirs: [], + sandboxRoot: join(dir, "runs"), + keepSandbox: false, + cases: [ + { + name: "target", + kind: "target", + prompt: "do the task", + grader: { type: "text", contains: ["ok"] }, + images: [], + addDirs: [], + }, + ], + }; + + await runCompare({ config, runners: { baseline: runner, proposed: runner } }); + const baselinePrompt = seenPrompts.find((entry) => entry.arm === "baseline")?.prompt; + // No baseline skills means assembleSystemPrompt has nothing to join in. + // The baseline arm's prompt is the agent file, verbatim, with no skill section. + expect(baselinePrompt).toBe("Agent persona"); + expect(baselinePrompt).not.toContain("===== SKILL"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + test("scenario files parse delivery and reject install with disabled tools", () => { const dir = mkdtempSync(join(tmpdir(), "promptdiff-delivery-config-")); try { diff --git a/test/measure.test.ts b/test/measure.test.ts index 59db4ce..adec557 100644 --- a/test/measure.test.ts +++ b/test/measure.test.ts @@ -94,6 +94,34 @@ test("singleArm loading accepts baselineSkills-only scenarios; compare still req } }); +test("singleArm loading accepts an empty baselineSkills; the mirrored empty proposed arm is never a validation error", () => { + const dir = mkdtempSync(join(tmpdir(), "promptdiff-measure-empty-config-")); + try { + writeFileSync(join(dir, "agent.md"), "Agent.", "utf8"); + const scenario = { + agent: "./agent.md", + baselineSkills: [], + model: "sonnet", + scenarios: [{ name: "t", prompt: "p", grader: { type: "text", contains: ["ok"] } }], + }; + const path = join(dir, "s.json"); + writeFileSync(path, JSON.stringify(scenario), "utf8"); + + // "Characterize the agent with no skills" is the measure-side version of + // the question an empty baseline enables in compare. Proposed mirrors + // baseline here and no run ever touches it, so an empty mirror must load. + const config = loadCompareConfig(path, {}, { singleArm: true }); + expect(config.baselineSkills).toEqual([]); + expect(config.proposedSkills).toEqual([]); + + // The same scenario as a real compare has no proposed arm at all: that + // still throws, since a compare with nothing proposed is meaningless. + expect(() => loadCompareConfig(path)).toThrow(/proposed skill paths/); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + test("measure warns when the measured model diverges from productionModel", async () => { const dir = mkdtempSync(join(tmpdir(), "promptdiff-measure-prod-")); try { diff --git a/test/skill-install.test.ts b/test/skill-install.test.ts index 7073a96..39db10b 100644 --- a/test/skill-install.test.ts +++ b/test/skill-install.test.ts @@ -100,6 +100,24 @@ test("installSkills warns when a description exceeds the registry limit", () => } }); +test("installSkills with an empty list installs nothing and creates no skills directory", () => { + const root = mkdtempSync(join(tmpdir(), "skill-install-test-")); + try { + const sandbox = join(root, "sandbox"); + mkdirSync(sandbox); + + const result = installSkills([], sandbox, join(root, "no-user-skills")); + + expect(result.installed).toEqual([]); + expect(result.warnings).toEqual([]); + // An empty baseline arm must leave the registry untouched, not an empty + // .claude/skills directory a later check could mistake for "installed but empty". + expect(existsSync(join(sandbox, ".claude", "skills"))).toBe(false); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + test("deliveryValue validates the delivery axis", () => { expect(deliveryValue(undefined, "inline")).toBe("inline"); expect(deliveryValue("install", "inline")).toBe("install");