feat(sessions): serialized worktree cleanup, stale-lock reporting, and merge-proven branch deletion

[roughcoder]

Settling eight children at once with cleanupWorktree: true failed all eight. Concurrent git worktree remove runs on one repository do not queue — they fight over .git/index.lock until each one hits our 15s command timeout, and the only cleanup that worked was the one that ran alone. A removal killed mid-write then left a zero-byte index.lock behind, which blocked every later git command on that repository. Settling also kept quiet about two things it should not have.

Serialize per repository. GitRepositoryLock is one Effect Semaphore per repository root. Worktree removal and branch deletion both run inside it — the ref delete takes a lock in the same repository, so leaving it outside would just move the race. The lock is built once where the sessions toolkit is registered; a per-call instance serializes nothing. Sessions still settle independently of cleanup, so a cleanup failure never blocks a settle.

Report a held lock instead of forcing through it. When a git failure names a lock file, settle_session answers with SessionOrchestrationGitLockError: the path, its age, whether it matches the stale heuristic (empty and older than 60s — git writes the new index into the lock, so an empty one means the writer died before writing), and the remedy. Phoenix never deletes the lock: nothing in this process can prove no live git — a developer's shell, a second Phoenix, an editor — owns it, and deleting a live lock corrupts the index. Naming the file is the part only we can do.

cleanupBranch: true, with proof. The trap: this repo squash-merges, so a merged branch is never an ancestor of main and git branch --merged reports nothing — trusting it would refuse every merged branch, and on a rebase-merging repo it would accept branches that were never merged. The proof is commit identity instead: local head == remote head == the head commit of a merged PR (gh pr list --state merged --head <branch> --json headRefOid), which also means zero commits ahead. Anything else is SessionOrchestrationBranchNotMergedError with the reason and the SHAs that disagree. The proof runs before the worktree is touched, so a refusal costs nothing and leaves a whole job rather than half of one.

No more silent settle-timeout leak (reviewer follow-up from #10). A plain settle whose 10s stop wait times out has nothing to withhold, so it still succeeds — but it now carries a warning naming the provider and the status the session was last seen in.

Tests

settleSession.test.ts drives the real handler against stubs, with the real lock. Eight parallel settles on one repository: all succeed, max concurrent removals is 1, and each removal is immediately followed by its own branch delete. Verified it fails without the lock (8 in flight). A second case proves the lock is per repository, not global. The stop-timeout cases use TestClock. The merge proof is covered per refusal reason, plus the pure helpers for lock-path parsing and staleness.

• npm run typecheck — exit 0
• vp lint --report-unused-disable-directives — exit 0 (warnings pre-existing)
• vp test run src/mcp/toolkits/sessions/ src/sourceControl/ — exit 0, 203 passed
• vp test run contracts — exit 0, 244 passed
• vp test run src/git/GitManager.test.ts -t "cross-repo PRs…" — exit 0 (the --json field assertion this touches)

Full suite left to the merge gate, per the orchestrator.

Written by Claude Opus 5 in Phoenix (Claude Code harness).

🤖 Generated with Claude Code

---

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

[roughcoder]

Verdict: REQUEST-CHANGES

1. [major] SessionOrchestrationGitLockError is unreachable for real worktree lock failures. describeGitFailure parses only cause.message (apps/server/src/mcp/toolkits/sessions/handlers.ts:906-913), but the production Git driver discards stderr and substitutes fallbackErrorDetail when constructing GitCommandError (apps/server/src/vcs/GitVcsDriverCore.ts:873-884; removeWorktree supplies "git worktree remove failed" at :2978-81). Thus the actual .lock path cannot reach the parser; the test injects raw stderr directly (settleSession.test.ts:497-514). Preserve a bounded stderr diagnostic / structured lock indication through the driver and test that real translation.

2. [major] The semaphore is keyed by a non-canonical spelling of the workspace root (apps/server/src/git/GitRepositoryLock.ts:37-38,67). A symlink or path alias to the same Git common dir receives a second permit, recreating the contention this PR fixes (and / normalizes to empty). Key it by canonical realpath/common-git-dir identity and cover aliases.

3. [major] The custom-branch proof is performed before acquiring the semaphore (handlers.ts:1124-45), then removeWorktree/forced deleteRef run later under it (:1149-77). If another actor changes the branch while this call queues, the already-proved SHA is stale and the current ref may be deleted. Move/revalidate proof inside the critical section immediately before mutation.

4. [minor] Lock parsing is English stderr/quoted-path matching (handlers.ts:331-37), and deleteRef lock failures are swallowed as a success detail (:1166-77) rather than yielding the structured lock remedy.

#12 ↔ #17 overlap: HIGH — actual merge-tree conflicts in handlers.ts, settleSession.test.ts, tools.ts, contracts, and docs. #12 introduces shared settleChildCascade + archive semantics; #17 has inline settle logic. Manual reconciliation is required; retain the shared cascade, apply hygiene to it/cleanup, and specify archive stop-timeout reporting.

Verified read-only: real-fiber concurrency test is meaningful; removal+branch deletion share a permit; operations themselves are bounded. Merge proof/unauthenticated gh refusal is conservative/structured. No tests/typecheck/lint run.

[roughcoder]

All four fixed in 6de08575e. Finding 1 was the one that mattered most — you were right that the whole lock path was theatre in production.

1. Unreachable lock error. Confirmed: executeGit built GitCommandError with a fixed detail and only stderrLength, so git's own words never left the driver and only an injected string could match. GitCommandError now carries a bounded stderrExcerpt (500 chars, tail first — git prints progress before the fatal line), attached by executeGit to every non-zero exit, so this fixes diagnosis for every git call site, not just cleanup. The handler reads the typed field rather than scraping .message.

The end-to-end test provokes the real thing: real repo, real branch, a real held refs/heads/feature/locked.lock, the real driver failing against it — then that exact error object goes through the handler and out as SessionOrchestrationGitLockError naming the real lock file. It also caught something an injected string never would: git reports the lock by its resolved path (/private/var/... vs /var/... on macOS), so the assertion had to realpath too.

Worth recording for the merge gate: I checked empirically that git worktree remove succeeds with a planted .git/index.lock — the reproducible lock failure in a real repo is the ref lock on branch delete. That is also why finding 4b is not cosmetic; it was the realistic path and it was being swallowed.

2. Symlink/alias keying. Key is now realPath + the repository's common git directory (a linked worktree's .git file points at …/.git/worktrees/<name>; everything above worktrees/ is shared). New GitRepositoryLock.test.ts covers symlinked alias, linked worktree, unrelated repos staying separate, and a non-repository path degrading to its real path. Four of those fail against the old trimmed-path key — I reverted the keying to check.

