diff --git a/README.md b/README.md index b6229b1b9..862064378 100644 --- a/README.md +++ b/README.md @@ -423,6 +423,10 @@ Broker and background-job lifecycle: | [#725](https://github.com/openai/codex-plugin-cc/pull/725) | nothing is spawned through the user's shell on Windows, where MSYS path conversion mangled switches like `taskkill /PID` and left background workers unkillable under Git Bash (this supersedes [#735](https://github.com/openai/codex-plugin-cc/pull/735)) | | [#656](https://github.com/openai/codex-plugin-cc/pull/656) | `/codex:cancel` exits non-zero when neither the turn interrupt nor the worker kill confirmed the job stopped, instead of reporting a cancellation nothing proved | | [#770](https://github.com/openai/codex-plugin-cc/pull/770) | `npm run build` works on Windows: npm runs scripts through cmd.exe, where `mkdir -p` made a stray `-p` directory and then failed every build after the first | +| [#775](https://github.com/openai/codex-plugin-cc/pull/775) | a `fileChange` start event without its change list no longer throws and aborts the turn | +| [#772](https://github.com/openai/codex-plugin-cc/pull/772) | the stop-review gate keeps a minute of headroom under the Stop hook's budget, so a timed-out review can still say so instead of ending the turn silently | +| [#774](https://github.com/openai/codex-plugin-cc/pull/774) | `status --wait` prints its timeout and exits non-zero, instead of looking like a finished status check | +| [#773](https://github.com/openai/codex-plugin-cc/pull/773) | a broker connect that never completes is given up on after 2s and falls back to a direct app-server (the probe half of that PR is not taken: ours already bounds each attempt *and* reports why it failed) | Commands and flags: diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index e4760f568..871243be3 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -1113,7 +1113,17 @@ async function handleStatus(argv) { pollIntervalMs: options["poll-interval-ms"] }) : buildSingleJobSnapshot(cwd, reference); - outputCommandResult(snapshot, renderJobStatusReport(snapshot.job), options.json); + if (snapshot.waitTimedOut) { + process.exitCode = 1; + } + outputCommandResult( + snapshot, + renderJobStatusReport(snapshot.job, { + waitTimedOut: snapshot.waitTimedOut, + timeoutMs: snapshot.timeoutMs + }), + options.json + ); return; } diff --git a/plugins/codex/scripts/lib/app-server.mjs b/plugins/codex/scripts/lib/app-server.mjs index 91961e0ee..ea37d92a5 100644 --- a/plugins/codex/scripts/lib/app-server.mjs +++ b/plugins/codex/scripts/lib/app-server.mjs @@ -334,6 +334,23 @@ class SpawnedCodexAppServerClient extends AppServerClientBase { } } +/** + * How long a client waits for the broker's socket to accept it before giving up. + * + * Without a bound this connect waits forever on an endpoint that never completes — a wedged + * broker, or a named pipe whose server is not accepting — and takes the command with it. The + * timeout is short because it is not a patience budget: a broker that is merely refusing is + * handled a level up, in ensureBrokerSession(), which waits for it to start accepting rather than + * replacing it. Reaching this timer means the endpoint never answered at all, and `withAppServer` + * then falls back to a direct app-server rather than leaving the caller stuck. + */ +export const BROKER_CONNECT_TIMEOUT_MS = 2000; + +export function resolveBrokerConnectTimeoutMs(options = {}) { + const requested = Number(options.connectTimeoutMs); + return Number.isFinite(requested) && requested > 0 ? requested : BROKER_CONNECT_TIMEOUT_MS; +} + class BrokerCodexAppServerClient extends AppServerClientBase { constructor(cwd, options = {}) { super(cwd, options); @@ -344,16 +361,44 @@ class BrokerCodexAppServerClient extends AppServerClientBase { async initialize() { await new Promise((resolve, reject) => { const target = parseBrokerEndpoint(this.endpoint); - this.socket = net.createConnection({ path: target.path }); + const createConnection = this.options.createConnection ?? ((path) => net.createConnection({ path })); + this.socket = createConnection(target.path); this.socket.setEncoding("utf8"); - this.socket.on("connect", resolve); + // One settle path, so the deadline timer cannot outlive the attempt: a timer left armed + // after the connect resolved or failed holds the event loop open for its whole budget, + // which on a short-lived command is the difference between exiting now and exiting in 2s. + let timer = null; + let settled = false; + const settle = (action) => { + if (settled) { + return; + } + settled = true; + clearTimeout(timer); + action(); + }; + timer = setTimeout(() => { + settle(() => { + this.socket.destroy(); + reject( + Object.assign(new Error("Timed out connecting to the Codex app-server broker."), { + code: "ETIMEDOUT" + }) + ); + }); + }, resolveBrokerConnectTimeoutMs(this.options)); + this.socket.on("connect", () => settle(resolve)); this.socket.on("data", (chunk) => { this.handleChunk(chunk); }); this.socket.on("error", (error) => { - if (!this.exitResolved) { - reject(error); - } + settle(() => { + if (!this.exitResolved) { + reject(error); + } + }); + // Reported whether or not this attempt had already settled: an error after connect is + // the transport dying, which the client's exit bookkeeping still has to see. this.handleExit(error); }); this.socket.on("close", () => { diff --git a/plugins/codex/scripts/lib/codex.mjs b/plugins/codex/scripts/lib/codex.mjs index 37380bc28..1224bda66 100644 --- a/plugins/codex/scripts/lib/codex.mjs +++ b/plugins/codex/scripts/lib/codex.mjs @@ -350,7 +350,10 @@ function describeStartedItem(state, item) { phase: looksLikeVerificationCommand(item.command) ? "verifying" : "running" }; case "fileChange": - return { message: `Applying ${item.changes.length} file change(s).`, phase: "editing" }; + return { + message: `Applying ${Array.isArray(item.changes) ? item.changes.length : 0} file change(s).`, + phase: "editing" + }; case "mcpToolCall": return { message: `Calling ${item.server}/${item.tool}.`, phase: "investigating" }; case "dynamicToolCall": @@ -753,7 +756,8 @@ async function withAppServer(cwd, fn) { const brokerRequested = client?.transport === "broker" || Boolean(process.env[BROKER_ENDPOINT_ENV]); const shouldRetryDirect = (client?.transport === "broker" && error?.rpcCode === BROKER_BUSY_RPC_CODE) || - (brokerRequested && (error?.code === "ENOENT" || error?.code === "ECONNREFUSED")); + (brokerRequested && + (error?.code === "ENOENT" || error?.code === "ECONNREFUSED" || error?.code === "ETIMEDOUT")); if (client) { await client.close().catch(() => {}); diff --git a/plugins/codex/scripts/lib/render.mjs b/plugins/codex/scripts/lib/render.mjs index cac73b9d0..93b9a2d3b 100644 --- a/plugins/codex/scripts/lib/render.mjs +++ b/plugins/codex/scripts/lib/render.mjs @@ -374,8 +374,13 @@ export function renderStatusReport(report) { return `${lines.join("\n").trimEnd()}\n`; } -export function renderJobStatusReport(job) { +export function renderJobStatusReport(job, options = {}) { const lines = ["# Codex Job Status", ""]; + if (options.waitTimedOut) { + const timeoutSeconds = Math.max(1, Math.round((Number(options.timeoutMs) || 0) / 1000)); + lines.push(`Timed out after ${timeoutSeconds}s while the job was still ${job.status || "active"}.`); + lines.push(""); + } pushJobDetails(lines, job, { showElapsed: job.status === "queued" || job.status === "running", showDuration: job.status !== "queued" && job.status !== "running", diff --git a/plugins/codex/scripts/stop-review-gate-hook.mjs b/plugins/codex/scripts/stop-review-gate-hook.mjs index e2d53b9c7..637d6b56f 100644 --- a/plugins/codex/scripts/stop-review-gate-hook.mjs +++ b/plugins/codex/scripts/stop-review-gate-hook.mjs @@ -13,7 +13,7 @@ import { reconcileJobsLiveness, sortJobsNewestFirst } from "./lib/job-control.mj import { SESSION_ID_ENV } from "./lib/tracked-jobs.mjs"; import { resolveWorkspaceRoot } from "./lib/workspace.mjs"; -const STOP_REVIEW_TIMEOUT_MS = 15 * 60 * 1000; +const STOP_REVIEW_TIMEOUT_MS = 14 * 60 * 1000; const SCRIPT_DIR = path.dirname(fileURLToPath(import.meta.url)); const ROOT_DIR = path.resolve(SCRIPT_DIR, ".."); const STOP_REVIEW_TASK_MARKER = "Run a stop-gate review of the previous Claude turn."; @@ -113,7 +113,7 @@ function runStopReview(cwd, input = {}) { return { ok: false, reason: - "The stop-time Codex review task timed out after 15 minutes. Run /codex:review --wait manually or bypass the gate." + "The stop-time Codex review task timed out after 14 minutes. Run /codex:review --wait manually or bypass the gate." }; } diff --git a/tests/app-server-connect.test.mjs b/tests/app-server-connect.test.mjs index a3c730647..73434c1ff 100644 --- a/tests/app-server-connect.test.mjs +++ b/tests/app-server-connect.test.mjs @@ -1,7 +1,11 @@ import test from "node:test"; import assert from "node:assert/strict"; -import { CodexAppServerClient } from "../plugins/codex/scripts/lib/app-server.mjs"; +import { + BROKER_CONNECT_TIMEOUT_MS, + CodexAppServerClient, + resolveBrokerConnectTimeoutMs +} from "../plugins/codex/scripts/lib/app-server.mjs"; /** Reject rather than hang, so a wedged connect fails the test instead of stalling the run. */ function withDeadline(promise, ms, label) { @@ -38,3 +42,80 @@ test("connect reports an empty broker endpoint instead of hanging", async () => /missing its path/ ); }); + +// A socket that is handed out but never connects, errors or closes: what a wedged endpoint looks +// like from the client's side. Everything the client attaches to it is a no-op, so only the +// connect deadline can end the attempt. +function neverSettlingSocket() { + const handlers = new Map(); + const socket = { + destroyed: false, + handlers, + setEncoding() {}, + setTimeout() {}, + on(event, handler) { + handlers.set(event, handler); + return socket; + }, + write() {}, + end() { + socket.destroy(); + }, + destroy() { + if (socket.destroyed) { + return; + } + socket.destroyed = true; + // A real socket reports its own teardown, and the client's cleanup waits for that report: + // a stub that stays silent here would hang the close path instead of the connect path. + queueMicrotask(() => handlers.get("close")?.()); + } + }; + return socket; +} + +test("a broker connect that never settles is given up on, not waited on forever", async () => { + const socket = neverSettlingSocket(); + await assert.rejects( + () => + withDeadline( + CodexAppServerClient.connect(process.cwd(), { + brokerEndpoint: "unix:/tmp/codex-never-accepts.sock", + connectTimeoutMs: 50, + createConnection: () => socket + }), + 5000, + "connect" + ), + (error) => { + assert.equal(error.code, "ETIMEDOUT"); + assert.match(error.message, /Timed out connecting to the Codex app-server broker/); + return true; + } + ); + assert.equal(socket.destroyed, true, "the abandoned socket must not be left open"); +}); + +test("a broker connect error is reported as itself, not as a timeout", async () => { + // The deadline timer must not outlive the failure: the caller distinguishes ECONNREFUSED (fall + // back to a direct app-server) from a timeout, and a masked code sends it down the wrong path. + const socket = neverSettlingSocket(); + const connecting = CodexAppServerClient.connect(process.cwd(), { + brokerEndpoint: "unix:/tmp/codex-refused.sock", + connectTimeoutMs: 50_000, + createConnection: () => socket + }); + const refused = Object.assign(new Error("connect ECONNREFUSED"), { code: "ECONNREFUSED" }); + socket.handlers.get("error")?.(refused); + + await assert.rejects(() => withDeadline(connecting, 5000, "connect"), /ECONNREFUSED/); +}); + +test("the broker connect budget defaults to 2s and honours an override", () => { + assert.equal(resolveBrokerConnectTimeoutMs({}), BROKER_CONNECT_TIMEOUT_MS); + assert.equal(BROKER_CONNECT_TIMEOUT_MS, 2000); + assert.equal(resolveBrokerConnectTimeoutMs({ connectTimeoutMs: 75 }), 75); + for (const unusable of [0, -1, "soon", null, undefined, Number.NaN]) { + assert.equal(resolveBrokerConnectTimeoutMs({ connectTimeoutMs: unusable }), BROKER_CONNECT_TIMEOUT_MS); + } +}); diff --git a/tests/commands.test.mjs b/tests/commands.test.mjs index 162ddbd92..89755ff36 100644 --- a/tests/commands.test.mjs +++ b/tests/commands.test.mjs @@ -256,6 +256,27 @@ test("hooks keep session-end cleanup and stop gating enabled", () => { assert.match(source, /session-lifecycle-hook\.mjs/); }); +test("stop-review task timeout leaves headroom under the Stop hook timeout", () => { + const hookSource = read("scripts/stop-review-gate-hook.mjs"); + const hooks = JSON.parse(read("hooks/hooks.json")); + const stopHook = hooks.hooks.Stop.flatMap((entry) => entry.hooks).find((hook) => + String(hook.command ?? "").includes("stop-review-gate-hook.mjs") + ); + const hookTimeoutMs = Number(stopHook?.timeout) * 1000; + const timeoutMatch = hookSource.match(/const STOP_REVIEW_TIMEOUT_MS = ([^;]+);/); + assert.ok(timeoutMatch, "STOP_REVIEW_TIMEOUT_MS should be declared"); + assert.match(timeoutMatch[1], /^[\d\s.*+\-/()]+$/); + const innerTimeoutMs = new Function(`return (${timeoutMatch[1]});`)(); + const timeoutMessageMatch = hookSource.match(/timed out after ([0-9]+) minutes/); + + assert.equal(Number.isFinite(hookTimeoutMs), true); + assert.equal(hookTimeoutMs, 900000); + assert.equal(Number.isFinite(innerTimeoutMs), true); + assert.equal(innerTimeoutMs < hookTimeoutMs, true); + assert.equal(hookTimeoutMs - innerTimeoutMs >= 30000, true); + assert.equal(Number(timeoutMessageMatch?.[1]) * 60 * 1000, innerTimeoutMs); +}); + test("setup command can offer Codex install and still points users to codex login", () => { const setup = read("commands/setup.md"); const readme = fs.readFileSync(path.join(ROOT, "README.md"), "utf8"); diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index 24b155e93..b63d33076 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -759,6 +759,13 @@ rl.on("line", (line) => { } const items = [ + ...(BEHAVIOR === "file-change-started-without-changes" + ? [ + { + started: { type: "fileChange", id: "fc_" + turnId } + } + ] + : []), ...(BEHAVIOR === "with-reasoning" ? [ { diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 1c0e34276..b26579775 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -176,6 +176,25 @@ test("task runs when the active provider does not require OpenAI login", () => { assert.match(result.stdout, /Handled the requested task/); }); +test("task survives fileChange started items that omit changes", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir, "file-change-started-without-changes"); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + + const result = run("node", [SCRIPT, "task", "apply the patch"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /Handled the requested task/); + assert.doesNotMatch(result.stderr, /Cannot read properties of undefined/); +}); + test("task runs without auth preflight so Codex can refresh an expired session", () => { const repo = makeTempDir(); const binDir = makeTempDir(); @@ -2098,13 +2117,70 @@ test("status --wait times out cleanly when a job is still active", () => { cwd: workspace }); - assert.equal(result.status, 0, result.stderr); + assert.equal(result.status > 0, true, result.stderr); const payload = JSON.parse(result.stdout); assert.equal(payload.job.id, "task-live"); assert.equal(payload.job.status, "running"); assert.equal(payload.waitTimedOut, true); }); +test("status --wait reports a timeout in text output when the job is still active", () => { + const workspace = makeTempDir(); + const stateDir = resolveStateDir(workspace); + const jobsDir = path.join(stateDir, "jobs"); + fs.mkdirSync(jobsDir, { recursive: true }); + + const logFile = path.join(jobsDir, "task-live.log"); + fs.writeFileSync(logFile, "[2026-03-18T15:30:00.000Z] Starting Codex Task.\n", "utf8"); + fs.writeFileSync( + path.join(jobsDir, "task-live.json"), + JSON.stringify( + { + id: "task-live", + status: "running", + title: "Codex Task", + logFile + }, + null, + 2 + ), + "utf8" + ); + + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: "task-live", + status: "running", + title: "Codex Task", + jobClass: "task", + summary: "Investigate flaky test", + logFile, + createdAt: "2026-03-18T15:30:00.000Z", + startedAt: "2026-03-18T15:30:01.000Z", + updatedAt: "2026-03-18T15:30:02.000Z" + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + + const result = run("node", [SCRIPT, "status", "task-live", "--wait", "--timeout-ms", "25"], { + cwd: workspace + }); + + assert.match(result.stdout, /Timed out after \d+s while the job was still running/i); + assert.equal(result.status > 0, true); +}); + test("result returns the stored output for the latest finished job by default", () => { const workspace = makeTempDir(); const stateDir = resolveStateDir(workspace);