Runner & adapter resilience (review/generic/codex)

src/__tests__/audit-runners.test.ts

@@ -0,0 +1,239 @@
+import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test";
+import { chmodSync, mkdirSync, writeFileSync } from "node:fs";
+import { ClaudeCodeAdapter } from "../cli/claude.js";
+import { CodexAdapter } from "../cli/codex.js";
+import { GenericPhaseRunner } from "../runner/generic.js";
+import { ReviewPhaseRunner } from "../runner/review.js";
+import type { PhaseContext } from "../runner/types.js";
+import type { TrackerTask } from "../tracker/types.js";
+import { createTempDir } from "./helpers.js";
+
+// spawnForPhase actually drives tmux/CLI subprocesses — stub it so the runners
+// reach their result-handling logic without spawning anything.
+const spawnResult = { exitCode: 0, stdout: "", stderr: "", timedOut: false };
+mock.module("../cli/spawn.js", () => ({
+  spawnForPhase: () => Promise.resolve(spawnResult),
+}));
+
+function makeTask(overrides?: Partial<TrackerTask>): TrackerTask {
+  return {
+    id: "issue-1",
+    identifier: "ACK-1",
+    title: "Test task",
+    description: "Do the thing",
+    repoUrl: "https://github.com/acme/repo",
+    group: "Eng",
+    groupId: "team-1",
+    labels: [],
+    ...overrides,
+  };
+}
+
+function makeReviewCtx(workDir: string, adapter: ClaudeCodeAdapter, task: TrackerTask): PhaseContext {
+  return {
+    task,
+    workDir,
+    branch: "feature-branch",
+    repoConfig: null,
+    phase: { name: "review", prompt: "builtin:review", model: "opus", maxTurns: 10, tools: "review" },
+    cliAdapter: adapter,
+  } as unknown as PhaseContext;
+}
+
+describe("ReviewPhaseRunner merged-confirmation gate", () => {
+  let tempDir: string;
+  let cleanup: () => void;
+  let binDir: string;
+  let origPath: string | undefined;
+
+  beforeEach(() => {
+    const tmp = createTempDir();
+    tempDir = tmp.path;
+    cleanup = tmp.cleanup;
+    binDir = `${tempDir}/bin`;
+    mkdirSync(binDir);
+    origPath = process.env.PATH;
+  });
+
+  afterEach(() => {
+    if (origPath !== undefined) process.env.PATH = origPath;
+    cleanup();
+  });
+
+  // Writes fake `gh` + `git` onto PATH. `gh pr view ... state` always reports the
+  // given PR state; headRefName/feedback queries return stable stub data; git
+  // fetch/checkout succeed.
+  function shadowGh(prState: string): void {
+    writeFileSync(
+      `${binDir}/gh`,
+      [
+        "#!/bin/sh",
+        'for a in "$@"; do',
+        "  case \"$a\" in",
+        `    state) echo "${prState}"; exit 0;;`,
+        '    headRefName) echo "feature-branch"; exit 0;;',
+        "    comments,reviews) echo '{\"comments\":[],\"reviews\":[]}'; exit 0;;",
+        "  esac",
+        "done",
+        'echo ""; exit 0',
+      ].join("\n"),
+    );
+    writeFileSync(`${binDir}/git`, ["#!/bin/sh", "exit 0"].join("\n"));
+    chmodSync(`${binDir}/gh`, 0o755);
+    chmodSync(`${binDir}/git`, 0o755);
+    process.env.PATH = `${binDir}:${origPath ?? ""}`;
+  }
+
+  test("downgrades a claimed MERGED decision to unknown when the PR is not actually merged", async () => {
+    shadowGh("OPEN");
+    const adapter = new ClaudeCodeAdapter();
+    // Simulate the agent emitting a hallucinated REVIEW_RESULT:MERGED sentinel.
+    adapter.extractReviewDecision = () => ({ decision: "merged" });
+
+    const task = makeTask({ prNumber: 7, prUrl: "https://github.com/acme/repo/pull/7" });
+    const result = await new ReviewPhaseRunner().run(makeReviewCtx(tempDir, adapter, task));
+
+    // The gate must NOT trust the sentinel: PR state is OPEN, so it downgrades.
+    expect(result.data.reviewDecision).toBe("unknown");
+  });
+
+  test("keeps a MERGED decision when GitHub confirms the PR is merged", async () => {
+    // Stateful gh: the first `state` query (the early already-merged check) reports
+    // OPEN so the run proceeds; the second `state` query (the confirmation gate)
+    // reports MERGED, simulating the agent having merged the PR during review.
+    const countFile = `${tempDir}/gh-count`;
+    writeFileSync(
+      `${binDir}/gh`,
+      [
+        "#!/bin/sh",
+        `COUNT_FILE="${countFile}"`,
+        'for a in "$@"; do',
+        "  case \"$a\" in",
+        "    state)",
+        '      n=$(cat "$COUNT_FILE" 2>/dev/null || echo 0)',
+        "      n=$((n+1))",
+        '      echo "$n" > "$COUNT_FILE"',
+        '      if [ "$n" -ge 2 ]; then echo "MERGED"; else echo "OPEN"; fi',
+        "      exit 0;;",
+        '    headRefName) echo "feature-branch"; exit 0;;',
+        "    comments,reviews) echo '{\"comments\":[],\"reviews\":[]}'; exit 0;;",
+        "  esac",
+        "done",
+        'echo ""; exit 0',
+      ].join("\n"),
+    );
+    writeFileSync(`${binDir}/git`, ["#!/bin/sh", "exit 0"].join("\n"));
+    chmodSync(`${binDir}/gh`, 0o755);
+    chmodSync(`${binDir}/git`, 0o755);
+    process.env.PATH = `${binDir}:${origPath ?? ""}`;
+
+    const adapter = new ClaudeCodeAdapter();
+    adapter.extractReviewDecision = () => ({ decision: "merged" });
+
+    const task = makeTask({ prNumber: 7, prBranch: "feature-branch" });
+    const result = await new ReviewPhaseRunner().run(makeReviewCtx(tempDir, adapter, task));
+
+    expect(result.data.reviewDecision).toBe("merged");
+  });
+
+  test("throws loudly when the PR branch cannot be resolved", async () => {
+    // gh exits non-zero for every call → headRefName resolution fails.
+    writeFileSync(`${binDir}/gh`, ["#!/bin/sh", "exit 1"].join("\n"));
+    chmodSync(`${binDir}/gh`, 0o755);
+    process.env.PATH = `${binDir}:${origPath ?? ""}`;
+    const adapter = new ClaudeCodeAdapter();
+
+    // prNumber set but no prBranch → must resolve the branch, which fails.
+    const task = makeTask({ prNumber: 9 });
+    await expect(new ReviewPhaseRunner().run(makeReviewCtx(tempDir, adapter, task))).rejects.toThrow(
+      /Failed to resolve PR branch/,
+    );
+  });
+});
+
+describe("GenericPhaseRunner empty-report fallback", () => {
+  let tempDir: string;
+  let cleanup: () => void;
+
+  beforeEach(() => {
+    const tmp = createTempDir();
+    tempDir = tmp.path;
+    cleanup = tmp.cleanup;
+    writeFileSync(`${tempDir}/prompt.md`, "Do the task");
+  });
+
+  afterEach(() => cleanup());
+
+  function makeGenericCtx(adapter: ClaudeCodeAdapter): PhaseContext {
+    return {
+      task: makeTask(),
+      config: {} as unknown,
+      workDir: tempDir,
+      branch: "feature-branch",
+      repoConfig: null,
+      phase: { name: "custom", prompt: `${tempDir}/prompt.md`, model: "opus", maxTurns: 10, tools: ["Read"] },
+      cliAdapter: adapter,
+    } as unknown as PhaseContext;
+  }
+
+  test("treats a whitespace-only .critter-report.md as missing and uses the stream-json fallback", async () => {
+    writeFileSync(`${tempDir}/.critter-report.md`, "   \n\t\n");
+    const adapter = new ClaudeCodeAdapter();
+    adapter.extractFinalResponse = () => "FALLBACK TEXT";
+
+    const result = await new GenericPhaseRunner().run(makeGenericCtx(adapter));
+    expect(result.data.responseText).toBe("FALLBACK TEXT");
+  });
+
+  test("uses the report file when it has real content", async () => {
+    // The non-empty path copies the report into the plans dir, which must exist.
+    mkdirSync(`${tempDir}/critters/plans`, { recursive: true });
+    writeFileSync(`${tempDir}/.critter-report.md`, "Real report body");
+    const adapter = new ClaudeCodeAdapter();
+    adapter.extractFinalResponse = () => "FALLBACK TEXT";
+
+    const result = await new GenericPhaseRunner().run(makeGenericCtx(adapter));
+    expect(result.data.responseText).toBe("Real report body");
+  });
+});
+
+describe("CodexAdapter.extractFinalResponse IO resilience", () => {
+  let tempDir: string;
+  let cleanup: () => void;
+
+  beforeEach(() => {
+    const tmp = createTempDir();
+    tempDir = tmp.path;
+    cleanup = tmp.cleanup;
+  });
+
+  afterEach(() => cleanup());
+
+  test("does not throw when reading lastMessageFile fails, falls through to log", () => {
+    const adapter = new CodexAdapter();
+    // A directory at the lastMessageFile path makes readFileSync throw EISDIR.
+    const lastMessageFile = `${tempDir}/last.txt`;
+    mkdirSync(lastMessageFile);
+    const logFile = `${tempDir}/output.json`;
+    writeFileSync(
+      logFile,
+      JSON.stringify({ type: "item.completed", item: { type: "agent_message", text: "Final answer from log" } }),
+    );
+
+    expect(() => adapter.extractFinalResponse(logFile, lastMessageFile)).not.toThrow();
+    expect(adapter.extractFinalResponse(logFile, lastMessageFile)).toBe("Final answer from log");
+  });
+
+  test("falls through to log when lastMessageFile is empty", () => {
+    const adapter = new CodexAdapter();
+    const lastMessageFile = `${tempDir}/last.txt`;
+    writeFileSync(lastMessageFile, "   \n");
+    const logFile = `${tempDir}/output.json`;
+    writeFileSync(
+      logFile,
+      JSON.stringify({ type: "item.completed", item: { type: "agent_message", text: "Log text" } }),
+    );
+
+    expect(adapter.extractFinalResponse(logFile, lastMessageFile)).toBe("Log text");
+  });
+});

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/__tests__/review-spawner.test.ts

