fix: suppress stdout pollution in create_task_worktree (t169, t173) #643
Conversation
git branch -D outputs 'Deleted branch ...' to stdout, which pollutes the return value when captured via $(...). This caused dispatch to fail with invalid worktree paths (7-byte logs containing only EXIT:1). Changes: - Change 2>/dev/null to &>/dev/null on git cleanup commands in create_task_worktree() to suppress both stdout and stderr - Add worktree path validation guard in cmd_dispatch() that checks the returned path is a real directory before proceeding - Add regression test (3 assertions) in test-supervisor-state-machine.sh verifying clean single-line output from create_task_worktree
Summary of ChangesHello @marcusquinn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug where Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughThis PR fixes stdout leakage in the supervisor script by silencing Git and worktree command outputs in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: Sun Feb 8 18:41:18 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: 2
🤖 Fix all issues with AI agents
In `@tests/test-supervisor-state-machine.sh`:
- Around line 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.
- Around line 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).
🧹 Nitpick comments (3)
.agents/scripts/supervisor-helper.sh (1)
2683-2690: Preserve stderr for cleanup failures while silencing stdout.
&>/dev/nullhides useful error output. To keep return-value cleanliness while retaining diagnostics, redirect stdout only and send stderr to the supervisor log.🔧 Suggested change
- git -C "$repo" worktree remove "$worktree_path" --force &>/dev/null || rm -rf "$worktree_path" - git -C "$repo" worktree prune &>/dev/null || true + git -C "$repo" worktree remove "$worktree_path" --force >/dev/null 2>>"$SUPERVISOR_LOG" || rm -rf "$worktree_path" + git -C "$repo" worktree prune >/dev/null 2>>"$SUPERVISOR_LOG" || true @@ - git -C "$repo" branch -D "$branch_name" &>/dev/null || true + git -C "$repo" branch -D "$branch_name" >/dev/null 2>>"$SUPERVISOR_LOG" || true @@ - git -C "$repo" push origin --delete "$branch_name" &>/dev/null || true + git -C "$repo" push origin --delete "$branch_name" >/dev/null 2>>"$SUPERVISOR_LOG" || trueAs per coding guidelines, automation scripts should emphasize clear logging and feedback.
tests/test-supervisor-state-machine.sh (2)
995-997: Section numbering is out of order."SECTION 7" is placed after Sections 14b and the Evaluate Worker section. This will confuse anyone reading the test output or scanning the file. Consider renaming to
SECTION 15(or the next available number) to maintain sequential ordering.🔢 Proposed fix
-# SECTION 7: Worktree Path Integrity +# SECTION 15: Worktree Path Integrity
1006-1006: Trap override silently drops any traps registered between line 59 and here.This replaces the original
trap 'rm -rf "$TEST_DIR"' EXITfrom line 59. It works today, but if a future section inserts another trap between lines 59 and 1006, it will be silently discarded. A more resilient pattern appends to the existing trap.🛡️ Safer trap chaining
-trap 'rm -rf "$WORKTREE_TEST_DIR"; rm -rf "$TEST_DIR"' EXIT +trap 'rm -rf "$WORKTREE_TEST_DIR" "$TEST_DIR"' EXITEven better, consider using a cleanup function that accumulates directories, or simply add
$WORKTREE_TEST_DIRto the samerm -rfas the original trap at line 59 so there's a single trap definition.
| 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) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the supervisor-related scripts
fd -type f -name "*supervisor*" | head -20Repository: marcusquinn/aidevops
Length of output: 234
🏁 Script executed:
# Read the test file around the flagged lines
wc -l tests/test-supervisor-state-machine.shRepository: marcusquinn/aidevops
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Find supervisor-related scripts correctly
find . -name "*supervisor*" -type fRepository: 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.shRepository: 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.shRepository: marcusquinn/aidevops
Length of output: 107
🏁 Script executed:
# Find the main function and entry point pattern
rg -n 'main\s*"\$@"' ./.agents/scripts/supervisor-helper.shRepository: marcusquinn/aidevops
Length of output: 78
🏁 Script executed:
# Find cmd_init function
rg -n 'cmd_init' ./.agents/scripts/supervisor-helper.sh | head -20Repository: marcusquinn/aidevops
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Read the main function dispatch logic
sed -n '8200,8240p' ./.agents/scripts/supervisor-helper.shRepository: marcusquinn/aidevops
Length of output: 1368
🏁 Script executed:
# Read cmd_init function
sed -n '906,950p' ./.agents/scripts/supervisor-helper.shRepository: marcusquinn/aidevops
Length of output: 1341
🏁 Script executed:
# Find the main function definition
rg -n '^main\s*\(' ./.agents/scripts/supervisor-helper.shRepository: 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 -5Repository: marcusquinn/aidevops
Length of output: 372
🏁 Script executed:
# Find ensure_db definition
rg -n 'ensure_db\s*\(' ./.agents/scripts/supervisor-helper.shRepository: marcusquinn/aidevops
Length of output: 81
🏁 Script executed:
# Find create_task_worktree definition
rg -n 'create_task_worktree\s*\(' ./.agents/scripts/supervisor-helper.shRepository: 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.shRepository: marcusquinn/aidevops
Length of output: 2391
🏁 Script executed:
# Read create_task_worktree function
sed -n '2619,2680p' ./.agents/scripts/supervisor-helper.shRepository: 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.shRepository: 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 -30Repository: 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.shRepository: 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 -40Repository: 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.shRepository: 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 |
There was a problem hiding this comment.
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.
| # 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.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request effectively addresses the stdout pollution issue in create_task_worktree() by redirecting both stdout and stderr of git commands to /dev/null. The addition of a defensive validation check in cmd_dispatch() further enhances the robustness of the system by ensuring that the worktree path is a valid directory. The new regression tests in test-supervisor-state-machine.sh provide excellent coverage for the fix, verifying single-line output, valid directory paths, and the absence of "Deleted branch" pollution. This is a solid fix that improves the reliability and correctness of the supervisor's worktree management.
1 participant
Summary
create_task_worktree()stdout leak wheregit branch -Doutput ("Deleted branch ...") polluted the function's return value when captured via$(...), causing dispatch to fail with invalid worktree pathscmd_dispatch()that checks the returned worktree path is a real directory before proceedingtest-supervisor-state-machine.sh(Section 7: Worktree Path Integrity) verifying clean single-line outputRoot Cause
When a stale branch needed cleanup,
git branch -D "$branch_name" 2>/dev/nullsuppressed stderr but not stdout. The outputDeleted branch feature/tXXX (was abc1234).was prepended to the worktree path, causingcdto fail in the nohup dispatch — resulting in 7-byte logs containing onlyEXIT:1.Changes
.agents/scripts/supervisor-helper.sh2>/dev/null→&>/dev/nullon 4 git cleanup commands increate_task_worktree().agents/scripts/supervisor-helper.sh[[ ! -d "$worktree_path" ]]guard aftercreate_task_worktree()returns incmd_dispatch()tests/test-supervisor-state-machine.shTesting
All 78 tests pass (77 pass, 1 pre-existing skip, 0 failures):
Unblocks t169 and t173 which both failed with
Max retries exceeded during re-promptdue to this bug.Summary by CodeRabbit
Release Notes
Bug Fixes
Tests