fix(evals): enforce trust checks and reliable comparison cleanup and reports

Author: edelauna

scripts/agent-evals/README.md

@@ -80,8 +80,10 @@ them:
   message still appears in `completionMessageSeen`.
 - `SESSION_LOST` covers disconnects and interrupts. A manual stop after
   completion output stays `SESSION_LOST`.
-- The usage drain runs only after a `taskCompleted` event. It stays inside the
-  bounded drain window.
+- The usage drain runs after a `taskCompleted` event and after an observer stop
+  that followed the final `completion_result` message. It stays inside the
+  bounded drain window. A plain stop, a run timeout, and a session loss get no
+  drain.
 
 ## Checks
 
@@ -147,9 +149,17 @@ A comparison runs the extension code of two commits with the provider credential
 
 The comparison driver, fixtures, grader, and report builder all run from the trusted checkout that invokes `coding-evals:compare`. Only the extension host loads an evaluated worktree. The side process receives only the allowlisted environment plus the one named credential variable. Each side installs dependencies from its own worktree's frozen lockfile and builds its own exact commit, so each side runs its own compiled extension. Each side has its own wall clock (`--side-timeout-minutes`, default 120).
 
+The side script runs the same gate on its own inputs. It requires `--trust-file`, resolves the full commit of its `--evaluated-root`, and rejects an unapproved or unreadable commit before it creates a host or reads the credential. The `--credential-env` name must come from the supported provider variable allowlist; an arbitrary name can never forward an unrelated parent secret. Worktree install and build children also receive only an allowlisted environment, so the driver's secrets never reach them.
+
+Every driver child, preparation steps and the side alike, runs detached in its own process group and is tracked by one guard. A SIGINT or SIGTERM that reaches only the driver terminates the tracked group through a bounded SIGTERM-then-SIGKILL escalation, and the worktree cleanup waits for that escalation before it removes any tree.
+
 ### Output publication contract
 
-Artifacts are published atomically: content lands in a same-directory temp file and is renamed into place. A reader never sees a partial file. `side-results.json` appears only after every repetition of that side finished, so its existence means a complete side. A missing `side-results.json` means an interrupted side; consumers must treat it as a failed side and never pair partial output. The comparison reports are written atomically and only after both sides published complete results; a failed comparison leaves no report files.
+Artifacts are published atomically: content lands in a same-directory temp file and is renamed into place. A reader never sees a partial file.
+
+A side run invalidates its output directory first. Before the first repetition executes, the side removes any previous `side-results.json` and the whole `artifacts` tree, so stale results from an earlier rerun can never survive. `side-results.json` is the completion marker: it appears only after every repetition of the current invocation finished, and consumers must require it and treat its absence as a failed side. A side rerun that cannot remove a stale artifact fails before any repetition runs.
+
+The comparison reports are published as one generation. `comparison-report.json` and `comparison-report.md` carry the report's `generatedAt` stamp, and `comparison-report.complete` holds that stamp; it is written last. Consumers must require the marker and match its stamp against the report files. `readPublishedComparisonReport` enforces this: it returns the report only when the marker exists and its stamp matches both files. The marker is removed before publication starts, so a crash between the two file publications leaves a detectable incomplete generation instead of a mixed one. A thrown failure rolls both files back; a failed comparison leaves no report files.
 
 ### Commands
 
@@ -158,9 +168,9 @@ Artifacts are published atomically: content lands in a same-directory temp file
 pnpm test:agent-evals:unit
 pnpm agent-evals:check-types
 
-# One comparison side against the real provider. Requires the credential
-# environment variable and the container runtime for grading.
-pnpm coding-evals:side --reps 3 --out /tmp/side-out
+# One comparison side against the real provider. Requires the trust policy,
+# the credential environment variable, and the container runtime for grading.
+pnpm coding-evals:side --reps 3 --out /tmp/side-out --trust-file trust-policy.json
 
 # Full paired comparison of two approved commits.
 pnpm coding-evals:compare --baseline <sha> --candidate <sha> --trust-file trust-policy.json --reps 3 --out /tmp/compare-out

scripts/agent-evals/coding-compare.mts

@@ -9,17 +9,38 @@
 // worktree. The credential never leaves the trusted processes except through
 // the one named provider variable. A commit runs only when its full SHA is
 // listed in the operator's trust policy; refs are resolved and checked
