feat(sessions): support explicit spawned-session checkouts

[roughcoder]

What

• Adds gitRef, baseRef, branchName, and checkoutPr to spawn_session, with early invalid-input checks.
• Resolves PR heads and fetched refs through GitWorkflowService during worktree bootstrap.
• Returns absolute worktree paths plus branch, SHA, and dirty state from spawn/read session calls.

Why

Review sessions can now be pinned to the same PR head or git revision, so parents and reviewers can verify the revision inspected.

Validation

• vp test run packages/contracts/src/sessionOrchestration.test.ts apps/server/src/mcp/toolkits/sessions/handlers.test.ts
• npm run typecheck

Gaps

• No live provider/session spawn against a remote PR was run.
• Worktree cleanup/reclamation is intentionally out of scope.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

[roughcoder]

Review: feat(sessions): support explicit spawned-session checkouts

Verdict: REQUEST-CHANGES — the feature itself (ref resolution, validation, injection safety) is in good shape. What blocks it is that both response paths now fail closed on git errors, which turns a best-effort metadata addition into a new way for read_session and spawn_session to break outright.

Reviewed by reading only — per orchestration resource limits I did not run tests, typecheck, or any live spawn.

---

Blocking

1. [blocker] read_session now hard-fails whenever the child's worktree is unreadable — apps/server/src/mcp/toolkits/sessions/handlers.ts:474-479

read_session used to be a pure snapshot read. It now runs localStatus + resolveCommit against child.worktreePath and propagates the error via Effect.mapError(operationError(...)).

If the worktree directory is gone (user removed it, cleanup after merge, disk moved) git rev-parse is spawned with a non-existent cwd and fails unconditionally — localStatus's non-repo fallback at GitWorkflowService.ts:271-278 does not save you, because resolveCommit in the same Effect.all has no such fallback. The result: the parent can never retrieve the child's report or messages again. That's the core orchestration loop, broken by an optional metadata field. Same failure for a worktree at a repo with no commits yet (rev-parse HEAD fails).

These four fields are already Schema.optional on ReadSessionResult (sessionOrchestration.ts:148-151) — the contract is explicitly built to tolerate their absence. Please make it best effort: .pipe(Effect.catch(() => Effect.succeed(null))), matching the existing precedent at handlers.ts:324-326.

2. [major] spawn_session can report failure for a session that was actually created — handlers.ts:400-405

Same pattern, worse consequence. requireShell(threadId) has already returned, so the child thread exists and is running. If the checkout resolution then fails, the caller gets an error and never receives threadId — an orphaned live session the parent cannot read, message, or stop. Rollback doesn't help here: cleanupCreatedThread is scoped to bootstrapTurnStart (ThreadTurnBootstrap.ts:389-396) and has long since completed successfully. Make it best-effort and fall back to spawned.branch / sha: null / dirty: null (which the result schema already permits).

---

Non-blocking

3. [major] gitRef/checkoutPr still pay for — and can be killed by — the startFromOrigin base resolution — ThreadTurnBootstrap.ts:293-316

When startFromOrigin is set, the code fetches origin and calls resolveRemoteTrackingCommit({refName: baseBranch}), then discards worktreeBaseRef at line 316 the moment checkoutRef/checkoutPr is present. So an explicit checkout request does a pointless fetch, and — the real problem — fails outright if the caller's baseRef has no remote-tracking branch, even though baseRef is documented as merge-base metadata only. Skip the startFromOrigin block entirely when checkoutRef/checkoutPr is set.

4. [major] Fetch failures are reported as "ref does not exist" — ThreadTurnBootstrap.ts:350-355

The Effect.mapError wraps the whole fetchRemote → resolveCommit chain, so a network outage, auth prompt, or proxy failure on git fetch origin is reported as Git ref "X" does not exist locally or on origin. That will send agents chasing a typo'd ref name instead of a broken remote. Distinguish the fetch failure from the post-fetch resolve failure.

5. [minor] baseRef's documented meaning doesn't match its behaviour — packages/contracts/src/sessionOrchestration.ts:52-54

The comment says "Base ref recorded for the spawned branch's merge-base metadata." But with gitRef and checkoutPr both absent, checkoutRef falls back to worktreeBaseRef (= baseRef) at ThreadTurnBootstrap.ts:316, so baseRef alone also decides what gets checked out. An agent reading only the tool schema will not expect that. Since this text is the MCP tool description an LLM acts on, please state both roles.

6. [minor] read_session does a forced git status on every poll — handlers.ts:470-479

