fix: critical quality-debt — supervisor fixes and misc (batch 4)#3835
fix: critical quality-debt — supervisor fixes and misc (batch 4)#3835johnwaldo wants to merge 3 commits intomarcusquinn:mainfrom
Conversation
Supervisor fixes: - issue-sync: replace 19 blanket 2>/dev/null on gh/db commands with stderr redirect to SUPERVISOR_LOG for debuggability (GH#3328) - deploy: fix git_dir vs trepo mismatch in rebase_sibling_pr remote URL check — now uses consistent git directory (GH#3414) Other: - local-hosting.md: remove 2>/dev/null from xargs kill -9 so permission errors are visible (GH#3297) - mcp-index-helper: remove incorrect %% percent escaping in SQL LIKE pattern (GH#3402) Already fixed (verified): - evaluate.sh heartbeat cleanup scope — code removed in t1312 (GH#3364) - .coderabbit.yaml schema compliance — all 4 violations resolved (GH#3384) - session-distill-helper.sh — JSON built safely with jq -n (GH#3402) - ai-lifecycle.sh worker_pid → session_id — already applied (GH#3333) - version-manager.sh improvements — already applied (GH#3404) Closes marcusquinn#3328, closes marcusquinn#3414, closes marcusquinn#3297, closes marcusquinn#3402, closes marcusquinn#3364, closes marcusquinn#3384, closes marcusquinn#3333, closes marcusquinn#3404, closes marcusquinn#3411
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
WalkthroughThis PR applies fixes for critical review feedback identified in merged pull requests, addressing error logging suppression issues, directory context mismatches, SQL query handling, and process management patterns across shell scripts and documentation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
Triage: Worker appears stuckThis PR was created by a worker that completed its initial push but never returned to address:
The SonarCloud failure is likely the missing Recommendation: A new worker should be dispatched to address the CodeRabbit feedback on this PR, or close and re-create if the branch has diverged too far from main. |
|
This PR is from an external contributor (johnwaldo, permission: read). Auto-merge is disabled. A maintainer must review and merge manually. CI failures on this PR are the contributor's responsibility to fix. |
|
This PR is from an external contributor (read-only access). Auto-merge is disabled. A maintainer must review and merge manually. CI failures should be addressed by the contributor. |
marcusquinn
added
the
external-contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review Review bots were rate-limited when this PR was created (affected: gemini-code-assist). Requesting a review retry. |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
![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
This pull request delivers a solid batch of quality improvements. The fixes in mcp-index-helper.sh and deploy.sh correctly address an SQL escaping issue and a variable mismatch. The changes in issue-sync.sh to replace 2>/dev/null with proper logging are a significant enhancement for debuggability, aligning with our guidelines to avoid blanket error suppression. For the change in local-hosting.md, the suggested robust command avoids displaying benign errors during normal operation. Overall, these are valuable fixes.
| repo_path=$(db "$SUPERVISOR_DB" "SELECT repo FROM tasks WHERE id = '$escaped_id';" 2>/dev/null || echo "") | ||
| repo_path=$(db "$SUPERVISOR_DB" "SELECT repo FROM tasks WHERE id = '$escaped_id';" 2>>"${SUPERVISOR_LOG:-/dev/null}" || echo "") | ||
| if [[ -z "$repo_path" ]]; then | ||
| repo_path=$(find_project_root 2>/dev/null || echo ".") |
There was a problem hiding this comment.
For consistency with the other changes in this file and to improve debuggability, consider redirecting stderr from find_project_root to the supervisor log instead of /dev/null. This would help diagnose issues if find_project_root fails unexpectedly.
| repo_path=$(find_project_root 2>/dev/null || echo ".") | |
| repo_path=$(find_project_root 2>>"${SUPERVISOR_LOG:-/dev/null}" || echo ".") |
References
- Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.
There was a problem hiding this comment.
Applied — fixed all 6 remaining find_project_root 2>/dev/null instances in the file (lines 115, 228, 373, 1404, 1508, 1643) to use 2>>"${SUPERVISOR_LOG:-/dev/null}" for consistency with the other 2>/dev/null replacements in this PR. Commit b65a577.
|
|
||
| # Terminal profile / Tabby start command (includes lock cleanup + port kill) | ||
| rm -f apps/web/.next/dev/lock; lsof -ti:3100 | xargs kill -9 2>/dev/null; pnpm dev:web | ||
| rm -f apps/web/.next/dev/lock; lsof -ti:3100 | xargs kill -9; pnpm dev:web |
There was a problem hiding this comment.
Removing 2>/dev/null is good for transparency, but it might cause benign error messages to appear if no process is running on port 3100, as xargs might run kill with no arguments. A more robust approach that avoids this issue is to use xargs -I {}, which only runs the command for non-empty input and is generally portable.
| rm -f apps/web/.next/dev/lock; lsof -ti:3100 | xargs kill -9; pnpm dev:web | |
| rm -f apps/web/.next/dev/lock; lsof -ti:3100 | xargs -I{} kill -9 {}; pnpm dev:web |
There was a problem hiding this comment.
Applied — changed to xargs -I{} kill -9 {} which only runs kill when there's actual input, avoiding benign errors when no process is on the port. Commit b65a577.
|
CI failures on this PR are systemic, not per-PR code issues:
This PR's code changes are sound. It will be mergeable once the systemic CI issues are resolved. |
|
Recommend closing this PR — it's stale and most changes are superseded:
If the mcp-index-helper.sh fix is desired, it's a 1-line change that can go in a future batch. The merge conflicts from 4+ days of main branch activity make rebasing this PR more effort than it's worth. |
|
Pulse 2026-03-09T19:56:23Z: All 8 worker slots filled this pulse. This PR has SonarCloud/Label CI failures — queued for CI fix dispatch next pulse when slots free up. |
|
CI failures on this PR are infrastructure issues, not code quality failures:
These failures are not caused by the code changes in this PR. The PR should be reviewed on its merits. Flagging for next pulse to re-run checks or address infrastructure issues. |
Three CI checks fail on fork PRs (e.g., PR #3835) due to GitHub's security model restricting GITHUB_TOKEN permissions for fork PRs: 1. SonarCloud Analysis: Add continue-on-error to handle cases where SONAR_TOKEN is available (maintainer re-runs) but SonarCloud can't analyze the fork's merge ref. Other quality tools still provide coverage. 2. Monitor & Auto-Fix Code Quality: Wrap PR comment API call in try-catch to handle 403 errors gracefully when the token lacks write permissions. Add if:always() to Summary step so it runs regardless of comment step outcome. 3. codacy-cli.sh: Fix unbound variable error ($3) when called with fewer than 4 arguments under set -u. Use ${N:-} defaults. The Label PR check (issue-sync.yml) was already fixed by the pull_request_target migration in t3851/t3862 — the latest run passed.
|
Dispatching worker to address unresolved review suggestions.
|
…ervisor log, use xargs -I{} for kill
- Replace all 6 remaining find_project_root 2>/dev/null with
2>>"${SUPERVISOR_LOG:-/dev/null}" for consistency with the rest of the file
- Use xargs -I{} kill -9 {} in local-hosting.md to avoid errors when
no process is found on the port (portability improvement)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.agents/scripts/supervisor-archived/deploy.sh (1)
2356-2361:⚠️ Potential issue | 🟡 MinorKeep stderr visible for the
githubremote probe.This still uses
&>/dev/null, so a broken repo/config just looks like “nogithubremote” and the mirror push is silently skipped. Redirect only stdout away and append stderr to"$SUPERVISOR_LOG"instead.Suggested fix
- if git -C "$git_dir" remote get-url github &>/dev/null; then + if git -C "$git_dir" remote get-url github >/dev/null 2>>"$SUPERVISOR_LOG"; thenAs per coding guidelines, automation scripts should provide clear logging and feedback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/supervisor-archived/deploy.sh around lines 2356 - 2361, The probe for the github remote currently silences both stdout and stderr with `&>/dev/null`, hiding configuration or repo errors; change the redirection on the `git -C "$git_dir" remote get-url github` check to redirect only stdout (e.g. `>/dev/null`) and append stderr to `"$SUPERVISOR_LOG"` (e.g. `2>>"$SUPERVISOR_LOG"`) so failures are logged but the boolean check still works; update the corresponding `git -C "$git_dir" remote get-url github` invocation in deploy.sh accordingly and keep the subsequent push/error-handling logic unchanged..agents/scripts/mcp-index-helper.sh (1)
440-447:⚠️ Potential issue | 🟡 MinorEscape
_in theLIKEprobe as well.Removing the doubled
%fixes the old bug, but_is still aLIKEwildcard. Tool names in this index contain underscores (e.g.,run_claude_code), so the query can return unintended matches. Escape both%and_in the user-supplied fragment and addESCAPE '\'to the clause, while keeping the outer%...%wildcards for partial matching.Suggested fix
- # Escape single quotes for SQL injection prevention - local tool_query_esc="${tool_query//\'/\'\'}" + # Escape LIKE metacharacters in the user fragment, then escape single quotes + local tool_query_like="${tool_query//\\/\\\\}" + tool_query_like="${tool_query_like//%/\\%}" + tool_query_like="${tool_query_like//_/\\_}" + local tool_query_esc="${tool_query_like//\'/\'\'}" @@ SELECT DISTINCT mcp_name FROM mcp_tools -WHERE tool_name LIKE '%$tool_query_esc%' +WHERE tool_name LIKE '%$tool_query_esc%' ESCAPE '\' LIMIT 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/mcp-index-helper.sh around lines 440 - 447, The LIKE probe currently only escapes single quotes via tool_query_esc and leaves underscores as wildcards; update the preprocessing of the user fragment (variable tool_query_esc) to also escape both % and _ (replace '%' with '\%' and '_' with '\_') while preserving the existing single-quote escaping, then use that escaped fragment inside the SQL LIKE with surrounding % wildcards (e.g., '%...%') and add "ESCAPE '\' " to the WHERE clause so the backslash-escaped percent/underscore are treated literally when querying mcp_tools.tool_name against INDEX_DB.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/scripts/supervisor-archived/issue-sync.sh:
- Line 115: The stderr redirections depend on SUPERVISOR_LOG being initialized,
so in issue-sync.sh set and export a safe default at the top (e.g.,
SUPERVISOR_LOG="${SUPERVISOR_LOG:-/tmp/issue-sync.log}"; export SUPERVISOR_LOG)
to guarantee a target even when sourced early, and add a small guard function
(e.g., require_supervisor_log) that checks SUPERVISOR_LOG and exits with a clear
error if it’s intentionally required to be provided; call this guard at the
start of any functions that must fail loudly (or omit the guard if you rely on
the exported default). Ensure you update usages like the find_project_root call
that use 2>>"${SUPERVISOR_LOG:-/dev/null}" to rely on the exported
SUPERVISOR_LOG instead of the inline default.
---
Outside diff comments:
In @.agents/scripts/mcp-index-helper.sh:
- Around line 440-447: The LIKE probe currently only escapes single quotes via
tool_query_esc and leaves underscores as wildcards; update the preprocessing of
the user fragment (variable tool_query_esc) to also escape both % and _ (replace
'%' with '\%' and '_' with '\_') while preserving the existing single-quote
escaping, then use that escaped fragment inside the SQL LIKE with surrounding %
wildcards (e.g., '%...%') and add "ESCAPE '\' " to the WHERE clause so the
backslash-escaped percent/underscore are treated literally when querying
mcp_tools.tool_name against INDEX_DB.
In @.agents/scripts/supervisor-archived/deploy.sh:
- Around line 2356-2361: The probe for the github remote currently silences both
stdout and stderr with `&>/dev/null`, hiding configuration or repo errors;
change the redirection on the `git -C "$git_dir" remote get-url github` check to
redirect only stdout (e.g. `>/dev/null`) and append stderr to
`"$SUPERVISOR_LOG"` (e.g. `2>>"$SUPERVISOR_LOG"`) so failures are logged but the
boolean check still works; update the corresponding `git -C "$git_dir" remote
get-url github` invocation in deploy.sh accordingly and keep the subsequent
push/error-handling logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ddb296cb-d3ad-4ab5-bc45-78f8764693d2
📒 Files selected for processing (4)
.agents/scripts/mcp-index-helper.sh.agents/scripts/supervisor-archived/deploy.sh.agents/scripts/supervisor-archived/issue-sync.sh.agents/services/hosting/local-hosting.md
| # Detect repo slug | ||
| if [[ -z "$project_root" ]]; then | ||
| project_root=$(find_project_root 2>/dev/null || echo ".") | ||
| project_root=$(find_project_root 2>>"${SUPERVISOR_LOG:-/dev/null}" || echo ".") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Call sites that source issue-sync.sh =="
rg -n -C2 'source .*issue-sync\.sh' .agents/scripts || true
echo
echo "== SUPERVISOR_LOG initialization / export sites =="
rg -n -C2 '(^|[[:space:]])SUPERVISOR_LOG=|export .*SUPERVISOR_LOG' .agents/scripts || true
echo
echo "== database.sh candidates (to inspect alternate bootstrap paths) =="
fd 'database\.sh$' . -x sed -n '1,140p' {}Repository: marcusquinn/aidevops
Length of output: 9010
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== backfill-blocked-comments.sh (full file or first 50 lines) ==="
head -50 .agents/scripts/supervisor-archived/backfill-blocked-comments.sh
echo
echo "=== database.sh SUPERVISOR_LOG initialization check (full search) ==="
rg -n 'SUPERVISOR_LOG' .agents/scripts/database.sh || echo "(not found)"
echo
echo "=== issue-sync.sh module-level code (not function definitions) ==="
# Look for lines outside function definitions that execute code
rg -n -v '^\s*(function|[a-z_]+\s*\(\)|local|#)' .agents/scripts/supervisor-archived/issue-sync.sh | head -30 || echo "(only functions/comments)"Repository: marcusquinn/aidevops
Length of output: 3554
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== issue-sync.sh: Check for module-level code (lines outside any function) ==="
# Extract actual module-level code by checking line context
awk '
/^[a-z_]+\s*\(\)|^function\s+[a-z_]+/ { in_func=1; func_indent=match($0, /[^ ]/)-1; next }
in_func && /^[^ \t]/ { in_func=0 }
!in_func && NF && !/^[[:space:]]*(#|local|return)/ { print NR": "$0 }
' .agents/scripts/supervisor-archived/issue-sync.sh | head -20
echo
echo "=== backfill-blocked-comments.sh: when are issue-sync.sh functions called? ==="
grep -n 'detect_repo_slug\|sync_issue\|get_model\|ensure_labels' .agents/scripts/supervisor-archived/backfill-blocked-comments.sh
echo
echo "=== Verify: does issue-sync.sh execute code before main()? ==="
head -15 .agents/scripts/supervisor-archived/issue-sync.shRepository: marcusquinn/aidevops
Length of output: 910
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== issue-sync.sh: First 25 lines (check for module-level code) ==="
sed -n '1,25p' .agents/scripts/supervisor-archived/issue-sync.sh
echo
echo "=== issue-sync.sh: Lines around 346 (the ALL_STATUS_LABELS line) ==="
sed -n '340,355p' .agents/scripts/supervisor-archived/issue-sync.sh
echo
echo "=== backfill-blocked-comments.sh: function calls to issue-sync functions ==="
grep -En 'detect_repo_slug|sync_issue|ensure_labels|get_model' .agents/scripts/supervisor-archived/backfill-blocked-comments.sh || echo "(none found)"
echo
echo "=== issue-sync.sh: Look for actual function definitions (not closing braces) ==="
grep -n '^[a-z_]*\s*()' .agents/scripts/supervisor-archived/issue-sync.sh | head -15Repository: marcusquinn/aidevops
Length of output: 3112
Enforce or document the bootstrap sequence—stderr redirections silently fail when SUPERVISOR_LOG is unset.
The stderr redirections 2>>"${SUPERVISOR_LOG:-/dev/null}" in these functions only work as intended when SUPERVISOR_LOG is already initialized. In backfill-blocked-comments.sh, issue-sync.sh is sourced before supervisor-helper.sh, leaving a window where SUPERVISOR_LOG is unset. This pattern works in practice because functions are called only after supervisor-helper.sh bootstraps the variable, but it's fragile and vulnerable to new callers that source the module without completing bootstrap first.
Either:
- Export a shared default log target from issue-sync.sh itself (e.g.,
SUPERVISOR_LOG="${SUPERVISOR_LOG:-/tmp/issue-sync.log}") - Add an explicit check at function entry to require
SUPERVISOR_LOGand fail loudly, or - Explicitly document this dependency and enforce bootstrap ordering in calling scripts.
Also applies to: 228, 371-374, 414-520, 1404, 1508-1513, 1643-1648
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/scripts/supervisor-archived/issue-sync.sh at line 115, The stderr
redirections depend on SUPERVISOR_LOG being initialized, so in issue-sync.sh set
and export a safe default at the top (e.g.,
SUPERVISOR_LOG="${SUPERVISOR_LOG:-/tmp/issue-sync.log}"; export SUPERVISOR_LOG)
to guarantee a target even when sourced early, and add a small guard function
(e.g., require_supervisor_log) that checks SUPERVISOR_LOG and exits with a clear
error if it’s intentionally required to be provided; call this guard at the
start of any functions that must fail loudly (or omit the guard if you rely on
the exported default). Ensure you update usages like the find_project_root call
that use 2>>"${SUPERVISOR_LOG:-/dev/null}" to rely on the exported
SUPERVISOR_LOG instead of the inline default.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…4017) Three CI checks fail on fork PRs (e.g., PR #3835) due to GitHub's security model restricting GITHUB_TOKEN permissions for fork PRs: 1. SonarCloud Analysis: Add continue-on-error to handle cases where SONAR_TOKEN is available (maintainer re-runs) but SonarCloud can't analyze the fork's merge ref. Other quality tools still provide coverage. 2. Monitor & Auto-Fix Code Quality: Wrap PR comment API call in try-catch to handle 403 errors gracefully when the token lacks write permissions. Add if:always() to Summary step so it runs regardless of comment step outcome. 3. codacy-cli.sh: Fix unbound variable error ($3) when called with fewer than 4 arguments under set -u. Use ${N:-} defaults. The Label PR check (issue-sync.yml) was already fixed by the pull_request_target migration in t3851/t3862 — the latest run passed.
|
Completed via PR #4017. merged to main. |
2 participants
Summary
Batch 4 of the quality-debt sprint. Supervisor debuggability, git consistency fix, and misc cleanup.
Changes
supervisor-archived/issue-sync.sh2>/dev/nullwith2>>"$SUPERVISOR_LOG"on gh/db commandssupervisor-archived/deploy.sh$git_dirvs$trepomismatch inrebase_sibling_prremote URL checkservices/hosting/local-hosting.md2>/dev/nullfromxargs kill -9mcp-index-helper.sh%%percent escaping in SQL LIKE patternVerified already fixed (no code changes)
Closes #3328, closes #3414, closes #3297, closes #3402, closes #3364, closes #3384, closes #3333, closes #3404, closes #3411
Summary by CodeRabbit