feat(worktree): auto-rescue dirty worktrees during cleanup

[dimakis]

Summary

• Currently, stale dirty worktrees are detected during cleanup but only get an inbox notification — the uncommitted work is never committed, pushed, or PR'd. If the worktree directory is later removed, that work is lost.
• This branch adds rescueDirtyWorktree() which stages all changes, commits them, pushes the session branch, and creates a draft PR via gh pr create, then posts the PR URL to the mgmt inbox.
• If rescue fails (no remote, push rejected, etc.), the existing skip-and-notify behavior is preserved as a fallback.
• The inbox notification is updated to distinguish between auto-rescued worktrees (with a PR link) and ones that still need manual rescue.

Test plan

• server/__tests__/rescue-worktree.test.ts covers getRepoRemote() (SSH/HTTPS parsing, error cases) and rescueDirtyWorktree() (success path, failure at each step)
• Manual verification: create a session, leave dirty files, wait for cleanup (or trigger manually), confirm draft PR is created and inbox entry contains the PR URL

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

[dimakis]

Centaur Review

LGTM — no issues found.

[dimakis]

Centaur Review

Found 6 issue(s) (3 warning).

server/__tests__/worktree.test.ts

Solid feature — rescue logic and tests for the new functions are well-structured. Main gap is missing integration tests for the rescue-success path within cleanupStaleWorktrees, and the existing dirty-worktree tests now pass by accident (rescue fails silently due to no remote) rather than by design.

• 🟡 regressions (L656): Existing test 'skips dirty stale worktrees and posts inbox proposal' now silently exercises the rescue-failure path (local test repos have no remote, so getRepoRemote returns null) rather than the skip-only path it was originally written for. The test still passes by accident, but it no longer validates the original skip behavior in isolation — and it doesn't validate the new rescue behavior either. Consider adding a test that mocks getRepoRemote/rescueDirtyWorktree (or sets up a remote) to explicitly cover: (a) successful rescue → worktree removed + inbox with PR URL, and (b) failed rescue → worktree preserved + inbox without PR URL. [fixable]

server/__tests__/rescue-worktree.test.ts

Solid feature — rescue logic and tests for the new functions are well-structured. Main gap is missing integration tests for the rescue-success path within cleanupStaleWorktrees, and the existing dirty-worktree tests now pass by accident (rescue fails silently due to no remote) rather than by design.

• 🟡 missing_tests: The new test file covers getRepoRemote and rescueDirtyWorktree in isolation (with mocks), but there is no integration test for the rescue path inside cleanupStaleWorktrees — specifically: when rescue succeeds, the worktree should be removed and cleaned incremented (not skipped). The existing cleanupStaleWorktrees tests all implicitly fail rescue (no git remote) and only cover the fallback skip path. [fixable]

server/worktree.ts

Solid feature — rescue logic and tests for the new functions are well-structured. Main gap is missing integration tests for the rescue-success path within cleanupStaleWorktrees, and the existing dirty-worktree tests now pass by accident (rescue fails silently due to no remote) rather than by design.

• 🟡 bugs (L648): cleanupStaleWorktrees calls rescueDirtyWorktree(fullPath, branch, entry) where branch is session/<entry>. After a successful rescue, removeWorktree(entry, baseRepo) force-removes the worktree directory. If removeWorktree throws (e.g. permission error, git lock), the PR and inbox item are already created, but cleaned++; continue is skipped, so the loop falls through to postDirtyWorktreeToInbox again (no — actually it doesn't because continue is inside the if (rescue.success) block). However, removeWorktree internally swallows most errors (catches execFileSync failure and falls back to rmSync), so this is unlikely to be an issue in practice. Just noting that there's no try/catch around removeWorktree in the rescue-success path. [fixable]
• 🔵 unsafe_assumptions (L450): rescueDirtyWorktree uses git add -A which stages everything in the worktree. While .gitignore should prevent sensitive files from being staged, worktrees inherit the repo's ignore rules — any files that were manually created outside .gitignore coverage (e.g. local scratch files, credential dumps) would be committed and pushed to a remote. Consider using git add . (respects .gitignore but only adds from cwd down) — though in practice both behave identically with respect to .gitignore. Alternatively, a brief log of what's being staged (count of files) would aid debugging.

server/mcp-config.ts

Solid feature — rescue logic and tests for the new functions are well-structured. Main gap is missing integration tests for the rescue-success path within cleanupStaleWorktrees, and the existing dirty-worktree tests now pass by accident (rescue fails silently due to no remote) rather than by design.

• 🔵 style (L28): The JSDoc on loadMcpServers (line 28-35) lists only two config sources: MCP_CONFIG_PATH and ~/.cursor/mcp.json. The new ~/.claude.json source should be documented in that list to keep the comment accurate. [fixable]

server/__tests__/mcp-config.test.ts

Solid feature — rescue logic and tests for the new functions are well-structured. Main gap is missing integration tests for the rescue-success path within cleanupStaleWorktrees, and the existing dirty-worktree tests now pass by accident (rescue fails silently due to no remote) rather than by design.

• 🔵 missing_tests: No test covers the new ~/.claude.json config path in getMcpConfigPaths. The existing getMcpConfigPaths tests only cover the MCP_CONFIG_PATH env var. Adding a test that verifies ~/.claude.json is included when it exists would confirm the new behavior. [fixable]