fix(cli): preserve committed worktrees on forced shutdown - #189
fix(cli): preserve committed worktrees on forced shutdown#189jasonqlwilliams-alt wants to merge 5 commits into
Conversation
The exit handler registered for new worktrees would unconditionally delete the worktree when process.exit() was called before the normal preservation block could null out worktreeCleanup. This happened on: - Double Ctrl+C (force-stop) - Graceful-shutdown timeout (5s) - Orchestrator crash via die() Fix: the exit handler now checks orchestrator state before cleaning up. If commitCount > 0 or hasPendingCommitFailure is true, the worktree is preserved even when the normal code path was bypassed.
![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: 3c6782dc6f
ℹ️ 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 state = getOrchestratorState?.(); | ||
| if (!state) return false; | ||
| return ( | ||
| state.commitCount > 0 || state.hasPendingCommitFailure === true |
There was a problem hiding this comment.
When a successful commitAll() is followed by an exception before recordSuccess() refreshes state.commitCount - for example, appendNotes() fails due to an I/O error - the fatal path calls process.exit() while this cached count is still zero. The exit handler therefore removes a worktree that already contains a commit, contrary to the required preservation rule; determine commit presence from the repository or update the state immediately after commitAll() succeeds.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
2 participants
Intent
Land community PR #168 to stop gnhf from deleting a --worktree checkout that still holds commits when force shutdown bypasses normal cleanup, specifically the double-Ctrl+C and shutdown-timeout half of issue #167. #168 is the selected fix over #170 and that choice must not be reopened. Keep the change narrowly limited to force-shutdown preservation: do not fix, refactor, or otherwise alter the separate Linux sleep-prevention re-exec parent-exit defect owned by sibling task gnhf-worktree-selfdelete-d2. Preserve Eric Cao's original commits and authorship, adding only follow-up commits needed for correctness or validation. A forced shutdown after commits must preserve the worktree, while a forced shutdown with zero commits and no pending commit failure must still remove it. Before timeout exit, when the worktree is preserved, print its absolute path on its own clear, copyable line. Add a committed automated regression test scoped to the existing preservation logic: assert that the commit-bearing cleanup path leaves the worktree intact and reports its absolute path. Do not build new double-SIGINT testing infrastructure. Unit tests alone are not sufficient evidence: retain the narrower automated regression and put the completed manual signal-path reproduction in the PR body with exact steps and observations. Manual proof: temporarily set FORCE_EXIT_TIMEOUT_MS to 0 and temporarily make the mock opencode agent complete iteration 1 and hang on iteration 2; in a fresh temporary git repository with a temporary HOME, sanitized git config, telemetry disabled, e2e fixtures on PATH, and prevent-sleep off, launch the built CLI with prompt hang after first commit and agent opencode in worktree mode; wait for the first commit and second message:start, then send two SIGINT signals 20 ms apart. Before #168 the result was exit 130, one timeout hit, 2 branch commits, 0 worktree directories, and 1 worktree registration. With #168 the result was exit 130, one timeout hit, 2 branch commits, 1 worktree directory, and 2 worktree registrations. The zero-commit control used prompt slow cleanup and produced exit 130, one timeout hit, 0 worktree directories, and 1 registration. The contributed regression test originally passed without the fix, so it was strengthened to hold the orchestrator open through timeout and proven to fail on unfixed behavior. Keep the diff tight and reviewable, do not add em dashes, and keep all repository CI commands green. If rebasing onto current main incorporates sibling d2 such that #168's state guard is redundant or contradictory, stop for a needs-decision keyed pr168-superseded-by-d2 rather than silently dropping or retaining it.
What Changed
--worktreecheckouts during exit cleanup when the run has commits or a pending commit failure, including forced-shutdown paths.Risk Assessment
✅ Low: The change narrowly preserves commit-bearing or commit-failure worktrees across forced exit paths, retains zero-commit cleanup, reports the absolute preserved path before timeout exit, and adds behavior-oriented regression coverage without altering the separate sleep-prevention path.
Testing
The focused regression and zero-commit signal control passed. The built-CLI double-SIGINT reproduction exited 130 with one timeout, two branch commits, one preserved worktree directory, two registrations, and its absolute path printed on a clear line; all temporary testing edits were restored.
Evidence: Double-SIGINT manual reproduction
Exit 130; one timeout; two branch commits; one preserved worktree directory; two worktree registrations; absolute path reported.Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
pnpm exec vitest run src/cli.test.ts -t "preserves and reports a worktree with commits when exit handler fires before preservation block"pnpm run build && pnpm exec vitest run e2e/e2e.test.ts -t "cleans up the worktree when no changes are made in --worktree mode"Temporarily setFORCE_EXIT_TIMEOUT_MSto 0 and made the mock OpenCode agent hang on iteration 2, then rannode /tmp/no-mistakes-evidence/01KZKC7FTMKEPYFQ73T0139REG/manual-signal-repro.mjsagainst the built CLI and sent two SIGINT signals 20 ms apartgit status --short && git diff -- src/cli.ts e2e/fixtures/mock-opencode-server.mjsafter restoring temporary instrumentation✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.