fix(hooks): create real git worktrees and clean them up in worktree hooks#74
fix(hooks): create real git worktrees and clean them up in worktree hooks#74gepenniman wants to merge 2 commits into
Conversation
…ooks WorktreeCreate previously logged tracking data but created nothing, which broke every agent spawn because the harness expects the hook to own worktree creation and echo the new path. It now creates a real git worktree on an agents/<name>-<ts> branch under $TMPDIR/pro-workflow/ worktrees (plain directory fallback outside a repo) and records the branch and source repo in worktrees.json. WorktreeRemove previously only logged and echoed stdin, so every isolated agent spawn leaked a registered worktree and branch. It now best-effort runs git worktree remove --force, deletes the matching agents/* branch (never any other namespace), prunes stale admin entries, and drops the ledger row. It always exits 0 and still echoes stdin, and it captures each raw payload to worktree-remove.log for diagnosis if the harness schema changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe worktree-create and worktree-remove hook scripts were rewritten. ChangesWorktree lifecycle hooks
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/worktree-create.js`:
- Around line 24-32: The fallback in worktree creation is too broad: after a
successful git repo check in worktreeCreate, a failure from git worktree add
should not be swallowed and converted into a plain directory. Update the
try/catch around execFileSync in worktree-create.js so that only the non-repo
case uses fs.mkdirSync(target) and returns a null repo, while git worktree
failures are surfaced or rethrown instead of returning { target, branch: null,
repo: null }. Use the existing worktreeCreate flow and the rev-parse/worktree
add calls to separate these cases clearly.
- Around line 19-22: The worktree naming logic in the script should produce
Git-safe branch and directory names and avoid collisions when multiple spawns
happen at the same time. Update the `rawName` sanitization used for `name` so it
cannot yield ref-invalid forms like leading dots, `..`, or repeated dots, and
make the `branch`/`target` suffix more collision-resistant than `Date.now()`
alone. Keep the fix localized around the `name`, `target`, and `branch`
generation so `git worktree add -b` in this flow can succeed without relying on
the fallback directory path.
In `@scripts/worktree-remove.js`:
- Around line 90-91: The worktree removal script is force-exiting after logging,
which can drop buffered stdout output from the hook harness. In the main flow
that prints the payload with console.log, remove the direct process.exit(0) call
and let the script terminate naturally, or set process.exitCode = 0 instead so
the output can flush reliably.
- Around line 48-72: The repo-level cleanup in worktree-remove.js is too
permissive because prune and branch deletion can still run for non-owned paths
once a repo is resolved. Update the cleanup flow around the existing ours,
entry, repo, and branch handling so any call to git worktree prune or git branch
-D only happens when the worktree is owned or has a ledger entry, and keep the
current agents/* branch guard intact. Use the existing worktree removal logic
and repo resolution block to gate all Git cleanup consistently.
🪄 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: 2111b21b-fc3c-4d6f-b689-abce36b2def7
📒 Files selected for processing (2)
scripts/worktree-create.jsscripts/worktree-remove.js
worktree-create.js: - Sanitize names so branch/dir suffixes can't produce git-ref-invalid forms (leading dot, "..", trailing ".lock"). - Add a random suffix alongside the timestamp so parallel spawns with the same name can't collide on the same branch/target. - Stop conflating "not a git repo" with "git worktree add failed inside a confirmed repo" - the latter now logs to stderr instead of silently downgrading to an indistinguishable plain directory. worktree-remove.js: - Gate the prune and branch-delete git calls on the same ownership check used for directory removal (ours || entry), not on repo alone. repo can resolve via input.cwd, which may be the session's main repo rather than a worktree this hook created. - Drop the trailing process.exit(0): process.stdout is written asynchronously when the destination is a pipe, and exit() doesn't wait for the flush, so the echoed payload could be truncated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes #73
Problem
scripts/worktree-create.jsends withconsole.log(data), echoing its stdin JSON on stdout. Claude Code treats WorktreeCreate stdout as the path of the worktree the hook created, so every isolatedAgentspawn fails withWorktreeCreate hook returned a path that is not a directory. Its companionscripts/worktree-remove.jsonly logs, so once creation works, every spawn would leak a registered worktree and branch. Details, contract notes, and repro are in the linked issue.Change
worktree-create.jsnow fulfills the contract it was accidentally claiming:git -C <input.cwd> worktree add -b agents/<name>-<timestamp>into$TMPDIR/pro-workflow/worktrees/, so the agent gets a real checkout of the session's HEAD.worktrees.jsonledger, now recording the real worktree path, branch, and source repo instead of"unknown".worktree-remove.jsnow actually cleans up:git worktree remove --forceon the payload'sworktree_path, deletes the matchingagents/*branch (never any other namespace), runsgit worktree prune, and drops the ledger row.worktree-remove.logfor diagnosis if the harness schema ever changes.No changes to
hooks/hooks.jsonor any other script.Testing
isolation: "worktree". Before: instant failure. After: agent lands in a working checkout (pwdequalsgit rev-parse --show-toplevel, tracked files present).agents/*branch whencwdis a repo, a plain directory otherwise, and survives malformed stdin; remove tears down the worktree, branch, and ledger entry, and exits 0 on missing or bogus paths.Alternative
If you would rather not own worktree creation in a logging plugin, the smaller fix is deleting the
WorktreeCreateandWorktreeRemoveentries fromhooks/hooks.jsonand letting the harness create worktrees itself (SubagentStart/SubagentStopalready log spawn lifecycle). Happy to rework this PR that way instead.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes