fix: reopen persisted managed worktrees - #107
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds root-based workspace session lookup and uses it to reopen persisted managed worktrees by path. Tests cover restoration across store reopening, managed worktree metadata, agent restoration, rejection of unregistered paths, and cleanup. ChangesManaged worktree restoration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant WorkspaceRegistry
participant SqliteWorkspaceStore
participant Workspace
Client->>WorkspaceRegistry: openCheckoutWorkspace(path)
WorkspaceRegistry->>SqliteWorkspaceStore: getSessionByRoot(resolved path)
SqliteWorkspaceStore-->>WorkspaceRegistry: matching session
WorkspaceRegistry->>Workspace: getWorkspace(session id)
Workspace-->>WorkspaceRegistry: workspace and agent data
WorkspaceRegistry-->>Client: restored WorkspaceContext
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/workspaces.test.ts`:
- Around line 183-196: Update the reopened workspace assertions around
reopenedRegistry.openWorkspace and the subsequent assert.rejects call to run
inside a try/finally block, with reopenedStore.close() in finally. Preserve all
existing assertions and ensure cleanup executes even when an assertion fails.
In `@src/workspaces.ts`:
- Around line 182-187: Update the path-based restoration branch around
getWorkspace and loadInitialAgentsFiles to load local agent profiles with
loadLocalAgentProfiles for the restored workspace, preserving them in the
returned workspace data. Ensure the persistence test covering restored managed
worktrees verifies that agent profiles are retained after restart.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 100315d5-7e75-4bc4-9716-3c8f1a21243d
📒 Files selected for processing (3)
src/workspace-store.tssrc/workspaces.test.tssrc/workspaces.ts
|
@Waishnav requesting review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private async openCheckoutWorkspace(path: string): Promise<WorkspaceContext> { | ||
| const restoredWorktree = this.store?.getSessionByRoot(resolve(expandHomePath(path))); | ||
| if (restoredWorktree?.mode === "worktree" && restoredWorktree.managed) { | ||
| const workspace = this.getWorkspace(restoredWorktree.id); |
There was a problem hiding this comment.
When a persisted managed worktree has been deleted or moved, this branch reconstructs and returns it without checking that its root still exists, causing subsequent file operations or process startup to fail against a nonexistent workspace.
Greptile SummaryThis PR adds exact-path restoration for persisted managed worktrees.
Confidence Score: 3/5The PR should not merge until path-based restoration rejects or handles persisted managed worktrees whose directories no longer exist. The new lookup can select a retained session after its worktree has been removed, and the restoration branch returns that reconstructed workspace without the filesystem validation performed on ordinary checkout roots. Files Needing Attention: src/workspaces.ts
|
| Filename | Overview |
|---|---|
| src/workspaces.ts | Adds path-based managed-worktree restoration, but can return a stale persisted workspace after its directory disappears. |
| src/workspace-store.ts | Adds an exact-root lookup ordered by most recent use; retained rows enable the stale-root restoration issue. |
| src/workspaces.test.ts | Covers successful restoration and denial of an unregistered sibling, but not restoration after the persisted worktree is removed. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Open checkout-mode path] --> B[Look up persisted session by exact root]
B --> C{Managed worktree session?}
C -- Yes --> D[Restore original workspace ID]
D --> E[Reload agents and context]
C -- No --> F[Validate against allowed checkout roots]
F --> G[Open or create checkout workspace]
Reviews (1): Last reviewed commit: "fix: restore managed worktree profiles" | Re-trigger Greptile
1 participant
Fixes #106
Summary
open_workspacerehydrate an exact persisted managed worktree by path.allowedRoots, plus an assertion that an unregistered sibling remains denied.Root cause
Persisted managed worktrees already restore safely by workspace ID: their source root is checked against
allowedRootsand their worktree root against the dedicated managed-worktree root. When the client loses that ID,open_workspaceonly followed the new-checkout path and rejected the same worktree because the worktree parent is intentionally outsideallowedRoots.Security
Rehydration applies only to exact persisted sessions marked as both
mode: "worktree"andmanaged: true; restored-worktree validation remains unchanged. Arbitrary paths below the managed-worktree parent are still rejected.Validation
tsx src/workspaces.test.tsnpm run typechecknpm run buildnpm testreaches an existing Node 24 failure insrc/process-sessions.test.ts:122before the workspace test runs (true !== false).Summary by CodeRabbit
New Features
Bug Fixes