feat: add --worktree flag for isolated write-capable rescue tasks#137
feat: add --worktree flag for isolated write-capable rescue tasks#137peterdrier wants to merge 7 commits into
Conversation
When /codex:rescue runs with --write, it edits files directly in the working directory. This creates conflicts with concurrent uncommitted work or parallel agent editing. The new --worktree flag creates an isolated git worktree so Codex operates on a separate copy of the repo. After completion, the user sees a diff summary and can choose to apply changes to their working tree or discard them. A new worktree-cleanup subcommand handles the keep/discard lifecycle. Changes: - git.mjs: worktree create/remove/diff/apply primitives - worktree.mjs: session lifecycle module (create, diff, cleanup) - render.mjs: worktree result and cleanup renderers - codex-companion.mjs: --worktree flag parsing, executeTaskRunWithWorktree, handleWorktreeCleanup subcommand, background task worker support - rescue.md, codex-rescue.md, codex-cli-runtime SKILL.md: flag routing Closes openai#135 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…bId rendering - Diff from baseCommit within the worktree path to capture uncommitted changes - Preserve worktree and branch when patch apply fails instead of destroying them - Use original repo cwd for state/thread lookups so --resume-last works in worktrees - Pass jobId explicitly to renderWorktreeTaskResult so cleanup commands are usable Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stage all changes (git add -A) before diffing so new files created in the worktree are included in the patch. Use a temp file for git apply to avoid stdin hang. Suppress spurious error in runCommand when exit status is 0. Adds 12 worktree tests covering session creation, diff capture (uncommitted, committed, untracked), cleanup safety, and render output. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b6472f9c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!result.applied) { | ||
| return result; |
There was a problem hiding this comment.
cleanupWorktreeSession returns early whenever applyWorktreePatch reports applied: false, but that includes the normal "No changes to apply." case, so worktree-cleanup --action keep leaves the worktree and branch behind after a no-op run. In practice, a task that makes no edits is reported as a failed keep operation and accumulates stale codex/* branches/worktrees instead of cleaning up.
Useful? React with 👍 / 👎.
| const worktreesDir = path.join(repoRoot, ".worktrees"); | ||
| fs.mkdirSync(worktreesDir, { recursive: true }); |
There was a problem hiding this comment.
Creating the worktree under repoRoot/.worktrees makes the main checkout immediately dirty (.worktrees/.../ becomes untracked) while the rescue task is active. That interacts badly with working-tree review flows: auto-scope sees untracked content and collectWorkingTreeContext tries to read untracked paths as files, which crashes on this directory entry (EISDIR) and breaks /codex:review until cleanup. Using an external temp location (or otherwise avoiding untracked directories in the repo root) avoids this regression.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The dirty-state concern is valid and we've addressed it, but we can't move worktrees outside the repo root. Claude Code's permission sandbox is scoped to the project directory — a worktree at an external path (e.g. /tmp/...) triggers an auth/permission prompt on every single tool use (Read, Edit, Write, Bash), making the feature unusable.
Instead, createWorktree now writes .worktrees/ to the target repo's .git/info/exclude (the per-repo gitignore that doesn't touch tracked files). This prevents the untracked-directory issue without modifying the user's .gitignore or requiring worktrees to live outside the sandbox boundary.
…ees from target repos cleanupWorktreeSession now distinguishes "no changes" from "apply failed" so keep=true with no edits still removes the worktree and branch instead of leaving them behind. createWorktree writes .worktrees/ to .git/info/exclude so the directory doesn't dirty the target repo's working tree. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bca062f80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const excludePath = path.join(repoRoot, ".git", "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`); |
There was a problem hiding this comment.
createWorktree builds excludePath as <repoRoot>/.git/info/exclude and then calls mkdirSync on that parent path, which fails with ENOTDIR when repoRoot is itself a linked Git worktree (where .git is a file, not a directory). In that common setup, task --worktree aborts before the run starts, so the new isolation flow is unusable from linked worktrees. Resolve the real git dir/common dir via Git instead of assuming .git is a directory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in peterdrier/codex-plugin-cc@92a3126 — now uses git rev-parse --git-dir to resolve the real git directory instead of hardcoding .git/. Works correctly when the repo is a linked worktree (where .git is a file).
| } else { | ||
| lines.push(`Failed to apply changes: ${result.detail}`); | ||
| lines.push(""); | ||
| lines.push(`The worktree and branch \`${session.branch}\` have been preserved at \`${session.worktreePath}\` for manual recovery.`); | ||
| } |
There was a problem hiding this comment.
When keep has no diff, cleanupWorktreeSession now correctly deletes the worktree/branch but still returns { applied: false, detail: "No changes to apply." }; this renderer branch interprets every applied:false as a failed apply and says the worktree was preserved. That produces incorrect operator guidance (it tells users to recover from a path that was already removed), so the no-op keep path should be rendered as a successful cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in peterdrier/codex-plugin-cc@92a3126 — added an explicit branch for detail === "No changes to apply." that renders as a successful cleanup instead of the misleading "failed/preserved" message.
6b5a3ef to
4bca062
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b5a3ef3bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| COMMAND=$(echo "$INPUT" | jq -r '.tool_input.command // empty') | ||
|
|
||
| # Match bare "codex" at the start of the command or after a pipe/semicolon/&& | ||
| if echo "$COMMAND" | grep -qE '(^|[;&|]\s*)codex\s'; then |
There was a problem hiding this comment.
codex invocations in the PreToolUse regex
The matcher (^|[;&|]\s*)codex\s only matches when codex is followed by whitespace, so a command that is exactly codex is not blocked and exits successfully. That bypasses the direct-CLI guard for the default interactive entrypoint, allowing untracked/non-plugin Codex runs despite this hook being installed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in peterdrier/codex-plugin-cc@28d3909 — pattern now uses codex([[:space:]]|$) to also catch bare codex with no arguments.
| if echo "$COMMAND" | grep -q 'codex-companion\.mjs'; then | ||
| exit 0 |
There was a problem hiding this comment.
The allow condition is a plain substring match for codex-companion.mjs, so commands like codex --help; echo codex-companion.mjs pass the check and are incorrectly allowed. Because the hook exits early on this match, direct codex execution can be bypassed by appending arbitrary text containing that filename.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already fixed in peterdrier/codex-plugin-cc@d910366 — the companion allowlist was removed entirely. The regex itself (codex([[:space:]]|$)) cannot match codex-companion since the hyphen is neither whitespace nor end-of-string.
- Use `git rev-parse --git-dir` instead of hardcoded `.git/` path for the exclude file, fixing ENOTDIR when repo is a linked worktree. - Render no-op keep cleanup as success instead of misleading "failed" message when there were simply no changes to apply. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70727cfbdb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const gitDir = gitChecked(repoRoot, ["rev-parse", "--git-dir"]).stdout.trim(); | ||
| const excludePath = path.join(gitDir, "info", "exclude"); |
There was a problem hiding this comment.
git rev-parse --git-dir can return a relative path (commonly .git), but excludePath is built from that value without anchoring it to repoRoot. When /codex:task --write --worktree is launched from a subdirectory, this makes fs.mkdirSync/appendFileSync write to <current-subdir>/.git/info/exclude instead of the repository’s real exclude file, creating a bogus nested .git directory and leaving the actual repo unprotected from .worktrees entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in peterdrier/codex-plugin-cc@256df3a — now resolves the relative path from git rev-parse --git-dir against repoRoot using path.resolve(repoRoot, rawGitDir).
git rev-parse --git-dir returns a relative path (e.g. ".git") in normal repos. Resolve it against repoRoot so the exclude file path is correct regardless of working directory. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 256df3a6e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| lines.push(`- **Keep**: \`node "\${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" worktree-cleanup ${resolvedJobId} --action keep\``); | ||
| lines.push(`- **Discard**: \`node "\${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" worktree-cleanup ${resolvedJobId} --action discard\``); |
There was a problem hiding this comment.
The rendered keep/discard commands omit --cwd, but worktree-cleanup resolves job state from the caller’s current workspace (resolveCommandWorkspace) and then readStoredJob(...). If a user follows the common flow of cd-ing into the shown worktree path before cleanup, the command fails with "No stored job found" because state was written under the original repo root. Please include the originating repo path in these commands (or make cleanup locate jobs by ID across workspaces) so cleanup works regardless of current directory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in peterdrier/codex-plugin-cc@4fe8077 — rendered cleanup commands now include --cwd with the original repo root so they work regardless of the caller's current directory.
Ensures cleanup works regardless of the caller's current directory by passing the original repo root via --cwd. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fe80773b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const result = cleanupWorktreeSession(session, { keep: action === "keep" }); | ||
| const rendered = renderWorktreeCleanupResult(action, result, session); | ||
| outputCommandResult({ jobId, action, result, session }, rendered, options.json); |
There was a problem hiding this comment.
The worktree-cleanup handler always exits successfully after rendering output, even when --action keep fails to apply the patch (result.applied === false with an error detail). In that conflict path the command reports a failure message but still returns exit code 0, so wrappers/automation cannot distinguish a successful keep from a preserved-worktree failure and may continue under a false success state. This should set a non-zero exit code for failed keep applies (while still allowing the explicit no-op No changes to apply. case to remain successful).
Useful? React with 👍 / 👎.
Add a disposable-git-worktree mode so /opencode:rescue --write --worktree runs OpenCode inside .worktrees/opencode-<ts> on a fresh opencode/<ts> branch instead of editing the working tree in place. Useful for exploratory runs, parallel rescues, and running against a dirty tree. Pieces: - lib/git.mjs: createWorktree / removeWorktree / deleteWorktreeBranch / getWorktreeDiff / applyWorktreePatch. Adds .worktrees/ to .git/info/exclude on first use so the dir never shows in status. - lib/worktree.mjs: session wrapper — createWorktreeSession, diffWorktreeSession, cleanupWorktreeSession (keep applies patch back, discard just removes). - opencode-companion.mjs: handleTask threads --worktree + swaps cwd + stores session data on the job record + renders a keep/discard footer. New worktree-cleanup subcommand reads the stored session and runs the keep or discard path. - agents/opencode-rescue.md, commands/rescue.md, skills/opencode-runtime: propagate --worktree through the forwarding layer. - tests/worktree.test.mjs: create, diff, keep-applies, discard, no-change no-op. Closes #43. Port of openai/codex-plugin-cc#137.
* fix: quote $ARGUMENTS in cancel/result/status commands Unquoted $ARGUMENTS allows shell splitting on user-supplied job IDs containing metacharacters. Wrap in double quotes to match review.md and adversarial-review.md. Closes #38. Port of openai/codex-plugin-cc#168. * fix: declare model: sonnet in opencode-rescue agent frontmatter Without a model declaration the agent tier was unpredictable. The rescue subagent is a thin forwarder that invokes the companion via a single Bash call and applies trivial routing logic — sonnet is sufficient and gives users a cost guarantee. Closes #39. Port of openai/codex-plugin-cc#169. * fix: scope /opencode:cancel default to current Claude session Without ref, resolveCancelableJob now filters running jobs by sessionId so a cancel in session A cannot kill jobs in session B. Explicit ref still searches all sessions — naming a job counts as intent. Closes #45. Port of openai/codex-plugin-cc#84. * fix: enforce hard wall-clock timeout on runTrackedJob Wrap the runner with Promise.race against a 30-minute default timeout. On expiry the job transitions to failed/phase:failed so zombie 'running' rows can't accumulate when a runner hangs. OPENCODE_COMPANION_JOB_TIMEOUT_MS overrides the default. Closes #41. Port of openai/codex-plugin-cc#184. * fix: reconcile dead-PID jobs on every status read Adds isProcessAlive helper and reconcileIfDead / reconcileAllJobs / markDeadPidJobFailed in job-control. buildStatusSnapshot and the handleResult/handleCancel paths now probe kill(pid, 0) on any active-state job and rewrite dead ones to failed before consuming the list. A single /opencode:status / result / cancel surfaces stuck workers without waiting for SessionEnd. markDeadPidJobFailed is race-safe: it re-reads state and refuses to downgrade terminal states or rewrite when the pid has changed. Closes #42. Port of openai/codex-plugin-cc#176 + dead-PID parts of #184. * fix: avoid embedding large diffs in review prompts Classify review scope before building the prompt. When the diff exceeds ~5 files or ~256 KB, fall back to a lightweight context (status, changed-files, diff_stat) and tell OpenCode to inspect the diff itself via read-only git commands. Prevents HTTP 400 / shallow findings on moderate-to-large changesets. Adversarial template grows a {{REVIEW_COLLECTION_GUIDANCE}} slot. Thresholds overridable via opts.maxInlineDiffFiles/Bytes. Closes #40. Port of openai/codex-plugin-cc#179. * fix: respect \$SHELL on Windows when spawning child processes Add platformShellOption() helper that picks false on POSIX, and \$SHELL || true on win32 so Git Bash users get their shell while cmd fallback still resolves .cmd/.bat shims. Apply to runCommand, spawnDetached, resolveOpencodeBinary, getOpencodeVersion, and the ensureServer spawn of 'opencode serve'. Uses 'where' instead of 'which' on win32, and parses the first line of its CRLF-separated output. Closes #46. Port of openai/codex-plugin-cc#178. * fix: migrate tmpdir state to CLAUDE_PLUGIN_DATA + fix /tmp literal The fallback path was hard-coded to '/tmp' — broken on Windows. Use os.tmpdir() so Windows and other platforms get a real tmp path. Additionally: when CLAUDE_PLUGIN_DATA is set on a later call but state was previously written to the tmpdir fallback, copy it into the plugin-data dir and rewrite absolute path references inside state.json and jobs/*.json so logFile pointers don't dangle. Prevents job history from being silently dropped when commands run under different env contexts within one Claude session. Closes #47. Port of openai/codex-plugin-cc#125. * feat: pass last review findings to rescue automatically After a successful /opencode:review or /opencode:adversarial-review, save the rendered output to ~/.opencode-companion/last-review-<hash>.md (per repo, SHA-256 of workspace path). Add a new 'last-review' subcommand that reports availability or streams the content. rescue.md now checks for a saved review when invoked without task text and asks via AskUserQuestion whether to fix the prior findings or describe a new task. The save is best-effort — a failed persistence never fails the review itself. Closes #44. Port of openai/codex-plugin-cc#129 (simplified: logic lives in the companion script rather than an inline node -e one-liner). * feat: throttle controls for stop-time review gate Add --review-gate-max and --review-gate-cooldown flags to /opencode:setup so users can bound the review gate's spend: /opencode:setup --review-gate-max 5 /opencode:setup --review-gate-cooldown 10 /opencode:setup --review-gate-max off The stop hook now loads state before touching stdin, checks reviewGateMaxPerSession and reviewGateCooldownMinutes against the current session's reviewGateUsage entry, and allows the stop without running OpenCode when a limit would be exceeded. Usage entries older than 7 days are pruned on each successful run so state.json doesn't grow unbounded. renderSetup surfaces the configured limits. Closes #48. Port of openai/codex-plugin-cc#20. * feat: --worktree flag for isolated write-capable rescue tasks Add a disposable-git-worktree mode so /opencode:rescue --write --worktree runs OpenCode inside .worktrees/opencode-<ts> on a fresh opencode/<ts> branch instead of editing the working tree in place. Useful for exploratory runs, parallel rescues, and running against a dirty tree. Pieces: - lib/git.mjs: createWorktree / removeWorktree / deleteWorktreeBranch / getWorktreeDiff / applyWorktreePatch. Adds .worktrees/ to .git/info/exclude on first use so the dir never shows in status. - lib/worktree.mjs: session wrapper — createWorktreeSession, diffWorktreeSession, cleanupWorktreeSession (keep applies patch back, discard just removes). - opencode-companion.mjs: handleTask threads --worktree + swaps cwd + stores session data on the job record + renders a keep/discard footer. New worktree-cleanup subcommand reads the stored session and runs the keep or discard path. - agents/opencode-rescue.md, commands/rescue.md, skills/opencode-runtime: propagate --worktree through the forwarding layer. - tests/worktree.test.mjs: create, diff, keep-applies, discard, no-change no-op. Closes #43. Port of openai/codex-plugin-cc#137. * fix: address pr51 review findings * fix: keep tracked job timeout referenced * fix: address pr51 review conversations * fix: add exclusive file lock to updateState for concurrency safety updateState's read-modify-write cycle was not protected against concurrent companion processes (background worker + status/cancel handler), which could silently lose each other's writes. Acquire an exclusive lock file (state.json.lock via O_EXCL) before reading, hold it through mutation and write, release in finally. Stale locks older than 30s are evicted. Blocks up to 5s with retry. Closes the pre-existing concurrency race amplified by PR #51's dead-PID reconciliation (which adds upsertJob calls on every status read). * fix: address brownfield discovery bugs Critical/high fixes: - BUG-1: saveLastReview use copyFileSync+unlinkSync instead of renameSync (fixes Windows compatibility issue where rename fails if target exists) - BUG-2: handleTask worktree leak - wrap in try/finally to guarantee cleanup - BUG-3: State migration race - add fallback directory lock during migration - BUG-4/13: handleTaskWorker missing signal handlers for graceful shutdown Medium fixes: - BUG-5: releaseStateLock now fsyncs directory after lock removal - BUG-11: Error from getConfiguredProviders now logged instead of swallowed Low fixes: - BUG-6: PR number validation now rejects negative values - BUG-7: getBundledConfigDir checks directory exists before returning - BUG-8: tailLines now properly filters empty lines after split - BUG-9: resolveReviewAgent always returns tools property (undefined if not used) - BUG-10: Diff retrieval failure now logs warning instead of silent swallow - BUG-12: resolveOpencodeBinary now handles spawn errors properly Additional pre-existing work included: - safe-command.mjs wrapper for secure command execution - Command documentation updates - Test improvements * fix: polish pr51 follow-up fixes * fix: address Copilot PR#51 review comments Four findings, all valid: C1 (prompts.mjs, git.mjs, process.mjs) — buildReviewPrompt materialized the full diff string before checking thresholds. For huge diffs the git/gh subprocess could OOM the companion before the size check ran. Fix: runCommand gains maxOutputBytes, killing the child and reporting overflowed once the cap is exceeded. getDiff and getPrDiff thread maxBytes through. buildReviewPrompt now bounds the read at maxBytes+1 and treats overflow as over-byte-limit without ever materializing the rest. C2 (git.mjs) — getDiffByteSize had a docstring claiming it avoided streaming the full contents, but the implementation did exactly that. It was also dead code (zero callers). Removed. C3 (tests/state-lock.test.mjs) — the test injected path.resolve(...) into a generated ESM import specifier. On Windows that path contains backslashes and a drive letter, producing an invalid module specifier. Fix: pathToFileURL(...).href for the injected specifier. C4 (tests/dead-pid-reconcile.test.mjs) — beforeEach mutated the object returned by loadState() without saving it back, leaving on-disk state from earlier tests intact. Fix: saveState(workspace, { config:{}, jobs:[] }). Adds coverage: - tests/process.test.mjs: runCommand overflow path and non-overflow path. - tests/review-prompt-size.test.mjs: bounds-huge-diff end-to-end test that writes a 50k-byte file and asserts fewer than 10k 'x' chars land in the prompt. All 167 tests pass. * fix: keep reading fallback state while migrate lock is held Addresses a Codex P1 on PR#51: when another migrator holds \`primaryDir.migrate.lock\`, migrateTmpdirStateIfNeeded waits up to 2s and then returns without copying anything. Before this fix, stateRoot still returned primaryDir — but primary/state.json didn't exist yet, so loadState returned an empty state and the next upsertJob created primary/state.json with only the new entry, orphaning every seeded fallback job. Fix: after a migration attempt, if primary/state.json is absent and fallback/state.json is present, stateRoot returns the fallback dir. Reads see real data, writes land in fallback, and a later migration retry can pick them up cleanly. Adds a regression test that pre-creates the migrate lock, seeds fallback with a job, switches to CLAUDE_PLUGIN_DATA, and verifies that stateRoot falls back, loadState sees the seeded job, and a subsequent write preserves both the seeded and the in-flight rows. The symlink-refusal test had to be updated because it was reusing stateRoot to name the "primary" dir — with the new fallback guard, that call now returns fallbackDir on failed migration. The test now computes the expected primary path directly.
Cycle-review of PR #11 (Codex + Claude subagent) found three defects, all leading to irreversible deletion of the only copy of worktree changes. All verified empirically; all present in upstream openai#137 too. 1. process.mjs runCommand normalized a signal-terminated child to status:0. spawnSync returns status:null when the child is killed by a signal; the old `status: result.status ?? 0` turned that into 0, so a git-apply killed mid-run (or any signal-terminated git op) read as success. In worktree cleanup that means reporting "applied" then destroying the source worktree and branch even though the patch never landed. Now: signal != null => status 1. (Codex critical) 2. git.mjs getWorktreeDiff / applyWorktreePatch used unchecked `git add -A`. A failed snapshot (index lock held by a concurrent git op, disk full, perm) was classified identically to an empty diff => "No changes to apply" => keep-then- force-remove the only copy. Switched to gitChecked (fail-closed throw) and separated status!==0 from the genuine empty case. (Codex critical) 3. git.mjs removeWorktree matched git's "is not a working tree" stderr text. That message is locale-translated (ru_RU: "не является рабочим каталогом") — on non-English locales the guard failed and removeWorktree threw on an already-removed path, so cleanupWorktreeSession aborted before deleteWorktreeBranch and the codex/<ts> branch leaked forever. Now locale- safe: check `git worktree list` and no-op when the path isn't registered. (Claude subagent HIGH — confirmed on this machine LANG=ru_RU.UTF-8) Regression tests: - process.test.mjs: signal-terminated child => non-zero status. - worktree.test.mjs: removeWorktree is a no-op when the worktree is already gone. Verified: worktree 13/13, process 4/4, git 11/11 green; full npm test 155 pass / 4 pre-existing (unchanged, unrelated). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189cGaohsYdpkB4otKZnpAt
… on openai#137) (#12) * feat(worktree): isolated rescue worktree, leave-branch cleanup (based on openai#137) 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#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 <path> && git branch -D <branch>` 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189cGaohsYdpkB4otKZnpAt * fix(worktree): staged-aware recovery instructions + signal/spawn fail-closed runCommand Cycle-1 review of PR #12 (Codex + Claude subagent). Two real findings: 1. (Codex) The rendered inspection command was plain `git -C <path> 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 <baseCommit> --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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189cGaohsYdpkB4otKZnpAt --------- Co-authored-by: axisrow <axisrow@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@peterdrier — first, thank you for this PR. It's the reason any of what follows exists. I forked openai/codex-plugin-cc and started from your worktree-isolation design — the session lifecycle, the diff capture, the test shape. That foundation is what gave me something concrete to build on. I spent the better part of a day on the cleanup path, and I wouldn't even have known what to work on if you hadn't gone first. So this comment is genuinely standing on your shoulders. What I ended up with, in case it's useful to you: I kept hitting one class of bug in May I suggest considering this angle:
Concretely: UPD (2026-07-20): after landing the leave-branch refactor in my fork, I ran a security audit on the worktree-isolation code and found four more issues — all in patterns inherited from this PR. All four are now fixed in my fork. Sharing in case any of it is useful to you:
The pattern across all four: detection-based fixes kept finding new bypasses each cycle; eliminating the vulnerable path (rather than guarding it) closed each class permanently. Same principle as leave-branch for the cleanup path. My fork PRs: #12 (leave-branch refactor), #18 (#1 fix), #19+#20 (#2 fix), #22 (#3 fix), #23 (#4 fix). All with attribution to this PR as the foundation. Happy to talk through any of it. UPD 2 (2026-07-21) — one more, on the rescue-turn side. Since a |
….gitignore (closes #16) Redesign (3rd eliminate in the worktree security series, after leave-branch and core.symlinks=false). The original createWorktree wrote .worktrees/ into <gitDir>/info/exclude to keep the worktree dir out of git status. That write was the entire #16 vulnerability: if gitDir resolved outside repoRoot (separate-git-dir, crafted .git gitfile, linked worktree), the exclude write landed at an attacker-controlled location. Detection-based fixes (boundary checks) broke after 2 iterations: git introspection cannot authenticate external common-dirs — legit linked worktrees gitdir in <main>/.git/worktrees/ is indistinguishable from attack separate-git-dir via path inspection. Two independent Codex runs converged on the same conclusion: eliminate the write, don't detect. Fix: delete the entire git-dir/info/exclude block. git worktree add works WITHOUT it (the exclude was cosmetic, from openai#137 commit 4bca062). Keep .worktrees/ out of git status via an in-repo .worktrees/.gitignore containing *, written inside the already-validated worktreesDir (#14 guard ensures inside repoRoot, not a symlink). No git-dir metadata write = no write outside repoRoot = #16 dead. Regression tests: separate-git-dir repo now works (previously attack case); linked worktree works (previously broken by boundary check); external info/exclude untouched. Verified: worktree 18/18; full npm test 161 pass / 4 pre-existing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
….gitignore (closes #16) (#22) Redesign (3rd eliminate in the worktree security series, after leave-branch and core.symlinks=false). The original createWorktree wrote .worktrees/ into <gitDir>/info/exclude to keep the worktree dir out of git status. That write was the entire #16 vulnerability: if gitDir resolved outside repoRoot (separate-git-dir, crafted .git gitfile, linked worktree), the exclude write landed at an attacker-controlled location. Detection-based fixes (boundary checks) broke after 2 iterations: git introspection cannot authenticate external common-dirs — legit linked worktrees gitdir in <main>/.git/worktrees/ is indistinguishable from attack separate-git-dir via path inspection. Two independent Codex runs converged on the same conclusion: eliminate the write, don't detect. Fix: delete the entire git-dir/info/exclude block. git worktree add works WITHOUT it (the exclude was cosmetic, from openai#137 commit 4bca062). Keep .worktrees/ out of git status via an in-repo .worktrees/.gitignore containing *, written inside the already-validated worktreesDir (#14 guard ensures inside repoRoot, not a symlink). No git-dir metadata write = no write outside repoRoot = #16 dead. Regression tests: separate-git-dir repo now works (previously attack case); linked worktree works (previously broken by boundary check); external info/exclude untouched. Verified: worktree 18/18; full npm test 161 pass / 4 pre-existing. Co-authored-by: axisrow <axisrow@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Closes #135
Summary
--worktreeflag to/codex:rescuethat runs Codex in an isolated git worktree with write access, returning keep/discard actions instead of editing the working tree in-placegit add -Abefore diffing--resume-laststate resolution to use the original repo root, not the worktree pathJOB_IDplaceholdergit applyto avoid stdin hang, suppresses spuriousspawnSyncerror on exit 0Test plan
tests/worktree.test.mjscovering session creation, diff capture (uncommitted, committed, untracked), cleanup safety (keep, discard, conflict preservation), and render outputkeepfailed gracefully preserving the worktree and branchprocess,git,rendersuites all pass)🤖 Generated with Claude Code