3. Merge-proof TOCTOU. The two heads are re-read inside the critical section immediately before the delete. A branch that moved while queued is kept and reported, not deleted, and not a hard failure: the worktree removal was authorized by the dirty check rather than by the proof, so this lands on the same outcome as never passing cleanupBranch instead of leaving half a job. The expensive half (the PR lookup) stays outside the lock — holding a repository hostage across a network call would defeat the purpose. Both new tests fail with the re-check disabled.

4. Robustness + swallowed detail. Matching is on the .lock path artifact now (quoted first, then a bare path-shaped token; a .lock mention with no path separator is ignored), not on quoted English — version, command, and locale all change the prose. Branch-delete lock failures raise the same structured error. One ordering detail I had to get right: the thread's worktreePath is cleared before that error is raised, because the directory really is gone and failing first would leave the thread pointing at it forever.

Kept the real-fiber concurrency test; it now also covers the alias case at the lock level.

Validation: npm run typecheck exit 0 · vp lint exit 0 · vp test run src/mcp/toolkits/sessions/ src/git/ src/sourceControl/ src/vcs/GitVcsDriverCore.test.ts exit 0 (20 files, 351 tests) · contracts exit 0 (244) · touched GitManager assertion exit 0. Root suite left to the merge gate.

Not touching the #12 overlap — waiting for it to land before rebasing and moving this into settleChildCascade.

[roughcoder]

Delta verdict: REQUEST-CHANGES.

1. [major] NOT RESOLVED safely: gitStderrExcerpt is bounded (500 chars, tail-first) but unredacted (apps/server/src/vcs/GitVcsDriverCore.ts:378-397) and is exported on GitCommandError (packages/contracts/src/git.ts:329-345), including generic RPC error paths. Git transport failures can echo credential-bearing HTTPS remotes (userinfo/tokens/query strings), so this can disclose secrets to clients/telemetry. Redact URL credentials/query/fragment before attaching; reuse the source-control transport-safe precedent and add a test.

