fix(archive): skip checkpoint when worktree was deleted externally#1910
Conversation
When a workspace's worktree directory was deleted by an external process, archiving failed because CaptureCheckpointSaga ran against a non-git path and threw "fatal: not a git repository". The rollback then restored the task, producing the flash-and-reappear behavior reported in #1798. Now we probe the worktree with isGitRepository before checkpointing; if the path is missing or invalid, we log the reason, set checkpointId to null, and continue with the remaining cleanup steps. A null checkpointId naturally causes unarchive to skip worktree restoration. Generated-By: PostHog Code Task-Id: eeaa1905-3fe5-423c-a853-08d75e78947b
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Prompt To Fix All With AIThis is a comment left during a code review.
Path: apps/code/src/main/services/archive/service.ts
Line: 187-218
Comment:
**Prefer `if/else` over two separate `if` blocks**
The two independent `if (!worktreeIsValid)` / `if (worktreeIsValid)` blocks are mutually exclusive and can be collapsed into a single `if/else`, removing the superfluous second condition check and making the intent clearer.
```suggestion
if (worktreeIsValid) {
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(archive): skip checkpoint when workt..." | Re-trigger Greptile |
| if (!worktreeIsValid) { | ||
| log.warn( | ||
| `Worktree at ${worktreePath} is missing or not a git repository; skipping checkpoint capture`, | ||
| ); | ||
| archivedTask.checkpointId = null; | ||
| } | ||
|
|
||
| if (worktreeIsValid) { | ||
| const actualBranch = await this.getCurrentBranchName(worktreePath); | ||
| if (actualBranch && actualBranch !== "HEAD") { | ||
| archivedTask.branchName = actualBranch; | ||
| } | ||
|
|
||
| await step( | ||
| async () => { | ||
| if (!archivedTask.checkpointId) { | ||
| throw new Error("checkpointId must be set for worktree mode"); | ||
| } | ||
| await this.captureWorktreeCheckpoint( | ||
| folderPath, | ||
| worktreePath, | ||
| archivedTask.checkpointId, | ||
| ); | ||
| }, | ||
| async () => { | ||
| if (archivedTask.checkpointId) { | ||
| const git = createGitClient(folderPath); | ||
| await deleteCheckpoint(git, archivedTask.checkpointId); | ||
| } | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Prefer
if/else over two separate if blocks
The two independent if (!worktreeIsValid) / if (worktreeIsValid) blocks are mutually exclusive and can be collapsed into a single if/else, removing the superfluous second condition check and making the intent clearer.
| if (!worktreeIsValid) { | |
| log.warn( | |
| `Worktree at ${worktreePath} is missing or not a git repository; skipping checkpoint capture`, | |
| ); | |
| archivedTask.checkpointId = null; | |
| } | |
| if (worktreeIsValid) { | |
| const actualBranch = await this.getCurrentBranchName(worktreePath); | |
| if (actualBranch && actualBranch !== "HEAD") { | |
| archivedTask.branchName = actualBranch; | |
| } | |
| await step( | |
| async () => { | |
| if (!archivedTask.checkpointId) { | |
| throw new Error("checkpointId must be set for worktree mode"); | |
| } | |
| await this.captureWorktreeCheckpoint( | |
| folderPath, | |
| worktreePath, | |
| archivedTask.checkpointId, | |
| ); | |
| }, | |
| async () => { | |
| if (archivedTask.checkpointId) { | |
| const git = createGitClient(folderPath); | |
| await deleteCheckpoint(git, archivedTask.checkpointId); | |
| } | |
| }, | |
| ); | |
| } | |
| if (worktreeIsValid) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/services/archive/service.ts
Line: 187-218
Comment:
**Prefer `if/else` over two separate `if` blocks**
The two independent `if (!worktreeIsValid)` / `if (worktreeIsValid)` blocks are mutually exclusive and can be collapsed into a single `if/else`, removing the superfluous second condition check and making the intent clearer.
```suggestion
if (worktreeIsValid) {
```
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!
| if (!worktreeIsValid) { | ||
| log.warn( | ||
| `Worktree at ${worktreePath} is missing or not a git repository; skipping checkpoint capture`, | ||
| ); | ||
| archivedTask.checkpointId = null; | ||
| } | ||
|
|
||
| if (worktreeIsValid) { | ||
| const actualBranch = await this.getCurrentBranchName(worktreePath); | ||
| if (actualBranch && actualBranch !== "HEAD") { | ||
| archivedTask.branchName = actualBranch; | ||
| } | ||
|
|
||
| await step( | ||
| async () => { | ||
| if (!archivedTask.checkpointId) { | ||
| throw new Error("checkpointId must be set for worktree mode"); | ||
| } | ||
| await this.captureWorktreeCheckpoint( | ||
| folderPath, | ||
| worktreePath, | ||
| archivedTask.checkpointId, | ||
| ); | ||
| }, | ||
| async () => { | ||
| if (archivedTask.checkpointId) { | ||
| const git = createGitClient(folderPath); | ||
| await deleteCheckpoint(git, archivedTask.checkpointId); | ||
| } | ||
| }, | ||
| ); | ||
| } |

Closes #1798