fix(agent-codex): address follow-up review — retry after failed connect, binary guard recovery, lint fixes
- Reset closed flag after failed connect() so client can retry - Wrap binary resolve guard in try/finally to clear on rejection - Fix duplicate import and unused variable lint errors in tests - Add test: client retries connect after transient handshake failure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
4e7129d50d
commit
c0105476da
|
|
@ -12,8 +12,7 @@ vi.mock("node:child_process", () => ({
|
|||
spawn: mockSpawn,
|
||||
}));
|
||||
|
||||
import { CodexAppServerClient } from "./app-server-client.js";
|
||||
import type { ApprovalDecision } from "./app-server-client.js";
|
||||
import { CodexAppServerClient, type ApprovalDecision } from "./app-server-client.js";
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Helpers: fake child process
|
||||
|
|
@ -591,10 +590,6 @@ describe("CodexAppServerClient", () => {
|
|||
await new Promise((r) => setTimeout(r, 10));
|
||||
|
||||
// Should have auto-responded with accept
|
||||
const responses = parseStdinMessages(proc).filter(
|
||||
(m) => m["id"] === "0" || m["id"] === 0,
|
||||
);
|
||||
// Response should contain decision: "accept"
|
||||
const approvalResponse = proc.stdinLines.find((l) => l.includes('"accept"'));
|
||||
expect(approvalResponse).toBeDefined();
|
||||
|
||||
|
|
@ -951,6 +946,30 @@ describe("CodexAppServerClient", () => {
|
|||
expect(client.isConnected).toBe(false);
|
||||
});
|
||||
|
||||
it("allows retry after a failed connect (does not permanently close)", async () => {
|
||||
// First attempt: spawn a process that times out during handshake
|
||||
const proc1 = createFakeProcess();
|
||||
vi.spyOn(proc1, "kill").mockImplementation((_signal?: string) => {
|
||||
proc1.simulateExit(1, "SIGTERM");
|
||||
return true;
|
||||
});
|
||||
const client = new CodexAppServerClient({ requestTimeout: 200 });
|
||||
|
||||
await expect(client.connect()).rejects.toThrow("timed out");
|
||||
expect(client.isConnected).toBe(false);
|
||||
|
||||
// Second attempt should NOT throw "Client is closed" — it should be retryable
|
||||
const proc2 = createFakeProcess();
|
||||
const connectPromise = client.connect();
|
||||
await new Promise((r) => setTimeout(r, 10));
|
||||
const initReq = findRequest(proc2, "initialize");
|
||||
proc2.sendLine(JSON.stringify({ id: initReq!["id"], result: {} }));
|
||||
await connectPromise;
|
||||
|
||||
expect(client.isConnected).toBe(true);
|
||||
await closeClient(client, proc2);
|
||||
});
|
||||
|
||||
it("drains stderr without blocking the child process", async () => {
|
||||
const proc = createFakeProcess();
|
||||
const client = new CodexAppServerClient({ requestTimeout: 500 });
|
||||
|
|
|
|||
|
|
@ -209,6 +209,9 @@ export class CodexAppServerClient extends EventEmitter {
|
|||
} catch (err) {
|
||||
this.connecting = false;
|
||||
await this.close();
|
||||
// Reset closed flag so the client can retry connect() after a
|
||||
// transient handshake failure instead of being permanently unusable.
|
||||
this.closed = false;
|
||||
throw err;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -759,8 +759,11 @@ function createCodexAgent(): Agent {
|
|||
if (!resolvingBinary) {
|
||||
resolvingBinary = resolveCodexBinary();
|
||||
}
|
||||
resolvedBinary = await resolvingBinary;
|
||||
resolvingBinary = null;
|
||||
try {
|
||||
resolvedBinary = await resolvingBinary;
|
||||
} finally {
|
||||
resolvingBinary = null;
|
||||
}
|
||||
}
|
||||
if (!session.workspacePath) return;
|
||||
await setupCodexWorkspace(session.workspacePath);
|
||||
|
|
|
|||
Loading…
Reference in New Issue