From 2a6fc712eb32fc891a2e814dd06ad5e8efe9c4e1 Mon Sep 17 00:00:00 2001 From: gensokyo-dev <342580529+gensokyo-dev@users.noreply.github.com> Date: Wed, 21 Oct 2026 00:16:29 +0800 Subject: [PATCH] fix(evaluation): join owned command cleanup before removing files (#5613) Use shared owned command supervision for all evaluation helpers, join streamed output, and retain/retry incomplete cleanup before deleting run files. Fixes #5094. --- docs/client-verification.md | 12 +- scripts/lib/agent-evaluation-commands.mjs | 193 ++++++++++++++++++ scripts/verify-agent-experience.mjs | 161 ++------------- .../process/agentEvaluationCommands.test.ts | 190 +++++++++++++++++ 4 files changed, 414 insertions(+), 142 deletions(-) create mode 100644 scripts/lib/agent-evaluation-commands.mjs create mode 100644 tests/boundary/process/agentEvaluationCommands.test.ts diff --git a/docs/client-verification.md b/docs/client-verification.md index 3c120686d..9cc3e81ce 100644 --- a/docs/client-verification.md +++ b/docs/client-verification.md @@ -457,13 +457,21 @@ disposable HOME and CODEX_HOME under `~/.cache/rea-agent-evaluations/`. Set `REA_AGENT_EVAL_ROOT` to choose another parent directory outside the OS temporary directory: current Codex refuses helper aliases under `/tmp`, preventing shell commands and skill reads even if MCP works. Each owned run directory is removed -after verification unless fixture retention is requested. Cleanup attempts the +after verification unless fixture retention is requested. Commands share the +owned-process supervisor: timeouts join bounded process-tree and output cleanup +before returning. If cleanup remains incomplete, the lane retries its retained +owner and preserves the run directory (including disposable auth) rather than +removing files still in use; the error retains ownership and recovery details. +Command stderr is limited to 32 KiB and combined version output to 8 KiB; +exceeding these diagnostic budgets fails explicitly. Scenario JSON events stream +to the evaluator without retaining a second raw stdout copy. Cleanup attempts the remaining run tree even if auth removal fails, and reports unresolved cleanup together with the original verification failure. The lane installs the packaged skill and MCP registration through public `rea setup`, rather than copying a skill into a project that also inherits the caller's skills and configuration. Only authentication is copied, when present, and it -is removed even with `REA_AGENT_EVAL_KEEP_FIXTURES=true`. No user configuration, +is removed after confirmed process cleanup even with +`REA_AGENT_EVAL_KEEP_FIXTURES=true`. No user configuration, rules, plugins or history is copied; shell snapshots and additional agents are disabled. Existing API-key environment authentication remains available. The native missing-provider scenario preapproves only `open_binary` and diff --git a/scripts/lib/agent-evaluation-commands.mjs b/scripts/lib/agent-evaluation-commands.mjs new file mode 100644 index 000000000..de6eb3657 --- /dev/null +++ b/scripts/lib/agent-evaluation-commands.mjs @@ -0,0 +1,193 @@ +import { randomUUID } from "node:crypto"; +import { createInterface } from "node:readline"; +import { PassThrough, Writable } from "node:stream"; + +import { + OwnedCommandFailure, + runOwnedCommand, +} from "../../dist/process/OwnedCommand.js"; + +/** Keep command ownership through failure and join cleanup before removing a run. */ +export function createAgentEvaluationCommands({ + codex, + evaluationRoot, + evaluationEnvironment, + optionalModel, + timeoutMs, + repositoryRoot, +}) { + const pendingCleanup = new Map(); + const diagnosticBytes = 32_768; + + async function execute( + command, + arguments_, + cwd, + env, + limits, + timeoutMessage, + ) { + try { + return await runOwnedCommand( + { + command, + arguments: arguments_, + // CLI shims may execute through a shebang interpreter. Ownership + // still uses the launch generation and token, not an argv prefix. + expectedCommand: null, + cwd, + hostEnvironment: env, + runId: randomUUID(), + sweepTokenOwnedProcesses: true, + }, + { diagnosticBytes, acceptNonZeroExit: true, ...limits }, + ); + } catch (cause) { + if (cause instanceof OwnedCommandFailure) { + if (cause.cleanupOwner !== undefined) + pendingCleanup.set(cause.cleanupOwner, cause); + if (cause.reason === "timeout") + throw new Error(timeoutMessage, { cause }); + } + throw cause; + } + } + + return { + async runCodex(prompt, approvedTools) { + const arguments_ = [ + "--no-daemon", + "exec", + "--ephemeral", + "--skip-git-repo-check", + "--sandbox", + "read-only", + "--json", + "--color", + "never", + "-C", + evaluationRoot, + "-c", + 'approval_policy="never"', + ...approvedTools.flatMap((name) => [ + "-c", + `mcp_servers.rea.tools.${name}.approval_mode="approve"`, + ]), + ...(optionalModel === undefined ? [] : ["--model", optionalModel]), + prompt, + ]; + const events = []; + const nonJson = []; + let nonJsonBytes = 0; + const stdout = new PassThrough(); + const lines = createInterface({ input: stdout }); + const linesClosed = new Promise((resolve) => + lines.once("close", resolve), + ); + let lineError; + lines.on("error", (cause) => { + // The owned pipeline reports stream failure or cancellation. Join this + // consumer too, without turning its forwarded error into a process crash. + lineError = cause; + lines.close(); + }); + lines.on("line", (line) => { + try { + events.push(JSON.parse(line)); + } catch { + if (nonJsonBytes >= diagnosticBytes) return; + const bytes = Buffer.from( + `non-json stdout: ${line.slice(0, diagnosticBytes - nonJsonBytes)}`, + ); + const retained = bytes.subarray(0, diagnosticBytes - nonJsonBytes); + if (retained.length > 0) nonJson.push(retained); + nonJsonBytes += retained.length; + } + }); + try { + const result = await execute( + codex, + arguments_, + evaluationRoot, + evaluationEnvironment, + { timeoutMs, stdout }, + `Codex evaluation timed out after ${String(timeoutMs)}ms`, + ); + await linesClosed; + if (lineError !== undefined) throw lineError; + return { + events, + exitCode: result.exitCode ?? 1, + stderr: Buffer.concat([...nonJson, Buffer.from(result.stderr.text)]) + .subarray(0, diagnosticBytes) + .toString("utf8"), + }; + } finally { + stdout.destroy(); + lines.close(); + await linesClosed; + } + }, + + async runProcess(command, arguments_, cwd, timeout, env = process.env) { + const result = await execute( + command, + arguments_, + cwd, + env, + { + timeoutMs: timeout, + stdout: new Writable({ write: (_chunk, _encoding, done) => done() }), + }, + `${command} timed out`, + ); + if (result.exitCode !== 0) + throw new Error( + `${command} failed with ${String(result.exitCode)}: ${result.stderr.text}`, + ); + }, + + async readCommandVersion(command) { + const result = await execute( + command, + ["--version"], + repositoryRoot, + process.env, + { timeoutMs: 10_000, diagnosticBytes: 8192 }, + `${command} --version timed out`, + ); + if (result.exitCode !== 0) + throw new Error( + `${command} --version failed with ${String(result.exitCode)}: ${result.stderr.text}`, + ); + const version = result.stdout.text.trim(); + if (version.length === 0) + throw new Error(`${command} --version returned empty output`); + return version; + }, + + async close() { + const failures = []; + for (const [owner, failure] of pendingCleanup) { + try { + const result = await owner.stop(); + if (result.status === "incomplete") + failures.push(new Error(result.reason, { cause: failure })); + else pendingCleanup.delete(owner); + } catch (cause) { + failures.push( + new AggregateError( + [failure, cause], + "Evaluation process cleanup failed", + ), + ); + } + } + if (failures.length > 0) + throw new AggregateError( + failures, + "Evaluation processes remain owned; preserve the run directory and retry cleanup through the retained owners", + ); + }, + }; +} diff --git a/scripts/verify-agent-experience.mjs b/scripts/verify-agent-experience.mjs index e8fefe14d..3ac69d1d2 100644 --- a/scripts/verify-agent-experience.mjs +++ b/scripts/verify-agent-experience.mjs @@ -1,4 +1,3 @@ -import { spawn } from "node:child_process"; import { createHash } from "node:crypto"; import { chmod, @@ -11,7 +10,6 @@ import { } from "node:fs/promises"; import { homedir } from "node:os"; import { delimiter, dirname, join, resolve } from "node:path"; -import { createInterface } from "node:readline"; import { fileURLToPath } from "node:url"; import { z } from "zod"; @@ -26,6 +24,7 @@ import { summarizeFactualCorrectness, } from "../dist/evaluation/AgentEvaluationReport.js"; import { PRODUCT_IDENTITY } from "../dist/identity.js"; +import { createAgentEvaluationCommands } from "./lib/agent-evaluation-commands.mjs"; import { closeVerifierResources } from "./lib/verifier-cleanup.mjs"; import { completeVerifierRun, createVerifierRun } from "./lib/verifier-run.mjs"; @@ -69,6 +68,16 @@ const evaluationEnvironment = { PATH: `${dirname(process.execPath)}${delimiter}${process.env.PATH ?? ""}`, }; +const commands = createAgentEvaluationCommands({ + codex, + evaluationRoot, + evaluationEnvironment, + optionalModel, + timeoutMs, + repositoryRoot, +}); +const { runCodex, runProcess, readCommandVersion } = commands; + let primaryError; try { const codexVersion = await readCommandVersion(codex); @@ -400,10 +409,16 @@ try { // Even a deliberately retained fixture directory must never retain auth. await closeVerifierResources( [ - () => rm(authPath, { force: true }), - ...(process.env.REA_AGENT_EVAL_KEEP_FIXTURES === "true" - ? [] - : [() => rm(evaluationRoot, { recursive: true, force: true })]), + async () => { + // A failed process cleanup retains both its exact owner and its files. + await commands.close(); + await closeVerifierResources([ + () => rm(authPath, { force: true }), + ...(process.env.REA_AGENT_EVAL_KEEP_FIXTURES === "true" + ? [] + : [() => rm(evaluationRoot, { recursive: true, force: true })]), + ]); + }, ], primaryError, ); @@ -487,137 +502,3 @@ function validTargetClarification(message) { throw cause; } } - -async function runCodex(prompt, approvedTools) { - const arguments_ = [ - "--no-daemon", - "exec", - "--ephemeral", - "--skip-git-repo-check", - "--sandbox", - "read-only", - "--json", - "--color", - "never", - "-C", - evaluationRoot, - "-c", - 'approval_policy="never"', - ...approvedTools.flatMap((name) => [ - "-c", - `mcp_servers.rea.tools.${name}.approval_mode="approve"`, - ]), - ...(optionalModel === undefined ? [] : ["--model", optionalModel]), - prompt, - ]; - const child = spawn(codex, arguments_, { - cwd: evaluationRoot, - env: evaluationEnvironment, - stdio: ["ignore", "pipe", "pipe"], - }); - const events = []; - const stderr = []; - const lines = createInterface({ input: child.stdout }); - const linesClosed = new Promise((resolveClose) => - lines.once("close", resolveClose), - ); - lines.on("line", (line) => { - try { - events.push(JSON.parse(line)); - } catch { - stderr.push(`non-json stdout: ${line}`); - } - }); - child.stderr.on("data", (chunk) => { - if (stderr.join("").length < 32_768) stderr.push(chunk.toString("utf8")); - }); - const exitCode = await new Promise((resolveExit, reject) => { - const timeout = setTimeout(() => { - child.kill("SIGTERM"); - reject( - new Error(`Codex evaluation timed out after ${String(timeoutMs)}ms`), - ); - }, timeoutMs); - child.once("error", (cause) => { - clearTimeout(timeout); - reject(cause); - }); - child.once("exit", (code) => { - clearTimeout(timeout); - resolveExit(code ?? 1); - }); - }); - await linesClosed; - return { events, exitCode, stderr: stderr.join("").slice(0, 32_768) }; -} - -async function runProcess( - command, - arguments_, - cwd, - timeout, - env = process.env, -) { - const child = spawn(command, arguments_, { - cwd, - env, - stdio: ["ignore", "ignore", "pipe"], - }); - let stderr = ""; - child.stderr.on("data", (chunk) => { - if (stderr.length < 32_768) stderr += chunk.toString("utf8"); - }); - const code = await new Promise((resolveExit, reject) => { - const timer = setTimeout(() => { - child.kill("SIGTERM"); - reject(new Error(`${command} timed out`)); - }, timeout); - child.once("error", (cause) => { - clearTimeout(timer); - reject(cause); - }); - child.once("exit", (value) => { - clearTimeout(timer); - resolveExit(value ?? 1); - }); - }); - if (code !== 0) - throw new Error(`${command} failed with ${String(code)}: ${stderr}`); -} - -async function readCommandVersion(command) { - const child = spawn(command, ["--version"], { - cwd: repositoryRoot, - stdio: ["ignore", "pipe", "pipe"], - }); - let stdout = ""; - let stderr = ""; - child.stdout.on("data", (chunk) => { - if (stdout.length < 4096) stdout += chunk.toString("utf8"); - }); - child.stderr.on("data", (chunk) => { - if (stderr.length < 4096) stderr += chunk.toString("utf8"); - }); - const code = await new Promise((resolveExit, reject) => { - const timer = setTimeout(() => { - child.kill("SIGTERM"); - reject(new Error(`${command} --version timed out`)); - }, 10_000); - child.once("error", (cause) => { - clearTimeout(timer); - reject(cause); - }); - child.once("exit", (value) => { - clearTimeout(timer); - resolveExit(value ?? 1); - }); - }); - if (code !== 0) - throw new Error( - `${command} --version failed with ${String(code)}: ${stderr}`, - ); - const version = stdout.trim(); - if (version.length === 0) - throw new Error(`${command} --version returned empty output`); - return version; -} diff --git a/tests/boundary/process/agentEvaluationCommands.test.ts b/tests/boundary/process/agentEvaluationCommands.test.ts new file mode 100644 index 000000000..505ecbe01 --- /dev/null +++ b/tests/boundary/process/agentEvaluationCommands.test.ts @@ -0,0 +1,190 @@ +import { readFile, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { expect } from "vitest"; + +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { processTest } from "../../support/process/processFixture.js"; + +processTest.skipIf(process.platform === "win32")( + "joins real evaluation command/output cleanup while retaining timeout diagnostics", + { timeout: 40_000 }, + async ({ processes }) => { + const root = await createTestTempDirectory("rea-agent-command-"); + const fixture = join(root, "codex-fixture.mjs"); + await writeFile( + fixture, + `#!${process.execPath} +import { spawn } from "node:child_process"; +import { writeFileSync } from "node:fs"; +if (process.env.FIXTURE_MODE === "success") { + process.stdout.write(process.argv.includes("--version") ? "fixture 1.2\\n" : '{"type":"fixture","text":"你好"}\\n'); +} else if (process.env.FIXTURE_MODE === "failure") { + process.stderr.write("synthetic command failure"); process.exitCode = 7; +} else if (process.env.FIXTURE_MODE === "overflow") { + process.stderr.write("x".repeat(65536)); +} else { + process.on("SIGTERM", () => { + writeFileSync(${JSON.stringify(join(root, "term"))}, "acknowledged"); + if (process.argv.includes("--no-daemon")) setTimeout(() => process.exit(0), 200); + }); + if (!process.argv.includes("--no-daemon")) + spawn(process.execPath, ["-e", 'process.on("SIGTERM",()=>{});setInterval(()=>{},1000);'], { stdio: ["ignore", "inherit", "inherit"] }); + setInterval(() => {}, 1000); + setTimeout(() => process.exit(2), 20000); +} +`, + { mode: 0o700 }, + ); + const driver = join(root, "driver.mjs"); + await writeFile( + driver, + `import assert from "node:assert/strict"; +import cp from "node:child_process"; +import { syncBuiltinESMExports } from "node:module"; +const actualSpawn = cp.spawn; +const children = []; +cp.spawn = (...args) => { + const child = actualSpawn(...args); + if (args[0] === ${JSON.stringify(fixture)}) { + const entry = { child, closed: false }; + child.once("close", () => { entry.closed = true; }); + children.push(entry); + } + return child; +}; +syncBuiltinESMExports(); +const { createAgentEvaluationCommands } = await import(${JSON.stringify( + new URL( + "../../../scripts/lib/agent-evaluation-commands.mjs", + import.meta.url, + ).href, + )}); +const { OwnedCommandFailure, prepareOwnedCommand } = await import(${JSON.stringify( + new URL("../../../dist/process/OwnedCommand.js", import.meta.url).href, + )}); +await prepareOwnedCommand(); +const root = ${JSON.stringify(root)}, fixture = ${JSON.stringify(fixture)}; +const commands = createAgentEvaluationCommands({ codex: fixture, evaluationRoot: root, + evaluationEnvironment: process.env, timeoutMs: 1000, repositoryRoot: root }); +for (const name of ["runCodex", "runProcess", "readCommandVersion"]) { + await assert.rejects(() => name === "runCodex" ? commands.runCodex("fixture", []) : + name === "runProcess" ? commands.runProcess(fixture, [], root, 1000) : commands.readCommandVersion(fixture), error => { + assert.match(error.message, /timed out/); + assert.ok(error.cause instanceof OwnedCommandFailure); + assert.equal(error.cause.reason, "timeout"); + assert.equal(error.cause.cleanupOwner === undefined, true, name + ": " + error.cause.cleanupFailure); + return true; + }); + const { child, closed } = children.at(-1); + assert.equal(closed, true, name + " returned before close"); + assert.ok(child.exitCode !== null || child.signalCode !== null); + assert.equal(child.stdout.destroyed, true); + assert.equal(child.stderr.destroyed, true); +} +await commands.close(); +process.env.FIXTURE_MODE = "success"; +const result = await commands.runCodex("fixture", ["open_binary"]); +assert.deepEqual(result, { events: [{ type: "fixture", text: "你好" }], exitCode: 0, stderr: "" }); +await commands.runProcess(fixture, [], root, 2000); +assert.equal(await commands.readCommandVersion(fixture), "fixture 1.2"); +process.env.FIXTURE_MODE = "failure"; +assert.equal((await commands.runCodex("fixture", [])).exitCode, 7); +await assert.rejects(() => commands.runProcess(fixture, [], root, 2000), /failed with 7: synthetic command failure/); +await assert.rejects(() => commands.readCommandVersion(fixture), /--version failed with 7: synthetic command failure/); +process.env.FIXTURE_MODE = "overflow"; +await assert.rejects(() => commands.runProcess(fixture, [], root, 2000), error => { + assert.ok(error instanceof OwnedCommandFailure); + assert.equal(error.reason, "output-limit"); + assert.ok(error.snapshot.stderr.bytes <= 32768); + return true; +}); +await commands.close(); +console.log(JSON.stringify({ passed: true, timeoutHelpers: 3, controls: 7, childrenClosed: children.every(x => x.closed) })); +`, + ); + const result = await processes.run(process.execPath, [driver], { + timeoutMs: 35_000, + }); + expect(result.exitCode, result.stderr).toBe(0); + expect(JSON.parse(result.stdout)).toEqual({ + passed: true, + timeoutHelpers: 3, + controls: 7, + childrenClosed: true, + }); + }, +); + +processTest.for([false, true])( + "retries retained process cleanup before removing evaluation files: %s", + async (incomplete, { processes }) => { + const root = await createTestTempDirectory("rea-agent-command-retry-"); + const parent = join(root, "evaluations"); + const loader = join(root, "loader.mjs"); + const receipt = join(root, "receipt.json"); + const source = ` + import { mkdir, readdir, writeFile } from "node:fs/promises"; + import { join } from "node:path"; + export class OwnedCommandFailure extends Error { + reason = "timeout"; + cleanupOwner = { stop: async () => { + await writeFile(${JSON.stringify(join(root, "retried"))}, "exact owner retried"); + return ${JSON.stringify(incomplete ? { status: "incomplete", reason: "fixture cleanup denied" } : { status: "stopped" })}; + }}; + } + export async function runOwnedCommand() { + const run = join(${JSON.stringify(parent)}, (await readdir(${JSON.stringify(parent)}))[0]); + await mkdir(join(run, "codex")); + await writeFile(join(run, "codex/auth.json"), "inert fixture"); + await writeFile(${JSON.stringify(receipt)}, JSON.stringify({ run })); + throw new OwnedCommandFailure("fixture timeout with retained owner"); + } + `; + await writeFile( + loader, + `import { registerHooks } from "node:module"; +registerHooks({ load(url, context, nextLoad) { + if (url === ${JSON.stringify(new URL("../../../dist/process/OwnedCommand.js", import.meta.url).href)}) + return { format: "module", shortCircuit: true, source: ${JSON.stringify(source)} }; + return nextLoad(url, context); +}});`, + ); + const result = await processes.run( + process.execPath, + [ + "--import", + loader, + fileURLToPath( + new URL( + "../../../scripts/verify-agent-experience.mjs", + import.meta.url, + ), + ), + ], + { + env: { ...process.env, REA_AGENT_EVAL_ROOT: parent }, + timeoutMs: 10_000, + }, + ); + expect(result.exitCode).toBe(1); + expect(result.stderr).toContain("--version timed out"); + expect(await readFile(join(root, "retried"), "utf8")).toBe( + "exact owner retried", + ); + const { run }: { run: string } = JSON.parse( + await readFile(receipt, "utf8"), + ); + if (incomplete) { + expect(result.stderr).toContain("Evaluation processes remain owned"); + expect(result.stderr).toContain("fixture cleanup denied"); + expect(await readFile(join(run, "codex/auth.json"), "utf8")).toBe( + "inert fixture", + ); + } else { + await expect( + readFile(join(run, "codex/auth.json")), + ).rejects.toMatchObject({ code: "ENOENT" }); + } + }, +);