Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .agents/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ worktree-helper.sh add feature/x # Fallback

**Branch types**: `feature/`, `bugfix/`, `hotfix/`, `refactor/`, `chore/`, `experiment/`, `release/`

**Worktree ownership** (CRITICAL): NEVER remove a worktree unless (a) you created it in this session, (b) it belongs to a task in your active batch, AND the task is deployed/complete, or (c) the user explicitly asks. Worktrees may belong to parallel sessions — removing them destroys another agent's working directory mid-work. When cleaning up, only touch worktrees for tasks you personally merged. Use `git worktree list` to see all worktrees but do NOT assume unrecognized ones are safe to remove.
**Worktree ownership** (CRITICAL): NEVER remove a worktree unless (a) you created it in this session, (b) it belongs to a task in your active batch, AND the task is deployed/complete, or (c) the user explicitly asks. Worktrees may belong to parallel sessions — removing them destroys another agent's working directory mid-work. When cleaning up, only touch worktrees for tasks you personally merged. Use `git worktree list` to see all worktrees but do NOT assume unrecognized ones are safe to remove. The ownership registry (`worktree-helper.sh registry list`) tracks which PID owns each worktree — `remove` and `clean` commands automatically refuse to touch worktrees owned by other live processes.

**Safety hooks** (Claude Code only): Destructive commands (`git reset --hard`, `rm -rf`, etc.) are blocked by a PreToolUse hook. Run `install-hooks.sh --test` to verify. See `workflows/git-workflow.md` "Destructive Command Safety Hooks" section.

Expand Down
170 changes: 170 additions & 0 deletions .agents/scripts/shared-constants.sh
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,176 @@ _todo_commit_push_inner() {
return 1
}

# =============================================================================
# Worktree Ownership Registry (t189)
# =============================================================================
# SQLite-backed registry that tracks which session/batch owns each worktree.
# Prevents cross-session worktree removal — the root cause of t189.
#
# Available to all scripts that source shared-constants.sh.

WORKTREE_REGISTRY_DIR="${HOME}/.aidevops/.agent-workspace"
WORKTREE_REGISTRY_DB="${WORKTREE_REGISTRY_DIR}/worktree-registry.db"

# SQL-escape a value for SQLite (double single quotes)
_wt_sql_escape() {
local val="$1"
echo "${val//\'/\'\'}"
}

# Initialize the registry database
_init_registry_db() {
mkdir -p "$WORKTREE_REGISTRY_DIR" 2>/dev/null || true
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
return 0
}

# Register ownership of a worktree
# Arguments:
# $1 - worktree path (required)
# $2 - branch name (required)
# Flags: --task <id>, --batch <id>, --session <id>
register_worktree() {
local wt_path="$1"
local branch="$2"
shift 2

local task_id="" batch_id="" session_id=""
while [[ $# -gt 0 ]]; do
case "$1" in
--task) task_id="${2:-}"; shift 2 ;;
--batch) batch_id="${2:-}"; shift 2 ;;
--session) session_id="${2:-}"; shift 2 ;;
*) shift ;;
esac
done

_init_registry_db

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
Comment on lines +563 to +574
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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

}

# Unregister ownership of a worktree
# Arguments:
# $1 - worktree path (required)
unregister_worktree() {
local wt_path="$1"

[[ ! -f "$WORKTREE_REGISTRY_DB" ]] && return 0

sqlite3 "$WORKTREE_REGISTRY_DB" "
DELETE FROM worktree_owners
WHERE worktree_path = '$(_wt_sql_escape "$wt_path")';
" 2>/dev/null || true
return 0
}

# Check who owns a worktree
# Arguments:
# $1 - worktree path
# Output: owner info (pid|session|batch|task|created_at) or empty
# Returns: 0 if owned, 1 if not owned
check_worktree_owner() {
local wt_path="$1"

[[ ! -f "$WORKTREE_REGISTRY_DB" ]] && return 1

local owner_info
owner_info=$(sqlite3 -separator '|' "$WORKTREE_REGISTRY_DB" "
SELECT owner_pid, owner_session, owner_batch, task_id, created_at
FROM worktree_owners
WHERE worktree_path = '$(_wt_sql_escape "$wt_path")';
" 2>/dev/null || echo "")

if [[ -n "$owner_info" ]]; then
echo "$owner_info"
return 0
fi
return 1
}

# Check if a worktree is owned by a DIFFERENT process (still alive)
# Arguments:
# $1 - worktree path
# Returns: 0 if owned by another live process, 1 if safe to remove
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 "")

