Skip to content

Commit fe7ca02

Browse files
steipetesteipete
committed
fix(agents): harden SSH sandbox stream errors
1 parent ba5e0b0 commit fe7ca02

3 files changed

Lines changed: 164 additions & 258 deletions

File tree

‎scripts/proof/ssh-sandbox-stream-errors.mts‎

Lines changed: 0 additions & 108 deletions
This file was deleted.

‎src/agents/sandbox/ssh.stream-errors.test.ts‎

Lines changed: 99 additions & 116 deletions
Original file line numberDiff line numberDiff line change
@@ -1,137 +1,120 @@
1-
// Regression tests for stdout/stderr stream errors in SSH sandbox commands.
2-
import { spawn, type ChildProcess, type SpawnOptions } from "node:child_process";
1+
import { spawn, type ChildProcess } from "node:child_process";
32
import { EventEmitter } from "node:events";
4-
import { beforeAll, describe, expect, it, vi } from "vitest";
3+
import fs from "node:fs/promises";
4+
import os from "node:os";
5+
import path from "node:path";
6+
import { PassThrough } from "node:stream";
7+
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
58

6-
type MockSpawnChild = EventEmitter & {
7-
stdout?: EventEmitter & { setEncoding?: (enc: string) => void };
8-
stderr?: EventEmitter & { setEncoding?: (enc: string) => void };
9-
stdin?: EventEmitter & { end?: (chunk?: unknown) => void };
10-
kill?: (signal?: string) => void;
9+
const spawnMock = vi.hoisted(() => vi.fn());
10+
11+
type MockChildProcess = EventEmitter & {
12+
stdin: PassThrough;
13+
stdout: PassThrough;
14+
stderr: PassThrough;
15+
kill: ReturnType<typeof vi.fn>;
1116
};
1217

13-
function createMockSpawnChild() {
14-
const child = new EventEmitter() as MockSpawnChild;
15-
const stdout = new EventEmitter() as MockSpawnChild["stdout"];
16-
stdout!.setEncoding = vi.fn();
17-
const stderr = new EventEmitter() as MockSpawnChild["stderr"];
18-
stderr!.setEncoding = vi.fn();
19-
const stdin = new EventEmitter() as MockSpawnChild["stdin"];
20-
stdin!.end = vi.fn();
21-
child.stdout = stdout;
22-
child.stderr = stderr;
23-
child.stdin = stdin;
24-
const kill = vi.fn(() => true);
25-
child.kill = kill;
26-
kill.mockImplementation(() => {
27-
(child as { killed?: boolean }).killed = true;
28-
return true;
29-
});
30-
return { child, stdout, stderr, stdin };
18+
function createMockChildProcess(): MockChildProcess {
19+
const child = new EventEmitter() as MockChildProcess;
20+
child.stdin = new PassThrough();
21+
child.stdout = new PassThrough();
22+
child.stderr = new PassThrough();
23+
child.kill = vi.fn(() => true);
24+
return child;
3125
}
3226

3327
vi.mock("node:child_process", async () => {
34-
const { mockNodeBuiltinModule } = await import("openclaw/plugin-sdk/test-node-mocks");
35-
const spawnLocal = vi.fn(
36-
(_command: string, _args: readonly string[], _options: SpawnOptions): ChildProcess => {
37-
const { child } = createMockSpawnChild();
38-
return child as unknown as ChildProcess;
39-
},
40-
);
41-
return mockNodeBuiltinModule(
42-
() => vi.importActual<typeof import("node:child_process")>("node:child_process"),
43-
{
44-
spawn: spawnLocal as unknown as typeof import("node:child_process").spawn,
45-
},
46-
);
28+
const actual = await vi.importActual<typeof import("node:child_process")>("node:child_process");
29+
return {
30+
...actual,
31+
spawn: spawnMock,
32+
};
4733
});
4834

49-
const spawnMock = vi.mocked(spawn);
35+
const spawnMocked = vi.mocked(spawn);
36+
const tempDirs: string[] = [];
5037

5138
let runSshSandboxCommand: typeof import("./ssh.js").runSshSandboxCommand;
39+
let uploadDirectoryToSshTarget: typeof import("./ssh.js").uploadDirectoryToSshTarget;
5240

53-
function fakeSession() {
41+
beforeEach(async () => {
42+
vi.resetModules();
43+
vi.clearAllMocks();
44+
({ runSshSandboxCommand, uploadDirectoryToSshTarget } = await import("./ssh.js"));
45+
});
46+
47+
afterEach(async () => {
48+
await Promise.all(
49+
tempDirs.splice(0).map(async (dir) => {
50+
await fs.rm(dir, { recursive: true, force: true });
51+
}),
52+
);
53+
});
54+
55+
function fakeSession(): import("./ssh.js").SshSandboxSession {
5456
return {
57+
command: "ssh",
5558
configPath: "/tmp/ssh-config",
5659
host: "host",
57-
user: "user",
58-
port: 22,
59-
privateKeyPath: "/tmp/key",
60-
remoteHost: "remote",
61-
} as unknown as import("./ssh.js").SshSandboxSession;
60+
};
6261
}
6362

64-
describe("ssh sandbox stream errors", () => {
65-
beforeAll(async () => {
66-
({ runSshSandboxCommand } = await import("./ssh.js"));
67-
});
68-
69-
it("rejects when stdout emits an error", async () => {
70-
let capturedChild: MockSpawnChild | undefined;
71-
spawnMock.mockImplementationOnce(
72-
(_command: string, _args: readonly string[], _options: SpawnOptions): ChildProcess => {
73-
const { child, stdout } = createMockSpawnChild();
74-
capturedChild = child;
75-
process.nextTick(() => {
76-
stdout?.emit("error", new Error("stdout read failed"));
77-
});
78-
return child as unknown as ChildProcess;
79-
},
80-
);
81-
82-
await expect(
83-
runSshSandboxCommand({
84-
session: fakeSession(),
85-
remoteCommand: "echo hi",
86-
allowFailure: false,
87-
}),
88-
).rejects.toThrow("stdout read failed");
89-
expect(capturedChild?.kill).toHaveBeenCalled();
90-
});
91-
92-
it("rejects when stderr emits an error", async () => {
93-
let capturedChild: MockSpawnChild | undefined;
94-
spawnMock.mockImplementationOnce(
95-
(_command: string, _args: readonly string[], _options: SpawnOptions): ChildProcess => {
96-
const { child, stderr } = createMockSpawnChild();
97-
capturedChild = child;
98-
process.nextTick(() => {
99-
stderr?.emit("error", new Error("stderr read failed"));
100-
});
101-
return child as unknown as ChildProcess;
102-
},
103-
);
104-
105-
await expect(
106-
runSshSandboxCommand({
63+
describe("SSH sandbox stream errors", () => {
64+
it.each(["stdout", "stderr", "stdin"] as const)(
65+
"rejects and terminates once when command %s fails",
66+
async (streamName) => {
67+
const child = createMockChildProcess();
68+
spawnMocked.mockReturnValueOnce(child as unknown as ChildProcess);
69+
const expected = `${streamName} failed`;
70+
const result = runSshSandboxCommand({
10771
session: fakeSession(),
10872
remoteCommand: "echo hi",
109-
allowFailure: false,
110-
}),
111-
).rejects.toThrow("stderr read failed");
112-
expect(capturedChild?.kill).toHaveBeenCalled();
113-
});
114-
115-
it("rejects when stdin emits an error", async () => {
116-
let capturedChild: MockSpawnChild | undefined;
117-
spawnMock.mockImplementationOnce(
118-
(_command: string, _args: readonly string[], _options: SpawnOptions): ChildProcess => {
119-
const { child, stdin } = createMockSpawnChild();
120-
capturedChild = child;
121-
process.nextTick(() => {
122-
stdin?.emit("error", new Error("stdin write failed"));
123-
});
124-
return child as unknown as ChildProcess;
125-
},
126-
);
127-
128-
await expect(
129-
runSshSandboxCommand({
73+
});
74+
75+
child[streamName].emit("error", new Error(expected));
76+
77+
await expect(result).rejects.toThrow(expected);
78+
expect(child.kill).toHaveBeenCalledExactlyOnceWith("SIGKILL");
79+
80+
child.emit("close", 0);
81+
child[streamName].emit("error", new Error("late stream error"));
82+
expect(child.kill).toHaveBeenCalledOnce();
83+
},
84+
);
85+
86+
it.each(["tar.stdout", "tar.stderr", "ssh.stdin", "ssh.stdout", "ssh.stderr"] as const)(
87+
"rejects and terminates both upload children once when %s fails",
88+
async (stream) => {
89+
const localDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-ssh-stream-test-"));
90+
tempDirs.push(localDir);
91+
const tar = createMockChildProcess();
92+
const ssh = createMockChildProcess();
93+
spawnMocked
94+
.mockReturnValueOnce(tar as unknown as ChildProcess)
95+
.mockReturnValueOnce(ssh as unknown as ChildProcess);
96+
const expected = `${stream} failed`;
97+
const result = uploadDirectoryToSshTarget({
13098
session: fakeSession(),
131-
remoteCommand: "echo hi",
132-
allowFailure: false,
133-
}),
134-
).rejects.toThrow("stdin write failed");
135-
expect(capturedChild?.kill).toHaveBeenCalled();
136-
});
99+
localDir,
100+
remoteDir: "/remote/workspace",
101+
});
102+
const rejection = expect(result).rejects.toThrow(expected);
103+
await vi.waitFor(() => expect(spawnMocked).toHaveBeenCalledTimes(2));
104+
const [childName, streamName] = stream.split(".") as ["tar" | "ssh", keyof MockChildProcess];
105+
const failedStream = { tar, ssh }[childName][streamName] as PassThrough;
106+
107+
failedStream.emit("error", new Error(expected));
108+
109+
await rejection;
110+
expect(tar.kill).toHaveBeenCalledExactlyOnceWith("SIGKILL");
111+
expect(ssh.kill).toHaveBeenCalledExactlyOnceWith("SIGKILL");
112+
113+
tar.emit("close", 0);
114+
ssh.emit("close", 0);
115+
failedStream.emit("error", new Error("late stream error"));
116+
expect(tar.kill).toHaveBeenCalledOnce();
117+
expect(ssh.kill).toHaveBeenCalledOnce();
118+
},
119+
);
137120
});

0 commit comments

Comments
 (0)