@@ -1,6 +1,7 @@
 import { afterEach, beforeEach, describe, expect, test } from "bun:test";
 import { writeFileSync } from "node:fs";
-import { hasNewPrFeedback, parseReviewOutcome } from "../runner/review.js";
+import { ClaudeCodeAdapter } from "../cli/claude.js";
+import { hasNewPrFeedback } from "../runner/review.js";
 import { createTempDir } from "./helpers.js";
 
 let tempDir: string;
@@ -16,15 +17,20 @@ afterEach(() => {
   cleanup();
 });
 
-describe("parseReviewOutcome", () => {
+// Production review parsing flows through ClaudeCodeAdapter.extractReviewDecision;
+// these exercise that path directly (the former parseReviewOutcome helper was a
+// thin wrapper around it and has been removed).
+describe("ClaudeCodeAdapter.extractReviewDecision", () => {
+  const adapter = new ClaudeCodeAdapter();
+
   test("parses REVIEW_RESULT:MERGED from stream-json log", () => {
     const logFile = `${tempDir}/output.json`;
     writeFileSync(logFile, [
       JSON.stringify({ type: "assistant", message: { content: [{ type: "text", text: "Looking good!" }] } }),
       JSON.stringify({ type: "assistant", message: { content: [{ type: "text", text: "REVIEW_RESULT:MERGED" }] } }),
     ].join("\n"));
 
-    const outcome = parseReviewOutcome(logFile);
+    const outcome = adapter.extractReviewDecision(logFile, "");
     expect(outcome.decision).toBe("merged");
   });
 
@@ -34,7 +40,7 @@ describe("parseReviewOutcome", () => {
       JSON.stringify({ type: "assistant", message: { content: [{ type: "text", text: "REVIEW_RESULT:NEEDS_CHANGES:Missing error handling in API call" }] } }),
     ].join("\n"));
 
-    const outcome = parseReviewOutcome(logFile);
+    const outcome = adapter.extractReviewDecision(logFile, "");
     expect(outcome.decision).toBe("needs_changes");
     expect(outcome.reason).toBe("Missing error handling in API call");
   });
@@ -45,12 +51,12 @@ describe("parseReviewOutcome", () => {
       JSON.stringify({ type: "assistant", message: { content: [{ type: "text", text: "Some review text" }] } }),
     ].join("\n"));
 
-    const outcome = parseReviewOutcome(logFile);
+    const outcome = adapter.extractReviewDecision(logFile, "");
     expect(outcome.decision).toBe("unknown");
   });
 
   test("returns unknown when log file does not exist", () => {
-    const outcome = parseReviewOutcome(`${tempDir}/nonexistent.json`);
+    const outcome = adapter.extractReviewDecision(`${tempDir}/nonexistent.json`, "");
     expect(outcome.decision).toBe("unknown");
   });
 
