Merge pull request #1071 from corbitsdev/cl-7997-keep-approval-hold-across-retry-and-prove-shell-guard-kill

fix(tui): keep approved hold across retry and prove shell-guard kill

Author: TheGreatAxios

src/subagent/run-shell-child-reap.test.ts

@@ -353,3 +353,54 @@ describe("CL-7990 shell-child reap: sessions holding a live shell child settle",
     { timeout: 60_000 },
   );
 });
+
+/**
+ * CL-7997 kill proof: the tests above prove the run settles under a wedged
+ * child, but none proves a shell-guard-tracked child is actually KILLED on
+ * close/dispose. This drives a real `sleep` through `runGuardedShell` (the
+ * shell-guard tracking primitive) and the exact `reapLiveChildren` call the
+ * plugin dispose runs, then asserts on the ChildProcess handle itself that
+ * the process is dead — not just that the run settled.
+ */
+describe("CL-7997 shell-guard kill proof: dispose leaves the tracked child dead", () => {
+  test(
+    "reapLiveChildren kills a shell-guard-tracked sleep child",
+    async () => {
+      if (process.platform === "win32") return;
+      const { runGuardedShell, reapLiveChildren } =
+        await import("../plugins/shell-guard-plugin.js");
+      const liveChildren = new Set<ChildProcess>();
+      const controller = new AbortController();
+      const running = runGuardedShell(
+        { command: "sleep 60" },
+        controller.signal,
+        liveChildren,
+      );
+      let child: ChildProcess | undefined;
+      try {
+        const started = Date.now();
+        while (liveChildren.size === 0 && Date.now() - started < 5_000) {
+          await new Promise((resolve) => setTimeout(resolve, 10));
+        }
+        expect(liveChildren.size).toBe(1);
+        child = defined([...liveChildren][0]);
+        // Live before the reap, so the death assertion below is not vacuous.
+        expect(child.exitCode).toBeNull();
+        expect(child.signalCode).toBeNull();
+        await reapLiveChildren(liveChildren);
+        await waitForChildExit(child);
+        await running;
+      } finally {
+        controller.abort();
+        if (child !== undefined) {
+          try {
+            child.kill("SIGKILL");
+          } catch {
+            // Already dead — best-effort orphan guard.
+          }
+        }
+      }
+    },
+    { timeout: 30_000 },
+  );
+});

src/tui/correlation-acceptance.test.ts

@@ -74,6 +74,40 @@ describe("createCorrelationAcceptance", () => {
     expect(settled).toBe(true);
   });
 
+  test("an approved hold survives abandon so a late tool.start settles the retry waiter", async () => {
+    const acceptance = createCorrelationAcceptance();
+    const first = acceptance.wait("corr-1");
+    let firstSettled = false;
+    void first.then(() => {
+      firstSettled = true;
+    });
+    acceptance.observe({
+      type: "message.correlated",
+      data: { correlationId: "corr-1", message: approvedMessage("corr-1") },
+    });
+    // Acceptance-timeout path: drops the waiter without resolving it, but the
+    // approved hold must survive so the retry waiter below still settles.
+    acceptance.abandon("corr-1");
+    await new Promise((resolve) => setTimeout(resolve, 5));
+    expect(firstSettled).toBe(false);
+    const retry = acceptance.wait("corr-1");
+    acceptance.observe({
+      type: "tool.start",
+      data: { call: { id: "call-1" } },
+    });
+    const settled = await Promise.race([
+      retry.then(() => true),
+      new Promise<boolean>((resolve) => setTimeout(() => resolve(false), 50)),
+    ]);
+    expect(settled).toBe(true);
+  });
+
+  test("late settle after abandon is a safe no-op", () => {
+    const acceptance = createCorrelationAcceptance();
+    acceptance.abandon("corr-1");
+    acceptance.settle("corr-1");
+  });
+
   test("a rejected correlation settles at message.correlated", async () => {
     const acceptance = createCorrelationAcceptance();
     const pending = acceptance.wait("corr-1");

src/tui/correlation-acceptance.ts

@@ -81,10 +81,12 @@ export function createCorrelationAcceptance() {
   /**
    * Drop a waiter without resolving it. The acceptance deadline path uses
    * this instead of settle: settling would fulfill the very promise the
-   * deadline race is trying to reject, so the timeout could never fire.
+   * deadline race is trying to reject, so the timeout could never fire. The
+   * approved hold survives: the retry re-awaits acceptance for the same
+   * correlation id, and a late tool.start must still settle that waiter
+   * instead of finding an empty hold set and false-timing-out again.
    */
   const abandon = (correlationId: string): void => {
-    holdUntilToolStart.delete(correlationId);
     waiters.delete(correlationId);
   };