-// against the policy before any worktree or process exists.
+// against the policy before any worktree or process exists. The side process
+// re-checks the root it was handed against the same policy.
+//
+// Process containment. Install, build, worktree, and side children receive
+// only an allowlisted environment, so the driver's secrets never reach them.
+// Each runs detached in its own process group under one guard, so SIGINT and
+// SIGTERM aimed at the driver alone terminate the tracked group with a bounded
+// SIGTERM-then-SIGKILL escalation before the scratch cleanup runs, and an
+// interrupt leaves no orphan host behind.
 import { execFile, spawn } from "node:child_process"
 import fs from "node:fs/promises"
 import os from "node:os"
 import path from "node:path"
 import { fileURLToPath } from "node:url"
 import { promisify } from "node:util"
-import { sideResultFileName, toSideResult, validateSideResultFile, type SideResultFile } from "./coding-compare.ts"
+import {
+	publishComparisonReport,
+	runBoundedPreparation,
+	sideResultFileName,
+	toSideResult,
+	validateSideResultFile,
+	type SideResultFile,
+} from "./coding-compare.ts"
 import { buildComparisonReport, renderMarkdownReport } from "./coding-report.ts"
-import { assertCommitsApproved, buildSideEnvironment, loadTrustPolicy, resolveCommit } from "./coding-trust.ts"
-import { writeJsonAtomic, writeFileAtomic } from "./fs-atomic.ts"
+import {
+	assertCommitsApproved,
+	assertCredentialEnvAllowed,
+	buildSideEnvironment,
+	loadTrustPolicy,
+	resolveCommit,
+} from "./coding-trust.ts"
+import { createSideProcessGuard, type SideKillOutcome } from "./side-process-guard.ts"
 
 const execFileAsync = promisify(execFile)
 const root = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "../..")
@@ -30,6 +51,22 @@ const WORKTREE_TIMEOUT_MS = 120_000
 const SIDE_KILL_GRACE_MS = 30_000
 const DEFAULT_SIDE_TIMEOUT_MINUTES = 120
 
