t1314.1: Migrate deploy.sh decision logic to AI judgment#2219
t1314.1: Migrate deploy.sh decision logic to AI judgment#2219marcusquinn merged 2 commits intomainfrom
Conversation
… decision logic (t1314.1) Implements three AI-powered decision functions that replace deterministic logic: - ai_check_pr_status(): replaces 210-line case statement with AI classification - ai_triage_review_feedback(): replaces keyword regex with semantic understanding - ai_verify_task_deliverables(): replaces file-counting with contextual judgment Architecture: GATHER (shell) → JUDGE (AI) → RETURN (shell) Each function falls back to the original deterministic logic if AI is unavailable. Feature flag: AI_DEPLOY_DECISIONS_ENABLED (default: true) Model: sonnet tier (fast, cheap, sufficient for classification tasks)
- Source ai-deploy-decisions.sh in supervisor-helper.sh (after ai-lifecycle.sh) - Replace check_pr_status() call in cmd_pr_lifecycle() with ai_check_pr_status() - Replace triage_review_feedback() call with ai_triage_review_feedback() - Replace verify_task_deliverables() call in todo-sync.sh with ai_verify_task_deliverables() - All replacements use declare -f guard for safe fallback if module not loaded - Merge/rebase execution remains 100% shell (chose AI-for-decisions only — matches existing ai-lifecycle.sh pattern)
WalkthroughThis PR introduces AI-powered deployment decision agents via a new shell module (ai-deploy-decisions.sh) that augments three core verification functions with AI-guided logic while maintaining deterministic fallbacks. The module is sourced into the supervisor framework and integrated into deploy and todo-sync workflows with conditional function dispatch. Changes
Sequence Diagram(s)sequenceDiagram
participant Deploy as Deploy/Todo Script
participant AIModule as AI-Deploy-Decisions
participant AICheck as Function Exists?
participant AICLI as AI CLI (opencode/claude)
participant Parser as JSON Parser
participant Fallback as Deterministic Function
Deploy->>AIModule: Call ai_*_function()
AIModule->>AICheck: Check if AI_DEPLOY_DECISIONS_ENABLED
alt AI Path Enabled
AIModule->>AIModule: Gather context (PR data, threads, etc.)
AIModule->>AICLI: Call _ai_deploy_call(prompt, timeout)
AICLI-->>AIModule: AI response (text + JSON)
AIModule->>Parser: Extract JSON via _ai_deploy_extract_json()
Parser-->>AIModule: Parsed JSON object
alt Validation Successful
AIModule->>AIModule: Log decision via _ai_deploy_log_decision()
AIModule-->>Deploy: Return AI-guided result
else Validation Failed
AIModule->>Fallback: Fall back to deterministic function
Fallback-->>Deploy: Return deterministic result
end
else AI Disabled or Unavailable
AIModule->>Fallback: Dispatch to deterministic function
Fallback-->>Deploy: Return deterministic result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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: Tue Feb 24 17:02:23 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
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 introduces AI-driven decision-making to the Highlights
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
|
|
![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 introduces AI-powered decision logic for PR status checks, review feedback triage, and deliverable verification, with well-implemented fallback mechanisms to deterministic logic. The architecture of gathering data, sending it to AI for judgment, and then processing the AI's response is clearly defined. The use of feature flags to enable/disable AI decisions and comprehensive logging for audit trails are good practices. However, there are several instances where error output is suppressed, which could hinder debugging, and some of the JSON extraction logic could be more robust.
| "$prompt" 2>/dev/null || echo "") | ||
| # Strip ANSI escape codes | ||
| ai_result=$(printf '%s' "$ai_result" | sed 's/\x1b\[[0-9;]*[mGKHF]//g; s/\x1b\[[0-9;]*[A-Za-z]//g; s/\x1b\]//g; s/\x07//g') | ||
| else | ||
| local claude_model="${ai_model#*/}" | ||
| ai_result=$(portable_timeout "$timeout_secs" claude \ | ||
| -p "$prompt" \ | ||
| --model "$claude_model" \ | ||
| --output-format text 2>/dev/null || echo "") |
There was a problem hiding this comment.
The 2>/dev/null suppresses all standard error output from the opencode and claude CLI calls. This can mask critical issues such as authentication failures, invalid model names, or other command execution errors, making debugging difficult. It violates the general rule against blanket stderr suppression.
Consider redirecting stderr to $SUPERVISOR_LOG or removing the suppression entirely to allow errors to be visible.
| "$prompt" 2>/dev/null || echo "") | |
| # Strip ANSI escape codes | |
| ai_result=$(printf '%s' "$ai_result" | sed 's/\x1b\[[0-9;]*[mGKHF]//g; s/\x1b\[[0-9;]*[A-Za-z]//g; s/\x1b\]//g; s/\x07//g') | |
| else | |
| local claude_model="${ai_model#*/}" | |
| ai_result=$(portable_timeout "$timeout_secs" claude \ | |
| -p "$prompt" \ | |
| --model "$claude_model" \ | |
| --output-format text 2>/dev/null || echo "") | |
| ai_result=$(portable_timeout "$timeout_secs" opencode run \ | |
| -m "$ai_model" \ | |
| --format default \ | |
| --title "deploy-${title_suffix}-$$" \ | |
| "$prompt" 2>>"$SUPERVISOR_LOG" || echo "") | |
| # Strip ANSI escape codes | |
| ai_result=$(printf '%s' "$ai_result" | sed 's/\x1b\[[0-9;]*[mGKHF]//g; s/\x1b\[[0-9;]*[A-Za-z]//g; s/\x1b\]//g; s/\x07//g') | |
| else | |
| local claude_model="${ai_model#*/}" | |
| ai_result=$(portable_timeout "$timeout_secs" claude \ | |
| -p "$prompt" \ | |
| --model "$claude_model" \ | |
| --output-format text 2>>"$SUPERVISOR_LOG" || 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.
| if parsed=$(printf '%s' "$response" | jq '.' 2>/dev/null) && [[ -n "$parsed" ]]; then | ||
| local jtype | ||
| jtype=$(printf '%s' "$parsed" | jq 'type' 2>/dev/null || echo "") | ||
| if [[ "$jtype" == '"object"' ]]; then | ||
| printf '%s' "$parsed" | ||
| return 0 | ||
| fi | ||
| fi | ||
|
|
||
| # Try 2: Extract from ```json block | ||
| local json_block | ||
| json_block=$(printf '%s' "$response" | awk ' | ||
| /^```json/ { capture=1; block=""; next } | ||
| /^```$/ && capture { capture=0; last_block=block; next } | ||
| capture { block = block (block ? "\n" : "") $0 } | ||
| END { if (capture && block) print block; else if (last_block) print last_block } | ||
| ') | ||
| if [[ -n "$json_block" ]]; then | ||
| if parsed=$(printf '%s' "$json_block" | jq '.' 2>/dev/null) && [[ -n "$parsed" ]]; then | ||
| printf '%s' "$parsed" | ||
| return 0 | ||
| fi | ||
| fi | ||
|
|
||
| # Try 3: Grep for JSON object | ||
| local obj_match | ||
| obj_match=$(printf '%s' "$response" | grep -oE '\{[^}]+\}' | tail -1) | ||
| if [[ -n "$obj_match" ]]; then | ||
| if parsed=$(printf '%s' "$obj_match" | jq '.' 2>/dev/null) && [[ -n "$parsed" ]]; then | ||
| printf '%s' "$parsed" | ||
| return 0 | ||
| fi | ||
| fi | ||
|
|
||
| # Try 4: Multi-line JSON object (between { and }) | ||
| local bracket_json | ||
| bracket_json=$(printf '%s' "$response" | awk ' | ||
| /^\s*\{/ { capture=1; block="" } | ||
| capture { block = block (block ? "\n" : "") $0 } | ||
| /^\s*\}/ && capture { capture=0; last_block=block } | ||
| END { if (last_block) print last_block } | ||
| ') | ||
| if [[ -n "$bracket_json" ]]; then | ||
| if parsed=$(printf '%s' "$bracket_json" | jq '.' 2>/dev/null) && [[ -n "$parsed" ]]; then | ||
| printf '%s' "$parsed" | ||
| return 0 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
The JSON extraction logic in _ai_deploy_extract_json is not fully robust, especially for the third and fourth attempts (grep -oE '{\[^}]+\}' and the multi-line awk for bracket_json).
grep -oE '{\[^}]+\}'will fail to correctly extract JSON objects with nested curly braces (e.g.,{"key": {"nested": "value"}}).- The multi-line
awkforbracket_jsonmight also be prone to issues if the response contains multiple JSON-like structures or non-JSON text with braces.
It's generally safer to rely on jq's robust parsing capabilities. If the first two attempts (direct jq parse and ````json` block extraction) fail, it might be better to consider the response unparsable rather than attempting less reliable regex-based extractions that could yield incomplete or malformed JSON.
| if parsed=$(printf '%s' "$response" | jq '.' 2>/dev/null) && [[ -n "$parsed" ]]; then | |
| local jtype | |
| jtype=$(printf '%s' "$parsed" | jq 'type' 2>/dev/null || echo "") | |
| if [[ "$jtype" == '"object"' ]]; then | |
| printf '%s' "$parsed" | |
| return 0 | |
| fi | |
| fi | |
| # Try 2: Extract from ```json block | |
| local json_block | |
| json_block=$(printf '%s' "$response" | awk ' | |
| /^```json/ { capture=1; block=""; next } | |
| /^```$/ && capture { capture=0; last_block=block; next } | |
| capture { block = block (block ? "\n" : "") $0 } | |
| END { if (capture && block) print block; else if (last_block) print last_block } | |
| ') | |
| if [[ -n "$json_block" ]]; then | |
| if parsed=$(printf '%s' "$json_block" | jq '.' 2>/dev/null) && [[ -n "$parsed" ]]; then | |
| printf '%s' "$parsed" | |
| return 0 | |
| fi | |
| fi | |
| # Try 3: Grep for JSON object | |
| local obj_match | |
| obj_match=$(printf '%s' "$response" | grep -oE '\{[^}]+\}' | tail -1) | |
| if [[ -n "$obj_match" ]]; then | |
| if parsed=$(printf '%s' "$obj_match" | jq '.' 2>/dev/null) && [[ -n "$parsed" ]]; then | |
| printf '%s' "$parsed" | |
| return 0 | |
| fi | |
| fi | |
| # Try 4: Multi-line JSON object (between { and }) | |
| local bracket_json | |
| bracket_json=$(printf '%s' "$response" | awk ' | |
| /^\s*\{/ { capture=1; block="" } | |
| capture { block = block (block ? "\n" : "") $0 } | |
| /^\s*\}/ && capture { capture=0; last_block=block } | |
| END { if (last_block) print last_block } | |
| ') | |
| if [[ -n "$bracket_json" ]]; then | |
| if parsed=$(printf '%s' "$bracket_json" | jq '.' 2>/dev/null) && [[ -n "$parsed" ]]; then | |
| printf '%s' "$parsed" | |
| return 0 | |
| fi | |
| fi | |
| # Try 2: Extract from ```json block | |
| local json_block | |
| json_block=$(printf '%s' "$response" | awk ' | |
| /^```json/ { capture=1; block=""; next } | |
| /^```$/ && capture { capture=0; last_block=block; next } | |
| capture { block = block (block ? "\n" : "") $0 } | |
| END { if (capture && block) print block; else if (last_block) print last_block } | |
| ') | |
| if [[ -n "$json_block" ]]; then | |
| if parsed=$(printf '%s' "$json_block" | jq '.' 2>>"$SUPERVISOR_LOG") && [[ -n "$parsed" ]]; then | |
| printf '%s' "$parsed" | |
| return 0 | |
| fi | |
| fi | |
| # If we reach here, no valid JSON object was found by robust methods. | |
| # It's better to fail explicitly than to attempt unreliable regex matching. | |
| return 1 |
References
- When a shell function is expected to return JSON, it should output a valid empty JSON array (
[]) or object ({}) on no results, rather than returning a non-zero exit code or empty string, to ensure consumers of the output (like pipes tojq) do not break.
| mkdir -p "$AI_DEPLOY_DECISIONS_LOG_DIR" 2>/dev/null || true | ||
|
|
||
| local timestamp | ||
| timestamp=$(date -u '+%Y%m%d-%H%M%S') | ||
| local log_file="$AI_DEPLOY_DECISIONS_LOG_DIR/${func_name}-${task_id}-${timestamp}.md" | ||
|
|
||
| { | ||
| echo "# $func_name: $task_id @ $timestamp" | ||
| echo "" | ||
| echo "Decision: $decision" | ||
| echo "" | ||
| if [[ -n "$context" ]]; then | ||
| echo "## Context" | ||
| echo "" | ||
| echo "$context" | ||
| fi | ||
| } >"$log_file" 2>/dev/null || true |
There was a problem hiding this comment.
Suppressing stderr with 2>/dev/null when writing to the log file can hide critical issues like disk full errors or permission problems, which would prevent the audit trail from being properly recorded. While || true prevents the script from exiting, it doesn't provide visibility into why the write failed.
It's better to allow stderr to be visible or redirect it to a dedicated error log for diagnostic purposes.
| mkdir -p "$AI_DEPLOY_DECISIONS_LOG_DIR" 2>/dev/null || true | |
| local timestamp | |
| timestamp=$(date -u '+%Y%m%d-%H%M%S') | |
| local log_file="$AI_DEPLOY_DECISIONS_LOG_DIR/${func_name}-${task_id}-${timestamp}.md" | |
| { | |
| echo "# $func_name: $task_id @ $timestamp" | |
| echo "" | |
| echo "Decision: $decision" | |
| echo "" | |
| if [[ -n "$context" ]]; then | |
| echo "## Context" | |
| echo "" | |
| echo "$context" | |
| fi | |
| } >"$log_file" 2>/dev/null || true | |
| mkdir -p "$AI_DEPLOY_DECISIONS_LOG_DIR" || true | |
| local timestamp | |
| timestamp=$(date -u '+%Y%m%d-%H%M%S') | |
| local log_file="$AI_DEPLOY_DECISIONS_LOG_DIR/${func_name}-${task_id}-${timestamp}.md" | |
| { | |
| echo "# $func_name: $task_id @ $timestamp" | |
| echo "" | |
| echo "Decision: $decision" | |
| echo "" | |
| if [[ -n "$context" ]]; then | |
| echo "## Context" | |
| echo "" | |
| echo "$context" | |
| fi | |
| } >"$log_file" || log_error "Failed to write AI decision log to $log_file" |
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.
- When reporting errors for failed file operations in shell scripts, such as 'jq' writes, include the file path in the error message. Avoid suppressing stderr with
2>/dev/nullto ensure that diagnostic information about malformed files or write failures is visible.
| pr_state=$(printf '%s' "$pr_json" | jq -r '.state // "UNKNOWN"' 2>/dev/null || echo "UNKNOWN") | ||
| is_draft=$(printf '%s' "$pr_json" | jq -r '.isDraft // false' 2>/dev/null || echo "false") | ||
| review_decision=$(printf '%s' "$pr_json" | jq -r '.reviewDecision // "NONE"' 2>/dev/null || echo "NONE") | ||
| merge_state=$(printf '%s' "$pr_json" | jq -r '.mergeStateStatus // "UNKNOWN"' 2>/dev/null || echo "UNKNOWN") | ||
| mergeable_state=$(printf '%s' "$pr_json" | jq -r '.mergeable // "UNKNOWN"' 2>/dev/null || echo "UNKNOWN") |
There was a problem hiding this comment.
The jq commands here use 2>/dev/null to suppress errors. If jq fails to parse the input JSON (e.g., due to malformed JSON), these errors will be silently ignored, potentially leading to UNKNOWN values being used without clear diagnostic information. This violates the general rule against blanket stderr suppression.
Consider removing 2>/dev/null to allow jq errors to be logged or visible, aiding in debugging issues with the GitHub API response parsing.
| pr_state=$(printf '%s' "$pr_json" | jq -r '.state // "UNKNOWN"' 2>/dev/null || echo "UNKNOWN") | |
| is_draft=$(printf '%s' "$pr_json" | jq -r '.isDraft // false' 2>/dev/null || echo "false") | |
| review_decision=$(printf '%s' "$pr_json" | jq -r '.reviewDecision // "NONE"' 2>/dev/null || echo "NONE") | |
| merge_state=$(printf '%s' "$pr_json" | jq -r '.mergeStateStatus // "UNKNOWN"' 2>/dev/null || echo "UNKNOWN") | |
| mergeable_state=$(printf '%s' "$pr_json" | jq -r '.mergeable // "UNKNOWN"' 2>/dev/null || echo "UNKNOWN") | |
| pr_state=$(printf '%s' "$pr_json" | jq -r '.state // "UNKNOWN"' 2>>"$SUPERVISOR_LOG" || echo "UNKNOWN") | |
| is_draft=$(printf '%s' "$pr_json" | jq -r '.isDraft // false' 2>>"$SUPERVISOR_LOG" || echo "false") | |
| review_decision=$(printf '%s' "$pr_json" | jq -r '.reviewDecision // "NONE"' 2>>"$SUPERVISOR_LOG" || echo "NONE") | |
| merge_state=$(printf '%s' "$pr_json" | jq -r '.mergeStateStatus // "UNKNOWN"' 2>>"$SUPERVISOR_LOG" || echo "UNKNOWN") | |
| mergeable_state=$(printf '%s' "$pr_json" | jq -r '.mergeable // "UNKNOWN"' 2>>"$SUPERVISOR_LOG" || echo "UNKNOWN") |
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.
- In shell scripts with 'set -e' enabled, use '|| true' to prevent the script from exiting when a command like 'jq' fails on an optional lookup. Do not suppress stderr with '2>/dev/null' so that actual syntax or system errors remain visible for debugging.
| pending=$(printf '%s' "$check_rollup" | jq '[.[] | select(.status == "IN_PROGRESS" or .status == "QUEUED" or .status == "PENDING")] | length' 2>/dev/null || echo "0") | ||
| failed=$(printf '%s' "$check_rollup" | jq '[.[] | select((.conclusion | test("FAILURE|TIMED_OUT|ACTION_REQUIRED")) or .state == "FAILURE" or .state == "ERROR")] | length' 2>/dev/null || echo "0") | ||
| passed=$(printf '%s' "$check_rollup" | jq '[.[] | select(.conclusion == "SUCCESS" or .state == "SUCCESS")] | length' 2>/dev/null || echo "0") | ||
| total=$(printf '%s' "$check_rollup" | jq 'length' 2>/dev/null || echo "0") | ||
|
|
||
| local failed_names | ||
| failed_names=$(printf '%s' "$check_rollup" | jq -r '[.[] | select((.conclusion | test("FAILURE|TIMED_OUT|ACTION_REQUIRED")) or .state == "FAILURE" or .state == "ERROR") | .name] | join(", ")' 2>/dev/null || echo "none") | ||
|
|
||
| local sonar_action_pass sonar_gate_fail | ||
| sonar_action_pass=$(printf '%s' "$check_rollup" | jq '[.[] | select(.name == "SonarCloud Analysis" and .conclusion == "SUCCESS")] | length' 2>/dev/null || echo "0") | ||
| sonar_gate_fail=$(printf '%s' "$check_rollup" | jq '[.[] | select(.name == "SonarCloud Code Analysis" and .conclusion == "FAILURE")] | length' 2>/dev/null || echo "0") |
There was a problem hiding this comment.
Similar to previous comments, the jq commands within the CI check details section use 2>/dev/null. This can hide parsing errors if the statusCheckRollup JSON is malformed or unexpected, leading to incorrect CI summary values without clear error messages. This violates the general rule against blanket stderr suppression.
Redirecting stderr to $SUPERVISOR_LOG would provide better visibility into potential issues.
| pending=$(printf '%s' "$check_rollup" | jq '[.[] | select(.status == "IN_PROGRESS" or .status == "QUEUED" or .status == "PENDING")] | length' 2>/dev/null || echo "0") | |
| failed=$(printf '%s' "$check_rollup" | jq '[.[] | select((.conclusion | test("FAILURE|TIMED_OUT|ACTION_REQUIRED")) or .state == "FAILURE" or .state == "ERROR")] | length' 2>/dev/null || echo "0") | |
| passed=$(printf '%s' "$check_rollup" | jq '[.[] | select(.conclusion == "SUCCESS" or .state == "SUCCESS")] | length' 2>/dev/null || echo "0") | |
| total=$(printf '%s' "$check_rollup" | jq 'length' 2>/dev/null || echo "0") | |
| local failed_names | |
| failed_names=$(printf '%s' "$check_rollup" | jq -r '[.[] | select((.conclusion | test("FAILURE|TIMED_OUT|ACTION_REQUIRED")) or .state == "FAILURE" or .state == "ERROR") | .name] | join(", ")' 2>/dev/null || echo "none") | |
| local sonar_action_pass sonar_gate_fail | |
| sonar_action_pass=$(printf '%s' "$check_rollup" | jq '[.[] | select(.name == "SonarCloud Analysis" and .conclusion == "SUCCESS")] | length' 2>/dev/null || echo "0") | |
| sonar_gate_fail=$(printf '%s' "$check_rollup" | jq '[.[] | select(.name == "SonarCloud Code Analysis" and .conclusion == "FAILURE")] | length' 2>/dev/null || echo "0") | |
| local pending failed passed total | |
| pending=$(printf '%s' "$check_rollup" | jq '[.[] | select(.status == "IN_PROGRESS" or .status == "QUEUED" or .status == "PENDING")] | length' 2>>"$SUPERVISOR_LOG" || echo "0") | |
| failed=$(printf '%s' "$check_rollup" | jq '[.[] | select((.conclusion | test("FAILURE|TIMED_OUT|ACTION_REQUIRED")) or .state == "FAILURE" or .state == "ERROR")] | length' 2>>"$SUPERVISOR_LOG" || echo "0") | |
| passed=$(printf '%s' "$check_rollup" | jq '[.[] | select(.conclusion == "SUCCESS" or .state == "SUCCESS")] | length' 2>>"$SUPERVISOR_LOG" || echo "0") | |
| total=$(printf '%s' "$check_rollup" | jq 'length' 2>>"$SUPERVISOR_LOG" || echo "0") | |
| local failed_names | |
| failed_names=$(printf '%s' "$check_rollup" | jq -r '[.[] | select((.conclusion | test("FAILURE|TIMED_OUT|ACTION_REQUIRED")) or .state == "FAILURE" or .state == "ERROR") | .name] | join(", ")' 2>>"$SUPERVISOR_LOG" || echo "none") | |
| local sonar_action_pass sonar_gate_fail | |
| sonar_action_pass=$(printf '%s' "$check_rollup" | jq '[.[] | select(.name == "SonarCloud Analysis" and .conclusion == "SUCCESS")] | length' 2>>"$SUPERVISOR_LOG" || echo "0") | |
| sonar_gate_fail=$(printf '%s' "$check_rollup" | jq '[.[] | select(.name == "SonarCloud Code Analysis" and .conclusion == "FAILURE")] | length' 2>>"$SUPERVISOR_LOG" || echo "0") |
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.
- In shell scripts with 'set -e' enabled, use '|| true' to prevent the script from exiting when a command like 'jq' fails on an optional lookup. Do not suppress stderr with '2>/dev/null' so that actual syntax or system errors remain visible for debugging.
| if [[ -n "${SUPERVISOR_DB:-}" && -f "${SUPERVISOR_DB:-}" ]]; then | ||
| local escaped_id |
There was a problem hiding this comment.
The db command uses 2>/dev/null to suppress errors. This can hide critical issues such as database connection problems, file permission errors, or SQL syntax errors, making it difficult to diagnose why the task description might be missing. This violates the general rule against blanket stderr suppression.
Consider removing 2>/dev/null or redirecting to $SUPERVISOR_LOG to ensure database errors are visible.
| if [[ -n "${SUPERVISOR_DB:-}" && -f "${SUPERVISOR_DB:-}" ]]; then | |
| local escaped_id | |
| escaped_id=$(sql_escape "$task_id") | |
| task_description=$(db "$SUPERVISOR_DB" "SELECT description FROM tasks WHERE id = '$escaped_id';" 2>>"$SUPERVISOR_LOG" || 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.
| local task_tags="" | ||
| if [[ -n "$repo" && -f "$repo/TODO.md" ]]; then | ||
| local task_line | ||
| task_line=$(grep -E "^\s*- \[.\] $task_id\b" "$repo/TODO.md" 2>/dev/null || echo "") |
There was a problem hiding this comment.
The grep -E ... 2>/dev/null command suppresses errors. If grep encounters issues (e.g., file permission problems or malformed regex if it were dynamic), these errors would be hidden. This violates the general rule against blanket stderr suppression.
Consider removing 2>/dev/null to allow grep errors to be visible.
| task_line=$(grep -E "^\s*- \[.\] $task_id\b" "$repo/TODO.md" 2>/dev/null || echo "") | |
| task_line=$(grep -E "^\s*-\[.\] $task_id\b" "$repo/TODO.md" 2>>"$SUPERVISOR_LOG" || 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.
| substantive_files=$(printf '%s' "$changed_files" | grep -vE '^(TODO\.md$|todo/|\.github/workflows/)' || echo "") | ||
| planning_files=$(printf '%s' "$changed_files" | grep -E '^(TODO\.md$|todo/|\.github/workflows/)' || echo "") |
There was a problem hiding this comment.
The grep -c '.' 2>/dev/null commands suppress errors. While grep -c is unlikely to produce stderr for valid input, suppressing it generally can hide issues if the input stream is problematic or if grep itself has issues. This violates the general rule against blanket stderr suppression.
Consider removing 2>/dev/null to ensure any unexpected errors are visible.
| substantive_files=$(printf '%s' "$changed_files" | grep -vE '^(TODO\.md$|todo/|\.github/workflows/)' || echo "") | |
| planning_files=$(printf '%s' "$changed_files" | grep -E '^(TODO\.md$|todo/|\.github/workflows/)' || echo "") | |
| substantive_count=$(printf '%s' "$substantive_files" | grep -c '.' 2>>"$SUPERVISOR_LOG" || echo "0") | |
| planning_count=$(printf '%s' "$planning_files" | grep -c '.' 2>>"$SUPERVISOR_LOG" || echo "0") |
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.
| ai_verified=$(printf '%s' "$json_result" | jq -r '.verified // ""' 2>/dev/null || echo "") | ||
| ai_reason=$(printf '%s' "$json_result" | jq -r '.reason // ""' 2>/dev/null || echo "") | ||
| ai_category=$(printf '%s' "$json_result" | jq -r '.category // "unknown"' 2>/dev/null || echo "unknown") |
There was a problem hiding this comment.
The jq commands used to extract ai_verified, ai_reason, and ai_category from the AI's JSON response use 2>/dev/null. If the AI returns malformed JSON or a structure different from expected, these errors will be silently suppressed, making it harder to diagnose why the AI's judgment might not be correctly interpreted. This violates the general rule against blanket stderr suppression.
Consider removing 2>/dev/null to allow jq parsing errors to be visible.
| ai_verified=$(printf '%s' "$json_result" | jq -r '.verified // ""' 2>/dev/null || echo "") | |
| ai_reason=$(printf '%s' "$json_result" | jq -r '.reason // ""' 2>/dev/null || echo "") | |
| ai_category=$(printf '%s' "$json_result" | jq -r '.category // "unknown"' 2>/dev/null || echo "unknown") | |
| local ai_verified ai_reason ai_category | |
| ai_verified=$(printf '%s' "$json_result" | jq -r '.verified // ""' 2>>"$SUPERVISOR_LOG" || echo "") | |
| ai_reason=$(printf '%s' "$json_result" | jq -r '.reason // ""' 2>>"$SUPERVISOR_LOG" || echo "") | |
| ai_category=$(printf '%s' "$json_result" | jq -r '.category // "unknown"' 2>>"$SUPERVISOR_LOG" || echo "unknown") |
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.
- In shell scripts with 'set -e' enabled, use '|| true' to prevent the script from exiting when a command like 'jq' fails on an optional lookup. Do not suppress stderr with '2>/dev/null' so that actual syntax or system errors remain visible for debugging.
| write_proof_log --task "$task_id" --event "deliverable_verified" --stage "complete" \ | ||
| --decision "verified:PR#$pr_number:ai_judgment:$ai_category" \ | ||
| --evidence "pr_state=$pr_state,file_count=$file_count,substantive=$substantive_count,ai_reason=$ai_reason" \ | ||
| --maker "ai_verify_task_deliverables" \ | ||
| --pr-url "$pr_url" 2>/dev/null || true |
There was a problem hiding this comment.
The write_proof_log command uses 2>/dev/null to suppress errors. This can hide critical issues such as file permission problems or disk space issues that prevent the proof log from being written. While || true prevents the script from exiting, it doesn't provide visibility into why the write failed. This violates the general rule against blanket stderr suppression for file operations.
Consider removing 2>/dev/null or redirecting to $SUPERVISOR_LOG to ensure these errors are visible for debugging.
| write_proof_log --task "$task_id" --event "deliverable_verified" --stage "complete" \ | |
| --decision "verified:PR#$pr_number:ai_judgment:$ai_category" \ | |
| --evidence "pr_state=$pr_state,file_count=$file_count,substantive=$substantive_count,ai_reason=$ai_reason" \ | |
| --maker "ai_verify_task_deliverables" \ | |
| --pr-url "$pr_url" 2>/dev/null || true | |
| write_proof_log --task "$task_id" --event "deliverable_verified" --stage "complete" \ | |
| --decision "verified:PR#$pr_number:ai_judgment:$ai_category" \ | |
| --evidence "pr_state=$pr_state,file_count=$file_count,substantive=$substantive_count,ai_reason=$ai_reason" \ | |
| --maker "ai_verify_task_deliverables" \ | |
| --pr-url "$pr_url" 2>>"$SUPERVISOR_LOG" || true |
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.
- When reporting errors for failed file operations in shell scripts, such as 'jq' writes, include the file path in the error message. Avoid suppressing stderr with
2>/dev/nullto ensure that diagnostic information about malformed files or write failures is visible.
![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
🤖 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/ai-deploy-decisions.sh:
- Around line 625-636: After fetching pr_state, enforce a hard gate that the PR
must be merged before any AI verification path can set verified=true: check the
pr_state variable (from the gh pr view call) and if it is not "MERGED", log a
warning (use log_warn and SUPERVISOR_LOG context) and either return non‑zero or
explicitly set verified=false and skip the AI verification flow; place this
check immediately after the pr_state retrieval and before any code that invokes
the AI verification or sets the verified flag so the AI path cannot mark a
non‑merged PR as verified.
- Around line 511-516: The current merge in enriched_result merges AI severities
by positional index (using threads_json and .threads), which can misassign
severities if the AI reorders/omits threads; change the jq merge to map
severities by a stable thread identifier (e.g., thread.id): build a lookup from
the AI response (.threads | map({(.id): .severity}) | add), then iterate the
original threads_json and for each thread set severity = lookup[thread.id] //
"medium" (fallback to "medium" when missing); update the enriched_result
construction to use this id-based mapping instead of index-based merging so
severities stay tied to the correct threads even when the AI output changes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.agents/scripts/supervisor-helper.sh.agents/scripts/supervisor/ai-deploy-decisions.sh.agents/scripts/supervisor/deploy.sh.agents/scripts/supervisor/todo-sync.sh
| enriched_result=$(printf '%s' "$json_result" | jq --argjson orig "$threads_json" ' | ||
| # Preserve original thread data, add severity from AI classification | ||
| .threads = [range(($orig | length)) as $i | | ||
| $orig[$i] + (if $i < (.threads | length) then {severity: .threads[$i].severity} else {severity: "medium"} end) | ||
| ] | ||
| ' 2>/dev/null || echo "") |
There was a problem hiding this comment.
Prevent AI severity assignments from drifting across threads.
The AI can reorder or omit threads; merging by index can misclassify feedback and send the fix worker after the wrong items. Use a stable index/id in the prompt and map by that, or fall back when the mapping is incomplete.
🔧 Suggested fix (index-based mapping)
- thread_details=$(printf '%s' "$threads_json" | jq -r '.[] | "[\(.author)] \(.path):\(.line // "?"): \(.body | split("\n")[0] | .[0:300]) (isBot: \(.isBot))"' 2>/dev/null || echo "")
+ thread_details=$(printf '%s' "$threads_json" | jq -r 'to_entries[] | "[#\(.key)] [\(.value.author)] \(.value.path):\(.value.line // "?"): \(.value.body | split("\n")[0] | .[0:300]) (isBot: \(.value.isBot))"' 2>/dev/null || echo "")
...
- \"threads\": [{\"author\": \"...\", \"severity\": \"...\", \"isBot\": true/false}],
+ \"threads\": [{\"index\": N, \"author\": \"...\", \"severity\": \"...\", \"isBot\": true/false}],
...
- .threads = [range(($orig | length)) as $i |
- $orig[$i] + (if $i < (.threads | length) then {severity: .threads[$i].severity} else {severity: "medium"} end)
- ]
+ .threads = [range(($orig | length)) as $i |
+ $orig[$i] + ((.threads[] | select(.index == $i) | {severity: .severity}) // {severity: "medium"})
+ ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/scripts/supervisor/ai-deploy-decisions.sh around lines 511 - 516,
The current merge in enriched_result merges AI severities by positional index
(using threads_json and .threads), which can misassign severities if the AI
reorders/omits threads; change the jq merge to map severities by a stable thread
identifier (e.g., thread.id): build a lookup from the AI response (.threads |
map({(.id): .severity}) | add), then iterate the original threads_json and for
each thread set severity = lookup[thread.id] // "medium" (fallback to "medium"
when missing); update the enriched_result construction to use this id-based
mapping instead of index-based merging so severities stay tied to the correct
threads even when the AI output changes.
| # GATHER: Fetch PR data | ||
| local pr_state | ||
| if ! pr_state=$(gh pr view "$pr_number" --repo "$repo_slug" --json state --jq '.state' 2>>"$SUPERVISOR_LOG"); then | ||
| log_warn "Failed to fetch PR state for $task_id (#$pr_number)" | ||
| return 1 | ||
| fi | ||
|
|
||
| local changed_files | ||
| if ! changed_files=$(gh pr view "$pr_number" --repo "$repo_slug" --json files --jq '.files[].path' 2>>"$SUPERVISOR_LOG"); then | ||
| log_warn "Failed to fetch PR files for $task_id (#$pr_number)" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
Enforce the “PR must be MERGED” hard gate before AI verification.
The AI path can return verified=true even when the PR isn’t merged, bypassing a deterministic safety check and risking premature TODO completion.
✅ Deterministic guard before AI judgment
if ! pr_state=$(gh pr view "$pr_number" --repo "$repo_slug" --json state --jq '.state' 2>>"$SUPERVISOR_LOG"); then
log_warn "Failed to fetch PR state for $task_id (#$pr_number)"
return 1
fi
+
+if [[ "$pr_state" != "MERGED" ]]; then
+ log_warn "PR #$pr_number for $task_id is not merged (state: ${pr_state:-unknown})"
+ return 1
+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.
| # GATHER: Fetch PR data | |
| local pr_state | |
| if ! pr_state=$(gh pr view "$pr_number" --repo "$repo_slug" --json state --jq '.state' 2>>"$SUPERVISOR_LOG"); then | |
| log_warn "Failed to fetch PR state for $task_id (#$pr_number)" | |
| return 1 | |
| fi | |
| local changed_files | |
| if ! changed_files=$(gh pr view "$pr_number" --repo "$repo_slug" --json files --jq '.files[].path' 2>>"$SUPERVISOR_LOG"); then | |
| log_warn "Failed to fetch PR files for $task_id (#$pr_number)" | |
| return 1 | |
| fi | |
| # GATHER: Fetch PR data | |
| local pr_state | |
| if ! pr_state=$(gh pr view "$pr_number" --repo "$repo_slug" --json state --jq '.state' 2>>"$SUPERVISOR_LOG"); then | |
| log_warn "Failed to fetch PR state for $task_id (#$pr_number)" | |
| return 1 | |
| fi | |
| if [[ "$pr_state" != "MERGED" ]]; then | |
| log_warn "PR #$pr_number for $task_id is not merged (state: ${pr_state:-unknown})" | |
| return 1 | |
| fi | |
| local changed_files | |
| if ! changed_files=$(gh pr view "$pr_number" --repo "$repo_slug" --json files --jq '.files[].path' 2>>"$SUPERVISOR_LOG"); then | |
| log_warn "Failed to fetch PR files for $task_id (#$pr_number)" | |
| return 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/scripts/supervisor/ai-deploy-decisions.sh around lines 625 - 636,
After fetching pr_state, enforce a hard gate that the PR must be merged before
any AI verification path can set verified=true: check the pr_state variable
(from the gh pr view call) and if it is not "MERGED", log a warning (use
log_warn and SUPERVISOR_LOG context) and either return non‑zero or explicitly
set verified=false and skip the AI verification flow; place this check
immediately after the pr_state retrieval and before any code that invokes the AI
verification or sets the verified flag so the AI path cannot mark a non‑merged
PR as verified.
1 participant
WIP - incremental commits. Migrates deploy.sh decision logic to AI judgment.
Summary
check_pr_status()state parsing with AI classificationtriage_review_feedback()keyword heuristics with AI semantic understandingverify_task_deliverables()file-counting with AI contextual judgmentRef #2210
Summary by CodeRabbit
New Features
Chores