Merge upstream: Move worktree bootstrap to server and persist terminal launch context (#1518)#45
Merged
aaditagrawal merged 2 commits intomainfrom Apr 5, 2026
Conversation
…text (pingdotgg#1518) Co-authored-by: codex <codex@users.noreply.github.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a ProjectSetupScriptRunner service and bootstrapped worktree flow: web client sends bootstrap payload, server prepares worktrees, invokes setup scripts via a terminal manager, records setup activity, and updates contracts/state to track per-worktree paths. Tests and schemas updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Web Client
participant WS as WebSocket Server
participant Git as GitManager
participant Setup as ProjectSetupScriptRunner
participant Term as TerminalManager
participant Orch as Orchestration Engine
Client->>WS: dispatchCommand(thread.turn.start { bootstrap })
WS->>Git: preparePullRequestThread(..., threadId?)
Git->>Git: create worktree (if needed)
Git->>Setup: runForThread(threadId, worktreePath)
Setup->>Orch: load project/read-model to resolve script
Setup->>Term: open(threadId, terminalId, cwd, env)
Term-->>Setup: started
Setup-->>Git: result { status: "started" | "no-script" }
WS->>Orch: thread.activity.append("setup-script.*")
WS->>Orch: thread.turn.start (bootstrap cleared)
Orch-->>WS: processed
WS-->>Client: RPC response + events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
github-actions
bot
added
vouch:trusted
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/server/src/terminal/Layers/Manager.ts (2)
1648-1702:⚠️ Potential issue | 🟠 MajorWorktree metadata is not updated for already-running sessions when only context changes.
If
cwdand env are unchanged,open()returns without applying a newly providedworktreePath, so session snapshots/events can keep stale context.💡 Proposed fix
const liveSession = existing.value; const nextRuntimeEnv = normalizedRuntimeEnv(input.env); const currentRuntimeEnv = liveSession.runtimeEnv; + const nextWorktreePath = + input.worktreePath === undefined ? liveSession.worktreePath : input.worktreePath ?? null; const targetCols = input.cols ?? liveSession.cols; const targetRows = input.rows ?? liveSession.rows; const runtimeEnvChanged = !Equal.equals(currentRuntimeEnv, nextRuntimeEnv); + const worktreePathChanged = liveSession.worktreePath !== nextWorktreePath; if (liveSession.cwd !== input.cwd || runtimeEnvChanged) { yield* stopProcess(liveSession); liveSession.cwd = input.cwd; - liveSession.worktreePath = input.worktreePath ?? null; + liveSession.worktreePath = nextWorktreePath; liveSession.runtimeEnv = nextRuntimeEnv; liveSession.history = ""; ... } else if (liveSession.status === "exited" || liveSession.status === "error") { liveSession.runtimeEnv = nextRuntimeEnv; - liveSession.worktreePath = input.worktreePath ?? null; + liveSession.worktreePath = nextWorktreePath; liveSession.history = ""; ... + } else if (worktreePathChanged) { + liveSession.worktreePath = nextWorktreePath; + liveSession.updatedAt = new Date().toISOString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/terminal/Layers/Manager.ts` around lines 1648 - 1702, The code currently returns a snapshot without updating worktree metadata when cwd and runtime env are unchanged, so ensure the session's worktreePath is updated from input.worktreePath even for already-running sessions: update liveSession.worktreePath = input.worktreePath ?? null (and set updatedAt if you track it) before the early return that calls snapshot(liveSession), and call persistHistory if your state-change invariants require it; refer to liveSession.worktreePath, input.worktreePath, snapshot(liveSession), startSession, and persistHistory to locate where to apply the change.
1808-1830:⚠️ Potential issue | 🟠 Major
restart()drops existing worktree context whenworktreePathis omitted.Current logic coerces omitted
worktreePathtonull, so restart calls that don’t pass this field clear previously persisted metadata.💡 Proposed fix
} else { session = existingSession.value; yield* stopProcess(session); session.cwd = input.cwd; - session.worktreePath = input.worktreePath ?? null; + session.worktreePath = + input.worktreePath === undefined ? session.worktreePath : input.worktreePath ?? null; session.runtimeEnv = normalizedRuntimeEnv(input.env); } ... yield* startSession( session, { threadId: input.threadId, terminalId, cwd: input.cwd, - ...(input.worktreePath !== undefined ? { worktreePath: input.worktreePath } : {}), + worktreePath: session.worktreePath, cols, rows, ...(input.env ? { env: input.env } : {}), }, "restarted", );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/terminal/Layers/Manager.ts` around lines 1808 - 1830, The restart logic unconditionally sets session.worktreePath to null when input.worktreePath is omitted; change the assignment so session.worktreePath is only updated when input.worktreePath is provided. Replace the line using the nullish-coalescing (session.worktreePath = input.worktreePath ?? null) with a conditional update (e.g. if (input.worktreePath !== undefined) session.worktreePath = input.worktreePath) so existing worktree context is preserved; adjust within the same restart flow where session.runtimeEnv is set if needed.apps/web/src/components/ThreadTerminalDrawer.tsx (1)
629-640:⚠️ Potential issue | 🟠 Major
worktreePathis not included in the effect dependency list, preventing session reinitialization when the path changes.The
openTerminal()function passesworktreePathtoapi.terminal.open(), but the enclosing effect dependency list at line 723 is[cwd, runtimeEnv, terminalId, threadId]—worktreePathis absent. ChatView can supply a newworktreePaththroughlaunchContextafter mount, and theTerminalManager.openmethod explicitly reuses the existing session for the samethreadId/terminalId. WhenworktreePathchanges, the effect doesn't re-run, so the updated path never reaches the live terminal.Add
worktreePathto the dependency list, or implement an explicit close/reopen mechanism when the path changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ThreadTerminalDrawer.tsx` around lines 629 - 640, The effect that calls openTerminal (which invokes api.terminal.open with worktreePath) omits worktreePath from its dependency list causing the terminal session not to reinitialize when worktreePath changes; update the effect dependencies to include worktreePath so the effect reruns and passes the new value to api.terminal.open, or alternatively detect worktreePath changes (using props/launchContext) and call the TerminalManager.close/open sequence for the current threadId/terminalId to force a reopen with the new worktreePath (referencing openTerminal, api.terminal.open, and TerminalManager.open/close).
🧹 Nitpick comments (2)
apps/server/src/checkpointing/Layers/CheckpointStore.test.ts (1)
118-118: Avoid hardcoded last-line marker in assertion.Line 118 is correct now, but this literal is brittle if
lineCountchanges again. Consider deriving it from the same source of truth.♻️ Proposed tweak
+ const lineCount = 5_000; - yield* writeTextFile(path.join(tmp, "README.md"), buildLargeText()); + yield* writeTextFile(path.join(tmp, "README.md"), buildLargeText(lineCount)); yield* checkpointStore.captureCheckpoint({ cwd: tmp, checkpointRef: toCheckpointRef, }); @@ - expect(diff).toContain("+line 04999"); + expect(diff).toContain(`+line ${String(lineCount - 1).padStart(5, "0")}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/checkpointing/Layers/CheckpointStore.test.ts` at line 118, Replace the brittle hardcoded assertion expect(diff).toContain("+line 04999") with a value computed from the same source of truth (the lineCount used to generate the diff). In the CheckpointStore.test.ts test, compute the expected last-line marker from lineCount (e.g., use lineCount - 1 and apply the same zero-padding/formatting used when building the diff) and assert expect(diff).toContain(expectedLastLine) so the test stays correct when lineCount changes; locate the assertion that references diff and lineCount and replace the literal with the derived expectedLastLine.apps/web/src/terminalStateStore.ts (1)
604-624: Logic is correct but could be clearer.The interplay between lines 610-616 and 617-619 is correct but subtle. The
normalizeThreadTerminalStatecall at line 623 will fix any inconsistencies if the restoredactiveTerminalGroupIdbecomes invalid afternewThreadTerminalmodifies the groups.Consider simplifying to make intent explicit:
♻️ Optional: Clearer active handling
ensureTerminal: (threadId, terminalId, options) => updateTerminal(threadId, (state) => { let nextState = state; if (!state.terminalIds.includes(terminalId)) { nextState = newThreadTerminal(nextState, terminalId); } - if (options?.active === false) { - nextState = { - ...nextState, - activeTerminalId: state.activeTerminalId, - activeTerminalGroupId: state.activeTerminalGroupId, - }; - } - if (options?.active ?? true) { + const shouldActivate = options?.active ?? true; + if (shouldActivate) { nextState = setThreadActiveTerminal(nextState, terminalId); + } else { + // Preserve original active terminal when active: false + nextState = { + ...nextState, + activeTerminalId: state.activeTerminalId, + activeTerminalGroupId: state.activeTerminalGroupId, + }; } if (options?.open) { nextState = setThreadTerminalOpen(nextState, true); } return normalizeThreadTerminalState(nextState); }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/terminalStateStore.ts` around lines 604 - 624, The ensureTerminal update is subtle: newThreadTerminal may change group structure then later code tries to preserve or set active IDs; make this explicit by capturing the original activeTerminalId and activeTerminalGroupId from the incoming state before any mutations, then apply newThreadTerminal, then if options?.active === false restore those captured IDs onto nextState, else if options?.active ?? true call setThreadActiveTerminal(nextState, terminalId); finally apply setThreadTerminalOpen if needed and call normalizeThreadTerminalState; reference ensureTerminal, updateTerminal, newThreadTerminal, setThreadActiveTerminal, setThreadTerminalOpen, and normalizeThreadTerminalState to locate where to implement the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/project/Layers/ProjectSetupScriptRunner.ts`:
- Around line 18-25: The current project resolution assigns to the variable
project by preferring input.projectId over input.projectCwd, which silently
permits mismatched selectors; modify the resolution in
ProjectSetupScriptRunner.ts so that if both input.projectId and input.projectCwd
are provided you resolve each against readModel.projects (using the same find
logic) and compare their ids/workspaceRoot — if they refer to different projects
throw a clear error (or reject) indicating conflicting selectors; allow the flow
to continue only if a single selector is provided or both resolve to the same
project.
In `@apps/web/src/components/ChatView.tsx`:
- Around line 2535-2546: The effect currently clears persisted launch context
whenever activeThreadId is falsy, which wipes
terminalLaunchContextByThreadId[threadId] during hydration; change the logic in
the useEffect that updates setTerminalLaunchContext so it does not call
storeClearTerminalLaunchContext(threadId) when activeThreadId is null/undefined
during hydration—only clear the persisted context when there is an actual thread
switch (i.e., when the current terminal launch context exists, current.threadId
!== activeThreadId, and activeThreadId is non-null/truthy). Keep references to
useEffect, setTerminalLaunchContext, storeClearTerminalLaunchContext,
activeThreadId, threadId, and the terminal launch context "current" to locate
and implement this conditional.
---
Outside diff comments:
In `@apps/server/src/terminal/Layers/Manager.ts`:
- Around line 1648-1702: The code currently returns a snapshot without updating
worktree metadata when cwd and runtime env are unchanged, so ensure the
session's worktreePath is updated from input.worktreePath even for
already-running sessions: update liveSession.worktreePath = input.worktreePath
?? null (and set updatedAt if you track it) before the early return that calls
snapshot(liveSession), and call persistHistory if your state-change invariants
require it; refer to liveSession.worktreePath, input.worktreePath,
snapshot(liveSession), startSession, and persistHistory to locate where to apply
the change.
- Around line 1808-1830: The restart logic unconditionally sets
session.worktreePath to null when input.worktreePath is omitted; change the
assignment so session.worktreePath is only updated when input.worktreePath is
provided. Replace the line using the nullish-coalescing (session.worktreePath =
input.worktreePath ?? null) with a conditional update (e.g. if
(input.worktreePath !== undefined) session.worktreePath = input.worktreePath) so
existing worktree context is preserved; adjust within the same restart flow
where session.runtimeEnv is set if needed.
In `@apps/web/src/components/ThreadTerminalDrawer.tsx`:
- Around line 629-640: The effect that calls openTerminal (which invokes
api.terminal.open with worktreePath) omits worktreePath from its dependency list
causing the terminal session not to reinitialize when worktreePath changes;
update the effect dependencies to include worktreePath so the effect reruns and
passes the new value to api.terminal.open, or alternatively detect worktreePath
changes (using props/launchContext) and call the TerminalManager.close/open
sequence for the current threadId/terminalId to force a reopen with the new
worktreePath (referencing openTerminal, api.terminal.open, and
TerminalManager.open/close).
---
Nitpick comments:
In `@apps/server/src/checkpointing/Layers/CheckpointStore.test.ts`:
- Line 118: Replace the brittle hardcoded assertion
expect(diff).toContain("+line 04999") with a value computed from the same source
of truth (the lineCount used to generate the diff). In the
CheckpointStore.test.ts test, compute the expected last-line marker from
lineCount (e.g., use lineCount - 1 and apply the same zero-padding/formatting
used when building the diff) and assert expect(diff).toContain(expectedLastLine)
so the test stays correct when lineCount changes; locate the assertion that
references diff and lineCount and replace the literal with the derived
expectedLastLine.
In `@apps/web/src/terminalStateStore.ts`:
- Around line 604-624: The ensureTerminal update is subtle: newThreadTerminal
may change group structure then later code tries to preserve or set active IDs;
make this explicit by capturing the original activeTerminalId and
activeTerminalGroupId from the incoming state before any mutations, then apply
newThreadTerminal, then if options?.active === false restore those captured IDs
onto nextState, else if options?.active ?? true call
setThreadActiveTerminal(nextState, terminalId); finally apply
setThreadTerminalOpen if needed and call normalizeThreadTerminalState; reference
ensureTerminal, updateTerminal, newThreadTerminal, setThreadActiveTerminal,
setThreadTerminalOpen, and normalizeThreadTerminalState to locate where to
implement the fix.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 662d575c-7c2d-4bd3-a512-c609c941c490
📒 Files selected for processing (30)
apps/server/src/checkpointing/Layers/CheckpointStore.test.tsapps/server/src/git/Layers/GitManager.test.tsapps/server/src/git/Layers/GitManager.tsapps/server/src/project/Layers/ProjectSetupScriptRunner.test.tsapps/server/src/project/Layers/ProjectSetupScriptRunner.tsapps/server/src/project/Services/ProjectSetupScriptRunner.tsapps/server/src/server.test.tsapps/server/src/server.tsapps/server/src/terminal/Layers/Manager.test.tsapps/server/src/terminal/Layers/Manager.tsapps/server/src/terminal/Services/Manager.tsapps/server/src/ws.tsapps/web/src/components/ChatView.browser.tsxapps/web/src/components/ChatView.tsxapps/web/src/components/PullRequestThreadDialog.tsxapps/web/src/components/ThreadTerminalDrawer.tsxapps/web/src/lib/gitReactQuery.tsapps/web/src/projectScripts.test.tsapps/web/src/projectScripts.tsapps/web/src/routes/__root.tsxapps/web/src/terminalActivity.test.tsapps/web/src/terminalStateStore.test.tsapps/web/src/terminalStateStore.tspackages/contracts/src/git.tspackages/contracts/src/orchestration.test.tspackages/contracts/src/orchestration.tspackages/contracts/src/terminal.test.tspackages/contracts/src/terminal.tspackages/shared/package.jsonpackages/shared/src/projectScripts.ts
💤 Files with no reviewable changes (1)
- apps/web/src/projectScripts.ts
…guard, conflicting selectors, and dependency fix - ProjectSetupScriptRunner: validate that projectId and projectCwd resolve to the same project when both are provided - ChatView: only clear terminal launch context on actual thread switch, not during hydration when activeThreadId is falsy - Terminal Manager open(): update worktreePath for already-running sessions even when cwd/env are unchanged - Terminal Manager restart(): preserve existing worktreePath when input omits it - ThreadTerminalDrawer: add worktreePath to effect dependency list so terminal reinitializes when the path changes
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
2 participants
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.
What
Cherry-picks upstream commit
8515f027(PR #1518) onto the fork.Upstream changes
Worktree bootstrap moved server-side — Thread creation, worktree preparation, and setup script execution now happen atomically on the server via a
bootstrappayload inthread.turn.start, eliminating client-side race conditions.New
ProjectSetupScriptRunnerservice — Effect service that runs project setup scripts (e.g.bun install) in a terminal for a given thread/worktree.Terminal launch context persistence —
terminalStateStorenow tracksterminalLaunchContextByThreadIdfor correct working directory even before server thread metadata settles.projectScriptCwdandprojectScriptRuntimeEnvmoved topackages/shared.Conflict resolution
CheckpointStore.test.ts— Both sides reducedlineCount; took upstream value (5,000).packages/contracts/src/git.ts— Kept both: upstream'sThreadIdimport + fork'sProviderKindimport.ChatView.tsx— Merged upstream's bootstrap/launchContext changes with fork's multi-provider model options, split-limit guard, and per-thread terminal visibility.Verification
bun typecheckpasses (all 7 packages)bun fmtcleanbun lintclean (0 errors, 20 pre-existing warnings)MessagesTimelinetimeout under full suite load) unrelated to this changeSummary by CodeRabbit
New Features
Bug Fixes / Reliability