From 90b22710207b8e36f398cef375b25c31d866d136 Mon Sep 17 00:00:00 2001
From: Steve Books <stevebooks@gmail.com>
Date: Thu, 27 Aug 2026 21:39:43 -0600
Subject: [PATCH] Keep the inferred-completion timer referenced

The timer scheduled by scheduleInferredCompletion is the only thing that
resolves state.completion when a turn arrives without an explicit final
turn marker. Because it was unref'd it did not hold the event loop open,
so once the app-server socket closed the loop could drain with
`await state.completion` still pending. Node then exited 0 having written
nothing to stdout or stderr.

Callers that expect JSON on stdout cannot tell this apart from a real
result. The stop-review gate parses that empty stdout and reports "the
stop-time Codex review task returned invalid JSON", which surfaces to the
user as a failed review rather than a tooling failure, blocking the turn.

The timer is bounded at 250ms and is always cleared, so keeping it
referenced cannot delay or hang shutdown.
---
 plugins/codex/scripts/lib/codex.mjs |  7 ++-
 tests/inferred-completion.test.mjs  | 82 +++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+), 1 deletion(-)
 create mode 100644 tests/inferred-completion.test.mjs

diff --git a/plugins/codex/scripts/lib/codex.mjs b/plugins/codex/scripts/lib/codex.mjs
index fead00cc4..5a7d7a1f8 100644
--- a/plugins/codex/scripts/lib/codex.mjs
+++ b/plugins/codex/scripts/lib/codex.mjs
@@ -390,7 +390,12 @@ function scheduleInferredCompletion(state) {
     }
     completeTurn(state, null, { inferred: true });
   }, 250);
-  state.completionTimer.unref?.();
+  // Intentionally referenced: this timer is the only thing that resolves
+  // state.completion on the inferred-completion path. If it is unref'd it stops
+  // holding the event loop open, so once the app-server socket closes the loop
+  // can drain while `await state.completion` is still pending -- node then exits
+  // 0 without writing anything. The timer is bounded at 250ms and is always
+  // cleared, so keeping it referenced cannot delay or hang shutdown.
 }
 
 function belongsToTurn(state, message) {
diff --git a/tests/inferred-completion.test.mjs b/tests/inferred-completion.test.mjs
new file mode 100644
index 000000000..af2776f77
--- /dev/null
+++ b/tests/inferred-completion.test.mjs
@@ -0,0 +1,82 @@
+import test from "node:test";
+import assert from "node:assert/strict";
+import fs from "node:fs";
+import os from "node:os";
+import path from "node:path";
+import { spawnSync } from "node:child_process";
+import { fileURLToPath } from "node:url";
+
+const HERE = path.dirname(fileURLToPath(import.meta.url));
+const CODEX_LIB = path.join(HERE, "..", "plugins", "codex", "scripts", "lib", "codex.mjs");
+
+// The inferred-completion timer in scheduleInferredCompletion() is the only thing
+// that resolves `state.completion` when a turn arrives without an explicit final
+// turn marker. If that timer is unref'd it does not hold the event loop open, so
+// once the app-server socket closes the loop can drain with the completion promise
+// still pending. Node then exits 0 having written nothing at all -- and callers
+// that expect JSON on stdout (the stop-review gate) fail with a parse error that
+// looks like a review verdict rather than a tooling failure.
+//
+// scheduleInferredCompletion is not exported, so this asserts the observable
+// contract in a subprocess: a pending await whose sole remaining handle is that
+// timer must still produce output rather than exiting silently.
+function runCompletionHarness({ unref }) {
+  const dir = fs.mkdtempSync(path.join(os.tmpdir(), "inferred-completion-"));
+  const script = path.join(dir, "harness.mjs");
+
+  fs.writeFileSync(
+    script,
+    `
+let resolveCompletion;
+const completion = new Promise((resolve) => { resolveCompletion = resolve; });
+const timer = setTimeout(() => resolveCompletion("completed"), 250);
+${unref ? "timer.unref?.();" : ""}
+async function main() {
+  const result = await completion;
+  process.stdout.write(JSON.stringify({ rawOutput: result }) + "\\n");
+}
+main().catch((error) => {
+  process.stderr.write(String(error) + "\\n");
+  process.exitCode = 1;
+});
+`
+  );
+
+  try {
+    return spawnSync(process.execPath, [script], { encoding: "utf8", timeout: 30_000 });
+  } finally {
+    fs.rmSync(dir, { recursive: true, force: true });
+  }
+}
+
+test("an unref'd completion timer lets the process exit 0 with no output", () => {
+  const result = runCompletionHarness({ unref: true });
+
+  // This is the failure mode being guarded against: a clean exit code with an
+  // empty stdout is indistinguishable from success to a caller parsing JSON.
+  assert.equal(result.status, 0);
+  assert.equal(result.stdout, "");
+});
+
+test("a referenced completion timer resolves the turn and emits parseable output", () => {
+  const result = runCompletionHarness({ unref: false });
+
+  assert.equal(result.status, 0);
+  assert.notEqual(result.stdout.trim(), "", "expected the completion timer to produce output");
+  assert.equal(JSON.parse(result.stdout).rawOutput, "completed");
+});
+
+test("scheduleInferredCompletion does not unref the completion timer", () => {
+  const source = fs.readFileSync(CODEX_LIB, "utf8");
+  const start = source.indexOf("function scheduleInferredCompletion");
+  assert.notEqual(start, -1, "expected to find scheduleInferredCompletion in codex.mjs");
+
+  const end = source.indexOf("\nfunction ", start + 1);
+  const body = source.slice(start, end === -1 ? undefined : end);
+
+  assert.doesNotMatch(
+    body.replace(/^\s*\/\/.*$/gm, ""),
+    /completionTimer\.unref/,
+    "the inferred-completion timer must stay referenced so the turn can resolve before the event loop drains"
+  );
+});