feat(worktree): isolated rescue worktree, leave-branch cleanup (based on openai#137)

plugins/codex/scripts/lib/git.mjs

@@ -297,6 +297,79 @@ function buildAdversarialCollectionGuidance(options = {}) {
   return "The repository context below is a lightweight summary. Inspect the target diff yourself with read-only git commands before finalizing findings.";
 }
 
+// Worktree isolation for write-capable rescue (write-race fix, openai#135).
+// Ported from @peterdrier's openai/codex-plugin-cc#137, refactored to a
+// "leave-branch" cleanup model: the worktree is NEVER auto-removed by this library —
+// keep applies the tracked patch and leaves the worktree + branch for the user to
+// inspect and remove manually. The original capture-then-remove model had a deep
+// fail-open class (git add -A skips ignored files; dirty submodules and
+// binary content can't be fully captured; TOCTOU between snapshot and remove)
+// — removing the Destroy path removes the class by construction.
+
+export function createWorktree(repoRoot) {
+  const ts = Date.now();
+  const worktreesDir = path.join(repoRoot, ".worktrees");
+  fs.mkdirSync(worktreesDir, { recursive: true });
+
+  // Ensure .worktrees/ is excluded from the target repo without modifying tracked files.
+  // Use git rev-parse to resolve the real git dir (handles linked worktrees where .git is a file).
+  const rawGitDir = gitChecked(repoRoot, ["rev-parse", "--git-dir"]).stdout.trim();
+  const gitDir = path.resolve(repoRoot, rawGitDir);
+  const excludePath = path.join(gitDir, "info", "exclude");
+  const excludeContent = fs.existsSync(excludePath) ? fs.readFileSync(excludePath, "utf8") : "";
+  if (!excludeContent.includes(".worktrees")) {
+    fs.mkdirSync(path.dirname(excludePath), { recursive: true });
+    fs.appendFileSync(excludePath, `${excludeContent.endsWith("\n") || !excludeContent ? "" : "\n"}.worktrees/\n`);
+  }
+
+  const worktreePath = path.join(worktreesDir, `codex-${ts}`);
+  const branch = `codex/${ts}`;
+  const baseCommit = gitChecked(repoRoot, ["rev-parse", "HEAD"]).stdout.trim();
+  gitChecked(repoRoot, ["worktree", "add", worktreePath, "-b", branch]);
+  return { worktreePath, branch, repoRoot, baseCommit, timestamp: ts };
+}
+
+export function getWorktreeDiff(worktreePath, baseCommit) {
+  // gitChecked (not git): a failed snapshot must throw, not be misclassified as
+  // "no changes" — that was the fail-open root of the capture-then-remove bugs.
+  gitChecked(worktreePath, ["add", "-A"]);
+  const result = git(worktreePath, ["diff", "--cached", baseCommit, "--stat"]);
+  if (result.status !== 0) {
+    throw new Error(`Failed to diff worktree: ${result.stderr.trim()}`);
+  }
+  if (!result.stdout.trim()) {
+    return { stat: "", patch: "" };
+  }
+  const stat = result.stdout.trim();
+  const patchResult = gitChecked(worktreePath, ["diff", "--cached", baseCommit]);
+  return { stat, patch: patchResult.stdout };
+}
+
+export function applyWorktreePatch(repoRoot, worktreePath, baseCommit) {
+  const patchPath = path.join(
+    repoRoot,
+    ".codex-worktree-" + Date.now() + "-" + Math.random().toString(16).slice(2) + ".patch"
+  );
+  try {
+    // Byte-preserving: write the patch via `git diff --cached --binary --output`
+    // (git→file, never through a JS string). runCommand decodes stdout as UTF-8,
+    // which would replace invalid bytes (0xff, NUL, legacy encodings) with U+FFFD
+    // and apply a corrupted patch as "success". Detect empty via file size.
+    gitChecked(worktreePath, ["add", "-A"]);
+    gitChecked(worktreePath, ["diff", "--cached", "--binary", baseCommit, "--output", patchPath]);
+    if (!fs.existsSync(patchPath) || fs.statSync(patchPath).size === 0) {
+      return { applied: false, detail: "No tracked changes to apply." };
+    }
+    const applyResult = git(repoRoot, ["apply", "--index", patchPath]);
+    if (applyResult.status !== 0) {
+      return { applied: false, detail: applyResult.stderr.trim() || "Patch apply failed (conflicts?)." };
+    }
+    return { applied: true, detail: "Tracked changes applied and staged." };
+  } finally {
+    fs.rmSync(patchPath, { force: true });
+  }
+}
+
 export function collectReviewContext(cwd, target, options = {}) {
   const repoRoot = getRepoRoot(cwd);
   const currentBranch = getCurrentBranch(repoRoot);

plugins/codex/scripts/lib/process.mjs

@@ -13,14 +13,23 @@ export function runCommand(command, args = [], options = {}) {
     windowsHide: true
   });
 
+  const succeeded = result.status === 0 && result.signal === null && !result.error;
+  // A child killed by a signal OR that failed to spawn (ENOENT/EAGAIN/maxBuffer)
+  // returns status:null from spawnSync. Treat any of those as a non-zero exit so
+  // callers that check status===0 don't mistake an abnormal process for success —
+  // for worktree patch capture/apply that would mean reporting a killed git op
+  // (false-success) while the work holds un-captured changes.
+  const failed = result.signal !== null || result.error;
+  const status = failed ? 1 : result.status ?? 0;
+
   return {
     command,
     args,
-    status: result.status ?? 0,
+    status,
     signal: result.signal ?? null,
     stdout: result.stdout ?? "",
     stderr: result.stderr ?? "",
-    error: result.error ?? null
+    error: succeeded ? null : result.error ?? null
   };
 }
 

