Skip to content

Commit bd9acd0

Browse files
Alix-007Alix-007
committed
fixup: keep codex supervisor stderr diagnostic
1 parent 7351faa commit bd9acd0

2 files changed

Lines changed: 150 additions & 58 deletions

File tree

‎extensions/codex-supervisor/src/json-rpc-client.test.ts‎

Lines changed: 147 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,28 @@ function createCodexStdioProcess() {
3232
return proc;
3333
}
3434

35+
function respondToJsonRpcRequests(proc: ReturnType<typeof createCodexStdioProcess>) {
36+
let buffer = "";
37+
proc.stdin.on("data", (chunk: Buffer | string) => {
38+
buffer += chunk.toString();
39+
for (;;) {
40+
const newline = buffer.indexOf("\n");
41+
if (newline < 0) {
42+
return;
43+
}
44+
const line = buffer.slice(0, newline).trim();
45+
buffer = buffer.slice(newline + 1);
46+
if (!line) {
47+
continue;
48+
}
49+
const request = JSON.parse(line) as { id?: unknown };
50+
if (request.id !== undefined) {
51+
proc.stdout.write(`${JSON.stringify({ id: request.id, result: {} })}\n`);
52+
}
53+
}
54+
});
55+
}
56+
3557
function waitForChildClose(proc: ChildProcessWithoutNullStreams) {
3658
return new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve, reject) => {
3759
const timer = setTimeout(() => {
@@ -76,73 +98,142 @@ describe("connectCodexAppServerEndpoint stdio transport", () => {
7698
spawnMock.mockReset();
7799
});
78100

79-
it("rejects initialization when stdio readable streams error", async () => {
80-
for (const streamName of ["stdout", "stderr"] as const) {
81-
const proc = createCodexStdioProcess();
82-
spawnMock.mockReturnValueOnce(proc);
101+
it("rejects initialization when the stdout JSON-RPC stream errors", async () => {
102+
const proc = createCodexStdioProcess();
103+
spawnMock.mockReturnValueOnce(proc);
104+
105+
const connectPromise = connectCodexAppServerEndpoint({
106+
id: "local",
107+
transport: "stdio-proxy",
108+
command: "codex",
109+
args: ["app-server", "--listen", "stdio://"],
110+
});
111+
112+
expect(() => proc.stdout.emit("error", new Error("EPIPE"))).not.toThrow();
113+
await expect(connectPromise).rejects.toThrow("EPIPE");
114+
expect(proc.kill).toHaveBeenCalledWith("SIGTERM");
115+
});
116+
117+
it("keeps stderr stream errors diagnostic-only during initialization", async () => {
118+
const proc = createCodexStdioProcess();
119+
respondToJsonRpcRequests(proc);
120+
spawnMock.mockReturnValueOnce(proc);
121+
122+
const connectPromise = connectCodexAppServerEndpoint({
123+
id: "local",
124+
transport: "stdio-proxy",
125+
command: "codex",
126+
args: ["app-server", "--listen", "stdio://"],
127+
});
128+
129+
expect(() => proc.stderr.emit("error", new Error("EPIPE"))).not.toThrow();
130+
const connection = await connectPromise;
131+
132+
expect(proc.kill).not.toHaveBeenCalled();
133+
await connection.close();
134+
expect(proc.kill).toHaveBeenCalledWith("SIGTERM");
135+
});
136+
137+
it("routes real child stdout stream failures through connection failure and shutdown", async () => {
138+
const actual = await vi.importActual<typeof import("node:child_process")>("node:child_process");
139+
140+
let proc: ChildProcessWithoutNullStreams | undefined;
141+
let killSpy: MockInstance<ChildKill> | undefined;
142+
try {
143+
spawnMock.mockImplementationOnce(
144+
(command: string, args: string[] | undefined, options: SpawnOptions) => {
145+
proc = actual.spawn(command, args ?? [], options) as ChildProcessWithoutNullStreams;
146+
return proc;
147+
},
148+
);
83149

84150
const connectPromise = connectCodexAppServerEndpoint({
85151
id: "local",
86152
transport: "stdio-proxy",
87-
command: "codex",
88-
args: ["app-server", "--listen", "stdio://"],
153+
command: process.execPath,
154+
args: [
155+
"-e",
156+
[
157+
"process.stdin.resume();",
158+
'process.on("SIGTERM", () => process.exit(143));',
159+
'process.stderr.write("ready\\n");',
160+
"setInterval(() => {}, 1000);",
161+
].join(""),
162+
],
89163
});
90164

91-
expect(() => proc[streamName].emit("error", new Error("EPIPE"))).not.toThrow();
92-
await expect(connectPromise).rejects.toThrow("EPIPE");
93-
expect(proc.kill).toHaveBeenCalledWith("SIGTERM");
165+
if (!proc) {
166+
throw new Error("expected stdio transport to spawn a child process");
167+
}
168+
expect(proc.pid).toEqual(expect.any(Number));
169+
killSpy = vi.spyOn(proc, "kill");
170+
const closePromise = waitForChildClose(proc);
171+
await waitForReadableData(proc.stderr);
172+
const message = "synthetic parent stdout read failure";
173+
174+
proc.stdout.destroy(new Error(message));
175+
176+
await expect(connectPromise).rejects.toThrow(message);
177+
expect(killSpy).toHaveBeenCalledTimes(1);
178+
expect(killSpy).toHaveBeenCalledWith("SIGTERM");
179+
await expect(closePromise).resolves.toStrictEqual({ code: 143, signal: null });
180+
} finally {
181+
killSpy?.mockRestore();
182+
if (proc && proc.exitCode === null && !proc.killed) {
183+
proc.kill("SIGKILL");
184+
}
94185
}
95186
});
96187

97-
it("routes real child readable stream failures through connection failure and shutdown", async () => {
188+
it("keeps real child stderr stream failures diagnostic-only", async () => {
98189
const actual = await vi.importActual<typeof import("node:child_process")>("node:child_process");
99190

100-
for (const streamName of ["stdout", "stderr"] as const) {
101-
let proc: ChildProcessWithoutNullStreams | undefined;
102-
let killSpy: MockInstance<ChildKill> | undefined;
103-
try {
104-
spawnMock.mockImplementationOnce(
105-
(command: string, args: string[] | undefined, options: SpawnOptions) => {
106-
proc = actual.spawn(command, args ?? [], options) as ChildProcessWithoutNullStreams;
107-
return proc;
108-
},
109-
);
110-
111-
const connectPromise = connectCodexAppServerEndpoint({
112-
id: "local",
113-
transport: "stdio-proxy",
114-
command: process.execPath,
115-
args: [
116-
"-e",
117-
[
118-
"process.stdin.resume();",
119-
'process.on("SIGTERM", () => process.exit(143));',
120-
'process.stderr.write("ready\\n");',
121-
"setInterval(() => {}, 1000);",
122-
].join(""),
123-
],
124-
});
125-
126-
if (!proc) {
127-
throw new Error("expected stdio transport to spawn a child process");
128-
}
129-
expect(proc.pid).toEqual(expect.any(Number));
130-
killSpy = vi.spyOn(proc, "kill");
131-
const closePromise = waitForChildClose(proc);
132-
await waitForReadableData(proc.stderr);
133-
const message = `synthetic parent ${streamName} read failure`;
134-
135-
proc[streamName].destroy(new Error(message));
136-
137-
await expect(connectPromise).rejects.toThrow(message);
138-
expect(killSpy).toHaveBeenCalledTimes(1);
139-
expect(killSpy).toHaveBeenCalledWith("SIGTERM");
140-
await expect(closePromise).resolves.toStrictEqual({ code: 143, signal: null });
141-
} finally {
142-
killSpy?.mockRestore();
143-
if (proc && proc.exitCode === null && !proc.killed) {
144-
proc.kill("SIGKILL");
145-
}
191+
let proc: ChildProcessWithoutNullStreams | undefined;
192+
let killSpy: MockInstance<ChildKill> | undefined;
193+
let connection: Awaited<ReturnType<typeof connectCodexAppServerEndpoint>> | undefined;
194+
try {
195+
spawnMock.mockImplementationOnce(
196+
(command: string, args: string[] | undefined, options: SpawnOptions) => {
197+
proc = actual.spawn(command, args ?? [], options) as ChildProcessWithoutNullStreams;
198+
return proc;
199+
},
200+
);
201+
202+
const connectPromise = connectCodexAppServerEndpoint({
203+
id: "local",
204+
transport: "stdio-proxy",
205+
command: process.execPath,
206+
args: [
207+
"-e",
208+
[
209+
'const readline = require("node:readline");',
210+
'process.on("SIGTERM", () => process.exit(143));',
211+
'process.stderr.write("ready\\n");',
212+
'readline.createInterface({ input: process.stdin }).on("line", (line) => {',
213+
" const request = JSON.parse(line);",
214+
" if (request.id !== undefined) {",
215+
' process.stdout.write(`${JSON.stringify({ id: request.id, result: {} })}\\n`);',
216+
" }",
217+
"});",
218+
"setInterval(() => {}, 1000);",
219+
].join(""),
220+
],
221+
});
222+
223+
if (!proc) {
224+
throw new Error("expected stdio transport to spawn a child process");
225+
}
226+
killSpy = vi.spyOn(proc, "kill");
227+
await waitForReadableData(proc.stderr);
228+
proc.stderr.destroy(new Error("synthetic parent stderr read failure"));
229+
230+
connection = await connectPromise;
231+
expect(killSpy).not.toHaveBeenCalled();
232+
} finally {
233+
await connection?.close().catch(() => undefined);
234+
killSpy?.mockRestore();
235+
if (proc && proc.exitCode === null && !proc.killed) {
236+
proc.kill("SIGKILL");
146237
}
147238
}
148239
});

‎extensions/codex-supervisor/src/json-rpc-client.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -199,9 +199,10 @@ class StdioCodexJsonRpcConnection extends BaseCodexJsonRpcConnection {
199199
this.fail(error);
200200
void this.close();
201201
});
202+
// Codex JSON-RPC is stdout-only; stderr is diagnostics and must not tear down the session.
202203
this.proc.stderr.once("error", (error) => {
203-
this.fail(error);
204-
void this.close();
204+
this.stderrTail.push(`stderr stream error: ${error.message}`);
205+
this.stderrTail.splice(0, Math.max(0, this.stderrTail.length - 40));
205206
});
206207
this.proc.stdin.once("error", (error) => this.fail(error));
207208
this.proc.once("error", (error) => this.fail(error));

0 commit comments

Comments
 (0)