invalidateLocalStatus deliberately busts the cache, so every poll pays a full git status + rev-parse on the child's worktree. read_session is the polling endpoint in this API; on a large repo with several children being watched this is a recurring cost for data that rarely changes between polls. Either drop the invalidation and accept cached status, or gate the refresh behind an opt-in parameter.

7. [minor] checkoutPr is GitHub-shaped with no signal to the caller — GitVcsDriverCore.ts:2819-2833

refs/pull/<n>/head doesn't exist on GitLab (refs/merge-requests/<n>/head) or Bitbucket. On those remotes users get a raw git fetch failure. Fine to defer, but worth a clearer error or a doc note.

8. [nit] No --end-of-options; refs aren't charset-validated — sessionOrchestration.ts:51-57

I checked this specifically and command injection is not a concern: everything goes through ChildProcess.make("git", args) with an argv array (GitVcsDriverCore.ts:731-741), no shell. checkoutPr is Schema.Int.check(isGreaterThan(0)) before it reaches the refs/pull/${n}/head template, so the PR ref is clean. Path traversal via branchName is also blocked — createWorktree sanitizes / and git rejects .. as a branch name.

What remains is ordinary option injection: gitRef/branchName are TrimmedNonEmptyString, so a leading - reaches git rev-parse --verify <ref>^{commit} and git worktree add -b <branch> as an option-looking argument. Worst case is a confusing git error, not code execution. Adding --end-of-options or a ref-name regex would close it cheaply.

---

Things I checked that are correct

• Silent project-root fallback is now an explicit error ✅ — validateSpawnCheckoutInput runs at handlers.ts:329, before the fallback at 333-341, and returns SessionOrchestrationInvalidInputError. Exactly what the brief asked for.
• Schema safety ✅ — every new input field is Schema.optional, so additive and backward compatible. No typeless/empty structs. Schema.Int is already exercised in this same tool by messageLimit (sessionOrchestration.ts:128), so no new MCP-schema risk.
• Rollback on ref-resolution failure ✅ — all new failures occur before createWorktree, and catchCause → cleanupCreatedThread (ThreadTurnBootstrap.ts:389-396) covers plain Error failures. No half-created worktrees from the new paths, and toBootstrapDispatchCommandCauseError preserves the custom messages via error instanceof Error ? error.message (line 58-62).
• refName: checkoutCommit.commitSha ✅ — resolving to a SHA before worktree add neatly avoids passing user ref text to that command.
• Detached-HEAD projects — repoBranch !== null is false on detached HEAD, so checkout fields are rejected there. The error message ("...not a git repository with a current branch") does cover it, so this reads intentional; noting it in case it wasn't.

Test quality

validateSpawnCheckoutInput tests assert real behaviour on a genuinely pure function — good call extracting it. The contracts tests assert schema shape only, which is the right scope for schemas.

The gap: the riskiest code in the PR has no tests at all. ThreadTurnBootstrap.ts:316-362 — the local-resolve → origin-fetch → re-resolve fallback, the no-origin branch, the PR-head path, and gitRef overriding startFromOrigin — is entirely uncovered, and findings #3 and #4 both live there. Also uncovered: SpawnSessionInput's inverse cases (SpawnSessionResult requires sha/dirty while ReadSessionResult makes them optional — inconsistent, though harmless since the handler always supplies them), and the missing-worktree path behind #1/#2. Given this toolkit had zero tests before, this is real progress and I wouldn't gate on coverage — but a fake GitWorkflowService exercising the ref-resolution branches would have caught #3.

[roughcoder]

Addressed in b365c82e2:

• Made spawned-session checkout metadata best-effort in both spawn_session and read_session; missing/unreadable/no-commit worktrees now preserve successful spawn/report/message access and return null SHA/dirty metadata.
• Removed forced local-status invalidation from read_session, so polling uses the existing status cache.
• Skip startFromOrigin base resolution for explicit gitRef/checkoutPr checkouts.
• Split remote-fetch failures from post-fetch missing-ref errors, with focused regression tests for both paths.
• Corrected baseRef MCP schema wording and added --end-of-options for commit resolution.

Deferred follow-ups: checkoutPr remains GitHub-specific (refs/pull/<n>/head); cross-host PR-ref support needs a provider-aware design. Worktree cleanup/reclamation remains intentionally out of scope for this PR.

[roughcoder]

Delta re-review — b365c82e2