Original findings:
2. [major] RESOLVED — real driver failures now carry stderr excerpt through the normal helper path (GitVcsDriverCore.ts:874-913); the ref-lock test uses a real held lock and maps to the structured handler error (settleSession.test.ts:561-615). Its scoped temp directory cleans the lock on failure.
3. [major] RESOLVED — repository keys canonicalize real paths and linked-worktree common git dirs, with alias serialization tests and graceful fallbacks (GitRepositoryLock.ts:85-111,148-154; GitRepositoryLock.test.ts:65-160).
4. [major] RESOLVED for deletion safety — head revalidation runs under the permit just before delete (handlers.ts:1259-1273) and permit release is scoped. [minor] A raced reproof returns successful partial cleanup/detail after removing the worktree, rather than the structured BranchNotMerged refusal requested (:1266-1271); test codifies it.

#12: unchanged as claimed; no cascade/archive reconciliation was smuggled in. It remains merge-ready only AFTER #12 lands and a manual shared-handler/contracts/tests/docs reconciliation. No tests/typecheck/lint run.

[roughcoder]

Correction to my delta review: retain a second [major]. The in-lock revalidation closes queued in-process races, but handlers.ts:1260-1282 still reads both heads and then runs forced deleteRef non-atomically. An external git writer (or another Phoenix process) can move the branch in that interval; this server-local semaphore cannot prevent it. For the stated merge-proof safety guarantee, delete with an expected-old-SHA ref transaction/CAS (or otherwise make proof and deletion atomic). Verdict remains REQUEST-CHANGES.

[roughcoder]

Round 3 fixed in 0aacc8c68. Finding 1 was a regression I introduced, and it had a witness I should have read.

1. Unredacted excerpt (security). Fixed, and worse than the report suggested: GitVcsDriverCore.test.ts already had a test named "does not retain git arguments or stderr in command failures" — the driver had a standing policy of keeping stderr off errors, and my excerpt quietly reversed it. The excerpt is now redacted before it is attached and before it is truncated, so a credential cannot survive by sitting on the 500-char boundary. Userinfo, query strings, fragments, and known token shapes (ghp_/gho_/ghu_/ghs_/ghr_/github_pat_/glpat-/sk-) go; scheme, host, and path stay, because which remote failed is the diagnostic value.

Two tests: unit cases over the redactor, and a real failing git fetch against a real credentialed URL asserting the token appears in no string field of the resulting error. One scope note, stated in the code rather than left implicit: argv is deliberately not treated as a secret channel in this driver (execute takes stdin for exactly that reason), so an argument git echoes back is not redacted — if that assumption is wrong anywhere, it is a separate and larger fix.

2. Proof-to-delete CAS. You're right that the in-lock re-check only binds writers in this process. deleteRef now takes expectedSha and routes through git update-ref -d <ref> <old>, so git arbitrates under its own ref lock. Driver tests against a real repo cover both directions: deletes at the expected commit, and refuses with the branch still intact when the ref moved. Temporary t3code/* branches keep the plain force delete — no proof was taken, so there is nothing to hold git to.

One caveat I want on the record rather than buried: update-ref is plumbing and does not refuse a branch checked out in another worktree the way git branch -d does. That is acceptable here only because the call site removes the worktree first, inside the same lock. It is documented on the driver method, since the next caller will not have that context.

3. Structured partial success. worktree.branchRefusal now carries branch, reason, localSha, remoteSha, and the expectedSha the proof took. The reason vocabulary is extracted and shared with SessionOrchestrationBranchNotMergedError (one new member: branch_moved_since_proof), so the two carriers cannot drift. A CAS rejection maps onto the same refusal rather than a prose detail.

Validation: npm run typecheck exit 0 · vp lint exit 0 · vp test run src/mcp/toolkits/sessions/ src/git/ src/sourceControl/ src/vcs/GitVcsDriverCore.test.ts exit 0 (20 files, 359 tests) · contracts exit 0 (244) · touched GitManager assertion exit 0. Targeted only, per the standing instruction.

Still holding on #12 — will rebase and move this into settleChildCascade when you signal.

[roughcoder]

Round-4 delta verdict: REQUEST-CHANGES.

1. [major] The CAS deletion bypasses Git’s checked-out-in-another-worktree protection. cleanupChildWorktree removes only this child worktree (handlers.ts:1263-1275), then a merge-proven branch uses update-ref -d (handlers.ts:1315-1321, GitVcsDriverCore.ts:3175-3189). Another linked worktree can still have that branch checked out (including an external interleaving); the repository semaphore cannot prevent it. git branch -D previously refused this state, but update-ref deletes the ref and leaves the other worktree on a dangling symbolic HEAD. Add a checked-out-worktree guard (with a real linked-worktree regression test) or retain Git’s branch safety check alongside CAS. The docs claim that removing the child worktree makes this safe, which is not sufficient.

