diff --git a/apps/connector/src/acp.test.ts b/apps/connector/src/acp.test.ts index 60a7c27ca..de41d5f9d 100644 --- a/apps/connector/src/acp.test.ts +++ b/apps/connector/src/acp.test.ts @@ -155,3 +155,23 @@ test.skipIf(process.platform === "win32")( expect(starts).toEqual([{ how: "exit 127", stderr: "agent: no credentials" }, undefined]); }, ); + +test.skipIf(process.platform === "win32")( + "an agent that exits before its stderr reaches us still reports what it printed", + async () => { + let starts: unknown[] = []; + // The agent exits at once; a process it left behind prints after the exit. + let failed = await runAgent({ + command: ["/bin/sh", "-c", "(sleep 0.3; echo 'agent: no credentials' >&2) & exit 127"], + cwd: process.cwd(), + prompt: "Investigate", + mcpServers: [], + signal: new AbortController().signal, + onUpdate: () => {}, + permission: async () => undefined, + onStart: (failure?: unknown) => void starts.push(failure), + }).catch(error => error); + expect(failed).toBeInstanceOf(Error); + expect(starts).toEqual([{ how: "exit 127", stderr: "agent: no credentials" }]); + }, +); diff --git a/apps/connector/src/acp.ts b/apps/connector/src/acp.ts index c4ba677d3..4b4f32881 100644 --- a/apps/connector/src/acp.ts +++ b/apps/connector/src/acp.ts @@ -85,6 +85,11 @@ function startAgent( (code, signal) => resolve(ended ??= signal ? `signal ${signal}` : `exit ${code}`), ); }); + // "exit" can come before the last of its stderr is read; "close" follows once that stream ends. + let closed = new Promise(resolve => { + child.once("error", resolve); + child.once("close", resolve); + }); let connection = app.connect(ndJsonStream( Writable.toWeb(child.stdin), Readable.toWeb(child.stdout) as unknown as ReadableStream, @@ -95,8 +100,12 @@ function startAgent( connection, kill, async failure(error: unknown): Promise { - // Its exit, and the rest of its stderr, may come just after the failure they caused. - await Promise.race([exited, new Promise(resolve => setTimeout(resolve, 1000))]); + // Its exit, and the rest of its stderr, may come just after the failure they caused. A + // process it left holding stderr open is not waited on past a second. + await Promise.race([ + exited.then(() => closed), + new Promise(resolve => setTimeout(resolve, 1000)), + ]); let line = head.split("\n").map(text => text.trim()).find(Boolean); return { how: ended ?? (error instanceof Error ? error.message : String(error)), diff --git a/apps/connector/src/planner.test.ts b/apps/connector/src/planner.test.ts index 2fbcf38f0..93e5d0d04 100644 --- a/apps/connector/src/planner.test.ts +++ b/apps/connector/src/planner.test.ts @@ -581,15 +581,23 @@ test.skipIf(process.platform === "win32")( let directory = await mkdtemp(join(scratch, "long-line-")); let script = join(directory, "backend.ts"); // The first 8 characters of the credential end a 65,545-character piece without a newline; - // the rest comes after a pause, then a normal line. + // the rest comes after a pause, then a normal line. Only the first start prints it: the + // Planner starts again on a backoff that may be a few milliseconds, and a second start + // printing the line before the test stops it would be a second, legitimate drop. await writeFile( script, [ + "import { existsSync, writeFileSync } from 'node:fs';", + `let ran = ${JSON.stringify(join(directory, "ran"))};`, + "if (existsSync(ran)) setInterval(() => {}, 1000);", + "else {", + "writeFileSync(ran, '');", "let token = process.env.CHOPIN_PLANNER_TOKEN;", "process.stderr.write(`${'x'.repeat(65536)} ${token.slice(0, 8)}`, () => setTimeout(() => {", "\tprocess.stderr.write(`${token.slice(8)}\\nnext CHOPIN_PLANNER_TOKEN=${token}\\n`,", "\t\t() => process.exit(127));", "}, 300));", + "}", "", ].join("\n"), );