+// Interrupt state. The handlers are registered before any child is spawned.
+// A signal terminates the tracked child group at once; the main flow observes
+// the child exit, the cleanup awaits the escalation before it removes the
+// worktrees, and the driver exits with the conventional signal exit code.
+const sideGuard = createSideProcessGuard(SIDE_KILL_GRACE_MS)
+let interruptSignal: NodeJS.Signals | undefined
+let interruptKill: Promise<SideKillOutcome> | undefined
+const isInterrupted = (): boolean => interruptSignal !== undefined
+const onInterruptSignal = (signal: NodeJS.Signals): void => {
+	if (interruptSignal !== undefined) return
+	interruptSignal = signal
+	interruptKill = sideGuard.interrupt()
+}
+process.on("SIGINT", () => onInterruptSignal("SIGINT"))
+process.on("SIGTERM", () => onInterruptSignal("SIGTERM"))
+
 const readArgs = (): {
 	baseline: string
 	candidate: string
@@ -58,42 +95,59 @@ const readArgs = (): {
 	if (!Number.isInteger(sideTimeoutMinutes) || sideTimeoutMinutes < 1) {
 		throw new Error("--side-timeout-minutes must be a positive integer")
 	}
+	const credentialEnv = read("credential-env") ?? "SOLHEIM_API_KEY"
+	// The variable name must be one the framework supports. An arbitrary name
+	// could forward an unrelated parent secret to the side.
+	assertCredentialEnvAllowed(credentialEnv)
 	return {
 		baseline,
 		candidate,
 		reps,
 		out: path.resolve(out),
-		credentialEnv: read("credential-env") ?? "SOLHEIM_API_KEY",
+		credentialEnv,
 		allowUnisolatedGrading: args.includes("--allow-unisolated-grading"),
 		trustFile: path.resolve(trustFile),
 		sideTimeoutMs: sideTimeoutMinutes * 60_000,
 	}
 }
 
-const runBounded = async (command: string, args: readonly string[], cwd: string, timeoutMs: number): Promise<void> => {
-	await execFileAsync(command, [...args], { cwd, timeout: timeoutMs, killSignal: "SIGKILL" })
-}
-
 // Prepares the worktree of one evaluated commit. Dependencies install from
 // the worktree's own frozen lockfile, and that exact commit's extension and
 // webview build from its own sources. No artifact crosses sides.
 const prepareWorktree = async (worktree: string): Promise<void> => {
-	await runBounded("pnpm", ["install", "--frozen-lockfile"], worktree, INSTALL_TIMEOUT_MS)
-	await runBounded("pnpm", ["build"], worktree, BUILD_TIMEOUT_MS)
+	await runBoundedPreparation(
+		sideGuard,
+		isInterrupted,
+		"pnpm",
+		["install", "--frozen-lockfile"],
+		worktree,
+		INSTALL_TIMEOUT_MS,
+	)
+	await runBoundedPreparation(sideGuard, isInterrupted, "pnpm", ["build"], worktree, BUILD_TIMEOUT_MS)
 }
 
 // Runs one trusted side against the evaluated worktree. The side script comes
 // from this checkout, receives only the allowlisted environment plus the
-// named credential variable, and must finish inside its own wall clock.
+// named credential variable, re-checks the worktree commit against the trust
+// policy, and must finish inside its own wall clock.
 const runSide = async (
 	side: "baseline" | "candidate",
 	commit: string,
 	inputs: ReturnType<typeof readArgs>,
 	scratchDir: string,
 ): Promise<SideResultFile> => {
+	if (isInterrupted()) throw new Error(`interrupted before the ${side} side started`)
 	const worktree = path.join(scratchDir, side)
-	await runBounded("git", ["worktree", "add", "--detach", worktree, commit], root, WORKTREE_TIMEOUT_MS)
+	await runBoundedPreparation(
+		sideGuard,
+		isInterrupted,
+		"git",
+		["worktree", "add", "--detach", worktree, commit],
+		root,
+		WORKTREE_TIMEOUT_MS,
+	)
 	await prepareWorktree(worktree)
+	if (isInterrupted()) throw new Error(`interrupted before the ${side} side started`)
 	const sideOut = path.join(scratchDir, `${side}-out`)
 	await fs.mkdir(sideOut, { recursive: true })
 	const exit = await new Promise<number>((resolve) => {
@@ -107,6 +161,8 @@ const runSide = async (
 				String(inputs.reps),
 				"--out",
 				sideOut,
+				"--trust-file",
+				inputs.trustFile,
 				"--credential-env",
 				inputs.credentialEnv,
 				"--evaluated-root",
@@ -120,6 +176,7 @@ const runSide = async (
 				detached: true,
 			},
 		)
+		sideGuard.track(child.pid)
 		let timedOut = false
 		const timer = setTimeout(() => {
 			timedOut = true
@@ -138,18 +195,20 @@ const runSide = async (
 		}, inputs.sideTimeoutMs)
 		child.once("error", () => {
 			clearTimeout(timer)
+			sideGuard.track(undefined)
 			resolve(1)
 		})
 		child.once("exit", (code) => {
 			clearTimeout(timer)
+			sideGuard.track(undefined)
 			resolve(timedOut ? 1 : (code ?? 1))
 		})
 	})
 	if (exit !== 0) throw new Error(`${side} side failed with exit code ${exit}`)
-	const parsed: unknown = JSON.parse(await fs.readFile(path.join(sideOut, sideResultFileName(side)), "utf8"))
-	// The side output must describe exactly the approved commit, or the pair
-	// is invalid.
-	return validateSideResultFile(parsed, commit)
+	const parsed: unknown = JSON.parse(await fs.readFile(path.join(sideOut, sideResultFileName()), "utf8"))
+	// The side output must describe exactly this side and its approved commit,
+	// or the pair is invalid.
+	return validateSideResultFile(parsed, side, commit)
 }
 
 const inputs = readArgs()
@@ -171,17 +230,19 @@ try {
 		candidate: toSideResult(await runSide("candidate", candidateCommit, inputs, scratchDir)),
 		repetitions: inputs.reps,
 	})
-	// The reports are the comparison's publication point. They appear
-	// atomically and only after both sides published complete results; any
-	// failure above leaves no report files behind.
-	await writeJsonAtomic(path.join(inputs.out, "comparison-report.json"), report)
-	await writeFileAtomic(path.join(inputs.out, "comparison-report.md"), renderMarkdownReport(report))
+	// The reports are the comparison's publication point. The pair is
+	// published as one generation with a completion marker; any failure above
+	// or during publication leaves no complete report generation behind.
+	await publishComparisonReport(inputs.out, report)
 	console.log(
 		report.compatibility.compatible
 			? `comparison complete: compatible pair, report in ${inputs.out}`
 			: `comparison complete: INCOMPATIBLE pair (${report.compatibility.differences.join(", ")}), report in ${inputs.out}`,
 	)
 } finally {
+	// An interrupt escalation must finish before the worktrees go away, so
+	// no child is still writing while its tree is deleted.
+	await interruptKill
 	await execFileAsync("git", ["worktree", "remove", "--force", path.join(scratchDir, "baseline")], {
 		cwd: root,
 		timeout: 30_000,
@@ -192,3 +253,7 @@ try {
 	}).catch(() => {})
 	await fs.rm(scratchDir, { recursive: true, force: true }).catch(() => {})
 }
+// An interrupt terminated the side group above, and the cleanup in the
+// finally block has completed. Exit with the conventional signal code.
+if (interruptSignal === "SIGINT") process.exit(130)
+if (interruptSignal === "SIGTERM") process.exit(143)