Spawner & lifecycle correctness (B6, B7, B8, B9 + resilience)

src/__tests__/audit-spawner.test.ts

@@ -0,0 +1,248 @@
+import { afterEach, beforeEach, describe, expect, test } from "bun:test";
+import { execSync } from "node:child_process";
+import { chmodSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { activeCritterIdentifiersFromPanes, parsePaneList } from "../cli/spawn.js";
+import { createConfigReloadHandler, type DaemonContext } from "../config-reload.js";
+import { createBranch, shallowClone } from "../git.js";
+import { salvagePartialProgress } from "../task-salvage.js";
+import type { IssueTracker, TrackerTask } from "../tracker/types.js";
+import type { Config } from "../types.js";
+import { UnifiedSpawner } from "../unified-spawner.js";
+import { makeTestConfig, makeTestCritterType } from "./helpers.js";
+
+// ───────────────────────────── B6 ─────────────────────────────
+// salvagePartialProgress must push commits even when a PR already exists.
+
+describe("B6 — salvage pushes commits when a PR already exists", () => {
+  let bareRepo: string;
+  let tempDirs: string[];
+  let binDir: string;
+  let savedPath: string | undefined;
+
+  beforeEach(() => {
+    tempDirs = [];
+    bareRepo = mkdtempSync(join(tmpdir(), "critters-audit-bare-"));
+    tempDirs.push(bareRepo);
+    execSync("git init --bare -b main", { cwd: bareRepo, stdio: "ignore" });
+
+    const seedDir = mkdtempSync(join(tmpdir(), "critters-audit-seed-"));
+    tempDirs.push(seedDir);
+    execSync(`git clone ${bareRepo} ${seedDir}/work`, { stdio: "ignore" });
+    execSync("git checkout -b main", { cwd: `${seedDir}/work`, stdio: "ignore" });
+    execSync("git config user.email [email protected]", { cwd: `${seedDir}/work`, stdio: "ignore" });
+    execSync("git config user.name Test", { cwd: `${seedDir}/work`, stdio: "ignore" });
+    writeFileSync(`${seedDir}/work/README.md`, "init");
+    execSync("git add -A && git commit -m 'init'", { cwd: `${seedDir}/work`, stdio: "ignore" });
+    execSync("git push -u origin main", { cwd: `${seedDir}/work`, stdio: "ignore" });
+    execSync("git symbolic-ref HEAD refs/heads/main", { cwd: bareRepo, stdio: "ignore" });
+
+    // Fake `gh` on PATH so `gh pr list` reports an existing PR.
+    binDir = mkdtempSync(join(tmpdir(), "critters-audit-bin-"));
+    tempDirs.push(binDir);
+    const ghPath = join(binDir, "gh");
+    writeFileSync(
+      ghPath,
+      `#!/bin/bash
+if [ "$1" = "pr" ] && [ "$2" = "list" ]; then
+  echo '[{"url":"https://github.com/org/repo/pull/1"}]'
+  exit 0
+fi
+exit 0
+`,
+    );
+    chmodSync(ghPath, 0o755);
+    savedPath = process.env.PATH;
+    process.env.PATH = `${binDir}:${savedPath ?? ""}`;
+  });
+
+  afterEach(() => {
+    process.env.PATH = savedPath;
+    for (const dir of tempDirs) rmSync(dir, { recursive: true, force: true });
+  });
+
+  test("pushes unpushed commits to the existing PR branch before returning", async () => {
+    const dir = mkdtempSync(join(tmpdir(), "critters-audit-clone-"));
+    tempDirs.push(dir);
+    const workDir = join(dir, "repo");
+    await shallowClone(bareRepo, workDir, "TEST-1");
+    execSync("git config user.email [email protected]", { cwd: workDir, stdio: "ignore" });
+    execSync("git config user.name Test", { cwd: workDir, stdio: "ignore" });
+
+    const branch = "critter/TEST-1-pr-exists";
+    await createBranch(workDir, branch, "TEST-1");
+    // Commit locally but DO NOT push — simulates a resumed attempt that
+    // committed new work after the PR already existed.
+    writeFileSync(join(workDir, "resumed.txt"), "resumed work");
+    execSync("git add -A && git commit -m 'resumed work'", { cwd: workDir, stdio: "ignore" });
+
+    const result = await salvagePartialProgress(workDir, branch, "TEST-1", "PR exists");
+
+    // Reports the existing PR url and that the branch was pushed.
+    expect(result.prUrl).toBe("https://github.com/org/repo/pull/1");
+    expect(result.branchPushed).toBe(true);
+
+    // The unpushed commit must now be on the remote (old code skipped the push).
+    const remoteBranches = execSync("git branch", { cwd: bareRepo, encoding: "utf-8" });
+    expect(remoteBranches).toContain(branch);
+    const remoteLog = execSync(`git log ${branch} --oneline`, { cwd: bareRepo, encoding: "utf-8" });
+    expect(remoteLog).toContain("resumed work");
+  });
+});
+
+// ───────────────────────────── B8 ─────────────────────────────
+// Config reload must revert the grouped immutable-field copies the runtime reads.
+
+describe("B8 — config reload reverts grouped immutable fields", () => {
+  function makeCtx(oldConfig: Config): DaemonContext {
+    const tracker = { init: async () => {} } as unknown as IssueTracker;
+    return {
+      config: oldConfig,
+      trackers: new Map([["linear", tracker]]),
+      watcher: { updateConfig: () => {} },
+      spawner: { updateConfig: () => {} },
+      slackNotifier: {},
+      circuitBreakers: new Map([["linear", { updateOptions: () => {} }]]),
+      healthContext: { trackers: new Map(), critterTypes: [], defaultProvider: "", repos: {}, teamRepos: {} },
+      webhookConfig: { critterTypes: [] },
+      autoUpdater: null,
+      jsonLogsCli: false,
+      ensureLabelsAndStatuses: async () => {},
+      updateRefs: () => {},
+    } as unknown as DaemonContext;
+  }
+
+  test("reverts both flat and grouped workDir / tmuxSession / metricsRetentionDays", async () => {
+    const oldConfig = makeTestConfig({
+      workDir: "/orig/work",
+      tmuxSession: "orig-session",
+      metricsRetentionDays: 90,
+    });
+    const newConfig = makeTestConfig({
+      workDir: "/changed/work",
+      tmuxSession: "changed-session",
+      metricsRetentionDays: 7,
+    });
+
+    const handler = createConfigReloadHandler(makeCtx(oldConfig));
+    handler(newConfig);
+
+    // Grouped copies (read live by the runtime) must be reverted — this is the
+    // bug: old code only reverted the flat copies below.
+    expect(newConfig.daemon.workDir).toBe("/orig/work");
+    expect(newConfig.daemon.tmuxSession).toBe("orig-session");
+    expect(newConfig.limits.metricsRetentionDays).toBe(90);
+
+    // Flat copies reverted too.
+    expect(newConfig.workDir).toBe("/orig/work");
+    expect(newConfig.tmuxSession).toBe("orig-session");
+    expect(newConfig.metricsRetentionDays).toBe(90);
+
+    // Let the fire-and-forget apply IIFE settle.
+    await new Promise((r) => setTimeout(r, 0));
+  });
+});
+
+// ───────────────────────────── B9 ─────────────────────────────
+// Pane-title identifier capture must handle keys containing digits (e.g. Jira ABC2-123).
+
+describe("B9 — pane-title regex matches digit-containing issue keys", () => {
+  test("parses ABC2-123 from a pane title", () => {
+    const line = "%3 4242 node ABC2-123: Fix the thing / exec";
+    const [pane] = parsePaneList(line);
+    expect(pane.identifier).toBe("ABC2-123");
+  });
+
+  test("still parses all-letter keys", () => {
+    const line = "%1 1111 node ACK-12: Do a thing / plan";
+    const [pane] = parsePaneList(line);
+    expect(pane.identifier).toBe("ACK-12");
+  });
+
+  test("digit-containing key counts as an active critter (recovery protection)", () => {
+    const panes = parsePaneList("%5 5555 node ABC2-123: Title / review");
+    const active = activeCritterIdentifiersFromPanes(panes);
+    expect(active.has("ABC2-123")).toBe(true);
+  });
+});
+
+// ───────────────────────────── B7 ─────────────────────────────
+// Every runTask invocation (including retried attempts) must (re)register the
+// task in activeCritterMap. The registration now lives at the top of runTask.
+
+describe("B7 — runTask re-registers the task in activeCritterMap", () => {
+  let workBase: string;
+
+  beforeEach(() => {
+    workBase = mkdtempSync(join(tmpdir(), "critters-audit-b7-"));
+  });
+
+  afterEach(() => {
+    rmSync(workBase, { recursive: true, force: true });
+  });
+
+  function makeTracker(onUpdateStatus: () => void): IssueTracker {
+    return {
+      provider: "linear",
+      init: async () => {},
+      findIssues: async () => [],
+      findIssueByIdentifier: async () => null,
+      updateStatus: async () => { onUpdateStatus(); },
+      comment: async () => {},
+      getComments: async () => [],
+      uploadAttachment: async () => null,
+      getAttachments: async () => [],
+      fetchAttachmentContent: async () => null,
+      ensureStatus: async () => {},
+      ensureLabel: async () => {},
+      removeLabel: async () => {},
+      createIssue: async () => ({ id: "x", identifier: "X-1", url: "" }),
+      listTeams: async () => [],
+    } as unknown as IssueTracker;
+  }
+
+  test("each runTask call registers the active critter (observed during the run)", async () => {
+    const captured: string[][] = [];
+
+    // A type with no phases and a success outcome that flips status: applyOutcome
+    // calls tracker.updateStatus while the task is still "running", letting us
+    // observe activeCritterMap mid-flight without spawning a CLI.
+    const critterType = makeTestCritterType({
+      name: "custom",
+      repo: { clone: false, branch: false },
+      phases: [],
+      outcomes: { success: { status: "Done" }, failure: { status: "Failed" } },
+      quietComments: true,
+    });
+    const config = makeTestConfig({ workDir: workBase, critterTypes: [critterType] });
+
+    let spawner: UnifiedSpawner;
+    const tracker = makeTracker(() => {
+      captured.push(spawner.getActiveDetails().map((d) => d.identifier));
+    });
+    spawner = new UnifiedSpawner(config, new Map([["linear", tracker]]));
+
+    const task: TrackerTask = {
+      id: "issue-1",
+      identifier: "ACK-777",
+      title: "Re-register me",
+      description: "",
+      repoUrl: "[email protected]:org/repo.git",
+      labels: [],
+    } as unknown as TrackerTask;
+
+    // Call runTask twice to mimic an initial attempt + a retried attempt.
+    // Both must register the task (old code registered only in processQueue).
+    const r1 = await (spawner as unknown as { runTask: (t: TrackerTask, c: typeof critterType) => Promise<{ success: boolean }> }).runTask(task, critterType);
+    const r2 = await (spawner as unknown as { runTask: (t: TrackerTask, c: typeof critterType) => Promise<{ success: boolean }> }).runTask(task, critterType);
+
+    expect(r1.success).toBe(true);
+    expect(r2.success).toBe(true);
+    expect(captured.length).toBe(2);
+    expect(captured[0]).toContain("ACK-777");
+    expect(captured[1]).toContain("ACK-777");
+    // Cleaned up after each attempt.
+    expect(spawner.getActiveDetails()).toHaveLength(0);
+  });
+});

src/__tests__/mcp.test.ts

@@ -1,4 +1,5 @@
 import { describe, expect, test } from "bun:test";
+import { homedir } from "node:os";
 import { ClaudeCodeAdapter } from "../cli/claude.js";
 import { CodexAdapter } from "../cli/codex.js";
 import { resolvePhaseMcpConfig } from "../cli/mcp.js";
@@ -25,7 +26,7 @@ describe("resolvePhaseMcpConfig", () => {
       } as Config,
     );
 
-    expect(result.mcpConfig).toEqual([`${process.env.HOME}/.critters/review-mcp.json`]);
+    expect(result.mcpConfig).toEqual([`${homedir()}/.critters/review-mcp.json`]);
     expect(result.strictMcpConfig).toBe(true);
   });
 

