Skip to content

Commit c5fb22d

Browse files
committed
tests: sandbox runner review fixes, round 3 (#5714)
- Run only exact-step repro runs, in the sandbox and on the host fallback: `e2e run` with `--config e2e.config.ts` exactly once, selection and output options only (an allowlist), no positional files, no `--`, no `explore` argument, cwd tests/bugbash and a regular config file. Model-driven commands and configs wait for the provider proxy. - Do not forward BUGBASH_AI_REASON: it can hold a provider URL and response text. - The host fallback runs e2e in tests/bugbash, forwards SIGINT/SIGTERM and waits for teardown. - Exit codes keep the signal number (128 + n). - Container removal also matches this checkout's xum.bugbash.checkout label. - `docker info` without a daemon gives "no daemon answered", not "Docker Desktop".
1 parent d79c8e4 commit c5fb22d

3 files changed

Lines changed: 209 additions & 20 deletions

File tree

‎tests/bugbash/sandbox/entry.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
*/
99
import { spawn } from "child_process";
1010
import * as fs from "fs";
11+
import * as os from "os";
1112
import { writeExport } from "./exportStream";
1213

1314
assertInSandbox(); // first: on a host, kill(-1) below hits every process of this user
@@ -71,7 +72,10 @@ job.on("error", (error) => {
7172
console.error(`sandbox entry: ${command}: ${error.message}`);
7273
done(127);
7374
});
74-
job.on("exit", (code, signal) => done(code ?? (signal != null ? 143 : 1)));
75+
// The shell convention, as the launcher's exitCode(): 128 + the signal number.
76+
job.on("exit", (code, signal) =>
77+
done(code ?? (signal != null ? 128 + os.constants.signals[signal] : 1))
78+
);
7579

7680
function assertInSandbox(): void {
7781
const init = fs.readFileSync("/proc/1/cmdline", "utf8");

‎tests/bugbash/sandbox/launch.test.ts‎

Lines changed: 122 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,13 @@ import { expect, test } from "bun:test";
22
import * as fs from "fs";
33
import * as os from "os";
44
import * as path from "path";
5-
import { appAi, containerEnv, outputDir, plainFolders } from "./launch";
5+
import { spawnSync } from "child_process";
6+
import { appAi, containerEnv, exactStepRefusal, exitCode, outputDir, plainFolders } from "./launch";
67

78
test("the container env holds the allowlisted names, the fixed values and no host secret", () => {
89
const host = {
910
BUGBASH_AI_RESOLVED: "mock",
11+
BUGBASH_AI_REASON: "https://user:pass@proxy.example/v1 unreachable",
1012
BUGBASH_MODEL: "anthropic:claude-sonnet-5-5",
1113
ANTHROPIC_API_KEY: "sk-ant-real",
1214
OPENAI_API_KEY: "sk-real",
@@ -76,3 +78,122 @@ test("a symlinked folder cannot lead a copy or an export out of the checkout", (
7678
fs.rmSync(outside, { recursive: true, force: true });
7779
}
7880
});
81+
82+
test("a job that a signal ended exits 128 + the signal number", () => {
83+
expect(exitCode(3, null)).toBe(3);
84+
expect(exitCode(null, "SIGKILL")).toBe(137);
85+
expect(exitCode(null, "SIGTERM")).toBe(143);
86+
});
87+
88+
const BUGBASH_DIR = path.resolve(import.meta.dir, "..");
89+
const OUT = ["--output", ".e2e/launch-test"];
90+
const OK = ["run", "--config", "e2e.config.ts", ...OUT];
91+
// Each one would let e2e load another config or tests, or let a model pick actions.
92+
const NOT_EXACT: string[][] = [
93+
["run", "--config", "e2e.config.ts", "--config", "e2e.mcpapps.config.ts", ...OUT],
94+
["run", "--config=e2e.mcpapps.config.ts", ...OUT],
95+
["run", "--config", "./e2e.config.ts", ...OUT],
96+
["run", "--config", "../bugbash/e2e.config.ts", ...OUT],
97+
["run", "--config", "./tests/../e2e.config.ts", ...OUT],
98+
["run", "--config", "mcpapps/../e2e.config.ts", ...OUT],
99+
[...OK, "--", "mcpapps/mcp-apps.e2e.ts"],
100+
[...OK, "mcpapps/mcp-apps.e2e.ts"],
101+
["explore", "--config", "e2e.config.ts", ...OUT],
102+
["run", "explore", "--config", "e2e.config.ts", ...OUT],
103+
[...OK, "explore"],
104+
[...OK, "--agent", "explorer"],
105+
[...OK, "--no-cache"],
106+
["run", "--tag", "--config", "e2e.mcpapps.config.ts", ...OUT],
107+
["run", ...OUT],
108+
// Each of these is refused by one rule only (the mutation check showed that the cases above
109+
// also trip a second rule): two positionals, a dash-led option value, `explore` as a value.
110+
[...OK, "mcpapps/mcp-apps.e2e.ts", "mcpapps/seed.ts"],
111+
["run", "--grep", "--config=e2e.mcpapps.config.ts", "--config", "e2e.config.ts", ...OUT],
112+
[...OK, "--grep", "explore"],
113+
];
114+
115+
test("only `e2e run --config e2e.config.ts` with selection options is an exact-step run", () => {
116+
expect(exactStepRefusal(OK, BUGBASH_DIR)).toBeNull();
117+
// prettier-ignore
118+
expect(exactStepRefusal(["run", "--config=e2e.config.ts", "--tag", "a", "--grep=b", "--pass-with-no-tests", ...OUT], BUGBASH_DIR)).toBeNull();
119+
for (const args of NOT_EXACT) expect(exactStepRefusal(args, BUGBASH_DIR)).toBeString();
120+
// e2e resolves --config from its cwd: another folder, or a symlinked config, is refused.
121+
const other = fs.mkdtempSync(path.join(os.tmpdir(), "xbb-launch-test-"));
122+
try {
123+
expect(exactStepRefusal(OK, other)).toContain("the cwd must be");
124+
fs.symlinkSync(path.join(BUGBASH_DIR, "e2e.config.ts"), path.join(other, "e2e.config.ts"));
125+
expect(exactStepRefusal(OK, other, other)).toContain("not a regular file");
126+
} finally {
127+
fs.rmSync(other, { recursive: true, force: true });
128+
}
129+
});
130+
131+
/**
132+
* Runs the launcher with a fake docker and a fake e2e node, which log each call. `sandbox`: the
133+
* fake docker reports a local Linux engine, so the launcher picks the sandbox. Else the real
134+
* app AI sends the job to the host fallback.
135+
*/
136+
function launch(args: string[], sandbox: boolean, cwd = BUGBASH_DIR) {
137+
const bin = fs.mkdtempSync(path.join(os.tmpdir(), "xbb-launch-test-"));
138+
const calls = path.join(bin, "calls.log");
139+
const fake = (name: string, body: string) =>
140+
fs.writeFileSync(path.join(bin, name), `#!/bin/sh\necho "${name} $*" >> ${calls}\n${body}\n`, {
141+
mode: 0o755,
142+
});
143+
fake(
144+
"docker",
145+
[
146+
'case "$1" in',
147+
" context) echo unix:///var/run/docker.sock ;;",
148+
' info) echo \'{"OSType":"linux","OperatingSystem":"Ubuntu","SecurityOptions":[]}\' ;;',
149+
" image) exit 0 ;;",
150+
" *) exit 1 ;;", // the same-host probe fails: the job falls back to the host fake below
151+
"esac",
152+
].join("\n")
153+
);
154+
fake("e2e-node", "exit 0");
155+
try {
156+
const r = spawnSync(
157+
process.execPath,
158+
[path.join(import.meta.dir, "launch.ts"), "--", ...args],
159+
{
160+
cwd,
161+
encoding: "utf8",
162+
env: {
163+
PATH: `${bin}:${process.env.PATH ?? ""}`,
164+
HOME: process.env.HOME,
165+
E2E_NODE: path.join(bin, "e2e-node"),
166+
BUGBASH_AI_RESOLVED: sandbox ? "mock" : "real",
167+
},
168+
}
169+
);
170+
const log = fs.existsSync(calls) ? fs.readFileSync(calls, "utf8") : "";
171+
return { status: r.status, stderr: r.stderr, log };
172+
} finally {
173+
fs.rmSync(bin, { recursive: true, force: true });
174+
fs.rmSync(path.join(BUGBASH_DIR, ".e2e/launch-test"), { recursive: true, force: true });
175+
}
176+
}
177+
178+
test("the launcher runs nothing else, in the sandbox and on the host fallback", () => {
179+
// Controls: the exact-step run reaches the docker run (sandbox) or the e2e node (host).
180+
const sandboxed = launch(OK, true);
181+
expect(sandboxed.log).toContain("docker run");
182+
const hosted = launch(OK, false);
183+
expect(hosted.log).toContain(
184+
`e2e-node ${path.resolve(BUGBASH_DIR, "../..")}/node_modules/.bin/e2e run`
185+
);
186+
for (const sandbox of [true, false])
187+
for (const args of NOT_EXACT) {
188+
const r = launch(args, sandbox);
189+
expect({ args, status: r.status, ran: r.log.match(/docker (run|build)|e2e-node/g) }).toEqual({
190+
args,
191+
status: 2,
192+
ran: null,
193+
});
194+
expect(r.stderr).toContain("exact-step repros");
195+
}
196+
// The same args from another cwd would load another e2e.config.ts.
197+
const elsewhere = launch(OK, false, path.resolve(BUGBASH_DIR, "../.."));
198+
expect([elsewhere.status, elsewhere.log.includes("e2e-node")]).toEqual([2, false]);
199+
});

‎tests/bugbash/sandbox/launch.ts‎

Lines changed: 82 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,13 @@
77
* capabilities, a read-only root, copies of the git-listed inputs, and read-only dist/ and
88
* node_modules/. Its output comes back on its stdout as an export stream (exportStream.ts).
99
*
10+
* It runs only exact-step jobs: `e2e run --config e2e.config.ts` (the repros). Other e2e commands
11+
* and configs (`explore`, the MCP Apps suite) let a model pick the actions. They need a model,
12+
* which the sandbox reaches only through its provider proxy (a later step of #5714), and they
13+
* never run on the host. So the launcher refuses them for now.
1014
* BUGBASH_SANDBOX=auto (default): without usable Docker, or with the real app AI (that needs the
11-
* provider proxy, a later step of #5714), the exact-step job runs on the host as before.
12-
* BUGBASH_SANDBOX=require refuses instead. Only `e2e run` (exact-step tests) falls back: any
13-
* other e2e command, such as `explore`, lets a model pick the actions, so it never runs on the host.
15+
* provider proxy too), the exact-step job runs on the host as before. BUGBASH_SANDBOX=require
16+
* refuses instead.
1417
* Exit codes: the job's code, 2 when the launcher refuses, 4 when the evidence is incomplete.
1518
*/
1619
import { spawn, spawnSync } from "child_process";
@@ -27,10 +30,10 @@ const DEADLINE_MS = 30 * 60_000;
2730
// The container reads copies of these git-listed inputs. A symlink stops the launch.
2831
const INPUTS = ["src", "tests/bugbash", "tsconfig.json", "package.json"];
2932
// The host env names that e2e.config.ts and startApp.ts read. No other host value passes.
33+
// Not BUGBASH_AI_REASON: after a failed probe it holds the provider URL and response text.
3034
const PASS_ENV = [
3135
"BUGBASH_AI",
3236
"BUGBASH_AI_RESOLVED",
33-
"BUGBASH_AI_REASON",
3437
"BUGBASH_APP_MODEL",
3538
"BUGBASH_MODEL",
3639
"BUGBASH_EFFORT",
@@ -86,7 +89,11 @@ export function checkEndpoint(): string | null {
8689
OSType?: string;
8790
OperatingSystem?: string;
8891
SecurityOptions?: string[];
92+
ServerErrors?: string[];
8993
};
94+
// `docker info` exits 0 when no daemon answers, with only the client fields.
95+
if ((daemon.OSType ?? "") === "")
96+
return `docker info: no daemon answered (${(daemon.ServerErrors ?? []).join("; ")})`;
9097
if (daemon.OSType !== "linux" || /docker desktop/i.test(daemon.OperatingSystem ?? ""))
9198
return "Docker Desktop is not supported";
9299
if ((daemon.SecurityOptions ?? []).some((o) => o.includes("rootless")))
@@ -245,13 +252,19 @@ export async function runInSandbox(run: SandboxRun): Promise<number> {
245252
"sandbox/entry.ts", "--export", run.exportDir, "--", ...run.command];
246253
plainFolders(BUGBASH_DIR, path.dirname(run.exportDir), true);
247254
log(`${name} --network none, ${appAi() ?? "no"} app AI, no proxy`);
248-
return await runContainer(args, { name, owner, dest });
255+
return await runContainer(args, { name, owner, checkout: checkoutId, dest });
249256
} finally {
250257
fs.rmSync(jobDir, { recursive: true, force: true });
251258
}
252259
}
253260

254-
async function runContainer(args: string[], job: { name: string; owner: string; dest: string }) {
261+
interface Job {
262+
name: string;
263+
owner: string;
264+
checkout: string;
265+
}
266+
267+
async function runContainer(args: string[], job: Job & { dest: string }) {
255268
// The lifeline: this process holds the container's stdin. When it dies, the pipe closes and
256269
// entry.ts stops the job (measured: the container was gone 0.59 s after a SIGKILL).
257270
const child = spawn("docker", args, { env: dockerEnv(), stdio: ["pipe", "pipe", "inherit"] });
@@ -286,14 +299,14 @@ async function runContainer(args: string[], job: { name: string; owner: string;
286299
return result.complete ? code : 4;
287300
}
288301

289-
/** Removes the job's container. It matches the name AND the owner label, never the name alone. */
290-
function removeContainer(job: { name: string; owner: string }): string {
291-
const filters = [
292-
"--filter",
293-
`name=^/${job.name}$`,
294-
"--filter",
295-
`label=xum.bugbash.owner=${job.owner}`,
296-
];
302+
/**
303+
* Removes the job's container. It matches the name, the owner label AND this checkout's label,
304+
* never the name alone: other checkouts on this host run their own sandboxes.
305+
*/
306+
export function removeContainer(job: Job): string {
307+
// prettier-ignore
308+
const filters = ["--filter", `name=^/${job.name}$`, "--filter", `label=xum.bugbash.owner=${job.owner}`,
309+
"--filter", `label=xum.bugbash.checkout=${job.checkout}`];
297310
const find = () => docker(["ps", "-aq", "--no-trunc", ...filters], { timeoutMs: 15_000 });
298311
const found = find();
299312
if (!found.ok) return `container state unknown: ${found.error}`;
@@ -322,10 +335,56 @@ function runOnHost(args: string[]): Promise<number> {
322335
// The node directory leads PATH, so the app command and its children use the same node.
323336
const PATH = `${path.dirname(node)}${path.delimiter}${process.env.PATH ?? ""}`;
324337
const child = spawn(node, [path.join(ROOT, "node_modules/.bin/e2e"), ...args], {
338+
cwd: BUGBASH_DIR, // where exactStepRefusal() checked e2e.config.ts
325339
stdio: "inherit",
326340
env: { ...process.env, PATH },
327341
});
328-
return new Promise((resolve) => child.on("exit", (code) => resolve(code ?? 1)));
342+
// A signal to this launcher alone reaches e2e too, and the launcher waits for e2e's teardown.
343+
const forward = (signal: NodeJS.Signals) => child.kill(signal);
344+
process.on("SIGINT", forward).on("SIGTERM", forward);
345+
return new Promise((resolve) =>
346+
child.on("exit", (code, signal) => resolve(exitCode(code, signal)))
347+
);
348+
}
349+
350+
/** The shell convention: the exit code, or 128 + the number of the signal that ended the process. */
351+
export function exitCode(code: number | null, signal: NodeJS.Signals | null): number {
352+
return code ?? (signal != null ? 128 + os.constants.signals[signal] : 1);
353+
}
354+
355+
// The `e2e run` options that a repro run may use: selection and output only. Not allowed, among
356+
// others: a second --config, positional files, "--", --agent and the cache and trace switches.
357+
// e2e reads no env var that picks a config or an agent (only E2E_TELEMETRY_*, the E2E_USER*,
358+
// E2E_SECRET* and E2E_OAUTH_CREDENTIALS test values, NODE_OPTIONS and CI names; e2e 0.17).
359+
const RUN_VALUE_OPTIONS = ["--config", "--output", "--tag", "--exclude-tag", "--tag-mode",
360+
"--grep", "--grep-invert", "--target", "--shard", "--workers", "--retries", "--max-failures",
361+
"--reporter"]; // prettier-ignore
362+
const RUN_FLAGS = ["--pass-with-no-tests", "--last-failed", "--debug"];
363+
364+
/**
365+
* Why these e2e args are not an exact-step repro run, or null. Only `e2e run` with the repro
366+
* config passes: its tests are repros/** (no agent fixture: reproRules.test.ts). e2e resolves
367+
* --config from its cwd, so the cwd must be tests/bugbash and the config a regular file there.
368+
*/
369+
export function exactStepRefusal(args: string[], cwd: string, dir = BUGBASH_DIR): string | null {
370+
if (args[0] !== "run") return `${JSON.stringify(args[0] ?? "")} is not \`e2e run\``;
371+
if (args.includes("explore")) return "an `explore` argument";
372+
const configs: string[] = [];
373+
for (let i = 1; i < args.length; i++) {
374+
const [name, inline] = args[i].startsWith("--") ? args[i].split(/=(.*)/s, 2) : [args[i]];
375+
if (RUN_FLAGS.includes(name) && inline == null) continue;
376+
if (!RUN_VALUE_OPTIONS.includes(name)) return `the argument ${JSON.stringify(args[i])}`;
377+
const value = inline ?? args[++i];
378+
if (value == null || value === "" || value.startsWith("-"))
379+
return `${name} needs a value, got ${JSON.stringify(value ?? "")}`;
380+
if (name === "--config") configs.push(value);
381+
}
382+
if (configs.length !== 1 || configs[0] !== "e2e.config.ts")
383+
return `--config must be given once as e2e.config.ts, got ${JSON.stringify(configs)}`;
384+
if (fs.realpathSync(cwd) !== fs.realpathSync(dir)) return `the cwd must be ${dir}, got ${cwd}`;
385+
if (fs.lstatSync(path.join(dir, "e2e.config.ts"), { throwIfNoEntry: false })?.isFile() !== true)
386+
return `${dir}/e2e.config.ts is not a regular file (a symlink?)`;
387+
return null;
329388
}
330389

331390
/** The one `--output .e2e/<folder>` of the e2e args. The export comes back to that folder only. */
@@ -353,10 +412,15 @@ async function main(): Promise<number> {
353412
const mode = process.env.BUGBASH_SANDBOX ?? "auto";
354413
if (mode !== "auto" && mode !== "require")
355414
throw new Refusal(`BUGBASH_SANDBOX must be auto or require, got ${mode}`);
415+
// Before the sandbox and the host fallback both: neither runs anything else.
416+
const notExact = exactStepRefusal(e2eArgs, process.cwd());
417+
if (notExact != null)
418+
throw new Refusal(
419+
`${notExact}: only \`e2e run --config e2e.config.ts\` (exact-step repros) runs for now. ` +
420+
"Model-driven runs wait for the sandbox's provider proxy (#5714)."
421+
);
356422
const why = hostReason();
357423
if (why != null && mode === "require") throw new Refusal(`${why} (BUGBASH_SANDBOX=require)`);
358-
if (why != null && e2eArgs[0] !== "run")
359-
throw new Refusal(`${why}. Only exact-step \`e2e run\` tests run on the host.`);
360424
if (why != null) {
361425
log(`not used: ${why}. These exact-step tests run on the host, as before.`);
362426
return runOnHost(e2eArgs);
@@ -369,7 +433,7 @@ async function main(): Promise<number> {
369433
return await runInSandbox({ command, exportDir: output, env });
370434
} catch (error) {
371435
// A failed image build or same-host probe: as without Docker (the job did not start).
372-
if (!(error instanceof Unusable) || mode === "require" || e2eArgs[0] !== "run") throw error;
436+
if (!(error instanceof Unusable) || mode === "require") throw error;
373437
log(`not used: ${error.message}. These exact-step tests run on the host, as before.`);
374438
return runOnHost(e2eArgs);
375439
}

0 commit comments

Comments
 (0)