From c4cbf366a622c6ee0857407085659f52e8f1a316 Mon Sep 17 00:00:00 2001 From: axisrow Date: Sun, 19 Jul 2026 23:23:17 +0800 Subject: [PATCH 1/2] feat(worktree): isolated rescue worktree, leave-branch cleanup (based on openai#137) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds git-worktree isolation for write-capable rescue (/codex:rescue --worktree), the write-race fix tracked in fork#10 / openai#135. This commit ports the library layer + tests only; companion (--worktree flag) and markdown wiring follow in subsequent PRs. Based on @peterdrier's openai/codex-plugin-cc#137, refactored to a "leave-branch" cleanup model. The original capture-then-remove model (snapshot -> apply patch -> force-remove worktree + delete branch) 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. Four adversarial review cycles against the straight port found 10 data-loss paths of this shape, each fix closing one edge and the next cycle finding another. Removing the Destroy path removes the entire class by construction. Design (leave-branch): cleanupWorktreeSession NEVER removes the worktree or its branch. keep applies the tracked patch into repoRoot (byte-preserving via `git diff --cached --binary --output`, no JS-string round-trip) and leaves the worktree + branch for the user to inspect and remove manually. discard is a no-op. The cost is one `git worktree remove --force && git branch -D ` per rescue — acceptable for a rare, high-stakes operation whose whole point is write-safety. TOCTOU and the discard-after-failed-keep trap close for free because there is no Destroy path to gate. Files: - plugins/codex/scripts/lib/git.mjs: createWorktree, getWorktreeDiff (gitChecked, throw on failure), applyWorktreePatch (byte-preserving patch transfer). No removeWorktree/deleteWorktreeBranch/hasIgnoredChanges — those belonged to the removed Destroy path. - plugins/codex/scripts/lib/worktree.mjs (NEW): createWorktreeSession / diffWorktreeSession / cleanupWorktreeSession (leave-branch contract). - plugins/codex/scripts/lib/render.mjs: renderWorktreeTaskResult / renderWorktreeCleanupResult — always preservation + manual-remove instructions. - tests/worktree.test.mjs (NEW, 11 tests): session creation, diff capture, and the leave-branch invariant (worktree + branch survive keep tracked-only / ignored-only / mixed tracked+ignored, and discard no-op), byte round-trip, render. The invariant tests replace the old removal-assertion tests and are the proof the fail-open class is closed. Verified: worktree 11/11, full npm test 152 pass / 4 pre-existing (unchanged). Refs fork#10, openai#135, openai#137 (@peterdrier). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_0189cGaohsYdpkB4otKZnpAt --- plugins/codex/scripts/lib/git.mjs | 73 +++++++ plugins/codex/scripts/lib/render.mjs | 52 +++++ plugins/codex/scripts/lib/worktree.mjs | 65 ++++++ tests/worktree.test.mjs | 284 +++++++++++++++++++++++++ 4 files changed, 474 insertions(+) create mode 100644 plugins/codex/scripts/lib/worktree.mjs create mode 100644 tests/worktree.test.mjs diff --git a/plugins/codex/scripts/lib/git.mjs b/plugins/codex/scripts/lib/git.mjs index c401f0ca1..601202b81 100644 --- a/plugins/codex/scripts/lib/git.mjs +++ b/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); diff --git a/plugins/codex/scripts/lib/render.mjs b/plugins/codex/scripts/lib/render.mjs index 2ec185236..a62e046cc 100644 --- a/plugins/codex/scripts/lib/render.mjs +++ b/plugins/codex/scripts/lib/render.mjs @@ -445,6 +445,58 @@ 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. Review the diff, then remove:"); + lines.push(""); + lines.push(`- \`git -C ${session.worktreePath} diff\` (review)`); + lines.push(`- \`git worktree remove --force ${session.worktreePath} && git branch -D ${session.branch}\` (discard)`); + lines.push(""); + lines.push("_Ignored files (e.g. .env, dist/) and submodule changes are NOT captured by the patch — copy them out of the worktree before removing._"); + + 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", diff --git a/plugins/codex/scripts/lib/worktree.mjs b/plugins/codex/scripts/lib/worktree.mjs new file mode 100644 index 000000000..059e1fa96 --- /dev/null +++ b/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 && git branch -D ` 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)}` + }; +} diff --git a/tests/worktree.test.mjs b/tests/worktree.test.mjs new file mode 100644 index 000000000..c704bf314 --- /dev/null +++ b/tests/worktree.test.mjs @@ -0,0 +1,284 @@ +import fs from "node:fs"; +import path from "node:path"; +import test from "node:test"; +import assert from "node:assert/strict"; + +import { + createWorktreeSession, + diffWorktreeSession, + cleanupWorktreeSession +} from "../plugins/codex/scripts/lib/worktree.mjs"; +import { getWorktreeDiff } from "../plugins/codex/scripts/lib/git.mjs"; +import { renderWorktreeTaskResult } from "../plugins/codex/scripts/lib/render.mjs"; +import { initGitRepo, makeTempDir, run } from "./helpers.mjs"; + +function gitStdout(cwd, args) { + const result = run("git", args, { cwd }); + assert.equal(result.status, 0, result.stderr); + return result.stdout.trim(); +} + +function commitFile(cwd, fileName = "app.js", contents = "export const value = 1;\n") { + fs.writeFileSync(path.join(cwd, fileName), contents); + assert.equal(run("git", ["add", fileName], { cwd }).status, 0); + const commit = run("git", ["commit", "-m", "init"], { cwd }); + assert.equal(commit.status, 0, commit.stderr); +} + +function createRepoWithInitialCommit() { + const repoRoot = makeTempDir(); + initGitRepo(repoRoot); + commitFile(repoRoot); + return { repoRoot }; +} + +// Manual worktree+branch cleanup for test fixtures (the production library +// leaves them in place by design; tests must tidy up themselves). +function removeSession(session) { + if (!session) { + return; + } + try { + run("git", ["worktree", "remove", "--force", session.worktreePath], { cwd: session.repoRoot }); + } catch { + // already gone + } + try { + run("git", ["branch", "-D", session.branch], { cwd: session.repoRoot }); + } catch { + // already gone + } +} + +test("createWorktreeSession returns session with worktreePath, branch, repoRoot, baseCommit", () => { + const { repoRoot } = createRepoWithInitialCommit(); + const session = createWorktreeSession(repoRoot); + + try { + // macOS symlinks /var → /private/var; git canonicalizes repoRoot while + // makeTempDir returns the symlinked path. Compare via realpath like #497. + assert.equal(fs.realpathSync(session.repoRoot), fs.realpathSync(repoRoot)); + assert.match(session.branch, /^codex\/\d+$/); + assert.equal(fs.realpathSync(session.worktreePath), fs.realpathSync(path.join(repoRoot, ".worktrees", `codex-${session.timestamp}`))); + assert.ok(session.baseCommit); + assert.ok(fs.existsSync(session.worktreePath)); + } finally { + removeSession(session); + } +}); + +test("createWorktreeSession baseCommit matches repo HEAD at creation time", () => { + const { repoRoot } = createRepoWithInitialCommit(); + const headAtCreation = gitStdout(repoRoot, ["rev-parse", "HEAD"]); + const session = createWorktreeSession(repoRoot); + + try { + fs.writeFileSync(path.join(repoRoot, "app.js"), "export const value = 2;\n"); + assert.equal(run("git", ["add", "app.js"], { cwd: repoRoot }).status, 0); + const commit = run("git", ["commit", "-m", "repo-root change"], { cwd: repoRoot }); + assert.equal(commit.status, 0, commit.stderr); + + const newHead = gitStdout(repoRoot, ["rev-parse", "HEAD"]); + assert.equal(session.baseCommit, headAtCreation); + assert.notEqual(newHead, session.baseCommit); + } finally { + removeSession(session); + } +}); + +test("diffWorktreeSession captures uncommitted changes in the worktree", () => { + const { repoRoot } = createRepoWithInitialCommit(); + const session = createWorktreeSession(repoRoot); + + try { + fs.writeFileSync(path.join(session.worktreePath, "app.js"), "export const value = 2;\n"); + + const diff = diffWorktreeSession(session); + + assert.deepEqual(diff, getWorktreeDiff(session.worktreePath, session.baseCommit)); + assert.notEqual(diff.stat, ""); + assert.match(diff.stat, /app\.js/); + } finally { + removeSession(session); + } +}); + +test("diffWorktreeSession captures new untracked files in the worktree", () => { + const { repoRoot } = createRepoWithInitialCommit(); + const session = createWorktreeSession(repoRoot); + + try { + fs.writeFileSync(path.join(session.worktreePath, "newfile.js"), "export const added = true;\n"); + + const diff = diffWorktreeSession(session); + + assert.notEqual(diff.stat, ""); + assert.match(diff.stat, /newfile\.js/); + assert.match(diff.patch, /added = true/); + } finally { + removeSession(session); + } +}); + +test("diffWorktreeSession returns empty when no changes made", () => { + const { repoRoot } = createRepoWithInitialCommit(); + const session = createWorktreeSession(repoRoot); + + try { + const diff = diffWorktreeSession(session); + assert.deepEqual(diff, { stat: "", patch: "" }); + } finally { + removeSession(session); + } +}); + +// --------------------------------------------------------------------------- +// Leave-branch invariant: the worktree + branch ALWAYS survive cleanup. +// These replace the old capture-then-remove tests. Under leave-branch there is +// no Destroy path, so the whole fail-open class (ignored/mixed/submodule/TOCTOU) +// is closed by construction — the assertions below are the proof. +// --------------------------------------------------------------------------- + +test("keep applies tracked changes to repoRoot AND preserves the worktree + branch", () => { + const { repoRoot } = createRepoWithInitialCommit(); + const session = createWorktreeSession(repoRoot); + + try { + fs.writeFileSync(path.join(session.worktreePath, "newfile.js"), "export const added = true;\n"); + + const result = cleanupWorktreeSession(session, { keep: true }); + + assert.equal(result.applied, true); + assert.equal(result.preserved, true); + assert.ok(fs.existsSync(path.join(repoRoot, "newfile.js")), "tracked change applied to repoRoot"); + // INVARIANT: worktree + branch still exist. + assert.equal(fs.existsSync(session.worktreePath), true, "worktree preserved"); + const branches = gitStdout(repoRoot, ["branch", "--list", session.branch]); + assert.match(branches, new RegExp(session.branch), "branch preserved"); + } finally { + removeSession(session); + } +}); + +test("keep with only ignored changes preserves the worktree (ignored files are NOT lost)", () => { + const { repoRoot } = createRepoWithInitialCommit(); + fs.writeFileSync(path.join(repoRoot, ".gitignore"), "dist/\n"); + assert.equal(run("git", ["add", ".gitignore"], { cwd: repoRoot }).status, 0); + assert.equal(run("git", ["commit", "-m", "gitignore"], { cwd: repoRoot }).status, 0); + + const session = createWorktreeSession(repoRoot); + + try { + fs.mkdirSync(path.join(session.worktreePath, "dist"), { recursive: true }); + fs.writeFileSync(path.join(session.worktreePath, "dist", "artifact.txt"), "precious\n"); + + const result = cleanupWorktreeSession(session, { keep: true }); + + // No tracked changes to apply, but the ignored artifact MUST survive. + assert.equal(result.preserved, true); + assert.equal(fs.existsSync(session.worktreePath), true); + assert.equal( + fs.readFileSync(path.join(session.worktreePath, "dist", "artifact.txt"), "utf8"), + "precious\n", + "ignored artifact preserved (not destroyed)" + ); + } finally { + removeSession(session); + } +}); + +test("keep with mixed tracked + ignored changes applies tracked AND preserves ignored", () => { + const { repoRoot } = createRepoWithInitialCommit(); + fs.writeFileSync(path.join(repoRoot, ".gitignore"), "dist/\n"); + assert.equal(run("git", ["add", ".gitignore"], { cwd: repoRoot }).status, 0); + assert.equal(run("git", ["commit", "-m", "gitignore"], { cwd: repoRoot }).status, 0); + + const session = createWorktreeSession(repoRoot); + + try { + // Tracked edit + ignored artifact at the same time. + fs.writeFileSync(path.join(session.worktreePath, "app.js"), "export const value = 2;\n"); + fs.mkdirSync(path.join(session.worktreePath, "dist"), { recursive: true }); + fs.writeFileSync(path.join(session.worktreePath, "dist", "build.txt"), "compiled\n"); + + const result = cleanupWorktreeSession(session, { keep: true }); + + assert.equal(result.applied, true, "tracked change applied"); + assert.equal(result.preserved, true); + assert.match(fs.readFileSync(path.join(repoRoot, "app.js"), "utf8"), /value = 2/); + // INVARIANT: worktree still exists, ignored artifact intact. + assert.equal(fs.existsSync(session.worktreePath), true); + assert.equal( + fs.readFileSync(path.join(session.worktreePath, "dist", "build.txt"), "utf8"), + "compiled\n", + "ignored artifact preserved alongside tracked apply" + ); + } finally { + removeSession(session); + } +}); + +test("discard is a no-op that preserves the worktree + branch (never destroys)", () => { + const { repoRoot } = createRepoWithInitialCommit(); + const session = createWorktreeSession(repoRoot); + + try { + fs.writeFileSync(path.join(session.worktreePath, "app.js"), "export const value = 2;\n"); + + const result = cleanupWorktreeSession(session, { keep: false }); + + assert.equal(result.applied, false); + assert.equal(result.preserved, true); + // INVARIANT: discard does NOT remove anything. The work + branch survive. + assert.equal(fs.existsSync(session.worktreePath), true, "worktree preserved on discard"); + assert.equal( + fs.readFileSync(path.join(session.worktreePath, "app.js"), "utf8"), + "export const value = 2;\n", + "discarded work still present in the preserved worktree" + ); + const branches = gitStdout(repoRoot, ["branch", "--list", session.branch]); + assert.match(branches, new RegExp(session.branch), "branch preserved on discard"); + } finally { + removeSession(session); + } +}); + +test("keep round-trips a non-UTF-8 byte (0xff) without corruption (byte-preserving patch)", () => { + const { repoRoot } = createRepoWithInitialCommit(); + const session = createWorktreeSession(repoRoot); + + try { + fs.writeFileSync(path.join(session.worktreePath, "binary.bin"), Buffer.from([0xff, 0x00, 0x41, 0xff])); + + const result = cleanupWorktreeSession(session, { keep: true }); + assert.equal(result.applied, true); + + const applied = fs.readFileSync(path.join(repoRoot, "binary.bin")); + assert.deepEqual( + Array.from(applied), + [0xff, 0x00, 0x41, 0xff], + `binary bytes must round-trip exactly; U+FFFD corruption would show 0xEF 0xBF 0xBD` + ); + } finally { + removeSession(session); + } +}); + +test("renderWorktreeTaskResult renders the manual-remove instructions (no destructive command)", () => { + const { repoRoot } = createRepoWithInitialCommit(); + const session = createWorktreeSession(repoRoot); + + try { + const diff = { stat: " app.js | 2 +-\n 1 file changed", patch: "..." }; + const output = renderWorktreeTaskResult({ rendered: "task output" }, session, diff, { jobId: "job-123" }); + + assert.match(output, /Worktree \(preserved\)/); + assert.match(output, /git worktree remove --force/); + assert.match(output, /git branch -D/); + assert.match(output, /Ignored files/); + // No keep/discard CLI commands — leave-branch has no destructive action. + assert.doesNotMatch(output, /--action (keep|discard)/); + } finally { + removeSession(session); + } +}); From 14651455a23d97dfe28088f9e3093cc1bcf6f0f8 Mon Sep 17 00:00:00 2001 From: axisrow Date: Sun, 19 Jul 2026 23:58:35 +0800 Subject: [PATCH 2/2] fix(worktree): staged-aware recovery instructions + signal/spawn fail-closed runCommand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cycle-1 review of PR #12 (Codex + Claude subagent). Two real findings: 1. (Codex) The rendered inspection command was plain `git -C diff`, but rescue changes are STAGED — getWorktreeDiff/applyWorktreePatch run `git add -A`, so `git diff` (unstaged-only) can read empty even when staged tracked/binary/formerly-untracked work exists. A user following the instructions would see "no changes", then force-remove the worktree + branch and irreversibly lose the staged work. Fixed: render a HEAD/base- based diff (`git diff --binary --submodule=diff`) that includes staged state, plus a `git status --porcelain --ignored -uall` step, and a warning that ignored/submodule work is NOT in the diff. Regression test asserts the inspection command references baseCommit (not a bare `diff`). 2. (Claude) runCommand still normalized a signal-terminated OR spawn-failed child to status:0 (`result.status ?? 0`). A killed git apply (OOM, timeout) or missing git binary (ENOENT) read as success → false-success reporting on worktree capture/apply. Under leave-branch this is no longer data-loss (worktree preserved), but it's a correctness defect with the same root cause. Fixed structurally in process.mjs runCommand: `failed = signal != null || error; status = failed ? 1 : ...`. Regression tests: SIGKILL self-kill and ENOENT both report non-zero status. Codex also flagged "worktree isolation is dead code" — IRRELEVANT to this PR by design: this is the library layer; companion (--worktree wiring) is the next PR. The leave-branch invariant claim (Claude) is confirmed: no production Destroy path exists. Verified: worktree 12/12, process 5/5; full npm test 155 pass / 4 pre-existing. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_0189cGaohsYdpkB4otKZnpAt --- plugins/codex/scripts/lib/process.mjs | 13 +++++++++++-- plugins/codex/scripts/lib/render.mjs | 12 ++++++++---- tests/process.test.mjs | 19 +++++++++++++++++- tests/worktree.test.mjs | 28 ++++++++++++++++++++++++++- 4 files changed, 64 insertions(+), 8 deletions(-) diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index 01fbbfe94..dc4e661ff 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/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 }; } diff --git a/plugins/codex/scripts/lib/render.mjs b/plugins/codex/scripts/lib/render.mjs index a62e046cc..ffe398713 100644 --- a/plugins/codex/scripts/lib/render.mjs +++ b/plugins/codex/scripts/lib/render.mjs @@ -478,12 +478,16 @@ export function renderWorktreeTaskResult(execution, session, diff, { jobId = nul lines.push("### Inspect and remove manually"); lines.push(""); - lines.push("The worktree and branch are kept for inspection. Review the diff, then remove:"); + 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\` (review)`); - lines.push(`- \`git worktree remove --force ${session.worktreePath} && git branch -D ${session.branch}\` (discard)`); + 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("_Ignored files (e.g. .env, dist/) and submodule changes are NOT captured by the patch — copy them out of the worktree before removing._"); + 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`; } diff --git a/tests/process.test.mjs b/tests/process.test.mjs index c9b8390fc..2649876f1 100644 --- a/tests/process.test.mjs +++ b/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}`); +}); diff --git a/tests/worktree.test.mjs b/tests/worktree.test.mjs index c704bf314..51c56ccd4 100644 --- a/tests/worktree.test.mjs +++ b/tests/worktree.test.mjs @@ -275,10 +275,36 @@ test("renderWorktreeTaskResult renders the manual-remove instructions (no destru assert.match(output, /Worktree \(preserved\)/); assert.match(output, /git worktree remove --force/); assert.match(output, /git branch -D/); - assert.match(output, /Ignored files/); + // Inspection command must be HEAD/base-based (shows staged work) — plain + // `git diff` hides staged changes and a user could force-remove thinking + // the worktree is empty (openai#137 review finding). + assert.match(output, /diff .*--binary/); + assert.match(output, new RegExp(`diff ${session.baseCommit}`)); + assert.match(output, /ignored/i); // No keep/discard CLI commands — leave-branch has no destructive action. assert.doesNotMatch(output, /--action (keep|discard)/); } finally { removeSession(session); } }); + +// Regression: rescue changes are STAGED (getWorktreeDiff/applyWorktreePatch run +// git add -A). The rendered inspection command must be HEAD/base-based so it +// shows staged work — plain `git diff` shows only unstaged and could read empty, +// tricking the user into force-removing a worktree that holds real staged work +// (openai#137 review finding). Assert the command references baseCommit, not a +// bare `diff`. +test("renderWorktreeTaskResult inspection command references baseCommit (staged-aware)", () => { + const { repoRoot } = createRepoWithInitialCommit(); + const session = createWorktreeSession(repoRoot); + + try { + const output = renderWorktreeTaskResult({ rendered: "" }, session, { stat: "", patch: "" }); + // The bare `git diff` (unstaged-only) must NOT be the inspection command. + assert.doesNotMatch(output, /git -C [^\s]+ diff\b(?! )/); + // baseCommit must appear in a diff command (HEAD-based => shows staged). + assert.match(output, new RegExp(`diff ${session.baseCommit}`)); + } finally { + removeSession(session); + } +});