Repository navigation
fix(server): old isolated threads from the pre-v2 fork open as a multi-root workspace - #350
Conversation
…pace file The fork kept an isolated run's .code-workspace inside the container, while V2 opens the one written beside it. Threads moved over by the fork worktree repair had none there, so "Open in" opened the bare container folder. A startup pass now writes V2's file from the project's workspace file for every container that holds the fork's own copy and has none beside it. The fork's copy stays so the pre-V2 build can still open it. Containers V2 created only link the project's file in, so they are left alone. Fork-only; not intended for upstream. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LoganRupe
left a comment
There was a problem hiding this comment.
Thanks for this, and for testing it on a real migrated database. The 17-thread run and the macOS privacy gotcha were useful to read.
I'd like to fix this somewhere other than startup, though. As written, the step never records that it has run. So every start re-checks every thread with a worktree path, one at a time, before the server takes commands. Threads it can never fix, like single-repo worktrees, get checked forever.
I think the bug sits in workspaceFile() (WorkspaceRepositories.ts:234). For an isolated run with no generated file, it returns null. Could it instead use the container's single .code-workspace, the same rule the non-isolated branch already uses? Your pre-v2 copy already points ./api and ./web at the worktrees, so "Open in" would work with no startup step and no extra file.
That change would also fix three other things I hit while reading:
holdsForkCopymatches more than fork containers. In a v2 run whose root repo commits a.code-workspace, that file is a real file in the container. So a v2 run with a missing generated file looks like a fork thread and gets<container>.code-workspacewritten. If the user deleted that file on purpose, it comes back on every start.- Deleting a repaired thread leaves the new
<container>.code-workspacebehind. The container survives because of the real file inside it, soremove()never reaches the cleanup branch. - A half-written target file is never repaired. The write isn't atomic and the step skips any thread whose target exists.
If there's a reason the fallback in workspaceFile() doesn't work, tell me, I may be missing something. If you'd rather keep the startup step, it needs a done marker and tests for the root-repo case above.
That was actually the first fix that Claude had suggested, but I thought that migrating to the same workspace file that normally gets generated for new isolated threads would be less likely to involve some form of whack-a-mole for bugs popping up for other places that might assume the new workspace file. Doing it the way I did just felt like a safer fix.
Hmm, that's a good point. I feel like a workspace file like that would trip things up regardless of whether we migrate how I've currently designed it or with your suggestion of just re-using it, either way it would be treating that workspace file as the workspace file for the whole thread. Not sure how we could go about fixing this |
|
Fair on the root-repo case, I overstated that one. Whichever file ends up used, the paths resolve to the run's worktrees, so the output is correct. Ignore it. On the whack-a-mole worry: I checked, and That said, I'm happy to keep your approach if you prefer migrating to v2's file. My one ask is to narrow the candidates in SQL the way |
…tten atomically The startup step now only considers threads the pre-v2 fork recorded with more than one worktree, so a database that never ran the fork touches no files. The workspace file is written via a temp file and rename, so an existing file is always complete and safe to skip. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. I've pushed 4792bfc. Here's what I did with each point:
|
LoganRupe
left a comment
There was a problem hiding this comment.
Thanks, this looks good. Narrowing the query to fork threads was what I was after, and the atomic write is a nice extra for new runs. Happy for the delete cleanup to land in the follow-up.
Problem
After #341 moves an old isolated thread from the pre-v2 branch onto its whole thread folder, "Open in" still opens that folder as a plain directory instead of a multi-root workspace. VS Code then shows the worktrees as nested folders rather than repos, plus the
.code-workspacefile itself.v2 opens the
.code-workspaceit writes next to a new isolated run's folder (#348). The pre-v2 branch kept its copy inside the folder instead (#294), so repaired threads have no file where v2 looks.Fix
A new startup step,
writeMultiRepoForkWorkspaceFiles, runs after the #341 repair. It applies to each v2 thread whose folder has v2's layout file but no.code-workspacenext to it, and whose folder holds a single real.code-workspace(the pre-v2 copy). For those, it writes v2's file using the same generator new isolated runs use, built from the project's.code-workspace.Known limit: deleting an old thread still leaves its folder behind, because the pre-v2 copy is a regular file (as noted on #340). That's left for a separate change.
Verification
repairMultiRepoForkWorktrees.test.ts:~/Documents), the step skips each thread and retries on the next start.Written with Claude Opus 5.5 in Claude Code.
🤖 Generated with Claude Code