src/cli/spawn.ts

@@ -365,7 +365,9 @@ export async function spawnForPhase(
 // ── Stale pane cleanup ───────────────────────────────────────────────────────
 
 /** Regex to detect critter-titled tmux panes. Captures the issue identifier. */
-const CRITTER_PANE_TITLE_RE = /^([A-Z]+-\d+): .+ \/ (plan|exec|review|[\w-]+)/;
+// Issue key: an uppercase letter followed by uppercase letters/digits, a dash,
+// then digits (e.g. ACK-12, ABC2-123 for Jira keys that contain digits).
+const CRITTER_PANE_TITLE_RE = /^([A-Z][A-Z0-9]*-\d+): .+ \/ (plan|exec|review|[\w-]+)/;
 
 interface ParsedPane {
   paneId: string;
@@ -459,7 +461,7 @@ export async function cleanupStalePanes(
   const activeIdentifiers = new Set<string>();
   for (const dir of activeWorkDirs) {
     const basename = dir.split("/").pop() ?? "";
-    const match = basename.replace(/^review-/, "").match(/^([A-Z]+-\d+)/);
+    const match = basename.replace(/^review-/, "").match(/^([A-Z][A-Z0-9]*-\d+)/);
     if (match) activeIdentifiers.add(match[1]);
   }
   for (const identifier of activePaneIdentifiers) {

src/config-reload.ts

@@ -37,20 +37,36 @@ export interface DaemonContext {
   updateRefs: (updates: { config: Config; trackers: Map<string, IssueTracker>; slackNotifier: SlackNotifier }) => void;
 }
 
-const immutableFields = ["workDir", "healthPort", "tmuxSession", "dashboardToken", "metricsRetentionDays"] as const;
+/**
+ * Fields that cannot change at runtime. Each lists its flat top-level key and
+ * the grouped path the runtime actually reads (e.g. `config.daemon.workDir`).
+ * loadConfig populates both copies, so both must be reverted to the running
+ * values — reverting only the flat copy let the change silently take effect.
+ */
+const immutableFields = [
+  { flat: "workDir", group: "daemon", key: "workDir" },
+  { flat: "healthPort", group: "daemon", key: "healthPort" },
+  { flat: "tmuxSession", group: "daemon", key: "tmuxSession" },
+  { flat: "dashboardToken", group: "daemon", key: "dashboardToken" },
+  { flat: "metricsRetentionDays", group: "limits", key: "metricsRetentionDays" },
+] as const;
 
 export function createConfigReloadHandler(ctx: DaemonContext): (newConfig: Config) => void {
   return (newConfig: Config) => {
     // Preserve runtime flag
     newConfig.noTmux = ctx.config.noTmux;
     newConfig.daemon.noTmux = ctx.config.daemon.noTmux;
 
-    // Override immutable fields with current values, warn if changed
-    for (const field of immutableFields) {
-      if (newConfig[field] !== ctx.config[field]) {
-        log(`Warning: '${field}' cannot be changed at runtime (ignoring ${JSON.stringify(ctx.config[field])} → ${JSON.stringify(newConfig[field])})`);
-        (newConfig as unknown as Record<string, unknown>)[field] = ctx.config[field];
+    // Override immutable fields with current values, warn if changed. Revert
+    // both the flat copy and the grouped copy the runtime reads.
+    for (const { flat, group, key } of immutableFields) {
+      const currentGrouped = (ctx.config[group] as unknown as Record<string, unknown>)[key];
+      const newGrouped = (newConfig[group] as unknown as Record<string, unknown>)[key];
+      if (newConfig[flat] !== ctx.config[flat] || newGrouped !== currentGrouped) {
+        log(`Warning: '${flat}' cannot be changed at runtime (ignoring ${JSON.stringify(ctx.config[flat])} → ${JSON.stringify(newConfig[flat] ?? newGrouped)})`);
       }
+      (newConfig as unknown as Record<string, unknown>)[flat] = ctx.config[flat];
+      (newConfig[group] as unknown as Record<string, unknown>)[key] = currentGrouped;
     }
 
     // Tunnel config is immutable at runtime (ngrok is a long-lived subprocess)

src/config-watcher.ts

@@ -1,4 +1,5 @@
 import { type FSWatcher, watch } from "node:fs";
+import { basename, dirname } from "node:path";
 import { loadConfig } from "./config.js";
 import { log, logError } from "./logger.js";
 import type { Config } from "./types.js";
@@ -16,7 +17,15 @@ export class ConfigWatcher {
 
 	start(): void {
 		try {
-			this.watcher = watch(this.configPath, () => {
+			// Watch the parent directory rather than the file itself. A single
+			// fs.watch on the file stops firing after the first atomic-rename
+			// save (most editors replace the inode), whereas the directory
+			// handle survives renames. Filter events down to our basename.
+			const dir = dirname(this.configPath);
+			const file = basename(this.configPath);
+			this.watcher = watch(dir, (_event, filename) => {
+				// filename can be null on some platforms; treat that as a match.
+				if (filename != null && filename !== file) return;
 				if (this.debounceTimer) {
 					clearTimeout(this.debounceTimer);
 				}

src/task-salvage.ts

@@ -41,7 +41,16 @@ export async function salvagePartialProgress(
       try {
         const prs = JSON.parse(listResult.stdout);
         if (prs.length > 0) {
-          return { prUrl: prs[0].url, branchPushed: true };
+          // A PR already exists, but a resumed attempt (or the auto-commit
+          // above) may have produced commits that were never pushed. Push
+          // before returning so cleanupWorkDir doesn't delete unpushed work.
+          // The push is an idempotent fast-forward when origin is already
+          // up to date.
+          const pushResult = await runCommand("git", ["push", "origin", branch], { cwd: workDir });
+          if (pushResult.code !== 0) {
+            logTaskError(identifier, `Salvage: push to existing PR branch failed: ${pushResult.stderr}`);
+          }
+          return { prUrl: prs[0].url, branchPushed: pushResult.code === 0 };
         }
       } catch {
         // JSON parse failed