Fix critical + high severity issues from code audit (B1–B5)

src/__tests__/audit-fixes.test.ts

@@ -0,0 +1,72 @@
+import { describe, expect, test } from "bun:test";
+import { buildPaneBanner } from "../cli/spawn.js";
+import { isConnectionRefused } from "../cli-clean.js";
+import { renderDashboard } from "../dashboard/main-page.js";
+import type { HealthStatus } from "../health.js";
+
+// B1 — Command injection via issue title in the generated tmux script.
+describe("buildPaneBanner (B1: RCE via issue title)", () => {
+  test("single-quotes the window name so a title can't execute shell", () => {
+    const malicious = "ACK-1: $(touch /tmp/pwned) / plan";
+    const line = buildPaneBanner("\x1b[1;36m", malicious, "\x1b[0m");
+    // The untrusted name must sit inside a single-quoted segment spliced between the
+    // double-quoted color parts: `"<color>━━━ "'<name>'" ━━━<reset>"`. Inside single
+    // quotes bash performs no command/parameter expansion, so `$(…)` stays literal.
+    expect(line).toContain(`"'${malicious}'"`);
+    // And the substitution must NOT appear unquoted inside a double-quoted run.
+    expect(line).not.toContain(`$(touch /tmp/pwned)" `);
+  });
+
+  test("escapes backticks the same way", () => {
+    const line = buildPaneBanner("L", "ACK-2: `id` now", "R");
+    expect(line).toContain("'ACK-2: `id` now'");
+  });
+
+  test("escapes embedded single quotes safely", () => {
+    const line = buildPaneBanner("L", "it's mine", "R");
+    // shellEscape turns ' into '\'' — still no way to break out of the quoting.
+    expect(line).toContain("'it'\\''s mine'");
+  });
+});
+
+// B3 — Reflected XSS via the window.__CRITTERS__ bootstrap.
+describe("renderDashboard (B3: reflected XSS in bootstrap)", () => {
+  const status = {} as HealthStatus;
+
+  test("escapes script-breaking chars in the identifier", () => {
+    const html = renderDashboard("", status, 0, undefined, undefined, "</script><img src=x onerror=alert(1)>");
+    // The raw breakout must never reach the HTML verbatim...
+    expect(html).not.toContain("</script><img");
+    // ...it must be present only in \uXXXX-escaped form.
+    expect(html).toContain("\\u003c/script\\u003e");
+  });
+
+  test("bootstrap stays valid JSON and round-trips to the original values", () => {
+    const html = renderDashboard("", status, 0, "type</script>", undefined, "ID&<>");
+    const m = html.match(/window\.__CRITTERS__ = (\{.*?\});/);
+    expect(m).toBeTruthy();
+    // JSON.parse natively decodes the \uXXXX escapes back to the real characters.
+    const parsed = JSON.parse(m?.[1] ?? "{}") as { typeFilter: string; identifier: string };
+    expect(parsed.identifier).toBe("ID&<>");
+    expect(parsed.typeFilter).toBe("type</script>");
+  });
+});
+
+// B4 — `clean` must fail closed when a daemon may be alive but health didn't answer.
+describe("isConnectionRefused (B4: clean fail-open guard)", () => {
+  test("treats genuine connection refusal as down (safe to clean)", () => {
+    expect(isConnectionRefused({ code: "ConnectionRefused" })).toBe(true);
+    expect(isConnectionRefused(new Error("connect ECONNREFUSED 127.0.0.1:7878"))).toBe(true);
+    expect(isConnectionRefused(new Error("Unable to connect. Is the computer able to access the url?"))).toBe(true);
+  });
+
+  test("treats timeouts/aborts as maybe-alive (unsafe — must NOT look like refusal)", () => {
+    expect(isConnectionRefused(new DOMException("The operation timed out.", "TimeoutError"))).toBe(false);
+    expect(isConnectionRefused(new Error("The operation was aborted"))).toBe(false);
+  });
+
+  test("unknown errors default to maybe-alive (fail closed)", () => {
+    expect(isConnectionRefused(new Error("something weird happened"))).toBe(false);
+    expect(isConnectionRefused(null)).toBe(false);
+  });
+});

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-clean.ts

@@ -32,30 +32,77 @@ function formatSize(bytes: number): string {
   return `${bytes} B`;
 }
 