# 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
}
Comment on lines +625 to +646
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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 1

At 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.

Suggested change
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.


# Prune stale registry entries (dead PIDs, missing directories)
prune_worktree_registry() {
[[ ! -f "$WORKTREE_REGISTRY_DB" ]] && return 0

local entries
entries=$(sqlite3 -separator '|' "$WORKTREE_REGISTRY_DB" "
SELECT worktree_path, owner_pid FROM worktree_owners;
" 2>/dev/null || echo "")

[[ -z "$entries" ]] && return 0

while IFS='|' read -r wt_path owner_pid; do
local should_prune=false

# Directory no longer exists
if [[ ! -d "$wt_path" ]]; then
should_prune=true
# Owner process is dead
elif [[ -n "$owner_pid" ]] && ! kill -0 "$owner_pid" 2>/dev/null; then
should_prune=true
fi

if [[ "$should_prune" == "true" ]]; then
unregister_worktree "$wt_path"
fi
done <<< "$entries"
return 0
}

# =============================================================================
# Export all constants for use in other scripts
# =============================================================================
Expand Down
37 changes: 36 additions & 1 deletion .agents/scripts/supervisor-helper.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2798,10 +2798,21 @@ create_task_worktree() {
fi

if [[ "$needs_cleanup" == "true" ]]; then
# 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
Comment on lines +2801 to +2809
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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
         fi

As 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.

Suggested change
# 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.

# Remove worktree if it exists
if [[ -d "$worktree_path" ]]; then
git -C "$repo" worktree remove "$worktree_path" --force &>/dev/null || rm -rf "$worktree_path"
git -C "$repo" worktree prune &>/dev/null || true
# Unregister ownership (t189)
unregister_worktree "$worktree_path"
fi
# Delete local branch — MUST suppress stdout (outputs "Deleted branch ...")
# which would pollute the function's return value captured by $()
Expand All @@ -2813,19 +2824,25 @@ create_task_worktree() {
# Try wt first (redirect its verbose output to stderr)
if command -v wt &>/dev/null; then
if wt switch -c "$branch_name" -C "$repo" >&2 2>&1; then
# Register ownership (t189)
register_worktree "$worktree_path" "$branch_name" --task "$task_id"
echo "$worktree_path"
return 0
fi
fi

# Fallback: raw git worktree add (quiet, reliable)
if git -C "$repo" worktree add "$worktree_path" -b "$branch_name" >&2 2>&1; then
# Register ownership (t189)
register_worktree "$worktree_path" "$branch_name" --task "$task_id"
echo "$worktree_path"
return 0
fi

# Branch may already exist without worktree (e.g. remote-only)
if git -C "$repo" worktree add "$worktree_path" "$branch_name" >&2 2>&1; then
# Register ownership (t189)
register_worktree "$worktree_path" "$branch_name" --task "$task_id"
echo "$worktree_path"
return 0
fi
Expand All @@ -2836,22 +2853,38 @@ create_task_worktree() {

#######################################
# Clean up a worktree for a completed/failed task
# Checks ownership registry (t189) before removal
#######################################
cleanup_task_worktree() {
local worktree_path="$1"
local repo="$2"

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
Comment on lines 2862 to 2873
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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
     fi

As 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.

Suggested change
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.

fi

# Try wt prune first
if command -v wt &>/dev/null; then
wt remove -C "$repo" "$worktree_path" 2>>"$SUPERVISOR_LOG" && return 0
if wt remove -C "$repo" "$worktree_path" 2>>"$SUPERVISOR_LOG"; then
unregister_worktree "$worktree_path"
return 0
fi
fi

# Fallback: git worktree remove
git -C "$repo" worktree remove "$worktree_path" --force 2>>"$SUPERVISOR_LOG" || true
# Unregister regardless of removal success (directory may be partially cleaned)
unregister_worktree "$worktree_path"
return 0
}

Expand Down Expand Up @@ -6019,7 +6052,9 @@ cmd_cleanup() {
done
fi

# Prune stale registry entries (t189)
if [[ "$dry_run" == "false" ]]; then
prune_worktree_registry
log_success "Cleaned up $cleaned worktrees, $process_cleaned worker processes"
fi

Expand Down
Loading
Loading