Fix stuck lane delete and Codex resume recovery#508
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Owner
Author
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughThis PR hardens lane operations against stale or missing worktrees by introducing comprehensive validation, updates AI agent guidance to permit read-only inspection outside worktrees, refactors PTY session resume logic to async with backfill support, and documents the new ChangesWorktree validation and deletion resilience
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Copilot stopped work on behalf of
arul28 due to an error
June 2, 2026 05:45
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1049
to
+1052
| const probe = spawnSync("git", ["rev-parse", "--path-format=absolute", "--show-toplevel"], { | ||
| cwd: resolvedRoot, | ||
| encoding: "utf8", | ||
| timeout: 8_000, |
There was a problem hiding this comment.
spawnSync with 8-second timeout in UI render path
isLaneWorktreeAvailable is called synchronously from laneWorktreeUnavailableMessage during TUI rendering. After the 2-second cache TTL expires, any lane whose directory has a .git marker will trigger spawnSync("git", ...) with timeout: 8_000. This blocks the Node.js event loop and the entire Ink TUI for up to 8 seconds on each cache miss. The original code did only an fs.statSync. With git lock contention or a slow filesystem this becomes a hard freeze.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/ade-cli/src/tuiClient/app.tsx
Line: 1049-1052
Comment:
**`spawnSync` with 8-second timeout in UI render path**
`isLaneWorktreeAvailable` is called synchronously from `laneWorktreeUnavailableMessage` during TUI rendering. After the 2-second cache TTL expires, any lane whose directory has a `.git` marker will trigger `spawnSync("git", ...)` with `timeout: 8_000`. This blocks the Node.js event loop and the entire Ink TUI for up to 8 seconds on each cache miss. The original code did only an `fs.statSync`. With git lock contention or a slow filesystem this becomes a hard freeze.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+67
to
+69
| function normAbs(p: string): string { | ||
| return path.resolve(p); | ||
| } |
There was a problem hiding this comment.
normAbs uses path.resolve, which does not resolve symlinks. On macOS /tmp is a symlink to /private/tmp, so if lane.worktreePath is stored as /tmp/ade-worktrees/my-lane but git rev-parse --show-toplevel returns /private/tmp/ade-worktrees/my-lane, the equality check fails and a valid worktree is rejected as stale. normalizeWorktreePath in app.tsx explicitly uses fs.realpathSync.native for this reason.
Suggested change
| function normAbs(p: string): string { | |
| return path.resolve(p); | |
| } | |
| function normAbs(p: string): string { | |
| const resolved = path.resolve(p); | |
| try { | |
| return fs.realpathSync.native(resolved); | |
| } catch { | |
| return resolved; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/git/gitOperationsService.ts
Line: 67-69
Comment:
`normAbs` uses `path.resolve`, which does **not** resolve symlinks. On macOS `/tmp` is a symlink to `/private/tmp`, so if `lane.worktreePath` is stored as `/tmp/ade-worktrees/my-lane` but `git rev-parse --show-toplevel` returns `/private/tmp/ade-worktrees/my-lane`, the equality check fails and a valid worktree is rejected as stale. `normalizeWorktreePath` in `app.tsx` explicitly uses `fs.realpathSync.native` for this reason.
```suggestion
function normAbs(p: string): string {
const resolved = path.resolve(p);
try {
return fs.realpathSync.native(resolved);
} catch {
return resolved;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+274
to
+280
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| // Test mocks can throw for unhandled git probes. The real runGit helper | ||
| // returns a GitRunResult for normal git failures, so keep those tests on | ||
| // their existing path without weakening production validation. | ||
| return /^Unexpected git call:/i.test(message); | ||
| } |
There was a problem hiding this comment.
true when the error message matches "Unexpected git call:" — a pattern that only originates from test mocks. Embedding test-fixture bypass logic in production code means any future mock refactor could silently disable the guard, and any production error starting with that phrase would also bypass validation.
Suggested change
| } catch (error) { | |
| const message = error instanceof Error ? error.message : String(error); | |
| // Test mocks can throw for unhandled git probes. The real runGit helper | |
| // returns a GitRunResult for normal git failures, so keep those tests on | |
| // their existing path without weakening production validation. | |
| return /^Unexpected git call:/i.test(message); | |
| } | |
| } catch { | |
| return false; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/lanes/laneService.ts
Line: 274-280
Comment:
This catch branch returns `true` when the error message matches `"Unexpected git call:"` — a pattern that only originates from test mocks. Embedding test-fixture bypass logic in production code means any future mock refactor could silently disable the guard, and any production error starting with that phrase would also bypass validation.
```suggestion
} catch {
return false;
}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| ["rev-parse", "--path-format=absolute", "--show-toplevel"], | ||
| { cwd: lane.worktreePath, timeoutMs: 8_000 }, | ||
| ); | ||
| if (!topLevelRes || typeof topLevelRes.exitCode !== "number") return; |
There was a problem hiding this comment.
assertLaneWorktreeRoot silently returns (passes validation) when runGit yields null, undefined, or an object without a numeric exitCode. In those cases the guard is skipped and the caller proceeds to mutate an unverified path. The same silent-pass pattern exists in isExpectedGitWorktreeRoot in laneService.ts (line 269). Treating an unrecognised response as valid inverts the safe-default principle.
Suggested change
| if (!topLevelRes || typeof topLevelRes.exitCode !== "number") return; | |
| if (!topLevelRes || typeof topLevelRes.exitCode !== "number") { | |
| throw laneWorktreeMissingError(lane); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/git/gitOperationsService.ts
Line: 451
Comment:
`assertLaneWorktreeRoot` silently returns (passes validation) when `runGit` yields `null`, `undefined`, or an object without a numeric `exitCode`. In those cases the guard is skipped and the caller proceeds to mutate an unverified path. The same silent-pass pattern exists in `isExpectedGitWorktreeRoot` in `laneService.ts` (line 269). Treating an unrecognised response as valid inverts the safe-default principle.
```suggestion
if (!topLevelRes || typeof topLevelRes.exitCode !== "number") {
throw laneWorktreeMissingError(lane);
}
```
How can I resolve this? If you propose a fix, please make it concise.
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
Validation
Summary by CodeRabbit
New Features
ade lanes deletecommand with--force,--delete-branch, and--delete-remote-branchoptions.Bug Fixes
Improvements
Greptile Summary
This PR adds stale-worktree guards across ADE: before any lane-scoped git mutation or read, the service verifies that the saved
worktreePathis still the actual Git worktree root (viagit rev-parse --show-toplevel), preventing operations from silently running against the primary checkout when a lane directory has been removed or orphaned. It also relaxes the agent system-prompt boundary to allow read-only inspection outside the worktree while keeping writes/mutations lane-scoped, exposesade lanes deletethrough the CLI, and fixes Codex session resume by backfilling storage targets on demand.assertLaneWorktreeRoot(git service) andisExpectedGitWorktreeRoot(lane service) are added and called before every lane-scoped git operation;isLaneWorktreeAvailablein the TUI is similarly hardened with a cachedspawnSyncprobe.resolveEndedResumeSessionis made fullyasyncand attempts an on-demand resume-target backfill (including Codex rollout storage) before declaring a missing target.Confidence Score: 3/5
The stale-worktree guard is a valuable safety feature but two defects can cause valid worktrees to be falsely rejected on macOS, and the TUI now blocks the event loop during rendering.
Both service files compare git rev-parse output using path.resolve without symlink resolution, which fails on macOS where /tmp resolves to /private/tmp. The TUI adds a spawnSync with an 8-second timeout in the synchronous render path. These affect the core lane-git path and TUI responsiveness.
apps/desktop/src/main/services/git/gitOperationsService.ts and apps/desktop/src/main/services/lanes/laneService.ts (normAbs symlink resolution); apps/ade-cli/src/tuiClient/app.tsx (spawnSync in render path)
Important Files Changed
isLaneWorktreeAvailablewith a TTL-cached.gitmarker check and aspawnSyncgit probe — but the 8-second synchronous timeout runs in the Ink TUI render path, which can block the event loop on every cache miss.assertLaneWorktreeRootguard to every lane-scoped git operation;normAbsusespath.resolverather thanfs.realpathSync.native, which can produce false stale-worktree rejections on macOS where /tmp is a symlink.isExpectedGitWorktreeRoot/assertExpectedGitWorktreeRoothelpers guarding status reads, stash listing, and change inspection; contains the same symlink issue innormAbsand a test-specific error-message bypass in production code.resolveEndedResumeSessionrefactored toasync, eliminating theinstanceof Promisedual-path. On-demand resume-target backfill attempted duringresume-launchbefore reporting a missing target.lanes deleteto the CLI help text andbuildCliPlanhandler with--force,--delete-branch, and--delete-remote-branchflags.Comments Outside Diff (1)
apps/desktop/src/main/services/lanes/laneService.ts, line 256-258 (link)gitOperationsService.ts:normAbsuses onlypath.resolve, so thenormAbs(topLevel) === normAbs(worktreePath)comparison inisExpectedGitWorktreeRootcan fail on macOS when the worktree path was stored without resolving/tmpto/private/tmp.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "ship: prepare lane for review" | Re-trigger Greptile