-async function getActiveWorkDirs(healthPort: number): Promise<Set<string>> {
-  const active = new Set<string>();
+type ActiveWorkDirsResult = {
+  /** true only when /healthz answered 200 and we have an authoritative active set. */
+  reachable: boolean;
+  /** true when something is (or may be) listening — a daemon could be running. */
+  daemonMaybeAlive: boolean;
+  workDirs: Set<string>;
+};
+
+/**
+ * A refused connection means nothing is listening on the port → the daemon is down,
+ * so its panes/work dirs are genuinely orphaned and safe to clean. Any other failure
+ * (timeout, reset, non-2xx) means a daemon may be alive but unresponsive — unsafe.
+ */
+export function isConnectionRefused(err: unknown): boolean {
+  const e = err as { message?: string; code?: string };
+  const code = String(e?.code ?? "").toLowerCase();
+  if (code === "connectionrefused" || code === "econnrefused") return true;
+  const msg = String(e?.message ?? err).toLowerCase();
+  // A timeout/abort means something may be there but is slow/wedged — NOT a refusal.
+  if (msg.includes("timeout") || msg.includes("timed out") || msg.includes("aborted")) return false;
+  return msg.includes("refused") || msg.includes("econnrefused") ||
+    msg.includes("connectionrefused") || msg.includes("unable to connect") ||
+    msg.includes("failed to connect");
+}
+
+async function getActiveWorkDirs(healthPort: number): Promise<ActiveWorkDirsResult> {
+  const workDirs = new Set<string>();
   try {
     const resp = await fetch(`http://localhost:${healthPort}/healthz`, { signal: AbortSignal.timeout(3000) });
     if (resp.ok) {
       const data = await resp.json() as { activeCritterDetails?: Array<{ workDir?: string | null }> };
       for (const d of data.activeCritterDetails ?? []) {
-        if (d.workDir) active.add(d.workDir);
+        if (d.workDir) workDirs.add(d.workDir);
       }
+      return { reachable: true, daemonMaybeAlive: true, workDirs };
     }
-  } catch {
-    // Daemon not running or unreachable — no protection available
+    // Something answered but not 200 — likely the daemon with a momentarily unhealthy probe.
+    return { reachable: false, daemonMaybeAlive: true, workDirs };
+  } catch (err) {
+    // Connection refused → daemon down (safe). Timeout/other → daemon may be alive (unsafe).
+    return { reachable: false, daemonMaybeAlive: !isConnectionRefused(err), workDirs };
   }
-  return active;
+}
+
+/**
+ * Resolve the active work dirs to protect during cleanup, or null when cleaning is
+ * UNSAFE: a daemon may be alive but its /healthz didn't answer cleanly, so we cannot tell
+ * which critters are live. Proceeding would fail open and could kill/delete live work.
+ */
+async function resolveActiveWorkDirs(healthPort: number): Promise<Set<string> | null> {
+  if (healthPort <= 0) return new Set();
+  const health = await getActiveWorkDirs(healthPort);
+  if (!health.reachable && health.daemonMaybeAlive) return null;
+  return health.workDirs;
 }
 
 async function cleanStaleTmuxPanes(configPath: string | undefined, dryRun: boolean): Promise<void> {
   const cleanConfig = loadCleanConfig(configPath);
   const tmuxSession = cleanConfig.tmuxSession;
 
-  // Get active work dirs from the daemon's health endpoint (if running)
-  const activeWorkDirs = cleanConfig.healthPort > 0
-    ? await getActiveWorkDirs(cleanConfig.healthPort)
-    : new Set<string>();
+  // Determine which critters are live so we never kill an active pane. If a daemon may be
+  // alive but its health endpoint didn't answer cleanly, we can't tell — fail closed.
+  const activeWorkDirs = await resolveActiveWorkDirs(cleanConfig.healthPort);
+  if (activeWorkDirs === null) {
+    console.error(
+      `Refusing to clean tmux panes: the daemon health endpoint on port ${cleanConfig.healthPort} ` +
+      `did not respond cleanly, but something appears to be listening. A daemon may be running — ` +
+      `cleaning now could kill live critter panes. Stop the daemon (or fix healthPort) and retry.`,
+    );
+    return;
+  }
 
   const stalePanes = await cleanupStalePanes(tmuxSession, activeWorkDirs);
 
@@ -114,10 +161,17 @@ export async function runClean(args: string[]): Promise<void> {
   let totalFreed = 0;
   let cleanedCount = 0;
 
-  // Query the daemon's health endpoint for active work directories
-  const activeWorkDirs = cleanConfig.healthPort > 0
-    ? await getActiveWorkDirs(cleanConfig.healthPort)
-    : new Set<string>();
+  // Query the daemon's health endpoint for active work directories. If a daemon may be
+  // alive but health didn't answer cleanly, fail closed rather than risk deleting live work.
+  const activeWorkDirs = await resolveActiveWorkDirs(cleanConfig.healthPort);
+  if (activeWorkDirs === null) {
+    console.error(
+      `Refusing to clean work directories: the daemon health endpoint on port ${cleanConfig.healthPort} ` +
+      `did not respond cleanly, but something appears to be listening. A daemon may be running — ` +
+      `cleaning now could delete in-use work dirs. Stop the daemon (or fix healthPort) and retry.`,
+    );
+    return;
+  }
 
   const dirs: { name: string; ageMs: number; size: number; stale: boolean; active: boolean }[] = [];
 

src/cli/spawn.ts

@@ -31,6 +31,20 @@ function buildPaneLabel(identifier: string, title: string, phase: string, repoSh
   return base;
 }
 
+/**
+ * Build the colored banner `echo` line for a critter's tmux pane.
+ *
+ * `windowName` embeds the untrusted issue title, so it MUST be single-quoted via
+ * shellEscape: interpolating it into the double-quoted `echo -e "…"` directly (as
+ * the old `.replace(/"/g, …)` did) lets `$(…)` / backticks in a title execute as
+ * shell when the generated script runs — i.e. arbitrary RCE on the daemon host.
+ * The color codes are build-time constants and stay inside the double quotes so
+ * `echo -e` still interprets their escape sequences.
+ */
+export function buildPaneBanner(colorLabel: string, windowName: string, reset: string): string {
+  return `echo -e "${colorLabel}━━━ "${shellEscape(windowName)}" ━━━${reset}"`;
+}
+
 async function spawnInTmux(
   adapter: CliAdapter,
   prompt: string,
@@ -99,7 +113,7 @@ async function spawnInTmux(
 set -o pipefail
 export PATH="$HOME/.bun/bin:$HOME/.local/bin:${currentPath}"
 ${cmd.env ? Object.entries(cmd.env).map(([k, v]) => v === undefined ? `unset ${k}` : `export ${k}=${shellEscape(v)}`).join("\n") : ""}
-echo -e "${color.label}\u2501\u2501\u2501 ${windowName.replace(/"/g, '\\"')} \u2501\u2501\u2501${reset}"
+${buildPaneBanner(color.label, windowName, reset)}
 echo ""
 cd ${shellEscape(workDir)}
 ${cmd.script} \\
@@ -121,7 +135,7 @@ sleep 5
 set -o pipefail
 export PATH="$HOME/.bun/bin:$HOME/.local/bin:${currentPath}"
 ${cmd.env ? Object.entries(cmd.env).map(([k, v]) => v === undefined ? `unset ${k}` : `export ${k}=${shellEscape(v)}`).join("\n") : ""}
-echo -e "${color.label}\u2501\u2501\u2501 ${windowName.replace(/"/g, '\\"')} \u2501\u2501\u2501${reset}"
+${buildPaneBanner(color.label, windowName, reset)}
 echo ""
 cd ${shellEscape(workDir)}
 ${cmd.script} \\

src/dashboard/main-page.ts

@@ -20,10 +20,20 @@ export function renderDashboard(
   _dashboardToken?: string,
   identifier?: string,
 ): string {
+  // Escape characters that could break out of the inline <script> below. JSON.stringify
+  // does NOT escape "</script>", "<", ">", "&", or the JS line separators, so an
+  // attacker-controlled `identifier` (the raw URL path segment, reflected here) could
+  // otherwise terminate the script tag and inject live HTML — reflected XSS on the
+  // unauthenticated /dashboard/<id> route. Escaping to \uXXXX keeps the JSON valid.
   const bootstrap = JSON.stringify({
     typeFilter: typeFilter ?? null,
     identifier: identifier ?? null,
-  });
+  })
+    .replace(/</g, "\\u003c")
+    .replace(/>/g, "\\u003e")
+    .replace(/&/g, "\\u0026")
+    .replace(/\u2028/g, "\\u2028")
+    .replace(/\u2029/g, "\\u2029");
 
   const title = identifier ? `${escapeHtml(identifier)} - Critters`
     : typeFilter ? `Critters \u00b7 ${escapeHtml(typeFilter)}`

