feat(worktree): add lifecycle management with automatic garbage collection#668
Open
JasonOA888 wants to merge 8 commits intoobra:mainfrom
Open
feat(worktree): add lifecycle management with automatic garbage collection#668JasonOA888 wants to merge 8 commits intoobra:mainfrom
JasonOA888 wants to merge 8 commits intoobra:mainfrom
Conversation
…ction ## Problem (obra#666) Git worktrees created by Superpowers persist indefinitely, accumulating in .worktrees/ or ~/.config/superpowers/worktrees/. After branches are merged or abandoned, leftover worktrees waste disk space. ## Solution Implement comprehensive worktree lifecycle management with automatic garbage collection: ### Core Components 1. **Worktree Registry** (~/.config/superpowers/worktree-registry.json) - Tracks all worktrees created by Superpowers - Stores metadata: branch, path, status, timestamps - Maintains GC policy configuration 2. **Worktree Manager CLI** (lib/worktree-manager) - register/unregister worktrees - Update status (active → pr_opened → merged → cleanup_pending) - List worktrees by project - Find worktrees by branch - Run garbage collection 3. **GC Daemon** (lib/worktree-gc-daemon) - Background process running every hour - Detects status changes (branch deleted, PR merged) - Safely cleans up pending worktrees 4. **Git Post-Merge Hook** (lib/hooks/post-merge) - Detects when branches are merged - Schedules worktree cleanup with 1-hour delay - Provides user-friendly notifications ### Status Lifecycle ### Safety Guarantees - Never delete worktrees with uncommitted changes - Never delete active worktrees - Grace period (1h for merged, immediate for abandoned) - Full audit log at ~/.config/superpowers/worktree-gc.log ### Integration - skill registers new worktrees - skill triggers cleanup - Optional: Install post-merge hook for automatic cleanup ## Files Added - lib/worktree-registry.schema.json - Registry schema - lib/worktree-manager - CLI tool for registry management - lib/worktree-gc-daemon - Background GC process - lib/hooks/post-merge - Git hook for merge detection - docs/worktree-lifecycle.md - User documentation ## Files Modified - skills/using-git-worktrees/SKILL.md - Register worktrees on creation Fixes obra#666
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a JSON-backed worktree registry and CLI manager, an hourly GC daemon, a post-merge git hook, a registry template, skill integration to auto-register created worktrees, and comprehensive lifecycle documentation. The post-merge hook includes a duplicated search/early-exit bug that can prevent updates and logging. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Skill
participant Git as Git
participant Hook as Post-Merge Hook
participant Manager as Worktree Manager (lib/worktree-manager)
participant Registry as Registry (JSON)
participant Daemon as GC Daemon (lib/worktree-gc-daemon)
User->>Git: git worktree add ...
Git-->>User: new worktree path
User->>Manager: register(path, branch, project_root, metadata)
Manager->>Registry: write entry (id, path, branch, status=active)
Registry-->>Manager: OK
Manager-->>User: worktree_id
Note over Git,Hook: later: branch merged
Git->>Hook: post-merge runs
Hook->>Manager: find_by_branch(merged_branch)
Manager->>Registry: query entries
Registry-->>Manager: matching entry
Manager->>Registry: update_status(id, merged)
Registry-->>Manager: OK
Daemon->>Manager: detect_status_changes()
Manager->>Registry: read active/pr_opened entries
Manager->>Git: check branch existence / gh for PR state
Manager->>Registry: update statuses (merged/pr_opened/abandoned/cleanup_pending)
Daemon->>Manager: run_gc()
Manager->>Registry: list cleanup_pending
Manager->>Git: git worktree remove / rm -rf (if safe)
Manager->>Registry: unregister_worktree(id)
Registry-->>Manager: OK
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
lib/worktree-manager (1)
180-186: Background cleanup scheduling is fragile and duplicative.Spawning
(sleep 3600 && ... ) &indetect_status_changescreates orphan processes that may be killed unexpectedly. Since the GC daemon already runsdetectandgchourly, this background scheduling is redundant—the next daemon cycle will pick up themergedstatus and handle the transition.♻️ Remove background scheduling, let daemon handle it
if [ "$pr_status" = "MERGED" ]; then update_status "$wt_id" "merged" - # Schedule cleanup after delay - (sleep 3600 && update_status "$wt_id" "cleanup_pending" && run_gc) & + # Daemon will transition to cleanup_pending on next cycle elif [ "$pr_status" = "OPEN" ]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/worktree-manager` around lines 180 - 186, The code spawns a background subshell "(sleep 3600 && update_status \"$wt_id\" \"cleanup_pending\" && run_gc) &" inside detect_status_changes which is redundant and fragile; remove that entire background scheduling line so that on MERGED you only call update_status "$wt_id" "merged" and rely on the existing GC daemon (which runs detect and gc hourly) to transition to cleanup_pending and run_gc; update the detect_status_changes function to drop the subshell and leave the merged branch with a single update_status call.lib/hooks/post-merge (1)
36-41: Background cleanup process is fragile and redundant.The subshell background process may be killed when the terminal closes since it lacks
nohupordisown. Additionally, this duplicates the GC daemon'sdetect_status_changeswhich already handles merged→cleanup_pending transitions hourly.Consider removing the background scheduling and relying on the daemon, or use
nohup ... &if immediate scheduling is required.♻️ Simplified approach relying on daemon
# Update status to merged first "$WORKTREE_MANAGER" update "$WORKTREE_ID" "merged" - - # Schedule cleanup in background - ( - sleep 3600 # 1 hour delay - "$WORKTREE_MANAGER" update "$WORKTREE_ID" "cleanup_pending" - "$WORKTREE_MANAGER" gc - ) & + # Daemon will handle transition to cleanup_pending after delay else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/post-merge` around lines 36 - 41, Remove the fragile subshell background scheduling block that calls "$WORKTREE_MANAGER" update "$WORKTREE_ID" "cleanup_pending" and "$WORKTREE_MANAGER" gc from lib/hooks/post-merge and rely on the existing GC daemon's detect_status_changes to handle merged→cleanup_pending transitions hourly; if immediate scheduling is actually required, replace the subshell with a nohup-wrapped invocation (e.g., nohup "$WORKTREE_MANAGER" update "$WORKTREE_ID" "cleanup_pending" && nohup "$WORKTREE_MANAGER" gc >/dev/null 2>&1 & or use disown) so the process is not killed when the terminal closes.lib/worktree-registry.schema.json (1)
1-14: File is a default registry template, not a JSON Schema.This file is named
worktree-registry.schema.jsonbut contains actual data (emptyworktreesarray, concretemetadatavalues) rather than a JSON Schema definition. A proper JSON Schema would usetype,properties,items, andrequiredkeywords to define structure constraints.Consider either:
- Rename to
worktree-registry.default.jsonorworktree-registry.template.json- Or create an actual JSON Schema for validation
📄 Example of what an actual JSON Schema would look like
{ "$schema": "https://json-schema.org/draft/2020-12/schema", "type": "object", "required": ["version", "worktrees", "metadata"], "properties": { "version": { "type": "integer" }, "worktrees": { "type": "array", "items": { "type": "object", "required": ["id", "path", "branch", "status"], "properties": { "id": { "type": "string" }, "path": { "type": "string" }, "branch": { "type": "string" }, "status": { "enum": ["active", "pr_opened", "merged", "abandoned", "cleanup_pending"] } } } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/worktree-registry.schema.json` around lines 1 - 14, The file currently contains concrete registry data (keys "version", "worktrees", "metadata") but is named as a JSON Schema; either rename the file to something like worktree-registry.default.json or worktree-registry.template.json, or convert it into a true JSON Schema by replacing the current data with schema definitions using "type", "properties", "required" and proper "items" for the "worktrees" array (and validate fields like "version", each worktree's "id", "path", "branch", "status", and the "metadata.gc_policy" structure) so consumers expecting a schema get a schema instead of sample data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/worktree-lifecycle.md`:
- Around line 52-57: Replace the incorrect and destructive startup instructions
that symlink ~/.config/superpowers/lib/worktree-gc-daemon into LaunchAgents and
that overwrite crontab; instead document creating a proper LaunchAgent plist
(e.g., com.superpowers.worktree-gc.plist) that lists ProgramArguments including
~/.config/superpowers/lib/worktree-gc-daemon and using launchctl load to enable
it on macOS, and on Linux show how to append the `@reboot` entry to the existing
crontab using a safe form like (crontab -l 2>/dev/null; echo "@reboot
~/.config/superpowers/lib/worktree-gc-daemon") | crontab - so existing crontab
entries are preserved.
In `@lib/hooks/post-merge`:
- Around line 13-17: The script currently sets MERGED_BRANCH from the current
branch (git rev-parse --abbrev-ref HEAD) which yields the target branch after a
merge; instead detect the merged-in branch by checking if HEAD is a merge commit
and, when it is, resolve the merged parent (HEAD^2) to a branch name (e.g., use
git name-rev --name-only HEAD^2 or similar) and assign that to MERGED_BRANCH;
fall back to the original HEAD logic if it’s not a merge commit; then continue
using WORKTREE_INFO="$("$WORKTREE_MANAGER" find "$MERGED_BRANCH" ...)" so
worktrees are looked up for the feature/source branch rather than the current
target branch.
In `@lib/worktree-gc-daemon`:
- Around line 27-31: The log() function writes to $LOG_FILE but doesn't ensure
its parent directory exists; update the code so before writing (either at daemon
startup before the first log call or inside log()) you create the parent
directory with mkdir -p "$(dirname "$LOG_FILE")" (quoted) and handle mkdir
failures (emit a stderr message and exit or fallback) so the initial log write
won't fail when worktree-manager init hasn't run yet; reference the log()
function and $LOG_FILE in your change.
- Around line 12-22: The PID-file logic has a TOCTOU race: checking
DAEMON_PID_FILE, reading OLD_PID, and later echoing $$ > "$DAEMON_PID_FILE" can
allow two instances to start concurrently; replace that sequence with an atomic
lock (e.g., create a lock directory or use flock) around the critical section so
only one process wins the lock, then inside the lock check the existing PID with
kill -0 "$OLD_PID" to detect a live daemon and write the PID (echo $$ >
"$DAEMON_PID_FILE") only while holding the lock; ensure the lock is released and
DAEMON_PID_FILE cleaned on exit, and handle stale PID cases when the lock is
acquired but the recorded PID is dead.
In `@lib/worktree-manager`:
- Around line 202-205: The register branch of the case statement calls
init_registry and then register_worktree with positional args that can be empty;
add argument validation before calling register_worktree to ensure required
values (e.g., path and branch) are present and non-empty, and print a clear
usage/error and exit if they are missing; update the register) block to validate
the inputs (or supply sensible defaults) and only call register_worktree when
validation passes so register_worktree never receives empty strings for required
fields.
- Around line 138-141: Replace the direct deletion rm -rf "$wt_path" with the
proper git cleanup: run git worktree remove "$wt_path" (use --force if the
worktree might be missing/stale) and check its exit status before calling
unregister_worktree "$wt_id"; keep wt_path and wt_id variables and the
unregister_worktree call, but ensure git worktree remove succeeds (or
handle/report its error) so git's internal worktree registry stays consistent.
- Around line 130-133: The base64 decode call is not portable (macOS uses -D);
update the decoding used for wt_b64 (which populates wt_json, wt_path, wt_id) to
a portable strategy: prefer base64 --decode, fall back to -d or -D as available,
and if those flags fail, use a reliable fallback (eg. python -m base64 or
openssl base64 -d) so wt_json is correctly produced on both GNU and BSD systems;
implement the fallback logic inline where wt_b64 is decoded so wt_json/jq
parsing remains unchanged.
In `@skills/using-git-worktrees/SKILL.md`:
- Around line 98-105: The problem is that the quoted "$path" (variable path) may
contain a leading tilde which won't expand when passed to the worktree-manager
register command; fix by expanding the tilde before calling the register command
(e.g. compute an expanded_path from path using parameter expansion like
replacing a leading '~' with $HOME or by resolving with realpath -m) and pass
that expanded_path to the worktree-manager invocation instead of "$path" so the
registry stores an absolute path.
---
Nitpick comments:
In `@lib/hooks/post-merge`:
- Around line 36-41: Remove the fragile subshell background scheduling block
that calls "$WORKTREE_MANAGER" update "$WORKTREE_ID" "cleanup_pending" and
"$WORKTREE_MANAGER" gc from lib/hooks/post-merge and rely on the existing GC
daemon's detect_status_changes to handle merged→cleanup_pending transitions
hourly; if immediate scheduling is actually required, replace the subshell with
a nohup-wrapped invocation (e.g., nohup "$WORKTREE_MANAGER" update
"$WORKTREE_ID" "cleanup_pending" && nohup "$WORKTREE_MANAGER" gc >/dev/null 2>&1
& or use disown) so the process is not killed when the terminal closes.
In `@lib/worktree-manager`:
- Around line 180-186: The code spawns a background subshell "(sleep 3600 &&
update_status \"$wt_id\" \"cleanup_pending\" && run_gc) &" inside
detect_status_changes which is redundant and fragile; remove that entire
background scheduling line so that on MERGED you only call update_status
"$wt_id" "merged" and rely on the existing GC daemon (which runs detect and gc
hourly) to transition to cleanup_pending and run_gc; update the
detect_status_changes function to drop the subshell and leave the merged branch
with a single update_status call.
In `@lib/worktree-registry.schema.json`:
- Around line 1-14: The file currently contains concrete registry data (keys
"version", "worktrees", "metadata") but is named as a JSON Schema; either rename
the file to something like worktree-registry.default.json or
worktree-registry.template.json, or convert it into a true JSON Schema by
replacing the current data with schema definitions using "type", "properties",
"required" and proper "items" for the "worktrees" array (and validate fields
like "version", each worktree's "id", "path", "branch", "status", and the
"metadata.gc_policy" structure) so consumers expecting a schema get a schema
instead of sample data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ebf9bbed-5084-44ef-9252-d1f1d2386f75
📒 Files selected for processing (6)
docs/worktree-lifecycle.mdlib/hooks/post-mergelib/worktree-gc-daemonlib/worktree-managerlib/worktree-registry.schema.jsonskills/using-git-worktrees/SKILL.md
## Critical Fixes
1. **post-merge: Get correct merged branch**
- Use git reflog to find source branch, not target
- Fix: `git reflog -1 --format='%gs'` parsing
2. **worktree-manager: Use git worktree remove**
- Replace `rm -rf` with proper `git worktree remove`
- Prevents git's internal registry inconsistency
3. **worktree-manager: Portable base64 decode**
- Add fallback for macOS (uses -D flag)
- Works on GNU, BSD, and falls back to Python
## Major Fixes
4. **Remove fragile background scheduling**
- Removed `(sleep 3600 && ...)` subshells
- Let GC daemon handle all transitions
- Prevents orphan processes
5. **worktree-gc-daemon: Atomic lock acquisition**
- Use flock for TOCTOU-safe locking
- Prevents concurrent daemon instances
- Create log directory on startup
6. **worktree-manager: Argument validation**
- Validate path and branch are non-empty
- Prevent invalid registry entries
7. **using-git-worktrees: Expand tilde**
- Use ${HOME} instead of ~ in path
- Prevents unexpanded tildes in registry
## Minor Fixes
8. **docs: Correct LaunchAgent/crontab setup**
- macOS: Create proper .plist file
- Linux: Append to crontab (not replace)
9. **Rename schema to template**
- worktree-registry.schema.json → worktree-registry.template.json
- Clarifies it's sample data, not JSON Schema
Addresses all 8 actionable + 3 nitpick comments from CodeRabbit
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
lib/worktree-manager (3)
242-253: Missing argument validation forupdate,find, andunregistercommands.While
registernow validates its arguments, the other commands that require arguments (update,find,unregister) will fail with confusing errors if arguments are omitted.♻️ Add argument validation
update) + if [ -z "${2:-}" ] || [ -z "${3:-}" ]; then + echo "Error: update requires <id> and <status> arguments" >&2 + exit 1 + fi update_status "$2" "$3" ;; list) list_project_worktrees "${2:-$(pwd)}" ;; find) + if [ -z "${2:-}" ]; then + echo "Error: find requires <branch> argument" >&2 + exit 1 + fi find_by_branch "$2" ;; unregister) + if [ -z "${2:-}" ]; then + echo "Error: unregister requires <id> argument" >&2 + exit 1 + fi unregister_worktree "$2" ;;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/worktree-manager` around lines 242 - 253, The case branches for update, find, and unregister currently call update_status, find_by_branch, and unregister_worktree without validating their arguments; add explicit argument checks in the corresponding case labels (update, find, unregister) to ensure required parameters are provided, e.g. verify $2 (or appropriate positional) is non-empty before calling update_status/find_by_branch/unregister_worktree, and if missing print a clear usage/error message and exit or return early to avoid confusing failures.
69-80: Consider validating status values.The function accepts any string for
new_status, but only specific values are valid (active,pr_opened,merged,abandoned,cleanup_pending). Invalid status values would silently corrupt the registry state.♻️ Optional: Add status validation
update_status() { local worktree_id="$1" local new_status="$2" # active | pr_opened | merged | abandoned | cleanup_pending + # Validate status + case "$new_status" in + active|pr_opened|merged|abandoned|cleanup_pending) ;; + *) + echo "Error: Invalid status '$new_status'" >&2 + return 1 + ;; + esac + if command -v jq >/dev/null 2>&1; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/worktree-manager` around lines 69 - 80, The update_status function currently accepts any new_status and may corrupt REGISTRY_FILE; add validation in update_status to allow only the values active, pr_opened, merged, abandoned, or cleanup_pending (e.g., compare $new_status against that whitelist) and abort with a non-zero return (and an error message to stderr) if the value is invalid, performing the jq update only after the check passes; reference the update_status function name and REGISTRY_FILE/jq block when making the change.
219-223: Redundant double status update for abandoned worktrees.When a branch doesn't exist,
update_statusis called twice consecutively—first with "abandoned" then immediately with "cleanup_pending". This is inefficient and the intermediate "abandoned" state is never observable.♻️ Simplify to single status update
else # Branch doesn't exist, mark as abandoned - update_status "$wt_id" "abandoned" - update_status "$wt_id" "cleanup_pending" + # Mark directly as cleanup_pending (abandoned state is implicit) + update_status "$wt_id" "cleanup_pending" fiAlternatively, if the "abandoned" status needs to be recorded for audit purposes, consider adding a separate
abandoned_attimestamp field rather than an ephemeral status transition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/worktree-manager` around lines 219 - 223, The code calls update_status "$wt_id" "abandoned" immediately followed by update_status "$wt_id" "cleanup_pending", which makes the intermediate "abandoned" state unobservable; change this to a single update_status call (e.g., update_status "$wt_id" "cleanup_pending") to remove the redundant status write, or if audit of abandonment is required add a separate field update (e.g., set_abandoned_at "$wt_id" "$(date --iso-8601=seconds)") instead of issuing the ephemeral "abandoned" status; locate the two calls to update_status in the else branch and replace with one appropriate action.lib/worktree-gc-daemon (1)
29-47: Consider log rotation and graceful shutdown.Two operational concerns:
Log growth: The log file grows indefinitely. Consider adding rotation or directing users to configure
logrotate.Graceful shutdown: The daemon doesn't handle SIGTERM gracefully—it will continue sleeping. Adding signal handling would allow cleaner stops.
♻️ Optional: Add graceful shutdown
+# Handle shutdown signals +shutdown() { + log "Daemon shutting down" + exit 0 +} +trap 'shutdown' SIGTERM SIGINT + log "Worktree GC daemon started (PID: $$)" # Main loop while true; doFor log rotation, you could add a note in documentation:
# Add to /etc/logrotate.d/superpowers-worktree-gc: ~/.config/superpowers/worktree-gc.log { weekly rotate 4 compress missingok }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/worktree-gc-daemon` around lines 29 - 47, Add log rotation guidance and implement graceful shutdown: document a recommended logrotate stanza for LOG_FILE, and modify the daemon to handle SIGTERM/SIGINT by adding a trap that sets a RUNNING flag (or writes a pid/stop file) and causes the main loop (replace while true with while [ "$RUNNING" = "1" ]) to exit so the process stops cleanly; update the sleep usage to wake early when RUNNING is cleared (e.g., use a short sleep loop or wait on a condition) and ensure any in-progress "$WORKTREE_MANAGER" detect/gc calls finish or are safely aborted before exit; keep the existing log() function and LOG_FILE usage for all shutdown and error messages so the shutdown is logged.lib/hooks/post-merge (1)
13-20: Branch detection may miss fast-forward and squash merges.The reflog parsing looks for "merge" in the entry, which works for true merge commits. However:
- Fast-forward merges may show as "checkout" in reflog
- Squash merges (when
$1is "1") have different reflog patternsThe silent exit fallback handles these gracefully, but these merge types won't trigger automatic status updates.
Consider documenting this limitation or enhancing detection:
# For squash merges, $1 is "1" if [ "${1:-}" = "1" ]; then # Squash merge - could try to detect branch differently exit 0 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/post-merge` around lines 13 - 20, The current reflog parsing only finds true merge commits via MERGED_BRANCH and misses fast-forward and squash merges; update the hook to first check for squash by testing if "${1:-}" = "1" (handle or exit early) and, when MERGED_BRANCH is empty, add a fallback detection using earlier reflog/HEAD entries (e.g., run git name-rev --name-only HEAD@{1} or git branch --contains HEAD@{1} and use that result as MERGED_BRANCH) so fast-forward merges are detected instead of silently exiting; ensure you still preserve the existing MERGED_BRANCH variable if the primary regex succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/hooks/post-merge`:
- Around line 22-35: The current logic calling "$WORKTREE_MANAGER" find
"$MERGED_BRANCH" can return multiple JSON objects and jq -r '.id' will
concatenate them; change the extraction to only take the first match instead of
all matches: after capturing WORKTREE_INFO, parse WORKTREE_ID and WORKTREE_PATH
with a jq slurp-first expression (e.g., using jq -s and selecting index 0) so
WORKTREE_ID and WORKTREE_PATH are single values before calling
"$WORKTREE_MANAGER" update "$WORKTREE_ID" "merged"; alternatively, if you prefer
stricter matching, update the find invocation to include the current
repository/project_root filter so the returned WORKTREE_INFO is unique.
In `@lib/worktree-gc-daemon`:
- Around line 36-47: The gc_policy fields are defined but not enforced: update
the daemon to read gc_policy.cleanup_delay_hours and gc_policy.max_age_days (and
use gc_policy.interval_hours instead of the hardcoded 3600) and enforce them
inside worktree manager calls; specifically, change detect_status_changes
(invoked via WORKTREE_MANAGER detect) to only mark worktrees as cleanup_pending
when merged_at + cleanup_delay_hours has elapsed, and change run_gc (invoked via
WORKTREE_MANAGER gc) to auto-abandon or remove active worktrees whose created_at
is older than max_age_days before cleanup; ensure logging to LOG_FILE still
captures decisions and keep variable names cleanup_pending, merged_at,
created_at, WORKTREE_MANAGER, LOG_FILE, and gc_policy for discoverability.
In `@lib/worktree-manager`:
- Around line 43-54: The heredoc building the JSON assigned to the variable
entry directly interpolates shell variables (worktree_id, worktree_path, branch,
project_root, metadata) and can produce invalid or unsafe JSON; replace the raw
heredoc with a safe JSON construction using a JSON tool (e.g., jq) or shell-safe
serialization: call jq -n (or equivalent) and pass each value via
--arg/--argjson to construct the object with keys id, path, branch,
project_root, created_at, status, metadata so all values (including branch and
path) are properly escaped and metadata is injected as JSON, then assign that
output to entry instead of the heredoc.
---
Nitpick comments:
In `@lib/hooks/post-merge`:
- Around line 13-20: The current reflog parsing only finds true merge commits
via MERGED_BRANCH and misses fast-forward and squash merges; update the hook to
first check for squash by testing if "${1:-}" = "1" (handle or exit early) and,
when MERGED_BRANCH is empty, add a fallback detection using earlier reflog/HEAD
entries (e.g., run git name-rev --name-only HEAD@{1} or git branch --contains
HEAD@{1} and use that result as MERGED_BRANCH) so fast-forward merges are
detected instead of silently exiting; ensure you still preserve the existing
MERGED_BRANCH variable if the primary regex succeeds.
In `@lib/worktree-gc-daemon`:
- Around line 29-47: Add log rotation guidance and implement graceful shutdown:
document a recommended logrotate stanza for LOG_FILE, and modify the daemon to
handle SIGTERM/SIGINT by adding a trap that sets a RUNNING flag (or writes a
pid/stop file) and causes the main loop (replace while true with while [
"$RUNNING" = "1" ]) to exit so the process stops cleanly; update the sleep usage
to wake early when RUNNING is cleared (e.g., use a short sleep loop or wait on a
condition) and ensure any in-progress "$WORKTREE_MANAGER" detect/gc calls finish
or are safely aborted before exit; keep the existing log() function and LOG_FILE
usage for all shutdown and error messages so the shutdown is logged.
In `@lib/worktree-manager`:
- Around line 242-253: The case branches for update, find, and unregister
currently call update_status, find_by_branch, and unregister_worktree without
validating their arguments; add explicit argument checks in the corresponding
case labels (update, find, unregister) to ensure required parameters are
provided, e.g. verify $2 (or appropriate positional) is non-empty before calling
update_status/find_by_branch/unregister_worktree, and if missing print a clear
usage/error message and exit or return early to avoid confusing failures.
- Around line 69-80: The update_status function currently accepts any new_status
and may corrupt REGISTRY_FILE; add validation in update_status to allow only the
values active, pr_opened, merged, abandoned, or cleanup_pending (e.g., compare
$new_status against that whitelist) and abort with a non-zero return (and an
error message to stderr) if the value is invalid, performing the jq update only
after the check passes; reference the update_status function name and
REGISTRY_FILE/jq block when making the change.
- Around line 219-223: The code calls update_status "$wt_id" "abandoned"
immediately followed by update_status "$wt_id" "cleanup_pending", which makes
the intermediate "abandoned" state unobservable; change this to a single
update_status call (e.g., update_status "$wt_id" "cleanup_pending") to remove
the redundant status write, or if audit of abandonment is required add a
separate field update (e.g., set_abandoned_at "$wt_id" "$(date
--iso-8601=seconds)") instead of issuing the ephemeral "abandoned" status;
locate the two calls to update_status in the else branch and replace with one
appropriate action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9b7d9021-b282-4de3-aad2-6baf7d6afc89
📒 Files selected for processing (6)
docs/worktree-lifecycle.mdlib/hooks/post-mergelib/worktree-gc-daemonlib/worktree-managerlib/worktree-registry.template.jsonskills/using-git-worktrees/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/using-git-worktrees/SKILL.md
- docs/worktree-lifecycle.md
Comment on lines
+22
to
+35
| # Find matching worktrees | ||
| WORKTREE_INFO=$("$WORKTREE_MANAGER" find "$MERGED_BRANCH" 2>/dev/null || true) | ||
|
|
||
| if [ -n "$WORKTREE_INFO" ]; then | ||
| WORKTREE_ID=$(echo "$WORKTREE_INFO" | jq -r '.id') | ||
| WORKTREE_PATH=$(echo "$WORKTREE_INFO" | jq -r '.path') | ||
|
|
||
| echo "🎉 Branch '$MERGED_BRANCH' merged!" | ||
|
|
||
| # Update status to merged (daemon will handle cleanup) | ||
| "$WORKTREE_MANAGER" update "$WORKTREE_ID" "merged" | ||
|
|
||
| echo "✅ Worktree marked as merged. GC daemon will clean up: $WORKTREE_PATH" | ||
| fi |
There was a problem hiding this comment.
Potential issue with multiple worktrees for the same branch.
If multiple worktrees are registered for the same branch (e.g., from different projects), find_by_branch returns multiple JSON objects, and jq -r '.id' will concatenate all IDs into a single string. The subsequent update call will fail or update the wrong entry.
🛡️ Proposed fix to handle first match only
-WORKTREE_INFO=$("$WORKTREE_MANAGER" find "$MERGED_BRANCH" 2>/dev/null || true)
+# Get first matching worktree (there could be multiple across projects)
+WORKTREE_INFO=$("$WORKTREE_MANAGER" find "$MERGED_BRANCH" 2>/dev/null | jq -s '.[0]' || true)
if [ -n "$WORKTREE_INFO" ]; then
+ if [ "$WORKTREE_INFO" = "null" ]; then
+ exit 0
+ fi
WORKTREE_ID=$(echo "$WORKTREE_INFO" | jq -r '.id')Alternatively, modify find_by_branch to also filter by project_root matching the current repository.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Find matching worktrees | |
| WORKTREE_INFO=$("$WORKTREE_MANAGER" find "$MERGED_BRANCH" 2>/dev/null || true) | |
| if [ -n "$WORKTREE_INFO" ]; then | |
| WORKTREE_ID=$(echo "$WORKTREE_INFO" | jq -r '.id') | |
| WORKTREE_PATH=$(echo "$WORKTREE_INFO" | jq -r '.path') | |
| echo "🎉 Branch '$MERGED_BRANCH' merged!" | |
| # Update status to merged (daemon will handle cleanup) | |
| "$WORKTREE_MANAGER" update "$WORKTREE_ID" "merged" | |
| echo "✅ Worktree marked as merged. GC daemon will clean up: $WORKTREE_PATH" | |
| fi | |
| # Find matching worktrees | |
| # Get first matching worktree (there could be multiple across projects) | |
| WORKTREE_INFO=$("$WORKTREE_MANAGER" find "$MERGED_BRANCH" 2>/dev/null | jq -s '.[0]' || true) | |
| if [ -n "$WORKTREE_INFO" ]; then | |
| if [ "$WORKTREE_INFO" = "null" ]; then | |
| exit 0 | |
| fi | |
| WORKTREE_ID=$(echo "$WORKTREE_INFO" | jq -r '.id') | |
| WORKTREE_PATH=$(echo "$WORKTREE_INFO" | jq -r '.path') | |
| echo "🎉 Branch '$MERGED_BRANCH' merged!" | |
| # Update status to merged (daemon will handle cleanup) | |
| "$WORKTREE_MANAGER" update "$WORKTREE_ID" "merged" | |
| echo "✅ Worktree marked as merged. GC daemon will clean up: $WORKTREE_PATH" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/hooks/post-merge` around lines 22 - 35, The current logic calling
"$WORKTREE_MANAGER" find "$MERGED_BRANCH" can return multiple JSON objects and
jq -r '.id' will concatenate them; change the extraction to only take the first
match instead of all matches: after capturing WORKTREE_INFO, parse WORKTREE_ID
and WORKTREE_PATH with a jq slurp-first expression (e.g., using jq -s and
selecting index 0) so WORKTREE_ID and WORKTREE_PATH are single values before
calling "$WORKTREE_MANAGER" update "$WORKTREE_ID" "merged"; alternatively, if
you prefer stricter matching, update the find invocation to include the current
repository/project_root filter so the returned WORKTREE_INFO is unique.
lib/worktree-gc-daemon
Outdated
Comment on lines
+36
to
+47
| while true; do | ||
| # Detect status changes | ||
| if [ -x "$WORKTREE_MANAGER" ]; then | ||
| "$WORKTREE_MANAGER" detect >> "$LOG_FILE" 2>&1 || true | ||
|
|
||
| # Run garbage collection | ||
| "$WORKTREE_MANAGER" gc >> "$LOG_FILE" 2>&1 || true | ||
| fi | ||
|
|
||
| # Sleep for 1 hour | ||
| sleep 3600 | ||
| done |
There was a problem hiding this comment.
gc_policy configuration is defined but not enforced.
The registry template defines cleanup_delay_hours: 24 and max_age_days: 30, but neither the daemon nor worktree-manager gc actually reads or enforces these values:
cleanup_delay_hours: Merged worktrees should wait 24 hours before cleanup, butrun_gcimmediately removescleanup_pendingentries.max_age_days: Old active worktrees should be auto-cleaned after 30 days, but this is never checked.
The hardcoded 3600-second sleep matches interval_hours, but this is coincidental rather than read from config.
Consider implementing the policy enforcement:
# In detect_status_changes: check merged_at + cleanup_delay_hours before setting cleanup_pending
# In run_gc: check created_at + max_age_days for auto-abandoning old worktreesOr remove the unused gc_policy fields from the template to avoid misleading users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/worktree-gc-daemon` around lines 36 - 47, The gc_policy fields are
defined but not enforced: update the daemon to read
gc_policy.cleanup_delay_hours and gc_policy.max_age_days (and use
gc_policy.interval_hours instead of the hardcoded 3600) and enforce them inside
worktree manager calls; specifically, change detect_status_changes (invoked via
WORKTREE_MANAGER detect) to only mark worktrees as cleanup_pending when
merged_at + cleanup_delay_hours has elapsed, and change run_gc (invoked via
WORKTREE_MANAGER gc) to auto-abandon or remove active worktrees whose created_at
is older than max_age_days before cleanup; ensure logging to LOG_FILE still
captures decisions and keep variable names cleanup_pending, merged_at,
created_at, WORKTREE_MANAGER, LOG_FILE, and gc_policy for discoverability.
## Critical Fixes 1. **Fix JSON injection vulnerability** - Replace heredoc with jq --arg for proper escaping - Prevents malformed JSON from branch names with quotes/special chars 2. **Fix multiple worktrees handling in post-merge** - Use jq -s '.[0]' to get first match only - Prevents concatenation of multiple IDs 3. **Add status validation** - Whitelist valid status values - Prevent registry corruption Addresses most critical issues from CodeRabbit round 2
## All Issues Fixed 1. **JSON injection vulnerability** ✅ - Use jq --arg for proper escaping - Prevents malformed JSON from special characters 2. **Multiple worktrees handling** ✅ - Use jq -s '.[0]' to get first match - Prevents ID concatenation 3. **Redundant double status update** ✅ - Remove intermediate 'abandoned' state - Mark directly as cleanup_pending 4. **Argument validation** ✅ - Add validation for update, find, unregister commands - Clear error messages for missing args 5. **Status validation** ✅ - Whitelist valid status values - Prevent registry corruption 6. **Graceful shutdown** ✅ - Handle SIGTERM/SIGINT signals - Use RUNNING flag for clean exit - Short sleep intervals for responsiveness 7. **Log rotation documentation** ✅ - Add logrotate configuration example - Weekly rotation with 4-week retention 8. **Improved LaunchAgent config** ✅ - Add RunAtLoad and KeepAlive keys - Proper plist structure Addresses all 8 actionable + 5 nitpick comments from CodeRabbit round 2
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
lib/hooks/post-merge (1)
23-35:⚠️ Potential issue | 🟠 MajorBranch name alone is not enough to identify the right worktree.
The registry is global, but this hook picks the first match for
$MERGED_BRANCH. If two repositories both havefeature/foo, the hook can mark the wrong worktree as merged. This should also match the current repo’sproject_root, not just the branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/post-merge` around lines 23 - 35, The hook currently selects the first WORKTREE_INFO matching MERGED_BRANCH which can be ambiguous across projects; change the lookup to filter results from "$WORKTREE_MANAGER" find "$MERGED_BRANCH" to the entry that matches the current repository’s project root (or worktree path) before using WORKTREE_ID/WORKTREE_PATH: capture the full JSON array from "$WORKTREE_MANAGER" into WORKTREE_INFO, iterate or jq-filter entries where .path or .project_root equals the current repo root (e.g., $(pwd) or the repo's top-level path) and then pick the matched object to extract .id and .path; ensure you still handle empty results and errors as before.lib/worktree-manager (1)
156-165:⚠️ Potential issue | 🟠 MajorDon’t
rm -rfwhengit worktree removefails.That fallback leaves Git’s internal worktree metadata behind while this script still unregisters the entry, so the registry and
git worktree listcan drift apart. If Git removal fails, this path should log and keep the entry pending instead of deleting the directory directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/worktree-manager` around lines 156 - 165, The current block attempts git worktree remove and falls back to rm -rf on failure, which can leave Git metadata inconsistent; change the failure branch so that if (cd "$main_repo" && git worktree remove "$wt_path" --force 2>/dev/null) fails you do NOT run rm -rf "$wt_path" but instead log an error (e.g., to stderr or a script logger) mentioning the failed git worktree removal for "$wt_path" and leave the directory/registry entry intact (pending) so the registry and git metadata don’t drift; keep the existing rm -rf only in the branch where main_repo is missing if that behavior is still desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/hooks/post-merge`:
- Around line 22-33: The script mistakenly exits early when a matching worktree
is found: the block using WORKTREE_INFO (from "$WORKTREE_MANAGER" find
"$MERGED_BRANCH" | jq -s '.[0]') currently does "if [ -n "$WORKTREE_INFO" ];
then exit 0" which prevents the update path from running; change that to exit
only when no worktree was found (use if [ -z "$WORKTREE_INFO" ]; then exit 0 fi)
or remove the duplicate/inverted early-exit block entirely so the merge-marking
logic runs when WORKTREE_INFO is non-null; look for the WORKTREE_INFO,
WORKTREE_MANAGER and MERGED_BRANCH references in the post-merge hook to apply
the fix.
In `@lib/worktree-manager`:
- Around line 64-74: The update_status function will accept any string and may
write invalid lifecycle states; add validation to reject unknown statuses before
writing: define the allowed set (active, pr_opened, merged, abandoned,
cleanup_pending) and in update_status (and in the update) branch that passes
user input to update_status, check the provided new_status against that set,
emit an error and return non-zero if invalid, and only proceed to run the jq
update when validation passes; ensure error messages reference the function
(update_status) and the caller (update)) so it's easy to locate.
- Around line 45-55: The registry read-modify-write sequences that use jq and
write to "$REGISTRY_FILE" (the block creating tmp_file and mv, and the similar
blocks around lines 69-73, 104-108, 179-181) must be protected by an exclusive
lock to prevent concurrent writers; wrap each mutation (the jq -> tmp_file -> mv
sequence that references REGISTRY_FILE, worktree_id, worktree_path, branch,
project_root, created_at, metadata) inside a flock (or equivalent lockdir) on a
dedicated lock file (e.g., "${REGISTRY_FILE}.lock"), acquire the lock before
reading/updating and release it after mv, handle lock acquisition failures with
a clear error/exit path, and ensure the lock is always released on exit (trap
cleanup) so registry updates are atomic and serialized.
- Around line 200-218: The code treats a missing local branch
(refs/heads/$wt_branch) as immediately "abandoned" and skips PR state lookup;
change the logic so PR state is checked regardless of a local branch first:
attempt gh pr view "$wt_branch" (or equivalent remote-qualified head)
unconditionally when gh is available and set pr_status to MERGED/OPEN/NONE from
that result; only if pr_status is NONE then consider local/remote branch
existence (git show-ref or git ls-remote) and if no branch exists mark
update_status "$wt_id" "abandoned" and "cleanup_pending". Update the block that
currently gates the gh pr view on git show-ref and use the variables wt_project,
wt_branch, pr_status and functions update_status to implement this flow.
- Around line 146-149: The current safety check uses git diff-index --quiet HEAD
-- which ignores untracked files; update the conditional around wt_path to
ensure there are no untracked files too by replacing/augmenting that test with a
porcelain status check such as (cd "$wt_path" && [ -z "$(git status --porcelain
--untracked-files=all 2>/dev/null)" ]) so the block that uses git worktree
remove only runs when both tracked and untracked changes are absent; reference
the wt_path variable and the existing git diff-index usage when making the
change.
---
Duplicate comments:
In `@lib/hooks/post-merge`:
- Around line 23-35: The hook currently selects the first WORKTREE_INFO matching
MERGED_BRANCH which can be ambiguous across projects; change the lookup to
filter results from "$WORKTREE_MANAGER" find "$MERGED_BRANCH" to the entry that
matches the current repository’s project root (or worktree path) before using
WORKTREE_ID/WORKTREE_PATH: capture the full JSON array from "$WORKTREE_MANAGER"
into WORKTREE_INFO, iterate or jq-filter entries where .path or .project_root
equals the current repo root (e.g., $(pwd) or the repo's top-level path) and
then pick the matched object to extract .id and .path; ensure you still handle
empty results and errors as before.
In `@lib/worktree-manager`:
- Around line 156-165: The current block attempts git worktree remove and falls
back to rm -rf on failure, which can leave Git metadata inconsistent; change the
failure branch so that if (cd "$main_repo" && git worktree remove "$wt_path"
--force 2>/dev/null) fails you do NOT run rm -rf "$wt_path" but instead log an
error (e.g., to stderr or a script logger) mentioning the failed git worktree
removal for "$wt_path" and leave the directory/registry entry intact (pending)
so the registry and git metadata don’t drift; keep the existing rm -rf only in
the branch where main_repo is missing if that behavior is still desired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6ab2e4fd-acf8-4469-981b-28f3b750e7a1
📒 Files selected for processing (2)
lib/hooks/post-mergelib/worktree-manager
Comment on lines
+22
to
+33
| # Find matching worktrees | ||
| +# Get first matching worktree (there could be multiple across projects) | ||
| +WORKTREE_INFO=$("$WORKTREE_MANAGER" find "$MERGED_BRANCH" 2>/dev/null | jq -s '.[0]' || true) | ||
| + | ||
| +if [ -n "$WORKTREE_INFO" ]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Find matching worktrees | ||
| +WORKTREE_INFO=$("$WORKTREE_MANAGER" find "$MERGED_BRANCH" 2>/dev/null | jq -s '.[0]' || true) | ||
| + | ||
| +if [ -n "$WORKTREE_INFO" ]; then |
There was a problem hiding this comment.
Remove the inverted early-exit block.
jq -s '.[0]' returns either an object or null, and both are non-empty strings, so Lines 26-28 exit before the update path ever runs. As written, this hook never marks a matching worktree as merged.
Suggested fix
-# Find matching worktrees
-# Get first matching worktree (there could be multiple across projects)
-WORKTREE_INFO=$("$WORKTREE_MANAGER" find "$MERGED_BRANCH" 2>/dev/null | jq -s '.[0]' || true)
-
-if [ -n "$WORKTREE_INFO" ]; then
- exit 0
-fi
-
-# Find matching worktrees
-WORKTREE_INFO=$("$WORKTREE_MANAGER" find "$MERGED_BRANCH" 2>/dev/null | jq -s '.[0]' || true)
-
-if [ -n "$WORKTREE_INFO" ]; then
+# Find matching worktrees
+WORKTREE_INFO=$("$WORKTREE_MANAGER" find "$MERGED_BRANCH" 2>/dev/null | jq -s '.[0]' || true)
+
+if [ -n "$WORKTREE_INFO" ] && [ "$WORKTREE_INFO" != "null" ]; then
WORKTREE_ID=$(echo "$WORKTREE_INFO" | jq -r '.id')
WORKTREE_PATH=$(echo "$WORKTREE_INFO" | jq -r '.path')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/hooks/post-merge` around lines 22 - 33, The script mistakenly exits early
when a matching worktree is found: the block using WORKTREE_INFO (from
"$WORKTREE_MANAGER" find "$MERGED_BRANCH" | jq -s '.[0]') currently does "if [
-n "$WORKTREE_INFO" ]; then exit 0" which prevents the update path from running;
change that to exit only when no worktree was found (use if [ -z
"$WORKTREE_INFO" ]; then exit 0 fi) or remove the duplicate/inverted early-exit
block entirely so the merge-marking logic runs when WORKTREE_INFO is non-null;
look for the WORKTREE_INFO, WORKTREE_MANAGER and MERGED_BRANCH references in the
post-merge hook to apply the fix.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
lib/worktree-manager (3)
44-56:⚠️ Potential issue | 🟠 MajorRegistry mutations lack file locking, risking data corruption under concurrent access.
With the daemon, post-merge hook, and CLI potentially running simultaneously, concurrent read-modify-write operations on the registry file can race and lose updates. Consider using
flockto serialize access.🔒 Suggested fix using flock
+LOCK_FILE="${REGISTRY_FILE}.lock" + +# Acquire exclusive lock for registry mutations +with_registry_lock() { + ( + flock -x 200 + "$@" + ) 200>"$LOCK_FILE" +} + # Register a new worktree register_worktree() { ... - jq --arg id "$worktree_id" \ + with_registry_lock jq --arg id "$worktree_id" \Apply similar locking to
update_status,unregister_worktree, andrun_gcmutations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/worktree-manager` around lines 44 - 56, The registry update block that uses jq to append a worktree writes directly to REGISTRY_FILE without locking, risking races; wrap the read-modify-write sequence (the jq -> tmp_file -> mv sequence that emits worktree_id) inside a flock on the registry file (or a dedicated lock file) to serialize access so concurrent processes cannot interleave. Apply the same flock-based serialization to the other mutators: update_status, unregister_worktree, and run_gc, ensuring each acquires the lock before reading REGISTRY_FILE and releases after mv completes.
156-158:⚠️ Potential issue | 🔴 CriticalUntracked files are not detected, risking deletion of uncommitted work.
git diff-index --quiet HEAD --only checks tracked files. A worktree with new, untracked files will pass this check and still be deleted, violating the documented safety guarantee.🐛 Proposed fix to include untracked files
- if (cd "$wt_path" && git diff-index --quiet HEAD -- 2>/dev/null); then + if (cd "$wt_path" && [ -z "$(git status --porcelain --untracked-files=all 2>/dev/null)" ]); then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/worktree-manager` around lines 156 - 158, The current safety check uses git diff-index --quiet HEAD -- which only detects changes to tracked files and will miss untracked files; update the condition around the cd "$wt_path" && git diff-index --quiet HEAD -- check to also detect untracked files (e.g., run git status --porcelain or git ls-files -o --exclude-standard and treat any output as dirty) and only proceed to git worktree remove when both tracked and untracked checks report clean; ensure you adjust the conditional that precedes git worktree remove so it fails if either check finds changes or untracked files.
209-226:⚠️ Potential issue | 🟠 MajorMissing local branch skips PR check, bypassing the "merged" state and grace period.
When a local branch is deleted (common after merging), the code immediately marks the worktree as
cleanup_pending(line 225) without checking if a PR exists and was merged. This bypasses the intendedmergedstate and its associated grace period.Consider checking PR status first regardless of local branch existence:
📝 Suggested approach
- # Check if branch still exists - if (cd "$wt_project" 2>/dev/null && git show-ref --verify --quiet "refs/heads/$wt_branch"); then - # Branch exists, check for PR status (requires gh CLI) - if command -v gh >/dev/null 2>&1; then - local pr_status - pr_status=$(cd "$wt_project" 2>/dev/null && gh pr view "$wt_branch" --json state -q '.state' 2>/dev/null || echo "none") - - if [ "$pr_status" = "MERGED" ]; then - update_status "$wt_id" "merged" - elif [ "$pr_status" = "OPEN" ]; then - update_status "$wt_id" "pr_opened" - fi - fi - else - # Branch doesn't exist, mark directly as cleanup_pending - update_status "$wt_id" "cleanup_pending" - fi + # Check PR status first (works even if local branch is deleted) + local pr_status="none" + if command -v gh >/dev/null 2>&1; then + pr_status=$(cd "$wt_project" 2>/dev/null && gh pr view "$wt_branch" --json state -q '.state' 2>/dev/null || echo "none") + fi + + if [ "$pr_status" = "MERGED" ]; then + update_status "$wt_id" "merged" + # Daemon will handle transition to cleanup_pending after grace period + elif [ "$pr_status" = "OPEN" ]; then + update_status "$wt_id" "pr_opened" + elif ! (cd "$wt_project" 2>/dev/null && git show-ref --verify --quiet "refs/heads/$wt_branch"); then + # No PR found and branch doesn't exist - truly abandoned + update_status "$wt_id" "abandoned" + update_status "$wt_id" "cleanup_pending" + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/worktree-manager` around lines 209 - 226, The branch-exists check currently jumps to update_status "$wt_id" "cleanup_pending" when the local branch is missing, skipping PR checks; change the logic so you always attempt to query PR state with gh (using gh pr view "$wt_branch" --json state -q '.state') before marking cleanup_pending: if gh reports "MERGED" call update_status "$wt_id" "merged", if "OPEN" call update_status "$wt_id" "pr_opened", otherwise (no PR or gh not available) only then call update_status "$wt_id" "cleanup_pending"; keep the existing command presence check (command -v gh) and use wt_project/wt_branch/pr_status/update_status identifiers to locate and implement the reordering.
🧹 Nitpick comments (1)
lib/worktree-manager (1)
188-190: Usejq --argfor the date value to avoid potential issues.While
dateoutput is predictable, using string interpolation in jq expressions is inconsistent with the safer--argpattern used elsewhere.♻️ Suggested fix
# Update last GC time local tmp_file="${REGISTRY_FILE}.tmp" - jq '.metadata.last_gc = "'$(date -u +%Y-%m-%dT%H:%M:%SZ)'"' "$REGISTRY_FILE" > "$tmp_file" + jq --arg ts "$(date -u +%Y-%m-%dT%H:%M:%SZ)" '.metadata.last_gc = $ts' "$REGISTRY_FILE" > "$tmp_file" mv "$tmp_file" "$REGISTRY_FILE"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/worktree-manager` around lines 188 - 190, The jq invocation that sets .metadata.last_gc currently injects the date via shell interpolation; change it to pass the timestamp with jq --arg to avoid embedding shell output into the program text. Specifically, when creating tmp_file and updating REGISTRY_FILE, compute the timestamp with date -u ... into a shell variable (e.g., ts) and call jq --arg ts "$ts" '.metadata.last_gc = $ts' "$REGISTRY_FILE" > "$tmp_file", then mv "$tmp_file" "$REGISTRY_FILE"; update the code around REGISTRY_FILE, tmp_file and the jq call accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/worktree-lifecycle.md`:
- Around line 46-47: The documentation currently conflicts about the default
cleanup delay: the step text "default: 1 hour for merged" and the configuration
example showing cleanup_delay_hours: 24; decide which is correct and make them
consistent by either updating the step text to reflect the example (e.g.,
"default: 24 hours for merged") or changing the example value to the real
default (e.g., cleanup_delay_hours: 1); update both the descriptive line ("Delay
period has passed...") and the configuration block containing
cleanup_delay_hours so they match, and ensure the symbol cleanup_delay_hours is
used consistently throughout the document.
- Around line 84-92: The fenced code block for the logrotate snippet is missing
a language specifier and uses an unexpanded shell variable; update the markdown
block to include a language tag (e.g., ```text) and replace the literal
$HOME/.config/superpowers/worktree-gc.log with an absolute path (e.g.,
/home/your-username/.config/superpowers/worktree-gc.log), and add a one-line
comment above the block like "# Replace /home/your-username with your actual
home directory" so readers know to substitute their real home path; ensure the
log filename worktree-gc.log and the logrotate directives (weekly, rotate 4,
compress, missingok, notifempty) remain unchanged.
- Around line 62-66: The LaunchAgent plist's ProgramArguments array currently
contains the literal string "$HOME/.config/superpowers/lib/worktree-gc-daemon"
which will not be reliably expanded; update the ProgramArguments entry (the
array containing "/bin/bash" and "-c" and the "$HOME/..." string) to use an
absolute, fully expanded home path (e.g.,
"/Users/yourname/.config/superpowers/lib/worktree-gc-daemon") or otherwise avoid
shell variable expansion by invoking the daemon with its full path so launchd
receives a concrete path.
---
Duplicate comments:
In `@lib/worktree-manager`:
- Around line 44-56: The registry update block that uses jq to append a worktree
writes directly to REGISTRY_FILE without locking, risking races; wrap the
read-modify-write sequence (the jq -> tmp_file -> mv sequence that emits
worktree_id) inside a flock on the registry file (or a dedicated lock file) to
serialize access so concurrent processes cannot interleave. Apply the same
flock-based serialization to the other mutators: update_status,
unregister_worktree, and run_gc, ensuring each acquires the lock before reading
REGISTRY_FILE and releases after mv completes.
- Around line 156-158: The current safety check uses git diff-index --quiet HEAD
-- which only detects changes to tracked files and will miss untracked files;
update the condition around the cd "$wt_path" && git diff-index --quiet HEAD --
check to also detect untracked files (e.g., run git status --porcelain or git
ls-files -o --exclude-standard and treat any output as dirty) and only proceed
to git worktree remove when both tracked and untracked checks report clean;
ensure you adjust the conditional that precedes git worktree remove so it fails
if either check finds changes or untracked files.
- Around line 209-226: The branch-exists check currently jumps to update_status
"$wt_id" "cleanup_pending" when the local branch is missing, skipping PR checks;
change the logic so you always attempt to query PR state with gh (using gh pr
view "$wt_branch" --json state -q '.state') before marking cleanup_pending: if
gh reports "MERGED" call update_status "$wt_id" "merged", if "OPEN" call
update_status "$wt_id" "pr_opened", otherwise (no PR or gh not available) only
then call update_status "$wt_id" "cleanup_pending"; keep the existing command
presence check (command -v gh) and use
wt_project/wt_branch/pr_status/update_status identifiers to locate and implement
the reordering.
---
Nitpick comments:
In `@lib/worktree-manager`:
- Around line 188-190: The jq invocation that sets .metadata.last_gc currently
injects the date via shell interpolation; change it to pass the timestamp with
jq --arg to avoid embedding shell output into the program text. Specifically,
when creating tmp_file and updating REGISTRY_FILE, compute the timestamp with
date -u ... into a shell variable (e.g., ts) and call jq --arg ts "$ts"
'.metadata.last_gc = $ts' "$REGISTRY_FILE" > "$tmp_file", then mv "$tmp_file"
"$REGISTRY_FILE"; update the code around REGISTRY_FILE, tmp_file and the jq call
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a3acde18-a575-465a-848e-1c2877db45d7
📒 Files selected for processing (3)
docs/worktree-lifecycle.mdlib/worktree-gc-daemonlib/worktree-manager
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/worktree-gc-daemon
Comment on lines
+46
to
+47
| 3. Delay period has passed (default: 1 hour for merged, immediate for abandoned) | ||
|
|
There was a problem hiding this comment.
Documentation inconsistency: default cleanup delay.
Line 46 states "default: 1 hour for merged", but the configuration example on line 130 shows cleanup_delay_hours: 24. Please reconcile which is the actual default.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/worktree-lifecycle.md` around lines 46 - 47, The documentation currently
conflicts about the default cleanup delay: the step text "default: 1 hour for
merged" and the configuration example showing cleanup_delay_hours: 24; decide
which is correct and make them consistent by either updating the step text to
reflect the example (e.g., "default: 24 hours for merged") or changing the
example value to the real default (e.g., cleanup_delay_hours: 1); update both
the descriptive line ("Delay period has passed...") and the configuration block
containing cleanup_delay_hours so they match, and ensure the symbol
cleanup_delay_hours is used consistently throughout the document.
Comment on lines
+62
to
+66
| <array> | ||
| <string>/bin/bash</string> | ||
| <string>-c</string> | ||
| <string>$HOME/.config/superpowers/lib/worktree-gc-daemon</string> | ||
| </array> |
There was a problem hiding this comment.
$HOME won't expand in LaunchAgent plist files.
Property list files are XML and do not perform shell variable expansion. The literal string $HOME/.config/superpowers/lib/worktree-gc-daemon will be passed to bash, but some environments may not expand it correctly depending on how launchd invokes the shell.
Consider using the full expanded path or a more reliable approach:
📝 Suggested fix using full path
<key>ProgramArguments</key>
<array>
<string>/bin/bash</string>
<string>-c</string>
- <string>$HOME/.config/superpowers/lib/worktree-gc-daemon</string>
+ <string>"$HOME"/.config/superpowers/lib/worktree-gc-daemon</string>
</array>Alternatively, instruct users to replace $HOME with their actual home directory path, e.g., /Users/username/.config/superpowers/lib/worktree-gc-daemon.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <array> | |
| <string>/bin/bash</string> | |
| <string>-c</string> | |
| <string>$HOME/.config/superpowers/lib/worktree-gc-daemon</string> | |
| </array> | |
| <array> | |
| <string>/bin/bash</string> | |
| <string>-c</string> | |
| <string>"$HOME"/.config/superpowers/lib/worktree-gc-daemon</string> | |
| </array> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/worktree-lifecycle.md` around lines 62 - 66, The LaunchAgent plist's
ProgramArguments array currently contains the literal string
"$HOME/.config/superpowers/lib/worktree-gc-daemon" which will not be reliably
expanded; update the ProgramArguments entry (the array containing "/bin/bash"
and "-c" and the "$HOME/..." string) to use an absolute, fully expanded home
path (e.g., "/Users/yourname/.config/superpowers/lib/worktree-gc-daemon") or
otherwise avoid shell variable expansion by invoking the daemon with its full
path so launchd receives a concrete path.
Comment on lines
+84
to
+92
| ``` | ||
| $HOME/.config/superpowers/worktree-gc.log { | ||
| weekly | ||
| rotate 4 | ||
| compress | ||
| missingok | ||
| notifempty | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Logrotate config has issues: missing language specifier and unexpanded $HOME.
- The fenced code block is missing a language specifier (flagged by markdownlint).
- Logrotate does not expand shell variables like
$HOME. Users must substitute their actual home directory path.
📝 Suggested fix
-```
-$HOME/.config/superpowers/worktree-gc.log {
+```text
+# Replace /home/username with your actual home directory
+/home/username/.config/superpowers/worktree-gc.log {
weekly
rotate 4
compress
missingok
notifempty
}
```🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 84-84: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/worktree-lifecycle.md` around lines 84 - 92, The fenced code block for
the logrotate snippet is missing a language specifier and uses an unexpanded
shell variable; update the markdown block to include a language tag (e.g.,
```text) and replace the literal $HOME/.config/superpowers/worktree-gc.log with
an absolute path (e.g.,
/home/your-username/.config/superpowers/worktree-gc.log), and add a one-line
comment above the block like "# Replace /home/your-username with your actual
home directory" so readers know to substitute their real home path; ensure the
log filename worktree-gc.log and the logrotate directives (weekly, rotate 4,
compress, missingok, notifempty) remain unchanged.
## Critical Fixes 1. **Fix post-merge hook early exit bug** - Remove duplicate/inverted early-exit block - Hook was exiting when worktree WAS found, preventing updates - Now correctly exits only when NO worktree found 2. **Include untracked files in safety check** - Replace git diff-index with git status --porcelain - Prevents deletion of worktrees with untracked files - Critical safety improvement 3. **Filter by project_root in post-merge** - Use jq to filter worktrees by current project - Prevents cross-project worktree confusion - More precise matching 4. **Don't rm -rf when git worktree remove fails** - Log error and keep registry entry pending - Prevents git metadata inconsistency - Forces manual cleanup instead of silent corruption Addresses critical issues from CodeRabbit round 3
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/worktree-manager (1)
7-7:SCRIPT_DIRis defined but never used.This variable is assigned but not referenced anywhere in the script. Consider removing it to reduce dead code, or use it if it was intended for relative path resolution (e.g., sourcing the registry template).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/worktree-manager` at line 7, The variable SCRIPT_DIR is assigned but never used; either remove the unused assignment "SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]:-$0}")" && pwd)"" to eliminate dead code, or update the script to use SCRIPT_DIR where relative paths are needed (for example when sourcing files or resolving the registry template); locate the assignment of SCRIPT_DIR in the file and either delete that line or replace direct path usages with "${SCRIPT_DIR}/<file>" (e.g., in any sourcing or file-resolution code) so the variable is actually used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/worktree-manager`:
- Line 7: The variable SCRIPT_DIR is assigned but never used; either remove the
unused assignment "SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]:-$0}")" &&
pwd)"" to eliminate dead code, or update the script to use SCRIPT_DIR where
relative paths are needed (for example when sourcing files or resolving the
registry template); locate the assignment of SCRIPT_DIR in the file and either
delete that line or replace direct path usages with "${SCRIPT_DIR}/<file>"
(e.g., in any sourcing or file-resolution code) so the variable is actually
used.
## Critical Fixes (CodeRabbit identified these as duplicates - not fully fixed) 1. **Untracked files detection** ✅ NOW FIXED - Replace git diff-index with git status --porcelain - Prevents deletion of worktrees with untracked files - CodeRabbit kept flagging this - now properly fixed 2. **PR check before marking abandoned** ✅ NOW FIXED - Always check gh pr view first (even if branch deleted) - Prevents bypassing merged state grace period - Correct logic flow: PR check → branch check → abandon 3. **Registry locking** ✅ ADDED - Add with_registry_lock() function - Prevents concurrent mutation races - flock-based serialization CodeRabbit kept reporting these as duplicates because they weren't actually fixed in worktree-manager - only in hooks. Now fixed in the right place.
## Changes Wrap all registry read-modify-write operations with flock for concurrent access protection: 1. **register_worktree** - flock around jq append 2. **update_status** - flock around status update 3. **unregister_worktree** - flock around removal 4. **run_gc** - flock around last_gc update Also fixed jq usage to use --arg for timestamp instead of shell interpolation. This prevents race conditions when daemon, post-merge hook, and CLI run simultaneously. Addresses CodeRabbit feedback on registry locking.
CodeRabbit nitpick: SCRIPT_DIR was assigned but never used.
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
1 participant
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.
Problem (#666)
Git worktrees created by Superpowers persist indefinitely, accumulating in
.worktrees/or~/.config/superpowers/worktrees/. After branches are merged or abandoned, leftover worktrees waste disk space.Solution
Implement comprehensive worktree lifecycle management with automatic garbage collection.
Core Components
1. Worktree Registry
~/.config/superpowers/worktree-registry.jsontracks all worktrees:{ "id": "wt_1234567890_feature_auth", "path": "/tmp/worktrees/feature-auth", "branch": "feature/auth-system", "status": "active", "created_at": "2026-03-10T10:00:00Z" }2. Worktree Manager CLI
lib/worktree-managerprovides full lifecycle control:3. GC Daemon
lib/worktree-gc-daemonruns in background:ghCLI)4. Git Post-Merge Hook
lib/hooks/post-mergeauto-installs:Status Lifecycle
Safety Guarantees
✅ Never delete worktrees with uncommitted changes
✅ Never delete active worktrees
✅ Grace period (1h for merged, immediate for abandoned)
✅ Full audit log at
~/.config/superpowers/worktree-gc.logInstallation
Integration
using-git-worktreesskill now registers worktrees automaticallyfinishing-a-development-branchskill will trigger cleanup (future)Architecture
Files Added
lib/worktree-registry.schema.json- Registry JSON schemalib/worktree-manager- CLI tool for registry managementlib/worktree-gc-daemon- Background GC processlib/hooks/post-merge- Git hook for merge detectiondocs/worktree-lifecycle.md- User documentationFiles Modified
skills/using-git-worktrees/SKILL.md- Register worktrees on creationFuture Enhancements
finishing-a-development-branchskillFixes #666