Verdict: APPROVE-WITH-NITS. All eight findings are genuinely resolved, and the fixes are the right shape rather than symptom patches. One mechanical issue in the new test files will almost certainly fail npm run typecheck — worth fixing before merge, but it doesn't touch production code.

Delta reviewed only (4aed46090..b365c82e2). No tests, typecheck, or lint run.

Findings status

#	Was	Status
1	read_session hard-fails on dead worktree	✅ Resolved
2	spawn_session orphans a live session	✅ Resolved
3	startFromOrigin kills explicit checkouts	✅ Resolved
4	Fetch failure reported as "ref does not exist"	✅ Resolved
5	baseRef doc mismatch	✅ Resolved
6	Forced git status on every poll	✅ Resolved
7	GitHub-only PR refs	⏭️ Deferred, accepted
8	No --end-of-options	✅ Resolved (main path)

#1/#2 — the important ones. resolveSessionCheckout (handlers.ts:89-93) is exactly right, and I specifically checked that the catch isn't over-broad: Effect.catch handles only the typed error channel, so it swallows GitManagerServiceError/GitCommandError and leaves defects and interrupts propagating. The declared channels on localStatus/resolveCommit are git-only, so nothing non-git is being hidden. The read_session spread also correctly moved from checkout ? to worktreePath ? with ?. fallbacks (handlers.ts:489-495), so a dead worktree now yields sha: null, dirty: null while still returning branch/worktreePath — and ReadSessionResult's Schema.optional(Schema.NullOr(...)) accepts that. Report and message access can no longer be blocked. Same on the spawn side against SpawnSessionResult's NullOr fields.

#4 — verified the pipe structure, not just the strings. Effect.mapError on fetchRemote is applied before Effect.andThen, and the post-fetch resolveCommit carries its own mapError (ThreadTurnBootstrap.ts:88-108). So the two failure modes genuinely can't bleed into each other — a real fix, not just reworded text.

#3 — !hasExplicitCheckout && also short-circuits the now-pointless remoteExists call. Good.

#8 — --end-of-options lands on the rev-parse path, which was the exposed one. git worktree add -b <branchName> still takes branchName positionally, but -b consumes its value verbatim, so a leading-dash name just becomes an invalid ref name git rejects. Fine to leave.

Must fix before merge

[major] The two new test files will fail tsgo --noEmit. apps/server/tsconfig.json includes src, so *.test.ts is typechecked under strict: true. Vitest transpiles without typechecking, which is why vp test run passes and this stays invisible until the root typecheck:

1. handlers.test.ts:40 — the localStatus stub returns { refName, hasWorkingTreeChanges }, but VcsStatusLocalResult (packages/contracts/src/git.ts:202-219) also requires isRepo, hasPrimaryRemote, isDefaultRef, and workingTree. Four missing required properties.
2. handlers.test.ts:41, ThreadTurnBootstrap.test.ts:12,14 — Effect.fail(new Error(...)) yields E = Error, but these methods declare E = GitCommandError, which is a Schema.TaggedErrorClass (git.ts:330) and therefore nominal. Error is not assignable to it.

Both stubs are typed against the exact service method types via Pick<...>, so there's no structural escape hatch. The repo's own convention for partial stubs is an as unknown as cast (e.g. ActivityPayloadProjection.test.ts:14) — applying that, or constructing a real GitCommandError, fixes it. This is precisely the category the PR flagged as unverifiable, so it's worth a targeted typecheck rather than a full one.

Nits

• ThreadTurnBootstrap.test.ts:20-26,50-56 — expect(error.cause).toMatchObject({ error: expect.objectContaining({...}) }) couples the assertion to Cause's internal shape. Cause.squash/Cause.failures would be more durable. (I couldn't run it; if it passes today, it's fragility rather than a defect.)
• sessionOrchestration.ts:145-146 — the comment still says these fields are "resolved live". Now that #6 removed the forced invalidation, branch/dirty come from the cached status while sha stays live. Worth softening the wording.
• handlers.ts:93 — the swallowed git error isn't logged. A silently-null sha is hard to debug; an Effect.logDebug in the catch would cost nothing.
• Test coverage — resolveSessionCheckout's test asserts the null result, which is real behaviour rather than a smoke test. Neither test covers the actual regression end-to-end (that read_session still returns report/messages when the worktree is gone), nor that non-git defects still propagate. Fine given the starting point.

Merge note

apps/server/src/mcp/toolkits/sessions/handlers.test.ts is still an add/add conflict with PR #5, which creates the same file with an @effect/vitest import. Whichever merges second needs a manual merge.