src/runner/execution.ts

@@ -5,6 +5,7 @@ import { logTask, logTaskError, logTaskWarn } from "../logger.js";
 import { buildExecutionPrompt, getExecutionAllowedTools } from "../prompt.js";
 import { buildPromptVars, resolveSkills, resolveTools } from "../prompt-template.js";
 import { withRetry } from "../retry.js";
+import { salvagePartialProgress } from "../task-salvage.js";
 import { formatDuration, runCommand } from "../utils.js";
 import { VERSION } from "../version.js";
 import type { PhaseContext, PhaseResult, PhaseRunner } from "./types.js";
@@ -52,10 +53,15 @@ export class ExecutionPhaseRunner implements PhaseRunner {
       return { spawn, data: { prUrl: null } };
     }
 
-    // Only look for a PR if the branch was pushed to the remote
+    // If the branch isn't on the remote, the agent committed locally but never pushed
+    // (ran out of turns, or `git push` failed while the CLI still exited 0). The work dir
+    // is the ONLY copy of those commits, and the spawner deletes it on the success path —
+    // so salvage the work (push + open a draft PR) instead of silently dropping it.
     const { stdout: remoteOut } = await runCommand("git", ["ls-remote", "--heads", "origin", branch], { cwd: workDir });
     if (!remoteOut.trim()) {
-      return { spawn, data: { prUrl: null } };
+      logTaskWarn(task.identifier, "Branch has local commits but was never pushed — salvaging to a draft PR to avoid data loss");
+      const salvage = await salvagePartialProgress(workDir, branch, task.identifier, task.title, task.repoUrl, task.baseBranch);
+      return { spawn, data: { prUrl: salvage.prUrl ?? null } };
     }
 
     // Detect PR