Resolved: stderr is redacted before 500-char truncation and attachment; the real credentialed-fetch test checks all string error fields. Actual argv is not exposed by this error/logging path (only argument count), though the comment claiming argv cannot contain secrets is too broad. The real-repo moved-ref CAS refusal/intact test is good.

Not run: tests/typecheck/lint, per constraints. #12 reconciliation remains a separate prerequisite.

[roughcoder]

Fixed in 65804cd9f — the caveat is now a guard.

Checked-out-branch protection. Inside the critical section, before the CAS, git worktree list --porcelain is consulted; a branch still held by any worktree is kept with a structured refusal naming the conflicting path (reason: "branch_checked_out_elsewhere", conflictingWorktreePath). The CAS stays as the final atomicity layer — the guard answers "is anyone sitting on this branch", the CAS answers "did it move under me", and neither substitutes for the other.

An unreadable worktree list fails closed (reason: "worktree_check_unavailable") rather than proceeding: the guard exists to establish that nothing else holds the branch, so failing to establish it is not permission.

Two tests, and I made the driver one prove the hazard rather than assume it:

• Driver: git branch -d refuses a checked-out branch; update-ref -d deletes it. That asymmetry is the whole reason the guard exists, and now it is pinned rather than living in a comment.
• Settle: the real regression you specified — real repository, real second worktree on the branch, settle refuses and both the branch and the other worktree's HEAD survive; remove the worktree; settle deletes. Verified load-bearing: with the guard neutered, that test goes red.

Doc nit. You're right, and the comment was wrong in a way my own test demonstrated — my credentialed-URL redaction test passes the URL as an argument. Corrected: credentialed URLs do reach argv (clone/fetch/push take the remote as an argument), which is precisely why redaction is written against URL shapes rather than a command allowlist. argv itself is never attached to an error — only argumentCount — so stderr echoes are the one outward channel, and that is what is redacted. Bare secrets still belong on stdin, and the comment now says so instead of claiming argv is clean.

Validation: npm run typecheck exit 0 · vp lint exit 0 · vp test run src/mcp/toolkits/sessions/ src/git/ src/sourceControl/ src/vcs/GitVcsDriverCore.test.ts exit 0 (20 files, 363 tests) · contracts exit 0 (244) · touched GitManager assertion exit 0.

Standing by for the #12 merge signal to rebase and move this into settleChildCascade.

[roughcoder]

Final delta verdict: REQUEST-CHANGES.

