feat(worktree): port worktree session core from openai#137 (step 3a/3)#11
Closed
axisrow wants to merge 4 commits into
Closed
feat(worktree): port worktree session core from openai#137 (step 3a/3)#11axisrow wants to merge 4 commits into
axisrow wants to merge 4 commits into
Conversation
Ports the isolated-git-worktree primitives that will back the rescue --worktree flag (write-race fix, openai#135 / fork#10). This step lands the library layer + tests; companion/markdown integration follows in 3b/3c. From openai#137 (@peterdrier): - plugins/codex/scripts/lib/worktree.mjs (NEW): createWorktreeSession / diffWorktreeSession / cleanupWorktreeSession — session lifecycle wrappers. - plugins/codex/scripts/lib/git.mjs: createWorktree, removeWorktree, deleteWorktreeBranch, getWorktreeDiff, applyWorktreePatch (add .worktrees/ to info/exclude, branch as codex/<ts>, diff captures untracked + uncommitted, keep applies patch via temp file with safe cleanup). - plugins/codex/scripts/lib/process.mjs: suppress spurious spawnSync error on exit 0 (error: succeeded ? null : ...). - plugins/codex/scripts/lib/render.mjs: renderWorktreeTaskResult + renderWorktreeCleanupResult (render jobId in keep/discard commands). Included here because tests/worktree.test.mjs imports them. - tests/worktree.test.mjs (NEW, 12 tests): session creation, diff capture (uncommitted/committed/untracked), cleanup safety (keep/discard/conflict preservation), render output. Fix one macOS-specific assertion (tests/worktree.test.mjs:52): git canonicalizes repoRoot to /private/var while makeTempDir returns /var — compare via realpathSync, same pattern as openai#497. Verified: worktree.test.mjs 12/12 green; git/process/render suites green; full npm test 153 pass / 4 pre-existing failures (unchanged, unrelated). Refs fork#10, openai#135, openai#137. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189cGaohsYdpkB4otKZnpAt
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
Owner
Author
🔍 Local review (cycle 1)Reviewed locally (Claude subagent + Codex companion), no bots pinged. Three data-loss defects, all leading to irreversible deletion of the only copy of worktree changes. All verified empirically; all present in upstream openai#137 too.
Triage of cycle 1/3 complete: 3 FIX (applied), 1 SKIP. Cycle 2 review re-runs on the updated diff (4d57e1c). |
Cycle-2 review of PR #11 (Codex found two more fail-open paths; Claude subagent verified the cycle-1 fixes clean but missed these). Both confirmed empirically. 1. process.mjs runCommand still normalized a spawn failure to status:0. The cycle-1 fix covered `signal !== null` but not `error` (ENOENT/EAGAIN/ maxBuffer — spawnSync returns status:null, signal:null, error:set). A missing/failed git op still read as success (status ?? 0 → 0), so worktree cleanup could destroy the only copy. Now: `failed = signal != null || error`; failed => status 1. (Codex critical) 2. git.mjs applyWorktreePatch round-tripped the patch through a JS string. runCommand decodes stdout as UTF-8, replacing invalid bytes (0xff, NUL, legacy encodings) with U+FFFD; git apply then applied the corrupted patch as "success" and the source worktree was destroyed — silent data loss for any binary or non-UTF-8 file Codex touched. Now byte-preserving: write the patch via `git diff --cached --binary --output=<file>` (git→file, no JS string), detect empty by file size, apply. (Codex critical) Regression tests: - process.test.mjs: ENOENT spawn failure => non-zero status. - worktree.test.mjs: keep round-trips a raw 0xff byte without U+FFFD corruption. Verified: worktree 14/14, process 5/5, git 11/11 green; full npm test 157 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
Owner
Author
🔍 Local review (cycle 2)Codex found two more fail-open paths; Claude subagent verified the cycle-1 fixes clean but missed these. Both confirmed empirically.
Triage cycle 2/3: 2 FIX applied, 0 open. Cycle 3 re-runs on 5748958. |
Cycle-3 review (Codex, final cycle) found a fourth data-loss path in the same fail-open class. `git add -A` skips git-ignored paths (.env, dist/, .output, build artifacts) — so a worktree whose only changes landed in ignored files produces an empty patch. cleanupWorktreeSession then returned "No changes to apply" and force-removed the worktree + branch, irreversibly destroying the artifacts. Confirmed empirically; this very repo ignores exactly .env/dist/ .output — paths Codex/rescue writes to routinely. Fix: when keep produces an empty tracked patch, check hasIgnoredChanges() (`git status --porcelain --ignored -uall`, !! entries). If ignored work exists, preserve the worktree and branch and surface the situation in the result detail instead of removing. Fail-safe: never lose work silently. If the user wants those ignored files gone, they remove the worktree explicitly. Regression test: worktree.test.mjs "keep preserves the worktree when only ignored files changed" — creates dist/artifact.txt in an ignored worktree, asserts keep returns applied:false + "ignored files" message AND the worktree + artifact survive. Verified: worktree 15/15, process 5/5, git 11/11 green; full npm test 158 pass / 4 pre-existing (unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189cGaohsYdpkB4otKZnpAt
Owner
Author
🔍 Local review (cycle 3)Codex found a fourth data-loss path (same fail-open class). User extended past the 3-cycle cap to close it.
Confirmed empirically — this repo ignores exactly .env/dist/.output (paths Codex/rescue writes to). Triage cycle 3: 1 FIX applied, 0 open. Regression test: worktree.test.mjs ignored-preserve. Cycle 4 re-runs on 3e42783. |
Owner
Author
|
Superseded by #12 (leave-branch refactor). This PR (#11) was the straight port of openai#137 with its capture-then-remove cleanup, patched across 4 review cycles for data-loss fail-open bugs. The class proved unclosable under that model, so #12 ports the same isolation with a leave-branch cleanup that removes the Destroy path entirely. Branch deleted; see #12 for the merged approach. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Step 3a of porting openai#137 (
--worktreeisolation for write-capable rescue, fixes fork#10 / openai#135 write-race). This step lands the library layer + tests; companion/markdown integration follows in 3b/3c.What
Isolated-git-worktree primitives from @peterdrier's openai#137:
plugins/codex/scripts/lib/worktree.mjs(NEW) —createWorktreeSession/diffWorktreeSession/cleanupWorktreeSession: session lifecycle wrappers over git.mjs.plugins/codex/scripts/lib/git.mjs—createWorktree,removeWorktree,deleteWorktreeBranch,getWorktreeDiff,applyWorktreePatch. Adds.worktrees/toinfo/exclude(no tracked-file change), branches ascodex/<ts>, diff captures uncommitted + committed + untracked,keepapplies a patch via temp file with safe cleanup; preserves the worktree on apply failure.plugins/codex/scripts/lib/process.mjs— suppress spuriousspawnSyncerror on exit 0.plugins/codex/scripts/lib/render.mjs—renderWorktreeTaskResult+renderWorktreeCleanupResult(render the real job id in keep/discard commands). Bundled here becausetests/worktree.test.mjsimports them.tests/worktree.test.mjs(NEW, 12 tests) — session creation, diff capture (uncommitted/committed/untracked), cleanup safety (keep/discard/conflict-preservation), render output.Adaptation vs upstream openai#137
One macOS-specific fix in
tests/worktree.test.mjs:52: git canonicalizesrepoRootto/private/var/...whilemakeTempDir()returns/var/...(the/var → /private/varsymlink). Compare viafs.realpathSync— same pattern as the openai#497 fix. Otherwise the port is byte-for-byte with openai#137.Why split
openai#137 changes 9 files / +496. Splitting into 3a (library+tests, this PR) → 3b (companion
--worktreeintegration) → 3c (markdown contract atop fork #8's default---background) keeps each PR reviewable and lets each ride its own cycle-review. This PR does not yet wire--worktreeintotask— that is 3b.Verified
worktree.test.mjs12/12 green;git/process/rendersuites green; fullnpm test153 pass / 4 pre-existing failures (unchanged, unrelated — env-leak on this machine).Refs fork#10, openai#135, openai#137.