src/unified-spawner.ts

@@ -568,7 +568,7 @@ export class UnifiedSpawner {
       const totalDuration = formatDuration(Date.now() - taskStart);
       logTask(task.identifier, `Completed in ${totalDuration}`);
 
-      await applyOutcome(critterType.outcomes.success, task, critterType, tracker);
+      await this.applySuccessOutcome(critterType.outcomes.success, task, critterType, tracker);
 
       // Upload report from the last phase (generic runner writes .critter-report.md)
       const lastPhaseData = phaseDataList.length > 0 ? phaseDataList[phaseDataList.length - 1] : null;
@@ -844,6 +844,27 @@ export class UnifiedSpawner {
     return lastResult ?? { success: false, error: "Auto-retry did not produce a task result" };
   }
 
+  /**
+   * Apply a terminal *success* outcome (status transition + optional label removal)
+   * defensively. A status-update failure on a genuinely successful task (API 5xx,
+   * rate limit, network blip) must never propagate: otherwise the outer catch would
+   * demote a real success into the FAILURE path — wrong status, a misleading "failed"
+   * comment, lost success comment/metrics, and (for transient errors) a wasteful full
+   * re-run of an already-created PR. The failure path already swallows these; mirror it.
+   */
+  private async applySuccessOutcome(
+    outcome: Parameters<typeof applyOutcome>[0],
+    task: TrackerTask,
+    critterType: CritterTypeConfig,
+    tracker: IssueTracker,
+  ): Promise<void> {
+    try {
+      await applyOutcome(outcome, task, critterType, tracker);
+    } catch (err) {
+      logTaskError(task.identifier, `Failed to apply success status transition (continuing — task succeeded): ${formatError(err)}`);
+    }
+  }
+
   private async handleCreateSuccess(
     task: TrackerTask,
     critterType: CritterTypeConfig,
@@ -855,7 +876,7 @@ export class UnifiedSpawner {
     taskStart: number,
     tracker: IssueTracker,
   ): Promise<TaskResult> {
-    await applyOutcome(critterType.outcomes.prCreated ?? critterType.outcomes.success, task, critterType, tracker);
+    await this.applySuccessOutcome(critterType.outcomes.prCreated ?? critterType.outcomes.success, task, critterType, tracker);
 
     try {
       await updatePrWithPlan(workDir, prUrl, task.identifier, allPhaseStats);
@@ -926,7 +947,7 @@ export class UnifiedSpawner {
     const totalDuration = formatDuration(Date.now() - taskStart);
 
     if (decision === "merged" || data.alreadyMerged) {
-      await applyOutcome(critterType.outcomes.merged, task, critterType, tracker);
+      await this.applySuccessOutcome(critterType.outcomes.merged, task, critterType, tracker);
       if (data.alreadyMerged) {
         await tracker.comment(task.id, "PR was already merged");
         logTask(task.identifier, "Review complete — PR was already merged");
@@ -974,7 +995,7 @@ export class UnifiedSpawner {
     }
 
     if (decision === "needs_changes") {
-      await applyOutcome(critterType.outcomes.needsChanges, task, critterType, tracker);
+      await this.applySuccessOutcome(critterType.outcomes.needsChanges, task, critterType, tracker);
       await tracker.comment(task.id, `Review critter (${critterType.phases[0].model}) requested changes: ${reason}`);
       await this.slackNotifier.notify(
         task.id,