Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Windows-specific detection and termination for 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/windows-worktree-setup-runner-kill.test.ts (1)
88-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the relative-gitdir branch.
resolveWorktreeGitDirPathLine 50 (resolve(trimmed, raw)) is the one untested path here; Git does write relativegitdir:entries.src/main/runtime/worktree-teardown.test.ts (1)
849-860: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert ordering, not just invocation.
The regression in
#10629is specifically that the sweep must run after PTY teardown. This test would still pass if the call were hoisted to the top ofkillAllProcessesForWorktree. Comparingmock.invocationCallOrderagainst the providershutdown/stop spy would pin the contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 93100688-e7ff-4b2e-847d-10d3dd8ba307
📥 Commits
Reviewing files that changed from the base of the PR and between c4b7aed and eb6e4bd716726026d617711ea10e4f3ed1785cba.
📒 Files selected for processing (4)
src/main/runtime/worktree-teardown.test.tssrc/main/runtime/worktree-teardown.tssrc/main/windows-worktree-setup-runner-kill.test.tssrc/main/windows-worktree-setup-runner-kill.ts
Sync update (
|
3d481a9 to
ee51018
Compare
Sync update (
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7168fd65-101d-43aa-86cf-16f37a78085c
📥 Commits
Reviewing files that changed from the base of the PR and between 3838f6f and ee51018be7ce73bd80a4cecb83176b04bfacc5f1.
📒 Files selected for processing (4)
src/main/runtime/worktree-teardown.test.tssrc/main/runtime/worktree-teardown.tssrc/main/windows-worktree-setup-runner-kill.test.tssrc/main/windows-worktree-setup-runner-kill.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/runtime/worktree-teardown.ts
- src/main/runtime/worktree-teardown.test.ts
- src/main/windows-worktree-setup-runner-kill.test.ts
…ai#10629) On Windows, cmd.exe processes left running setup-runner.cmd from a worktree's private gitdir keep directory handles open so git worktree remove / rmdir fail with Permission denied. After PTY teardown, scan process CommandLines and taskkill /T /F matching setup-runner trees.
…#10629) Address CodeRabbit: race the setup-runner kill with the teardown deadline via settleBeforeDeadline, and drop basename-only linked-worktree matching so same-named worktrees under other repos are not force-killed.
ee51018 to
911968d
Compare
Sync update (
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Sync update (
|
There was a problem hiding this comment.
🧹 Nitpick comments (7)
src/main/windows-worktree-setup-runner-kill.ts (3)
95-104: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePaths that contain spaces are truncated by this pattern.
[^\s"']*?stops at the first space. Windows worktree paths often contain spaces, for exampleC:\Users\Jane Doe\repo\.... For a quoted command line the extractor then yields a partial path that starts after the space, andisPathInsideOrEqualat Line 141 rejects it.The substring fallback at Lines 149-153 still matches these command lines, so behavior stays correct today. Consider extracting quoted arguments first so the primary path-containment check handles them.
242-257: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe returned count reports attempted kills, not confirmed kills.
Each
killTreerejection is swallowed at Line 246, andkilledPidsis the fulltargetPidslist. The caller logs this value askilled N Windows setup-runner process tree(s)at Line 177. Whentaskkillfails with access denied, the log reports a kill that did not happen. The test at Line 191 encodes this behavior.No control flow depends on the count, so this is an observability wording issue. Either count only the settled successes, or change the log text to
attempted.
85-91: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle directory-read error codes before falling back.
readFileon a directory can returnEPERMorEACCESas well asEISDIRon Windows. IfresolveWorktreeGitDirPathdrops those cases,.gitmain-checkouts can lose their anchor and the sweep falls back to worktree-path-only matching. Accept the alternate error codes to keep the fallback reliable.♻️ Proposed fix
} catch (error) { // Why: main checkouts store objects under a `.git` directory, not a gitdir file. - if ((error as NodeJS.ErrnoException | undefined)?.code === 'EISDIR') { + const code = (error as NodeJS.ErrnoException | undefined)?.code + if (code === 'EISDIR' || code === 'EPERM' || code === 'EACCES') { return join(trimmed, '.git') } return null }src/main/windows-worktree-setup-runner-kill.test.ts (4)
45-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese two tests are identical; replace the second one.
The test at Lines 55-63 passes the same command line, the same
worktreePath, and the samepathAnchorsas the test at Lines 45-53, and asserts the same result. Its title claims a separate scenario, so it adds no coverage and misleads a reader.Retarget it at the substring fallback branch in
commandLineTargetsWorktreeSetupRunner(implementation Lines 149-153). No current test reaches that branch, because every positive case also satisfies the path-containment branch at Line 141.♻️ Proposed replacement
- it('matches linked-worktree setup-runner when the command references the gitdir anchor', () => { - expect( - commandLineTargetsWorktreeSetupRunner( - 'C:\\Windows\\system32\\cmd.exe /c D:/repo/.git/worktrees/feature/orca/setup-runner.cmd', - worktreePath, - ['D:/repo/.git/worktrees/feature'] - ) - ).toBe(true) - }) + it('matches via the substring fallback when the runner path is not drive-qualified', () => { + expect( + commandLineTargetsWorktreeSetupRunner( + `cmd.exe /c setup-runner.cmd --worktree ${worktreePath}`, + worktreePath + ) + ).toBe(true) + })
100-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a relative
gitdir:value.
resolveWorktreeGitDirPathresolves a relative gitdir against the worktree path at implementation Line 84. Git writes relative gitdir pointers when a worktree is created with--relative-paths, so this branch is reachable. No test pins it.♻️ Proposed additional test
it('returns null when .git is missing', async () => { + // placeholder anchor for the added test belowAdd this test inside the
resolveWorktreeGitDirPathdescribe block:it('resolves a relative gitdir against the worktree path', async () => { const readFileImpl = vi.fn( async () => 'gitdir: ../../.git/worktrees/feature\n' ) as unknown as typeof fsPromises.readFile await expect( resolveWorktreeGitDirPath(join('/repo', 'workspaces', 'feature'), { readFileImpl }) ).resolves.toBe(resolve(join('/repo', 'workspaces', 'feature'), '../../.git/worktrees/feature')) })Import
resolvealongsidejoinat Line 1.
217-230: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInject
resolveGitDirPathto remove the real filesystem read.This test omits
pathAnchors, soterminateWindowsSetupRunnersForWorktreecalls the realresolveWorktreeGitDirPath, which readsD:\orca\workspaces\repo\feature\.gitfrom disk. The assertion still holds on macOS, Linux, and Windows because enumeration throws regardless. Injecting the dependency keeps the test hermetic and faster.♻️ Proposed fix
terminateWindowsSetupRunnersForWorktree('D:\\orca\\workspaces\\repo\\feature', { platform: 'win32', + resolveGitDirPath: async () => null, listProcessRows, killTree })
331-345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the
console.infospy in afinallyblock or anafterEachhook.
info.mockRestore()runs only when every assertion between Lines 333 and 344 passes. If one fails, the spy stays installed and suppressesconsole.infofor the remaining tests in this file. That turns one failure into cascading, hard-to-diagnose failures.♻️ Proposed fix
describe('terminateWindowsSetupRunnersForWorktreeId', () => { + afterEach(() => { + vi.restoreAllMocks() + }) + it('resolves the worktree path and kills matching runners', async () => {expect(info).toHaveBeenCalledWith( expect.stringContaining('killed 1 Windows setup-runner process tree(s)') ) - info.mockRestore() })Add
afterEachto the Vitest import at Line 3.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fec3029e-1424-4002-8994-fbfcbfe6b4cd
📒 Files selected for processing (4)
src/main/runtime/worktree-teardown.test.tssrc/main/runtime/worktree-teardown.tssrc/main/windows-worktree-setup-runner-kill.test.tssrc/main/windows-worktree-setup-runner-kill.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/runtime/worktree-teardown.ts
- src/main/runtime/worktree-teardown.test.ts
|
Closing because #10629 was marked completed. I am not keeping a conflicting Windows setup-runner teardown branch open against a completed issue. Happy to reopen if a current-main reproduction still shows orphaned |
3 participants
Summary
setup-runner.cmdprocesses whose CommandLine references the worktree’s private gitdir path.killAllProcessesForWorktreesogit worktree remove/ rmdir are not blocked by orphan handles.Fixes
Closes #10629
Test plan
windows-worktree-setup-runner-kill+worktree-teardownunit testsELI5
Orphan
setup-runner.cmdprocesses could hold Windows worktree directories open so delete failed. Orca force-kills those orphans tied to the worktree's gitdir before removal.