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
20 changes: 15 additions & 5 deletions .agents/scripts/supervisor-helper.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2680,13 +2680,14 @@ create_task_worktree() {
if [[ "$needs_cleanup" == "true" ]]; then
# Remove worktree if it exists
if [[ -d "$worktree_path" ]]; then
git -C "$repo" worktree remove "$worktree_path" --force 2>/dev/null || rm -rf "$worktree_path"
git -C "$repo" worktree prune 2>/dev/null || true
git -C "$repo" worktree remove "$worktree_path" --force &>/dev/null || rm -rf "$worktree_path"
git -C "$repo" worktree prune &>/dev/null || true
fi
# Delete local branch
git -C "$repo" branch -D "$branch_name" 2>/dev/null || true
# Delete local branch — MUST suppress stdout (outputs "Deleted branch ...")
# which would pollute the function's return value captured by $()
git -C "$repo" branch -D "$branch_name" &>/dev/null || true
# Delete remote branch (best-effort, don't fail if remote is gone)
git -C "$repo" push origin --delete "$branch_name" 2>/dev/null || true
git -C "$repo" push origin --delete "$branch_name" &>/dev/null || true
fi

# Try wt first (redirect its verbose output to stderr)
Expand Down Expand Up @@ -3050,6 +3051,15 @@ cmd_dispatch() {
return 1
}

# Validate worktree path is an actual directory (guards against stdout
# pollution from git commands inside create_task_worktree)
if [[ ! -d "$worktree_path" ]]; then
log_error "Worktree path is not a directory: '$worktree_path'"
log_error "This usually means a git command leaked stdout into the path variable"
cmd_transition "$task_id" "failed" --error "Worktree path invalid: $worktree_path"
return 1
fi

local branch_name="feature/${task_id}"

# Set up log file
Expand Down
62 changes: 62 additions & 0 deletions tests/test-supervisor-state-machine.sh
Original file line number Diff line number Diff line change
Expand Up @@ -992,6 +992,68 @@ else
fail "Exit 0 with error strings should NOT be blocked" "Got: $eval_result"
fi

# ============================================================
# SECTION 7: Worktree Path Integrity
# ============================================================
section "Worktree Path Integrity"

# Test: create_task_worktree returns a clean single-line path (no stdout pollution)
# This is a regression test for the bug where `git branch -D` output ("Deleted branch ...")
# leaked into the function's return value, causing dispatch to fail with invalid paths.

WORKTREE_TEST_DIR=$(mktemp -d)
WORKTREE_TEST_REPO="$WORKTREE_TEST_DIR/test-repo"
trap 'rm -rf "$WORKTREE_TEST_DIR"; rm -rf "$TEST_DIR"' EXIT

# Set up a minimal git repo for worktree testing
git init "$WORKTREE_TEST_REPO" &>/dev/null
git -C "$WORKTREE_TEST_REPO" commit --allow-empty -m "initial" &>/dev/null

# Create a branch that will need cleanup (simulates stale branch from prior failed dispatch)
git -C "$WORKTREE_TEST_REPO" branch "feature/wt-test-001" &>/dev/null

# Add task to supervisor DB
sup add wt-test-001 --repo "$WORKTREE_TEST_REPO" --description "Worktree stdout leak test" >/dev/null 2>&1 || true

# Call create_task_worktree directly. The script calls main "$@" at the bottom
# when sourced, so we pass "init" to avoid show_usage. We redirect the source's
# stdout to /dev/null (suppresses cmd_init output) — function definitions still
# register in the current shell. Then we call the function we want to test.
worktree_output=$(bash -c "
export AIDEVOPS_SUPERVISOR_DIR='$TEST_DIR'
set -- init
source '$SUPERVISOR_SCRIPT' >/dev/null
create_task_worktree 'wt-test-001' '$WORKTREE_TEST_REPO' true
" 2>/dev/null)
Comment on lines +1022 to +1027
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 | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find the supervisor-related scripts
fd -type f -name "*supervisor*" | head -20

Repository: marcusquinn/aidevops

Length of output: 234


🏁 Script executed:

# Read the test file around the flagged lines
wc -l tests/test-supervisor-state-machine.sh

Repository: marcusquinn/aidevops

Length of output: 107


🏁 Script executed:

#!/bin/bash
# Find supervisor-related scripts correctly
find . -name "*supervisor*" -type f

Repository: marcusquinn/aidevops

Length of output: 185


🏁 Script executed:

# Read test file around the flagged lines with context
sed -n '1015,1035p' tests/test-supervisor-state-machine.sh

Repository: marcusquinn/aidevops

Length of output: 1104


🏁 Script executed:

#!/bin/bash
# Read supervisor-helper.sh to understand the dispatch mechanism
wc -l ./.agents/scripts/supervisor-helper.sh

Repository: marcusquinn/aidevops

Length of output: 107


🏁 Script executed:

# Find the main function and entry point pattern
rg -n 'main\s*"\$@"' ./.agents/scripts/supervisor-helper.sh

Repository: marcusquinn/aidevops

Length of output: 78


🏁 Script executed:

# Find cmd_init function
rg -n 'cmd_init' ./.agents/scripts/supervisor-helper.sh | head -20

Repository: marcusquinn/aidevops

Length of output: 116


🏁 Script executed:

#!/bin/bash
# Read the main function dispatch logic
sed -n '8200,8240p' ./.agents/scripts/supervisor-helper.sh

Repository: marcusquinn/aidevops

Length of output: 1368


🏁 Script executed:

# Read cmd_init function
sed -n '906,950p' ./.agents/scripts/supervisor-helper.sh

Repository: marcusquinn/aidevops

Length of output: 1341


🏁 Script executed:

# Find the main function definition
rg -n '^main\s*\(' ./.agents/scripts/supervisor-helper.sh

Repository: marcusquinn/aidevops

Length of output: 77


🏁 Script executed:

#!/bin/bash
# Find log_success and log_info definitions
rg -n 'log_success|log_info' ./.agents/scripts/supervisor-helper.sh | head -5

Repository: marcusquinn/aidevops

Length of output: 372


🏁 Script executed:

# Find ensure_db definition
rg -n 'ensure_db\s*\(' ./.agents/scripts/supervisor-helper.sh

Repository: marcusquinn/aidevops

Length of output: 81


🏁 Script executed:

# Find create_task_worktree definition
rg -n 'create_task_worktree\s*\(' ./.agents/scripts/supervisor-helper.sh

Repository: marcusquinn/aidevops

Length of output: 93


🏁 Script executed:

#!/bin/bash
# Read ensure_db to check error handling
sed -n '594,630p' ./.agents/scripts/supervisor-helper.sh

Repository: marcusquinn/aidevops

Length of output: 2391


🏁 Script executed:

# Read create_task_worktree function
sed -n '2619,2680p' ./.agents/scripts/supervisor-helper.sh

Repository: marcusquinn/aidevops

Length of output: 2987


🏁 Script executed:

# Check if there's error handling in cmd_init after ensure_db
sed -n '906,920p' ./.agents/scripts/supervisor-helper.sh

Repository: marcusquinn/aidevops

Length of output: 487


🏁 Script executed:

#!/bin/bash
# Check if ensure_db has any set -e or error handling
sed -n '594,670p' ./.agents/scripts/supervisor-helper.sh | grep -E 'set -e|return [1-9]|exit [1-9]||| *return'

Repository: marcusquinn/aidevops

Length of output: 3970


🏁 Script executed:

# Check if cmd_init checks ensure_db return value
sed -n '906,920p' ./.agents/scripts/supervisor-helper.sh | grep -E 'ensure_db.*\$?||| *true|&& *return'

Repository: marcusquinn/aidevops

Length of output: 487


🏁 Script executed:

# Verify main dispatch pattern - check if set -- modifies what main receives
rg -n 'show_usage' ./.agents/scripts/supervisor-helper.sh -B2 -A2 | head -30

Repository: marcusquinn/aidevops

Length of output: 467


🏁 Script executed:

#!/bin/bash
# Check if cmd_init relies on ensure_db succeeding
# Look for any error handling pattern in cmd_init
sed -n '906,920p' ./.agents/scripts/supervisor-helper.sh

Repository: marcusquinn/aidevops

Length of output: 487


🏁 Script executed:

# Check if create_task_worktree depends on DB state or just function definitions
sed -n '2619,2650p' ./.agents/scripts/supervisor-helper.sh | head -40

Repository: marcusquinn/aidevops

Length of output: 1510


🏁 Script executed:

# Verify what happens in test after create_task_worktree - does it validate DB state?
sed -n '1022,1045p' tests/test-supervisor-state-machine.sh

Repository: marcusquinn/aidevops

Length of output: 968


Error suppression masks whether cmd_init / database initialization actually succeeded — test can pass even if DB is non-functional.

The set -- init workaround is necessary and already well-documented in the preceding comment. However, the actual risk is deeper: cmd_init calls ensure_db without checking its return code (line 906 of supervisor-helper.sh), and both stdout and stderr from sourcing are redirected away. If ensure_db fails silently (e.g., DB initialization fails but doesn't exit), cmd_init continues and returns 0.

More importantly, create_task_worktree operates only on git state (worktrees/branches), not the database — so the test can pass and return a valid directory path even if the supervisor database was never initialized. This test validates that function definitions load and create_task_worktree returns no extraneous stdout, but provides no assurance the supervisor DB is actually ready when the function runs.

Consider adding an explicit DB validation step (e.g., verify $SUPERVISOR_DB exists and can execute a simple query) before or after calling create_task_worktree to catch silent initialization failures.

🤖 Prompt for AI Agents
In `@tests/test-supervisor-state-machine.sh` around lines 1022 - 1027, After
sourcing SUPERVISOR_SCRIPT and invoking cmd_init (which calls ensure_db), add an
explicit database validation step: verify the SUPERVISOR_DB file exists and that
a simple query can be executed against it (e.g., a SELECT 1) to ensure ensure_db
actually succeeded before calling create_task_worktree; also stop silencing
source's stderr/stdout or assert cmd_init/ensure_db exit codes so failures are
detected (references: SUPERVISOR_SCRIPT, cmd_init, ensure_db, SUPERVISOR_DB,
create_task_worktree, TEST_DIR, WORKTREE_TEST_REPO).


# Count lines — should be exactly 1 (the path)
line_count=$(echo "$worktree_output" | wc -l | tr -d ' ')
if [[ "$line_count" -eq 1 ]]; then
pass "create_task_worktree returns exactly 1 line (no stdout pollution)"
else
fail "create_task_worktree returned $line_count lines (stdout pollution detected)" \
"Output: $(echo "$worktree_output" | head -3)"
fi
Comment on lines +1029 to +1036
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 | 🟡 Minor

Empty output passes the "exactly 1 line" check — false-positive risk.

If create_task_worktree produces no output (empty string), echo "" | wc -l still returns 1, so this assertion would incorrectly PASS. The directory check on line 1039 would catch the failure downstream, but the first test gives a misleading result. Guard against this explicitly.

🐛 Proposed fix
+# Guard: empty output is an immediate failure
+if [[ -z "$worktree_output" ]]; then
+    fail "create_task_worktree returned empty output"
+else
 # Count lines — should be exactly 1 (the path)
 line_count=$(echo "$worktree_output" | wc -l | tr -d ' ')
 if [[ "$line_count" -eq 1 ]]; then
     pass "create_task_worktree returns exactly 1 line (no stdout pollution)"
 else
     fail "create_task_worktree returned $line_count lines (stdout pollution detected)" \
         "Output: $(echo "$worktree_output" | head -3)"
 fi
+fi
📝 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
# Count lines — should be exactly 1 (the path)
line_count=$(echo "$worktree_output" | wc -l | tr -d ' ')
if [[ "$line_count" -eq 1 ]]; then
pass "create_task_worktree returns exactly 1 line (no stdout pollution)"
else
fail "create_task_worktree returned $line_count lines (stdout pollution detected)" \
"Output: $(echo "$worktree_output" | head -3)"
fi
# Guard: empty output is an immediate failure
if [[ -z "$worktree_output" ]]; then
fail "create_task_worktree returned empty output"
else
# Count lines — should be exactly 1 (the path)
line_count=$(echo "$worktree_output" | wc -l | tr -d ' ')
if [[ "$line_count" -eq 1 ]]; then
pass "create_task_worktree returns exactly 1 line (no stdout pollution)"
else
fail "create_task_worktree returned $line_count lines (stdout pollution detected)" \
"Output: $(echo "$worktree_output" | head -3)"
fi
fi
🤖 Prompt for AI Agents
In `@tests/test-supervisor-state-machine.sh` around lines 1029 - 1036, The test
falsely passes when worktree_output is empty because echo "" | wc -l yields 1;
add an explicit empty-output guard before the line_count check: test if
worktree_output is empty (using the worktree_output variable) and fail
immediately with a clear message if so, otherwise compute line_count as before
and assert it equals 1; reference create_task_worktree, worktree_output, and
line_count so you update the existing condition that currently does the wc -l
check.


# Verify the returned path is a real directory
if [[ -d "$worktree_output" ]]; then
pass "create_task_worktree returns a valid directory path"
else
fail "create_task_worktree path is not a directory" "Got: '$worktree_output'"
fi

# Verify path doesn't contain "Deleted branch" (the specific pollution we fixed)
if echo "$worktree_output" | grep -qi "deleted branch"; then
fail "create_task_worktree output contains 'Deleted branch' pollution" \
"Got: '$worktree_output'"
else
pass "create_task_worktree output is free of 'Deleted branch' pollution"
fi

# Clean up the test worktree
git -C "$WORKTREE_TEST_REPO" worktree remove "$worktree_output" --force &>/dev/null || rm -rf "$worktree_output"
git -C "$WORKTREE_TEST_REPO" worktree prune &>/dev/null || true

# ============================================================
# SUMMARY
# ============================================================
Expand Down
Loading