1. [major] The new guard runs only when provenBranch !== null (handlers.ts:1337), so it misses auto-deleted t3code/* branches (which have no merge proof). Those still reach update-ref -d (:1377-1383); a user can hold one via git worktree add --force, leaving the same dangling-HEAD regression. Apply the guard to every branch deletion and add the real temp-branch case.

2. [major] The stated residual is not bounded by CAS: after a clean list, another worktree can check out the exact branch without moving its OID; CAS succeeds and deletes its HEAD ref. The external-worktree TOCTOU therefore remains. Need a design that retains Git’s worktree protection or a controlled interleaving test/explicitly accepted contract.

Resolved: proven-branch guard is fail-closed and real-test-backed; redaction scope comment is corrected. No checks run.

[roughcoder]

Reversed in 31729eb6e. You're right and my round-4 reasoning was wrong in a way I should have caught: I claimed the CAS "bounds the damage", but a checkout doesn't move the branch OID, so the compare-and-swap isn't bounding that race — it can't see it at all. The guard was doing all the work and the CAS was providing false comfort.

1. Porcelain deletion. deleteRef is git branch -D again, and I removed the expectedSha parameter rather than leaving it in the driver — an unused primitive whose only property is "bypasses the checked-out protection" is an invitation to reintroduce exactly this hazard.

The driver test now pins the reason rather than the mechanism: a checkout leaves the OID unchanged (asserted), porcelain refuses, and plumbing with that very SHA deletes anyway. Anyone who later wonders why we don't compare-and-swap has the answer in the suite instead of in a commit message.

2. In-lock re-proof kept, residual documented. The trade is stated at the code that accepts it and in the internals doc, in your framing: a checkout we get wrong corrupts someone else's directory and no reflog repairs it; a ref move we miss costs a pointer the reflog still holds. Worktree safety absolute, ref-pointer safety best-effort-plus-reflog.

3. Guard on every deletion. Moved out of the provenBranch branch — temporary t3code/* deletions go through it too, since nothing stops a user checking one of those out. Real test for that case: real repo, real worktree on a t3code/… branch, refused; worktree removed; deleted.

One thing worth flagging, because it made my first attempt at these tests worthless: with branch -D as the mechanism and git's own refusal parsed back into the same structured shape, the guard became unobservable — I disabled it and all 29 tests still passed. Both real-worktree tests now record whether git was asked to delete at all, so they distinguish "the guard short-circuited" from "git refused". With the guard neutered they fail; that is the version I'm submitting.

Validation: npm run typecheck exit 0 · vp lint exit 0 · vp test run src/mcp/toolkits/sessions/ src/git/ src/sourceControl/ src/vcs/GitVcsDriverCore.test.ts exit 0 (20 files, 359 tests) · contracts exit 0 (244) · touched GitManager assertion exit 0.

Waiting on the #12 merge signal for the settleChildCascade reconciliation.

[roughcoder]

Final delta verdict: APPROVE.

Verified the deletion reversal is complete: production deleteRef is porcelain git branch -D, with no expectedSha driver path. The in-lock worktree guard is unconditional, so it covers both merge-proven custom branches and automatic t3code/* deletion; unreadable lists fail closed. A late porcelain checked-out-worktree refusal is parsed back into the structured branch_checked_out_elsewhere result with the conflicting path, rather than returning raw stderr.

The accepted residual is documented accurately: an external ref move between re-proof and porcelain delete may lose a recoverable reflog pointer, while porcelain atomically protects a checked-out worktree. Real custom and temporary linked-worktree tests are load-bearing.

Not run: tests/typecheck/lint, per review constraints. Merge-ready after #12 lands and the planned shared-cascade reconciliation.

[roughcoder]

Rebased onto origin/main (6406721) and force-pushed with lease.

New head: 4b808afb052f2c46bbf7ff0d9391e81c11845a73 — 9 commits, main verified as an ancestor.

Conflicts, both keep-everything:

• sessionOrchestration.ts — the SessionOrchestrationError union: SessionOrchestrationReportAlreadySupersededError (feat(sessions): report amendment/supersession #14) plus my BranchNotMerged/GitLock. All three kept.
• handlers.ts — an import list: SessionUsageSnapshot + supersededReportNotice (feat(sessions): report amendment/supersession #14/feat(sessions): usage snapshot for ping_session and post_report #15) alongside SettleSessionBranchRefusal. All kept.

One semantic conflict git could not see, and worth flagging since it's the class of thing a merge preview misses: #14's new postReport.test.ts builds the sessions handlers, which my branch gave two new service requirements (GitRepositoryLock, SourceControlProviderRegistry). Textually clean, wouldn't compile. Fixed in its own commit (4b808afb0) with empty stubs — post_report reaching either would be a bug. Typecheck caught it; nothing else would have until CI.

Verified supersession + usage + my additions all present in the merged contract: supersedesReportId, usage on report/envelope/ping, cleanupBranch, branchProof, branchRefusal, warning, both structured errors.

Exit codes:

• npm run typecheck — 0 (this is what caught the semantic conflict; first post-rebase run was 1)
• contracts vp test run — 0, 19 files / 253 tests (up from 244: feat(sessions): report amendment/supersession #14/feat(sessions): usage snapshot for ping_session and post_report #15's contract tests came along)
• settleSession.test.ts — 0, and all 4 src/mcp/toolkits/sessions/ files pass
• GitVcsDriverCore.test.ts — mixed, and I want to be precise rather than tidy: this box is running six workers at load 40–120, and across five runs a rotating pair of long-running real-git tests hits the 120s test timeout after ~900s of wall-clock starvation. Different victims each run ("unborn HEAD", "pushes upstream branches", "ref snapshot invalidated", and once one of mine). Every one passes in isolation — the test of mine that starved at 903s passes in 3.45s at load 8. None of the recurring victims are in my diff.

I'm not going to claim a green driver file I didn't observe in one run. The evidence that it's contention and not the change: rotating identity, ~900s wall against a 120s budget, and clean passes in isolation. The gate runs serialized, which is the condition where these hold.