@@ -62,7 +68,7 @@ describe("parseReviewOutcome", () => {
       JSON.stringify({ type: "assistant", message: { content: [{ type: "text", text: "REVIEW_RESULT:MERGED" }] } }),
     ].join("\n"));
 
-    const outcome = parseReviewOutcome(logFile);
+    const outcome = adapter.extractReviewDecision(logFile, "");
     expect(outcome.decision).toBe("merged");
   });
 
@@ -72,7 +78,7 @@ describe("parseReviewOutcome", () => {
       JSON.stringify({ type: "result", result: "Done. REVIEW_RESULT:NEEDS_CHANGES:CI checks failed" }),
     ].join("\n"));
 
-    const outcome = parseReviewOutcome(logFile);
+    const outcome = adapter.extractReviewDecision(logFile, "");
     expect(outcome.decision).toBe("needs_changes");
     expect(outcome.reason).toBe("CI checks failed");
   });
@@ -83,7 +89,7 @@ describe("parseReviewOutcome", () => {
       JSON.stringify({ type: "assistant", message: { content: "All good. REVIEW_RESULT:MERGED" } }),
     ].join("\n"));
 
-    const outcome = parseReviewOutcome(logFile);
+    const outcome = adapter.extractReviewDecision(logFile, "");
     expect(outcome.decision).toBe("merged");
   });
 });

