worktree: remove platform-specific process chdir guard, handle all OSes uniformly#126
Merged
shayne-snap merged 3 commits intoMay 25, 2026
Merged
Conversation
Close cancelled the service context but returned without waiting for in-flight turn, side-question, local-submit, and MCP startup goroutines to drain. After Close, those goroutines could still write to caller- owned state (e.g. ~/.whale under a t.TempDir HOME), racing test cleanup and producing flaky "directory not empty" failures like the one in TestSilentPromptRewriteAppliesBeforeSkillDetection on CI. Track every Service-owned goroutine on a WaitGroup via a goTracked helper and Wait on it in Close before tearing the app down. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
RemoveCurrentWorktree unconditionally chdir'd the process out of the
worktree before removal. The comment explains the constraint is
Windows-only ("Windows refuses to remove a directory that is a
process's current working directory") — but a process-wide chdir on
Linux/macOS races with any concurrent goroutine that resolves relative
paths, and `git worktree remove` works fine on those platforms with cwd
inside the removed tree. Gate the chdir on runtime.GOOS == "windows".
When removal failed we also silently swallowed the chdir-back error
with `_ = os.Chdir(previousCwd)`, leaving the process stuck in the
wrong cwd with no signal. Wrap any restore failure into the returned
error so the caller learns about it.
Both behaviours are exercised through package-level indirection
(worktreeRemoveNeedsProcessChdir, worktreeChdirFn) so the Windows path
and the restore-failure path can be covered on any host.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…es uniformly Replace the runtime.GOOS=="windows" toggle with a general cwd-in-worktree detection using path comparison (pathInside). This ensures the process cwd is moved out of the managed worktree before 'git worktree remove' on all platforms, not just Windows: - Adds worktreeRemovalCwdState() helper that checks whether the current directory is inside the worktree using resolved absolute paths. - Adds pathInside() utility for directory containment checks with symlink resolution. - Restores the previous cwd after removal regardless of OS, surfacing any restore failure explicitly to the caller. - Removes the worktreeRemoveNeedsProcessChdir indirection point and simplifies the test helpers accordingly.
shayne-snap
force-pushed
the
fix/test-silent-prompt-rewrite-tempdir-cleanup
branch
from
5d06431 to
ce4c119
Compare
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.
Summary
Remove the Windows-only guard for process cwd management during worktree removal. The new implementation uses general path-inside-worktree detection that works uniformly across all operating systems.
Changes
git worktree removeon all platforms, not just WindowsworktreeRemoveNeedsProcessChdirindirection point and simplified the test helpersWhy
The old approach restricted process cwd management to Windows only, based on the assumption that POSIX systems handle removal fine when cwd is inside the worktree. However, on POSIX systems the process is left with a deleted cwd after a successful removal, which can lead to confusing behaviour for subsequent operations. This change ensures consistent, safe behaviour across all platforms.
Testing
make testpasses