feat: worktree ownership safety — prevent cross-session removal (t189) #695
Conversation
…l (t189) Three layers of protection against the root cause (supervisor force-removing a worktree belonging to a parallel interactive session): 1. Ownership registry (SQLite-backed, shared-constants.sh): - register_worktree/unregister_worktree track PID ownership - is_worktree_owned_by_others checks if owner PID is alive - prune_worktree_registry cleans stale entries (dead PIDs, missing dirs) 2. In-use detection in worktree-helper.sh: - cmd_add registers ownership on creation - cmd_remove refuses removal if owned by another live process - cmd_clean skips worktrees owned by other active sessions - cmd_registry provides list/prune subcommands - --force flag available for explicit override 3. Supervisor integration (supervisor-helper.sh): - create_task_worktree registers ownership with --task metadata - cleanup_task_worktree checks ownership before removal - cmd_cleanup prunes stale registry entries - Stale worktree cleanup refuses to touch owned worktrees Also updated AGENTS.md ownership rule and worktree.md documentation.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
WalkthroughA SQLite-backed worktree ownership registry system is introduced across shell scripts to track which process/session owns each worktree. The registry prevents unintended cleanup of worktrees managed by other live processes and includes commands to view and prune stale entries. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Script
participant WH as worktree-helper.sh
participant SC as shared-constants.sh
participant DB as SQLite DB
participant FS as Filesystem
Client->>WH: add worktree
WH->>FS: create worktree (git worktree add)
FS-->>WH: success
WH->>SC: register_worktree(path, branch)
SC->>DB: INSERT into worktree_owners
DB-->>SC: ownership recorded
SC-->>WH: registered
WH-->>Client: worktree ready
Client->>WH: remove worktree
WH->>SC: check_worktree_owner(path)
SC->>DB: SELECT owner from worktree_owners
DB-->>SC: {pid, session, ...}
SC->>SC: is_worktree_owned_by_others()?
alt Owned by Other Session
SC-->>WH: return owner info
WH-->>Client: error: owned by other session
else Owned by Current Session
WH->>FS: remove worktree
FS-->>WH: success
WH->>SC: unregister_worktree(path)
SC->>DB: DELETE from worktree_owners
DB-->>SC: unregistered
SC-->>WH: done
WH-->>Client: worktree removed
end
sequenceDiagram
participant Task as Task Dispatcher
participant SH as supervisor-helper.sh
participant SC as shared-constants.sh
participant DB as SQLite DB
participant WH as worktree-helper.sh
Task->>SH: create_task_worktree
SH->>WH: create worktree via wt/git
WH-->>SH: success
SH->>SC: register_worktree(path, branch, task_id, batch_id, session_id)
SC->>DB: INSERT with task metadata
DB-->>SC: ownership recorded
SC-->>SH: registered
SH-->>Task: worktree ready
Task->>SH: cleanup_task_worktree
SH->>SC: check_worktree_owner(path)
SC->>DB: SELECT owner, task_id from worktree_owners
DB-->>SC: owner metadata
alt Owned by Other Session
SC-->>SH: return owner info
SH-->>Task: defer cleanup
else Owned by Current Session
SH->>WH: remove/clean worktree
WH-->>SH: success
SH->>SC: unregister_worktree(path)
SC->>DB: DELETE from worktree_owners
DB-->>SC: unregistered
SH->>SC: prune_worktree_registry()
SC->>DB: DELETE stale entries
DB-->>SC: pruned
SC-->>SH: done
SH-->>Task: cleanup complete
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Mon Feb 9 00:38:25 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @.agents/scripts/shared-constants.sh:
- Around line 625-646: The current lookup of owner_pid uses "|| echo """ which
masks sqlite3 failures and causes the function to incorrectly return
safe-to-remove; change the logic to detect sqlite3 failure explicitly: capture
the sqlite3 output into owner_pid, check sqlite3's exit status (or test for
command failure) and if sqlite3 failed, print a warning to stderr (including
$WORKTREE_REGISTRY_DB and any sqlite error if available) and return 0 (NOT safe
to remove); keep the existing semantics for an empty owner_pid (no owner =>
return 1) and for a dead owner (use kill -0 and call unregister_worktree
"$wt_path"), so references to owner_pid, WORKTREE_REGISTRY_DB,
unregister_worktree, and kill -0 should be used to locate and update the code.
In @.agents/scripts/supervisor-helper.sh:
- Around line 2801-2809: The condition that detects a worktree owned by another
session (is_worktree_owned_by_others) must not return success; change the block
in the cleanup logic so you still capture owner info using the local
var="$(...)" pattern (e.g. local stale_owner_info;
stale_owner_info=$(check_worktree_owner "$worktree_path" || echo "unknown")),
keep the log_warn call, still echo the path for the caller to inspect if needed,
but return a non‑zero status (e.g. return 1 or another distinct code) instead of
return 0 so cmd_dispatch and callers can detect and skip/defer dispatch rather
than reusing/stomping the worktree.
- Around line 2862-2873: The ownership check currently returns success (0) when
skipping cleanup, which causes callers like cmd_cleanup and cleanup_after_merge
to wrongly treat the worktree as cleaned; change the behavior so the block
returns a distinct non-zero status (e.g., return 2) to signal an ownership
conflict and prevent callers from clearing state, and also adopt the local
assignment pattern for owner_info (use local owner_info="$(check_worktree_owner
"$worktree_path" || echo "unknown")") to follow shell guidelines; keep the
log_warn call but ensure callers handle the non-zero exit to preserve
DB/worktree state.
In @.agents/scripts/worktree-helper.sh:
- Around line 213-214: The call to register_worktree in the worktree creation
flow doesn't check its return value, so failures (e.g., sqlite3 unavailable)
leave the worktree unregistered and vulnerable to deletion; update the caller
around register_worktree "$path" "$branch" to capture its exit status and, on
non-zero return, emit a clear warning via the existing logging facility (and
optionally fail the operation) so users know registration didn’t complete; also
ensure register_worktree (and related helpers referenced in shared-constants.sh)
return non-zero on error so the caller can detect and act on failures.
🧹 Nitpick comments (6)
.agents/scripts/shared-constants.sh (3)
519-522: Missing explicitreturnstatement.Per coding guidelines, shell functions should include explicit returns.
Proposed fix
_wt_sql_escape() { local val="$1" echo "${val//\'/\'\'}" + return 0 }As per coding guidelines, "Include explicit returns in shell scripts".
515-516: Registry constants are notreadonly, unlike every other constant in this file.All other constants in
shared-constants.share declaredreadonly. These two are mutable, which means any sourcing script could accidentally overwrite them, potentially redirecting registry operations to an unexpected path.Proposed fix
-WORKTREE_REGISTRY_DIR="${HOME}/.aidevops/.agent-workspace" -WORKTREE_REGISTRY_DB="${WORKTREE_REGISTRY_DIR}/worktree-registry.db" +readonly WORKTREE_REGISTRY_DIR="${HOME}/.aidevops/.agent-workspace" +readonly WORKTREE_REGISTRY_DB="${WORKTREE_REGISTRY_DIR}/worktree-registry.db"
525-539: DB initialization silences errors — consider logging.If
sqlite3is not installed or the directory isn't writable, this will fail silently, and all downstream operations will too. This aligns with thelog_stderrpattern already established earlier in this file (line 317). Using it here would provide visibility without disrupting the flow.Proposed fix
_init_registry_db() { mkdir -p "$WORKTREE_REGISTRY_DIR" 2>/dev/null || true - sqlite3 "$WORKTREE_REGISTRY_DB" " + log_stderr "registry_init" sqlite3 "$WORKTREE_REGISTRY_DB" " CREATE TABLE IF NOT EXISTS worktree_owners ( worktree_path TEXT PRIMARY KEY, branch TEXT, owner_pid INTEGER, owner_session TEXT DEFAULT '', owner_batch TEXT DEFAULT '', task_id TEXT DEFAULT '', created_at TEXT DEFAULT (strftime('%Y-%m-%dT%H:%M:%SZ', 'now')) ); - " 2>/dev/null || true + " || true return 0 }Note:
log_stderris already defined earlier in this file (line 317) and is recommended forsqlite3calls per the guidelines comment on line 299..agents/scripts/worktree-helper.sh (2)
286-306: Unnecessaryexportand dual-variable pattern for force flag.
force_remove(local, line 287) andWORKTREE_FORCE_REMOVE(exported, line 305) track the same intent. Since the check on line 352 is in the same function, the local variable suffices — no child processes readWORKTREE_FORCE_REMOVE. The export also leaks the flag into the environment for any subsequent commands.Proposed simplification
# Parse arguments while [[ $# -gt 0 ]]; do case "$1" in --force|-f) force_remove=true; shift ;; *) target="$1"; shift ;; esac done if [[ -z "$target" ]]; then echo -e "${RED}Error: Path or branch name required${NC}" echo "Usage: worktree-helper.sh remove <path|branch> [--force]" return 1 fi - - # Export for ownership check - if [[ "$force_remove" == "true" ]]; then - export WORKTREE_FORCE_REMOVE="true" - fiThen at line 352:
- if [[ "${WORKTREE_FORCE_REMOVE:-}" != "true" ]]; then + if [[ "$force_remove" != "true" ]]; then
581-623:cmd_registryis clean and well-structured.The list/prune subcommands, alive/dead PID status display, and early exits are all well-handled. The
return 0at the end covers all branches.One small note: the
*case (line 618-620) prints usage but returns 0. Consider returning 1 for unrecognized subcommands to match the pattern inmain()(line 732-734).Proposed fix
*) echo "Usage: worktree-helper.sh registry [list|prune]" + return 1 ;; esac return 0.agents/workflows/worktree.md (1)
385-410: All command syntax verified, but consider consolidating ownership guidance per single source of truth principle.All documented commands and registry path are technically correct:
worktree-helper.sh registry listandregistry prunesubcommands exist and match documentationworktree-helper.sh remove <branch> --forceflag is implemented- Registry path
~/.aidevops/.agent-workspace/worktree-registry.dbmatches WORKTREE_REGISTRY_DB constantHowever, per the learnings and coding guidelines, AGENTS.md is the authoritative single source of truth for all instructions and operational guidance. AGENTS.md already contains a comprehensive worktree ownership section (marked CRITICAL). Consider consolidating this worktree.md section to avoid split guidance: move operational procedures into AGENTS.md with specific command examples, or establish clear cross-references between AGENTS.md (principles) and worktree.md (procedures).
| sqlite3 "$WORKTREE_REGISTRY_DB" " | ||
| INSERT OR REPLACE INTO worktree_owners | ||
| (worktree_path, branch, owner_pid, owner_session, owner_batch, task_id) | ||
| VALUES | ||
| ('$(_wt_sql_escape "$wt_path")', | ||
| '$(_wt_sql_escape "$branch")', | ||
| $$, | ||
| '$(_wt_sql_escape "$session_id")', | ||
| '$(_wt_sql_escape "$batch_id")', | ||
| '$(_wt_sql_escape "$task_id")'); | ||
| " 2>/dev/null || true | ||
| return 0 |
There was a problem hiding this comment.
Silent failure on registration leaves worktrees unprotected.
If the INSERT OR REPLACE fails (disk full, permissions, missing sqlite3), the function returns 0 (success) and the caller proceeds as if the worktree is registered. A subsequent is_worktree_owned_by_others call will find no entry and allow removal — defeating the safety goal.
Consider propagating the sqlite3 exit code so the caller can at least log a warning that ownership registration failed.
Proposed fix
sqlite3 "$WORKTREE_REGISTRY_DB" "
INSERT OR REPLACE INTO worktree_owners
(worktree_path, branch, owner_pid, owner_session, owner_batch, task_id)
VALUES
('$(_wt_sql_escape "$wt_path")',
'$(_wt_sql_escape "$branch")',
$$,
'$(_wt_sql_escape "$session_id")',
'$(_wt_sql_escape "$batch_id")',
'$(_wt_sql_escape "$task_id")');
- " 2>/dev/null || true
+ " 2>/dev/null || {
+ print_shared_warning "Failed to register worktree ownership for: $wt_path"
+ return 1
+ }
return 0
}📝 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.
| sqlite3 "$WORKTREE_REGISTRY_DB" " | |
| INSERT OR REPLACE INTO worktree_owners | |
| (worktree_path, branch, owner_pid, owner_session, owner_batch, task_id) | |
| VALUES | |
| ('$(_wt_sql_escape "$wt_path")', | |
| '$(_wt_sql_escape "$branch")', | |
| $$, | |
| '$(_wt_sql_escape "$session_id")', | |
| '$(_wt_sql_escape "$batch_id")', | |
| '$(_wt_sql_escape "$task_id")'); | |
| " 2>/dev/null || true | |
| return 0 | |
| sqlite3 "$WORKTREE_REGISTRY_DB" " | |
| INSERT OR REPLACE INTO worktree_owners | |
| (worktree_path, branch, owner_pid, owner_session, owner_batch, task_id) | |
| VALUES | |
| ('$(_wt_sql_escape "$wt_path")', | |
| '$(_wt_sql_escape "$branch")', | |
| $$, | |
| '$(_wt_sql_escape "$session_id")', | |
| '$(_wt_sql_escape "$batch_id")', | |
| '$(_wt_sql_escape "$task_id")'); | |
| " 2>/dev/null || { | |
| print_shared_warning "Failed to register worktree ownership for: $wt_path" | |
| return 1 | |
| } | |
| return 0 |
| local owner_pid | ||
| owner_pid=$(sqlite3 "$WORKTREE_REGISTRY_DB" " | ||
| SELECT owner_pid FROM worktree_owners | ||
| WHERE worktree_path = '$(_wt_sql_escape "$wt_path")'; | ||
| " 2>/dev/null || echo "") | ||
|
|
||
| # No owner registered | ||
| [[ -z "$owner_pid" ]] && return 1 | ||
|
|
||
| # We own it | ||
| [[ "$owner_pid" == "$$" ]] && return 1 | ||
|
|
||
| # Owner process is dead — stale entry, safe to remove | ||
| if ! kill -0 "$owner_pid" 2>/dev/null; then | ||
| # Clean up stale entry | ||
| unregister_worktree "$wt_path" | ||
| return 1 | ||
| fi | ||
|
|
||
| # Owner process is alive and it's not us — NOT safe to remove | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
Fail-open on SQLite errors silently bypasses ownership safety.
If sqlite3 is unavailable, the DB is corrupt, or the query fails for any reason, owner_pid becomes "" (via || echo ""), and the function returns 1 ("safe to remove"). This silently degrades the entire safety feature to a no-op — exactly the scenario this PR exists to prevent.
For a protective mechanism, fail-closed is the safer default: if you can't confirm ownership status, refuse removal and let --force handle it.
Proposed fail-closed approach
is_worktree_owned_by_others() {
local wt_path="$1"
[[ ! -f "$WORKTREE_REGISTRY_DB" ]] && return 1
local owner_pid
owner_pid=$(sqlite3 "$WORKTREE_REGISTRY_DB" "
SELECT owner_pid FROM worktree_owners
WHERE worktree_path = '$(_wt_sql_escape "$wt_path")';
- " 2>/dev/null || echo "")
+ " 2>/dev/null)
+
+ # If sqlite3 failed entirely ($? != 0 and empty result), check if sqlite3 is functional
+ if [[ -z "$owner_pid" ]] && ! command -v sqlite3 &>/dev/null; then
+ print_shared_warning "sqlite3 not available — ownership check cannot run"
+ return 1 # No sqlite3 means registry was never populated; safe fallback
+ fi
# No owner registered
[[ -z "$owner_pid" ]] && return 1At minimum, consider logging a warning when sqlite3 fails so operators can diagnose why the registry isn't protecting worktrees.
📝 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.
| local owner_pid | |
| owner_pid=$(sqlite3 "$WORKTREE_REGISTRY_DB" " | |
| SELECT owner_pid FROM worktree_owners | |
| WHERE worktree_path = '$(_wt_sql_escape "$wt_path")'; | |
| " 2>/dev/null || echo "") | |
| # No owner registered | |
| [[ -z "$owner_pid" ]] && return 1 | |
| # We own it | |
| [[ "$owner_pid" == "$$" ]] && return 1 | |
| # Owner process is dead — stale entry, safe to remove | |
| if ! kill -0 "$owner_pid" 2>/dev/null; then | |
| # Clean up stale entry | |
| unregister_worktree "$wt_path" | |
| return 1 | |
| fi | |
| # Owner process is alive and it's not us — NOT safe to remove | |
| return 0 | |
| } | |
| local owner_pid | |
| owner_pid=$(sqlite3 "$WORKTREE_REGISTRY_DB" " | |
| SELECT owner_pid FROM worktree_owners | |
| WHERE worktree_path = '$(_wt_sql_escape "$wt_path")'; | |
| " 2>/dev/null) | |
| # If sqlite3 failed entirely ($? != 0 and empty result), check if sqlite3 is functional | |
| if [[ -z "$owner_pid" ]] && ! command -v sqlite3 &>/dev/null; then | |
| print_shared_warning "sqlite3 not available — ownership check cannot run" | |
| return 1 # No sqlite3 means registry was never populated; safe fallback | |
| fi | |
| # No owner registered | |
| [[ -z "$owner_pid" ]] && return 1 | |
| # We own it | |
| [[ "$owner_pid" == "$$" ]] && return 1 | |
| # Owner process is dead — stale entry, safe to remove | |
| if ! kill -0 "$owner_pid" 2>/dev/null; then | |
| # Clean up stale entry | |
| unregister_worktree "$wt_path" | |
| return 1 | |
| fi | |
| # Owner process is alive and it's not us — NOT safe to remove | |
| return 0 | |
| } |
🤖 Prompt for AI Agents
In @.agents/scripts/shared-constants.sh around lines 625 - 646, The current
lookup of owner_pid uses "|| echo """ which masks sqlite3 failures and causes
the function to incorrectly return safe-to-remove; change the logic to detect
sqlite3 failure explicitly: capture the sqlite3 output into owner_pid, check
sqlite3's exit status (or test for command failure) and if sqlite3 failed, print
a warning to stderr (including $WORKTREE_REGISTRY_DB and any sqlite error if
available) and return 0 (NOT safe to remove); keep the existing semantics for an
empty owner_pid (no owner => return 1) and for a dead owner (use kill -0 and
call unregister_worktree "$wt_path"), so references to owner_pid,
WORKTREE_REGISTRY_DB, unregister_worktree, and kill -0 should be used to locate
and update the code.
| # Ownership check (t189): refuse to clean worktrees owned by other sessions | ||
| if [[ -d "$worktree_path" ]] && is_worktree_owned_by_others "$worktree_path"; then | ||
| local stale_owner_info | ||
| stale_owner_info=$(check_worktree_owner "$worktree_path" || echo "unknown") | ||
| log_warn "Cannot clean stale worktree $worktree_path — owned by another active session (owner: $stale_owner_info)" >&2 | ||
| # Return existing path — let the caller decide | ||
| echo "$worktree_path" | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
Avoid reusing a worktree owned by another live session
Line 2806 returns the existing path with success, so cmd_dispatch will keep going and may stomp another session’s worktree. Return a distinct non‑zero code and let callers defer/skip dispatch instead of reusing the path. Also align the local assignment style for guideline compliance.
🔧 Suggested fix
- if [[ -d "$worktree_path" ]] && is_worktree_owned_by_others "$worktree_path"; then
- local stale_owner_info
- stale_owner_info=$(check_worktree_owner "$worktree_path" || echo "unknown")
+ if [[ -d "$worktree_path" ]] && is_worktree_owned_by_others "$worktree_path"; then
+ local stale_owner_info="$(check_worktree_owner "$worktree_path" || echo "unknown")"
log_warn "Cannot clean stale worktree $worktree_path — owned by another active session (owner: $stale_owner_info)" >&2
- # Return existing path — let the caller decide
- echo "$worktree_path"
- return 0
+ # Signal ownership conflict; caller should defer/skip dispatch
+ return 2
fiAs per coding guidelines, "Use local var="$1" pattern in shell scripts".
📝 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.
| # Ownership check (t189): refuse to clean worktrees owned by other sessions | |
| if [[ -d "$worktree_path" ]] && is_worktree_owned_by_others "$worktree_path"; then | |
| local stale_owner_info | |
| stale_owner_info=$(check_worktree_owner "$worktree_path" || echo "unknown") | |
| log_warn "Cannot clean stale worktree $worktree_path — owned by another active session (owner: $stale_owner_info)" >&2 | |
| # Return existing path — let the caller decide | |
| echo "$worktree_path" | |
| return 0 | |
| fi | |
| # Ownership check (t189): refuse to clean worktrees owned by other sessions | |
| if [[ -d "$worktree_path" ]] && is_worktree_owned_by_others "$worktree_path"; then | |
| local stale_owner_info="$(check_worktree_owner "$worktree_path" || echo "unknown")" | |
| log_warn "Cannot clean stale worktree $worktree_path — owned by another active session (owner: $stale_owner_info)" >&2 | |
| # Signal ownership conflict; caller should defer/skip dispatch | |
| return 2 | |
| fi |
🤖 Prompt for AI Agents
In @.agents/scripts/supervisor-helper.sh around lines 2801 - 2809, The condition
that detects a worktree owned by another session (is_worktree_owned_by_others)
must not return success; change the block in the cleanup logic so you still
capture owner info using the local var="$(...)" pattern (e.g. local
stale_owner_info; stale_owner_info=$(check_worktree_owner "$worktree_path" ||
echo "unknown")), keep the log_warn call, still echo the path for the caller to
inspect if needed, but return a non‑zero status (e.g. return 1 or another
distinct code) instead of return 0 so cmd_dispatch and callers can detect and
skip/defer dispatch rather than reusing/stomping the worktree.
| if [[ ! -d "$worktree_path" ]]; then | ||
| # Directory gone — clean up registry entry if any | ||
| unregister_worktree "$worktree_path" | ||
| return 0 | ||
| fi | ||
|
|
||
| # Ownership check (t189): refuse to remove worktrees owned by other sessions | ||
| if is_worktree_owned_by_others "$worktree_path"; then | ||
| local owner_info | ||
| owner_info=$(check_worktree_owner "$worktree_path" || echo "unknown") | ||
| log_warn "Skipping cleanup of $worktree_path — owned by another active session (owner: $owner_info)" | ||
| return 0 |
There was a problem hiding this comment.
Propagate ownership‑conflict as a non‑success code
Line 2873 returns 0 even though cleanup is skipped, so callers (e.g., cmd_cleanup, cleanup_after_merge) will likely clear the DB/worktree state anyway. That loses tracking of a still‑owned worktree. Return a distinct code and let callers preserve state when ownership blocks cleanup; also use the local assignment pattern.
🔧 Suggested fix
- if is_worktree_owned_by_others "$worktree_path"; then
- local owner_info
- owner_info=$(check_worktree_owner "$worktree_path" || echo "unknown")
+ if is_worktree_owned_by_others "$worktree_path"; then
+ local owner_info="$(check_worktree_owner "$worktree_path" || echo "unknown")"
log_warn "Skipping cleanup of $worktree_path — owned by another active session (owner: $owner_info)"
- return 0
+ return 2
fiAs per coding guidelines, "Use local var="$1" pattern in shell scripts".
📝 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.
| if [[ ! -d "$worktree_path" ]]; then | |
| # Directory gone — clean up registry entry if any | |
| unregister_worktree "$worktree_path" | |
| return 0 | |
| fi | |
| # Ownership check (t189): refuse to remove worktrees owned by other sessions | |
| if is_worktree_owned_by_others "$worktree_path"; then | |
| local owner_info | |
| owner_info=$(check_worktree_owner "$worktree_path" || echo "unknown") | |
| log_warn "Skipping cleanup of $worktree_path — owned by another active session (owner: $owner_info)" | |
| return 0 | |
| if [[ ! -d "$worktree_path" ]]; then | |
| # Directory gone — clean up registry entry if any | |
| unregister_worktree "$worktree_path" | |
| return 0 | |
| fi | |
| # Ownership check (t189): refuse to remove worktrees owned by other sessions | |
| if is_worktree_owned_by_others "$worktree_path"; then | |
| local owner_info="$(check_worktree_owner "$worktree_path" || echo "unknown")" | |
| log_warn "Skipping cleanup of $worktree_path — owned by another active session (owner: $owner_info)" | |
| return 2 |
🤖 Prompt for AI Agents
In @.agents/scripts/supervisor-helper.sh around lines 2862 - 2873, The ownership
check currently returns success (0) when skipping cleanup, which causes callers
like cmd_cleanup and cleanup_after_merge to wrongly treat the worktree as
cleaned; change the behavior so the block returns a distinct non-zero status
(e.g., return 2) to signal an ownership conflict and prevent callers from
clearing state, and also adopt the local assignment pattern for owner_info (use
local owner_info="$(check_worktree_owner "$worktree_path" || echo "unknown")")
to follow shell guidelines; keep the log_warn call but ensure callers handle the
non-zero exit to preserve DB/worktree state.
| # Register ownership (t189) | ||
| register_worktree "$path" "$branch" |
There was a problem hiding this comment.
Registration return value not checked — unprotected worktree on failure.
If register_worktree fails (e.g., sqlite3 unavailable), the worktree is created but never registered in the ownership DB. A subsequent clean or remove from another session won't see an owner and will proceed with deletion — the exact problem this PR aims to solve.
Consider at least logging a warning so the user knows the safety net isn't active.
Proposed fix
# Register ownership (t189)
- register_worktree "$path" "$branch"
+ if ! register_worktree "$path" "$branch"; then
+ print_warning "Failed to register worktree ownership — safety checks may not protect this worktree"
+ fiThis depends on register_worktree propagating errors (see the related comment in shared-constants.sh).
📝 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.
| # Register ownership (t189) | |
| register_worktree "$path" "$branch" | |
| # Register ownership (t189) | |
| if ! register_worktree "$path" "$branch"; then | |
| print_warning "Failed to register worktree ownership — safety checks may not protect this worktree" | |
| fi |
🤖 Prompt for AI Agents
In @.agents/scripts/worktree-helper.sh around lines 213 - 214, The call to
register_worktree in the worktree creation flow doesn't check its return value,
so failures (e.g., sqlite3 unavailable) leave the worktree unregistered and
vulnerable to deletion; update the caller around register_worktree "$path"
"$branch" to capture its exit status and, on non-zero return, emit a clear
warning via the existing logging facility (and optionally fail the operation) so
users know registration didn’t complete; also ensure register_worktree (and
related helpers referenced in shared-constants.sh) return non-zero on error so
the caller can detect and act on failures.
1 participant
Summary
worktree-helper.sh removeandcleanrefuse to touch worktrees owned by other live processessupervisor-helper.shregisters ownership on dispatch and checks before cleanupChanges
shared-constants.sh (+170 lines)
_wt_sql_escape()— SQL escape helperregister_worktree()— register PID ownership with optional task/batch/session metadataunregister_worktree()— remove ownership entrycheck_worktree_owner()— query owner infois_worktree_owned_by_others()— returns 0 if owned by another live PIDprune_worktree_registry()— clean stale entries (dead PIDs, missing dirs)worktree-helper.sh (+135 lines)
cmd_addregisters ownership on creationcmd_removechecks ownership before removal, supports--forceoverridecmd_cleanskips worktrees owned by other active sessionscmd_registrycommand (list/prunesubcommands)supervisor-helper.sh (+37 lines)
create_task_worktreeregisters ownership with--taskmetadatacreate_task_worktreerefuses to clean stale worktrees owned by otherscleanup_task_worktreechecks ownership before removal, unregisters aftercmd_cleanupprunes stale registry entriesAGENTS.md
worktree.md
Testing
Subtasks
Summary by CodeRabbit
Release Notes
New Features
--forceflag to override ownership checks when needed.Documentation