plugins/codex/scripts/lib/render.mjs

@@ -445,6 +445,62 @@ export function renderStoredJobResult(job, storedJob) {
   return `${lines.join("\n").trimEnd()}\n`;
 }
 
+// Worktree isolation render (leave-branch model). The worktree + branch are
+// ALWAYS preserved — keep applies the tracked patch and leaves them for manual
+// inspection. There is no destructive discard command.
+export function renderWorktreeTaskResult(execution, session, diff, { jobId = null } = {}) {
+  const lines = [];
+
+  if (execution.rendered) {
+    lines.push(execution.rendered.trimEnd());
+    lines.push("");
+  }
+
+  lines.push("---");
+  lines.push("");
+  lines.push("## Worktree (preserved)");
+  lines.push("");
+  lines.push(`Branch: \`${session.branch}\``);
+  lines.push(`Path: \`${session.worktreePath}\``);
+  lines.push("");
+
+  if (diff.stat) {
+    lines.push("### Changes");
+    lines.push("");
+    lines.push("```");
+    lines.push(diff.stat);
+    lines.push("```");
+    lines.push("");
+  } else {
+    lines.push("Codex made no tracked file changes in the worktree.");
+    lines.push("");
+  }
+
+  lines.push("### Inspect and remove manually");
+  lines.push("");
+  lines.push("The worktree and branch are kept for inspection. Rescue changes are staged here — review with a HEAD-based diff that includes staged work:");
+  lines.push("");
+  lines.push(`- \`git -C ${session.worktreePath} diff ${session.baseCommit} --binary --submodule=diff\` (review tracked changes vs base, including staged)`);
+  lines.push(`- \`git -C ${session.worktreePath} status --porcelain --ignored -uall\` (also check for ignored/submodule work the diff will not show)`);
+  lines.push("");
+  lines.push(`Only once you have verified (or copied out) everything you need, remove:`);
+  lines.push("");
+  lines.push(`- \`git worktree remove --force ${session.worktreePath} && git branch -D ${session.branch}\``);
+  lines.push("");
+  lines.push("_Warning: ignored files (e.g. .env, dist/) and submodule changes are NOT in the diff above and are NOT captured by the apply patch — copy them out of the worktree before removing, or they are lost._");
+
+  return `${lines.join("\n").trimEnd()}\n`;
+}
+
+export function renderWorktreeCleanupResult(action, result, session) {
+  const lines = ["# Worktree Cleanup", ""];
+  lines.push(`Action: ${action}.`);
+  lines.push(`Result: ${result.detail}`);
+  lines.push("");
+  lines.push(`The worktree \`${session.worktreePath}\` and branch \`${session.branch}\` are preserved regardless of this action — remove them manually after inspecting.`);
+  return `${lines.join("\n").trimEnd()}\n`;
+}
+
 export function renderCancelReport(job) {
   const lines = [
     "# Codex Cancel",

plugins/codex/scripts/lib/worktree.mjs

@@ -0,0 +1,65 @@
+// Worktree isolation for write-capable rescue (write-race fix, openai#135).
+// Ported from @peterdrier's openai/codex-plugin-cc#137, refactored to a
+// "leave-branch" cleanup model.
+//
+// DESIGN: cleanupWorktreeSession NEVER removes the worktree or its branch.
+//   - keep:   applies the tracked patch into repoRoot, then leaves the worktree
+//              + branch in place for the user to inspect and remove manually.
+//   - discard: no-op; leaves the worktree + branch in place.
+//
+// Why: the original capture-then-remove model (snapshot → apply → force-remove)
+// had a deep fail-open class — `git add -A` skips ignored files (.env, dist/),
+// dirty submodules and binary content can't be fully captured, and there is an
+// inherent TOCTOU window between snapshot and remove. Each fix closed one edge
+// and the next review cycle found another (4 cycles, 10 findings). Removing the
+// Destroy path removes the entire class by construction. The cost is one manual
+// `git worktree remove --force <path> && git branch -D <branch>` per rescue —
+// acceptable for a rare, high-stakes operation whose whole point is write-safety.
+
+import {
+  createWorktree,
+  getWorktreeDiff,
+  applyWorktreePatch,
+  ensureGitRepository
+} from "./git.mjs";
+
+export function createWorktreeSession(cwd) {
+  const repoRoot = ensureGitRepository(cwd);
+  return createWorktree(repoRoot);
+}
+
+export function diffWorktreeSession(session) {
+  return getWorktreeDiff(session.worktreePath, session.baseCommit);
+}
+
+// Returns the manual-removal hint included in every cleanup result. The worktree
+// and branch are ALWAYS preserved; the user removes them after inspecting.
+function preservationNote(session) {
+  return `Worktree and branch preserved for inspection. Remove manually when done: git worktree remove --force ${session.worktreePath} && git branch -D ${session.branch}`;
+}
+
+/**
+ * Leave-branch cleanup. Never destroys the worktree.
+ *
+ * keep=true:  applies the tracked patch into session.repoRoot (staged), leaves
+ *             the worktree + branch in place. Returns {applied, detail}.
+ * keep=false: no-op, leaves the worktree + branch in place. Returns {applied:false, detail}.
+ *
+ * In both cases the worktree survives — the caller renders preservationNote()
+ * so the user knows how to remove it manually after inspecting.
+ */
+export function cleanupWorktreeSession(session, { keep = false } = {}) {
+  if (keep) {
+    const result = applyWorktreePatch(session.repoRoot, session.worktreePath, session.baseCommit);
+    return {
+      ...result,
+      preserved: true,
+      detail: `${result.detail} ${preservationNote(session)}`
+    };
+  }
+  return {
+    applied: false,
+    preserved: true,
+    detail: `Worktree discarded-from-transfer (left in place). ${preservationNote(session)}`
+  };
+}

tests/process.test.mjs

@@ -1,7 +1,7 @@
 import test from "node:test";
 import assert from "node:assert/strict";
 
-import { terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs";
+import { terminateProcessTree, runCommand } from "../plugins/codex/scripts/lib/process.mjs";
 
 test("terminateProcessTree uses taskkill on Windows", () => {
   let captured = null;
@@ -84,3 +84,20 @@ test("terminateProcessTree recognizes a missing Windows process with localized o
   assert.equal(outcome.delivered, false);
   assert.equal(outcome.method, "taskkill");
 });
+
+// Regression: a child killed by a signal OR that failed to spawn returns
+// status:null from spawnSync. runCommand must NOT normalize that to status:0 —
+// callers checking status===0 would mistake a killed/missing git op (e.g. git
+// apply killed mid-run, or git missing) for success. For worktree patch capture/
+// apply that means false-success while the work holds un-captured changes.
+test("runCommand reports a signal-terminated child as failed (non-zero status)", () => {
+  const result = runCommand(process.execPath, ["-e", "process.kill(process.pid, 'SIGKILL')"]);
+  assert.ok(result.signal !== null, `expected a signal, got signal=${result.signal}`);
+  assert.notEqual(result.status, 0, `signal-terminated child must not report status 0, got ${result.status}`);
+});
+
+test("runCommand reports a spawn failure (ENOENT) as failed (non-zero status)", () => {
+  const result = runCommand("/nonexistent-codex-binary-xyz", []);
+  assert.ok(result.error, `expected an error, got ${result.error}`);
+  assert.notEqual(result.status, 0, `spawn failure must not report status 0, got ${result.status}`);
+});