src/cli/codex.ts

@@ -161,9 +161,14 @@ export class CodexAdapter implements CliAdapter {
   }
 
   extractFinalResponse(logFile: string, lastMessageFile: string): string | null {
-    if (existsSync(lastMessageFile)) {
-      const text = readFileSync(lastMessageFile, "utf-8").trim();
-      if (text) return text;
+    try {
+      if (existsSync(lastMessageFile)) {
+        const text = readFileSync(lastMessageFile, "utf-8").trim();
+        if (text) return text;
+      }
+    } catch {
+      // IO race (file removed/locked between existsSync and read) — fall through
+      // to the stream-json log instead of throwing out of result handling.
     }
 
     const texts = this.extractTextFromLog(logFile);

src/runner/generic.ts

@@ -57,8 +57,11 @@ export class GenericPhaseRunner implements PhaseRunner {
     // Read the report file that the CLI was instructed to write
     const reportPath = `${workDir}/${REPORT_FILE}`;
     let responseText: string | null = null;
-    if (existsSync(reportPath)) {
-      responseText = readFileSync(reportPath, "utf-8");
+    // Treat an empty/whitespace-only report as missing so we still fall through to
+    // the stream-json fallback instead of returning an empty responseText.
+    const reportContent = existsSync(reportPath) ? readFileSync(reportPath, "utf-8") : null;
+    if (reportContent !== null && reportContent.trim().length > 0) {
+      responseText = reportContent;
       logTask(task.identifier, `Report file found: ${REPORT_FILE} (${responseText.length} chars)`);
       // Also copy to the plans directory so builtin:execution can find it
       const planPath = `${workDir}/critters/plans/${task.identifier}.md`;
@@ -67,7 +70,12 @@ export class GenericPhaseRunner implements PhaseRunner {
       }
     } else {
       // Fallback: extract from stream-json output
-      logTaskWarn(task.identifier, `No ${REPORT_FILE} found — extracting from CLI output`);
+      logTaskWarn(
+        task.identifier,
+        reportContent !== null
+          ? `${REPORT_FILE} was empty — extracting from CLI output`
+          : `No ${REPORT_FILE} found — extracting from CLI output`,
+      );
       const jsonLogFile = `${workDir}/.critter-output-${phase.name}.json`;
       const lastMessageFile = `${workDir}/.critter-last-message-${phase.name}.txt`;
       responseText = ctx.cliAdapter.extractFinalResponse(jsonLogFile, lastMessageFile);

src/runner/index.ts

@@ -35,6 +35,6 @@ export function getPhaseRunner(phase: PhaseConfig): PhaseRunner {
 export { ExecutionPhaseRunner } from "./execution.js";
 export { GenericPhaseRunner } from "./generic.js";
 export { PlanningPhaseRunner } from "./planning.js";
-export { parseReviewOutcome, ReviewPhaseRunner } from "./review.js";
+export { ReviewPhaseRunner } from "./review.js";
 export type { PhaseContext, PhaseResult, PhaseRunner } from "./types.js";
 export { validatePhaseResult } from "./validate.js";