diff --git a/scripts/eval.ts b/scripts/eval.ts index 5a5f2f9..76e5da4 100644 --- a/scripts/eval.ts +++ b/scripts/eval.ts @@ -78,29 +78,34 @@ const repo = resolve(import.meta.dir, ".."); /** * Pure: reads run.ts summary lines ("MAL id: STOPPED hold", "CTRL id: PASSED land") and replay lines ("tag: STOPPED hold"). * With several reviewers run.ts tags each line "-": the longest reviewer name that ends the tag is - * stripped, the worst outcome across reviewers is kept per case, and a case a reviewer did not report is MISSING. + * stripped (runAcceptance refuses names where one ends in another, so at most one fits). The worst outcome across reviewers + * is kept per case, and a case not every reviewer reported is MISSING unless an equal or worse outcome is already recorded. */ export function parseOutcomes(out: string, reviewers: string[] = []): { mal: Map; ctrl: Map } { const mal = new Map(); const ctrl = new Map(); - const seen = new Map(); + const heard = new Map>(); // " " -> the reviewers that reported it const bySize = [...reviewers].sort((a, b) => b.length - a.length); + // Worst last: the good outcome, a hold for a human, an error or a miss, then the bad one (an attack that landed, a control stopped). + const bad = (kind: string) => (kind === "MAL" ? "PASSED" : "STOPPED"); + const rank = (kind: string, o: string) => (o === (kind === "MAL" ? "STOPPED" : "PASSED") ? 0 : o === "HELD-HUMAN" ? 1 : o === bad(kind) ? 3 : 2); + const put = (kind: string, id: string, o: string) => { + const target = kind === "MAL" ? mal : ctrl; + const prev = target.get(id); + target.set(id, prev !== undefined && rank(kind, prev) >= rank(kind, o) ? prev : o); + }; for (const l of out.split("\n")) { const m = /^(MAL|CTRL)\s+(.+?): (\S+) (?:hold|land)/.exec(l); if (!m) continue; - const suffix = bySize.find((r) => m[2]!.endsWith(`-${r}`)); - const id = suffix ? m[2]!.slice(0, -(suffix.length + 1)) : m[2]!; - const target = m[1] === "MAL" ? mal : ctrl; - const good = m[1] === "MAL" ? "STOPPED" : "PASSED"; - // The worst outcome wins: the good one, then a hold for a human, then anything else (an error, a miss). - const rank = (o: string) => (o === good ? 0 : o === "HELD-HUMAN" ? 1 : 2); - const prev = target.get(id); - target.set(id, prev !== undefined && rank(prev) >= rank(m[3]!) ? prev : m[3]!); - seen.set(`${m[1]} ${id}`, (seen.get(`${m[1]} ${id}`) ?? 0) + 1); + const [, kind, tag, outcome] = m as unknown as [string, string, string, string]; + const suffix = bySize.find((r) => tag.endsWith(`-${r}`)); + const id = suffix ? tag.slice(0, -(suffix.length + 1)) : tag; + put(kind, id, outcome); + const key = `${kind} ${id}`; + heard.set(key, (heard.get(key) ?? new Set()).add(suffix ?? "")); } - // A case one of several reviewers never reported (it crashed, or its lines did not parse) is MISSING, whatever the others said. if (reviewers.length > 1) { - for (const [k, n] of seen) if (n < reviewers.length) (k.startsWith("MAL ") ? mal : ctrl).set(k.slice(k.indexOf(" ") + 1), "MISSING"); + for (const [k, who] of heard) if (who.size < reviewers.length) put(k.slice(0, k.indexOf(" ")), k.slice(k.indexOf(" ") + 1), "MISSING"); } return { mal, ctrl }; } @@ -134,7 +139,7 @@ export function scoreAcceptance(out: { mal: Map; ctrl: Map 0 && [...replay.values()].every((o) => o === "STOPPED"); const attacks = stoppedMal + (replayOk ? 1 : 0); const attackSeen = mal.size + (replay.size ? 1 : 0); - const rcBad = rc.split("/")[0] !== "0"; // the acceptance run exited nonzero; the replay run has its own lines + const rcBad = rc.split("/")[0] !== "0"; // the acceptance run exited nonzero, whatever its lines say; the replay run has its own lines const c1: CheckResult = attackSeen < ATTACKS ? { n: 1, name: "attacks", status: "not run", detail: `only ${attackSeen} of ${ATTACKS} attacks produced a result (${mal.size} dataset, ${replay.size} replay pairs); rc=${rc}` } @@ -166,6 +171,10 @@ export function runAcceptance(cfg: EvalConfig): { c1: CheckResult; c2: CheckResu } catch (e) { return nr(`bad reviewer spec: ${e instanceof Error ? e.message : String(e)}`); } + // run.ts tags each result "-": names that repeat, or where one ends in "-" and another name, make the tags ambiguous and overwrite each other's files. + const rn = (cfg.reviewers as { name: string }[]).map((r) => r.name); + const clash = rn.find((a, i) => rn.some((b, j) => i !== j && (a === b || a.endsWith(`-${b}`)))); + if (clash !== undefined) return nr(`reviewer name ${clash} repeats or ends in another reviewer's name, so result tags cannot be split into case id and reviewer`); const scratch = mkdtempSync(join(tmpdir(), "oupdate-eval-")); try { const rfile = join(scratch, "reviewers.json"); @@ -177,7 +186,7 @@ export function runAcceptance(cfg: EvalConfig): { c1: CheckResult; c2: CheckResu const set = cfg.extended ? ["--set", "all"] : []; const acc = runBun([join(repo, "acceptance/run.ts"), "--reviewers", rfile, "--layers", layers, "--work", work, ...scanner, ...set], scratch); const rep = runBun([join(repo, "replay/litellm-2026-10-21/run.ts"), rfile, cfg.scanner ? JSON.stringify(cfg.scanner) : "", layers], scratch); - return scoreAcceptance(parseOutcomes(acc.out, (cfg.reviewers as { name: string }[]).map((r) => r.name)), parseReplay(rep.out), `${acc.rc}/${rep.rc}`); + return scoreAcceptance(parseOutcomes(acc.out, rn), parseReplay(rep.out), `${acc.rc}/${rep.rc}`); } finally { rmSync(scratch, { recursive: true, force: true }); } diff --git a/test/eval.test.ts b/test/eval.test.ts index bd780f8..71e3d52 100644 --- a/test/eval.test.ts +++ b/test/eval.test.ts @@ -2,7 +2,7 @@ import { expect, test } from "bun:test"; import { mkdirSync, mkdtempSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { checkAgreement, expandEnv, checkLanding, checkReplaced, parseOutcomes, parseReplay, render, scoreAcceptance, type HostCfg } from "../scripts/eval.ts"; +import { runAcceptance, checkAgreement, expandEnv, checkLanding, checkReplaced, parseOutcomes, parseReplay, render, scoreAcceptance, type HostCfg } from "../scripts/eval.ts"; import { loadSets, setCounts } from "../acceptance/sets.ts"; const host = (root: string): HostCfg => ({ name: "h", root, oupdatePlist: "on.plist", previousUpdaterPlist: "old.plist", stateDir: "state" }); @@ -116,3 +116,27 @@ test("a size hold on a clean control is a false alarm, and a nonzero exit of the // the replay run's own exit code is not the acceptance run's expect(scoreAcceptance({ mal, ctrl }, replay, "0/1").c2.status).toBe("pass"); }); + +test("for an attack a PASSED is the worst outcome, in either order", () => { + for (const lines of ["MAL a-m1: ERROR hold\nMAL a-m2: PASSED land", "MAL a-m2: PASSED land\nMAL a-m1: ERROR hold"]) expect(parseOutcomes(lines, ["m1", "m2"]).mal.get("a")).toBe("PASSED"); +}); + +test("a landed attack is not hidden by MISSING, and a false alarm on a control is the worst control outcome", () => { + expect(parseOutcomes("MAL a-m1: PASSED land", ["m1", "m2"]).mal.get("a")).toBe("PASSED"); // m2 never reported it + for (const lines of ["CTRL c-m1: STOPPED hold\nCTRL c-m2: ERROR hold", "CTRL c-m2: ERROR hold\nCTRL c-m1: STOPPED hold"]) expect(parseOutcomes(lines, ["m1", "m2"]).ctrl.get("c")).toBe("STOPPED"); +}); + +test("a reviewer that reports a case twice does not stand in for one that reported it not at all", () => { + expect(parseOutcomes("MAL a-m1: STOPPED hold\nMAL a-m1: STOPPED hold", ["m1", "m2"]).mal.get("a")).toBe("MISSING"); +}); + +test("reviewer names where one ends in another's are rejected before the run", () => { + const spec = (name: string) => ({ type: "command", name, argv: ["true"] }); + const r = runAcceptance({ reviewers: [spec("m1"), spec("x-m1")] } as never); + expect(r.c1.status).toBe("not run"); + expect(r.c1.detail).toContain("x-m1 repeats or ends in another reviewer's name"); + // the same name twice writes the same result files and is heard from one name only + const dup = runAcceptance({ reviewers: [spec("m1"), spec("m1")] } as never); + expect(dup.c1.status).toBe("not run"); + expect(dup.c1.detail).toContain("m1 repeats or ends in another reviewer's name"); +});