From 0bea1c590d42e812ce19a60d875d782ca06f0bef Mon Sep 17 00:00:00 2001
From: Stefano Fiorini <stefano@fiorinis.com>
Date: Wed, 20 May 2026 14:17:28 -0500
Subject: [PATCH] =?UTF-8?q?fix:=20review=20feedback=20=E2=80=94=20signal?=
 =?UTF-8?q?=20handling,=20cancel=20race,=20stderr=20consistency?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Address issues found by code review:

1. Bug: timeout/signal-killed child reported as 'completed' with exit
   code 0 because close handler ignored the signal parameter. Now
   treats any signal termination as timed_out.

2. Bug: cancelled job gets overwritten by watcher on child exit. The
   watcher now re-reads the job file before writing and skips if the
   status has been changed to 'cancelled'.

3. Inconsistency: watcher path skipped stderr noise filtering. Added
   filterStderrNoise to the watcher (duplicated from execute.ts to
   keep the watcher self-contained).

4. getJobResult now guards against missing result field instead of
   using non-null assertion.
---
 tools/ai-cli-dispatch/src/job-watcher.ts | 35 +++++++++++++++++++++---
 tools/ai-cli-dispatch/src/jobs.ts        |  5 +++-
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/tools/ai-cli-dispatch/src/job-watcher.ts b/tools/ai-cli-dispatch/src/job-watcher.ts
index c1c4a99..b667a77 100644
--- a/tools/ai-cli-dispatch/src/job-watcher.ts
+++ b/tools/ai-cli-dispatch/src/job-watcher.ts
@@ -10,7 +10,29 @@
  */
 import { spawn } from "node:child_process";
 import { readFileSync, writeFileSync } from "node:fs";
-import type { JobRecord, ExecResult, JobStatus } from "./types.js";
+import type { JobRecord, ExecResult, JobStatus, ClientName } from "./types.js";
+
+// Must import CLIENT_ARGS to know the client command mapping
+// And the noise filter for consistent stderr handling
+
+/**
+ * Known stderr noise patterns per client (duplicated from execute.ts to keep
+ * the watcher self-contained with no runtime dependency on execute.ts).
+ */
+const STDERR_NOISE_PATTERNS: Partial<Record<ClientName, RegExp[]>> = {
+  codex: [
+    /^\d{4}-\d{2}-\d{2}T[\d:.]+Z\s+ERROR\s+codex_core::util:\s+ReasoningSummary\w*\s+/,
+  ],
+};
+
+function filterStderrNoise(client: ClientName, stderr: string, exitCode: number): string {
+  if (exitCode !== 0) return stderr;
+  const patterns = STDERR_NOISE_PATTERNS[client];
+  if (!patterns) return stderr;
+  const lines = stderr.split("\n");
+  const filtered = lines.filter((line) => !patterns.some((p) => p.test(line)));
+  return filtered.join("\n").replace(/\n+$/, "");
+}
 
 const jobFile = process.argv[2];
 const command = process.argv[3];
@@ -69,12 +91,17 @@ function finalize(status: JobStatus, result?: ExecResult, error?: string) {
     ...record,
     status,
     stdout,
-    stderr,
+    stderr: result ? filterStderrNoise(record.client, stderr, result.exitCode) : stderr,
     result: result ? { ...result, durationMs } : undefined,
     error,
     completedAt,
   };
   try {
+    // Check if the job was cancelled while we were running
+    const current = JSON.parse(readFileSync(jobFile, "utf-8")) as JobRecord;
+    if (current.status === "cancelled") {
+      return; // Don't overwrite a cancelled job
+    }
     writeFileSync(jobFile, JSON.stringify(finalRecord, null, 2));
   } catch { /* best effort */ }
   process.exit(0);
@@ -84,8 +111,8 @@ child.on("error", (err: NodeJS.ErrnoException) => {
   finalize("failed", undefined, err.message);
 });
 
-child.on("close", (code: number | null) => {
-  if (timedOut) {
+child.on("close", (code: number | null, signal: NodeJS.Signals | null) => {
+  if (timedOut || signal) {
     finalize("timed_out", {
       stdout, stderr, exitCode: -1, client: record.client, durationMs: 0,
     });
diff --git a/tools/ai-cli-dispatch/src/jobs.ts b/tools/ai-cli-dispatch/src/jobs.ts
index 22db581..abb32e5 100644
--- a/tools/ai-cli-dispatch/src/jobs.ts
+++ b/tools/ai-cli-dispatch/src/jobs.ts
@@ -176,7 +176,10 @@ export function getJobResult(jobId: string, options: JobOperationsOptions = {}):
   if (job.status !== "completed") {
     throw new JobResultUnavailableError(jobId, job.status);
   }
-  return job.result!;
+  if (!job.result) {
+    throw new JobResultUnavailableError(jobId, "completed");
+  }
+  return job.result;
 }
 
 export function cancelJob(jobId: string, options: JobOperationsOptions = {}): void {