t1319: Extract auto-pickup decision logic to AI module#2228
t1319: Extract auto-pickup decision logic to AI module#2228marcusquinn wants to merge 1 commit intomainfrom
Conversation
Migrates ~460 lines of inline dispatch gating, blocked-by chain checking, and task selection decisions from cmd_auto_pickup() in cron.sh to a new ai-pickup-decisions.sh module. Architecture: GATHER (shell) → JUDGE (AI) → RETURN (shell) - Shell gathers candidate tasks from TODO.md (4 strategies preserved) - Shell gathers DB state, blocker info, metadata - AI receives structured context and decides which tasks to pick up - Shell executes the pickup actions (cmd_add, cmd_batch, decomposition) - Falls back to deterministic logic if AI is unavailable What stays in shell: TODO.md parsing, DB queries, is_task_blocked(), _is_cross_repo_misregistration(), cmd_add/cmd_batch execution, dispatch_decomposition_worker(), scheduler install/uninstall. What moves to AI: dispatch gating decisions, priority/ordering, blocker tag interpretation, subtask inheritance decisions, batch strategy. Feature flag: AI_PICKUP_DECISIONS_ENABLED (default: true) Model: sonnet (fast, cheap enough for structured classification)
WalkthroughThis PR introduces an AI-driven auto-pickup decision pipeline by creating a new Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as Scheduler
participant cmd_auto_pickup as cmd_auto_pickup
participant ai_auto_pickup as ai_auto_pickup
participant Gatherer as GATHER Phase
participant Judge as JUDGE Phase
participant Executor as EXECUTE Phase
participant AI as AI Engine
participant DB as Database
participant Fallback as Deterministic<br/>Fallback
Scheduler->>cmd_auto_pickup: trigger auto-pickup
cmd_auto_pickup->>ai_auto_pickup: delegate with repo path
ai_auto_pickup->>Gatherer: scan TODO.md & strategies
Gatherer->>Gatherer: collect from auto-dispatch tags
Gatherer->>Gatherer: collect from dispatch queue
Gatherer->>Gatherer: collect from PLANS.md decomposition
Gatherer->>DB: fetch blocker info & state
Gatherer-->>ai_auto_pickup: return candidate JSON
ai_auto_pickup->>Judge: evaluate candidates
Judge->>AI: prompt AI with decision rules
alt AI Available & Parse Success
AI-->>Judge: return JSON decisions
Judge->>Judge: strip ANSI, extract array
else AI Unavailable or Parse Fails
Judge->>Fallback: recreate inline logic
Fallback->>DB: check assignee, blockers, status
Fallback-->>Judge: return deterministic decisions
end
Judge-->>ai_auto_pickup: finalized decisions
ai_auto_pickup->>Executor: apply decisions
Executor->>Executor: process pickup (inherit model if needed)
Executor->>Executor: process skip
Executor->>Executor: process decompose
Executor->>DB: update task state & log decisions
Executor-->>ai_auto_pickup: execution complete
ai_auto_pickup-->>cmd_auto_pickup: return status
cmd_auto_pickup-->>Scheduler: exit status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
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 significantly refactors the task auto-pickup mechanism by migrating its complex decision-making logic to a dedicated AI module. This change aims to enhance the flexibility and intelligence of task selection, allowing for more nuanced prioritization and gating, while ensuring system stability through a configurable AI enablement and a comprehensive deterministic fallback that mirrors the original behavior. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 successfully refactors the cmd_auto_pickup logic into a new, well-structured AI module, ai-pickup-decisions.sh. This significantly cleans up cron.sh and centralizes the decision-making process. The new module follows a clear GATHER -> JUDGE -> EXECUTE pattern with a deterministic fallback, which is a robust design. The code is generally well-documented. My main feedback concerns the repeated use of 2>/dev/null to suppress errors, which can hide important debugging information and violates established repository rules. I've provided specific suggestions to improve error handling and logging.
| ai_result=$(portable_timeout "$timeout_secs" opencode run \ | ||
| -m "$ai_model" \ | ||
| --format default \ | ||
| --title "pickup-${title_suffix}-$$" \ | ||
| "$prompt" 2>/dev/null || echo "") |
There was a problem hiding this comment.
Suppressing stderr with 2>/dev/null for the opencode command call hides critical debugging information. If the AI call fails due to authentication issues, network problems, or invalid arguments, the error messages are lost, making it difficult to diagnose problems. The script only checks for an empty result, which is not enough to understand the root cause of a failure. It's better to let stderr be visible for logging and debugging purposes, especially for external API calls. The same applies to the claude call on lines 129-132.
| ai_result=$(portable_timeout "$timeout_secs" opencode run \ | |
| -m "$ai_model" \ | |
| --format default \ | |
| --title "pickup-${title_suffix}-$$" \ | |
| "$prompt" 2>/dev/null || echo "") | |
| ai_result=$(portable_timeout "$timeout_secs" opencode run \ | |
| -m "$ai_model" \ | |
| --format default \ | |
| --title "pickup-${title_suffix}-$$" \ | |
| "$prompt" 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.
|
|
||
| # Portable timeout alias — uses portable_timeout from _common.sh when sourced, | ||
| # or defines a local fallback for standalone execution. | ||
| if ! declare -f portable_timeout &>/dev/null; then |
There was a problem hiding this comment.
The use of &>/dev/null to suppress output from declare -f is overly broad. The declare -f command only outputs to stdout, so redirecting stderr is unnecessary and can hide potential syntax errors in the command itself.
| if ! declare -f portable_timeout &>/dev/null; then | |
| if ! declare -f portable_timeout >/dev/null; then |
References
- In shell scripts, avoid blanket suppression of errors with '2>/dev/null'. For 'declare -f', redirecting only stdout ('>/dev/null') is sufficient as it does not output to stderr, allowing potential syntax errors to remain visible.
|
|
||
| # Try 1: Direct parse as array | ||
| local parsed | ||
| if parsed=$(printf '%s' "$response" | jq '.' 2>/dev/null) && [[ -n "$parsed" ]]; then |
There was a problem hiding this comment.
Suppressing stderr from jq with 2>/dev/null can hide important errors. If jq is not installed, or if there's a syntax error in the filter, the failure will be silent. While the if statement handles a failed parse, the reason for the failure is lost, making debugging harder. It's better to allow stderr to be visible for logging. This pattern is repeated throughout the script.
| if parsed=$(printf '%s' "$response" | jq '.' 2>/dev/null) && [[ -n "$parsed" ]]; then | |
| if parsed=$(printf '%s' "$response" | jq '.' 2>>"$SUPERVISOR_LOG") && [[ -n "$parsed" ]]; then |
References
- 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.
| local decision="$1" | ||
| local context="${2:-}" | ||
|
|
||
| mkdir -p "$AI_PICKUP_DECISIONS_LOG_DIR" 2>/dev/null || true |
There was a problem hiding this comment.
Suppressing stderr from mkdir with 2>/dev/null can mask underlying filesystem issues, such as permission errors. If the log directory cannot be created, it's better to see the error message to diagnose the problem.
| mkdir -p "$AI_PICKUP_DECISIONS_LOG_DIR" 2>/dev/null || true | |
| mkdir -p "$AI_PICKUP_DECISIONS_LOG_DIR" 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.
| auto_base_concurrency=2 | ||
| fi | ||
| local auto_batch_id | ||
| auto_batch_id=$(cmd_batch "$auto_batch_name" --concurrency "$auto_base_concurrency" --tasks "$task_csv" 2>/dev/null) |
There was a problem hiding this comment.
Suppressing stderr from cmd_batch with 2>/dev/null hides potential errors during batch creation. If cmd_batch fails, the script will silently continue without creating the batch, and the reason for the failure will be lost. It's better to log these errors for debugging.
| auto_batch_id=$(cmd_batch "$auto_batch_name" --concurrency "$auto_base_concurrency" --tasks "$task_csv" 2>/dev/null) | |
| auto_batch_id=$(cmd_batch "$auto_batch_name" --concurrency "$auto_base_concurrency" --tasks "$task_csv" 2>>"$SUPERVISOR_LOG") |
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.
![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
🤖 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-pickup-decisions.sh:
- Around line 689-752: In _execute_pickup_decisions validate each AI decision
against the provided candidates and re-apply hard gates before executing: for
each task_id (from decisions) ensure it exists in candidates (use jq to select
by .task_id) and if not, log_warn and skip; fetch the candidate entry and
enforce non-negotiable gates (reject if assignee is non-empty, merged == true,
db_status indicates closed, blockers present, or tags include "needed" or other
blocking tags) by turning the decision into skip and logging the gate that
blocked it; if a candidate contains tag "plan_decomposition" force decision to
"decompose"; validate inherit_model exists on the candidate before passing it to
cmd_add (otherwise ignore model_arg); only allow executing pickup/decompose/skip
after these checks and log when decisions are altered due to validation.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.agents/aidevops/supervisor-module-map.md.agents/scripts/supervisor-helper.sh.agents/scripts/supervisor/ai-pickup-decisions.sh.agents/scripts/supervisor/cron.sh
| _execute_pickup_decisions() { | ||
| local decisions="$1" | ||
| local candidates="$2" | ||
| local repo="$3" | ||
| local todo_file="$4" | ||
|
|
||
| local picked_up=0 | ||
| local decision_count | ||
| decision_count=$(printf '%s' "$decisions" | jq 'length' 2>/dev/null || echo "0") | ||
|
|
||
| local i=0 | ||
| while [[ "$i" -lt "$decision_count" ]]; do | ||
| local task_id decision reason inherit_model | ||
| task_id=$(printf '%s' "$decisions" | jq -r ".[$i].task_id" 2>/dev/null || echo "") | ||
| decision=$(printf '%s' "$decisions" | jq -r ".[$i].decision" 2>/dev/null || echo "") | ||
| reason=$(printf '%s' "$decisions" | jq -r ".[$i].reason // \"\"" 2>/dev/null || echo "") | ||
| inherit_model=$(printf '%s' "$decisions" | jq -r ".[$i].inherit_model // \"\"" 2>/dev/null || echo "") | ||
|
|
||
| i=$((i + 1)) | ||
|
|
||
| [[ -z "$task_id" || -z "$decision" ]] && continue | ||
|
|
||
| case "$decision" in | ||
| pickup) | ||
| local model_arg="" | ||
| if [[ -n "$inherit_model" && "$inherit_model" != "null" ]]; then | ||
| model_arg="--model $inherit_model" | ||
| log_info " $task_id: inheriting model:$inherit_model from parent" | ||
| fi | ||
|
|
||
| # shellcheck disable=SC2086 | ||
| if cmd_add "$task_id" --repo "$repo" $model_arg; then | ||
| picked_up=$((picked_up + 1)) | ||
| log_success " Auto-picked: $task_id ($reason)" | ||
| fi | ||
| ;; | ||
| decompose) | ||
| # Look up plan_anchor from candidates | ||
| local plan_anchor | ||
| plan_anchor=$(printf '%s' "$candidates" | jq -r \ | ||
| --arg id "$task_id" \ | ||
| '[.[] | select(.task_id == $id)] | .[0].plan_anchor // ""' 2>/dev/null || echo "") | ||
|
|
||
| if cmd_add "$task_id" --repo "$repo"; then | ||
| picked_up=$((picked_up + 1)) | ||
| log_success " Auto-picked: $task_id (#plan task for decomposition)" | ||
|
|
||
| if [[ -n "$plan_anchor" ]]; then | ||
| log_info " Dispatching decomposition worker for $task_id..." | ||
| dispatch_decomposition_worker "$task_id" "$plan_anchor" "$repo" | ||
| else | ||
| log_warn " $task_id: decompose decision but no plan_anchor found" | ||
| fi | ||
| fi | ||
| ;; | ||
| skip) | ||
| if [[ -n "$reason" && "$reason" != "null" ]]; then | ||
| log_info " $task_id: skipped — $reason" | ||
| fi | ||
| ;; | ||
| *) | ||
| log_warn " $task_id: unknown decision '$decision' — skipping" | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
Enforce hard gates + candidate validation before executing AI decisions.
AI output is untrusted; as written, a hallucinated task_id or a hard‑gate violation can be executed. Validate decisions against the candidate set and re‑apply non‑negotiable gates (assignee/merged/db_status/blockers/needed tags). Also force plan_decomposition → decompose for safety.
🔒️ Proposed safeguard for AI decisions
@@
- [[ -z "$task_id" || -z "$decision" ]] && continue
+ [[ -z "$task_id" || -z "$decision" ]] && continue
+
+ # Validate AI output against candidate set + hard gates
+ local candidate
+ candidate=$(printf '%s' "$candidates" | jq -r --arg id "$task_id" '.[] | select(.task_id == $id)' 2>/dev/null)
+ if [[ -z "$candidate" || "$candidate" == "null" ]]; then
+ log_warn " $task_id: AI returned unknown task_id — skipping"
+ continue
+ fi
+
+ local has_assignee has_merged_pr db_status unresolved_blockers needed_tags source
+ has_assignee=$(printf '%s' "$candidate" | jq -r '.has_assignee' 2>/dev/null || echo "")
+ has_merged_pr=$(printf '%s' "$candidate" | jq -r '.has_merged_pr' 2>/dev/null || echo "")
+ db_status=$(printf '%s' "$candidate" | jq -r '.db_status' 2>/dev/null || echo "")
+ unresolved_blockers=$(printf '%s' "$candidate" | jq -r '.unresolved_blockers' 2>/dev/null || echo "")
+ needed_tags=$(printf '%s' "$candidate" | jq -r '.needed_tags' 2>/dev/null || echo "")
+ source=$(printf '%s' "$candidate" | jq -r '.source' 2>/dev/null || echo "")
+
+ if [[ "$has_assignee" == "true" || "$has_merged_pr" == "true" || -n "$unresolved_blockers" || -n "$needed_tags" || ( -n "$db_status" && "$db_status" != "none" ) ]]; then
+ log_info " $task_id: hard gate matched — skipping AI decision"
+ continue
+ fi
+ if [[ "$source" == "plan_decomposition" && "$decision" != "decompose" ]]; then
+ log_info " $task_id: forcing decompose for plan task"
+ decision="decompose"
+ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/scripts/supervisor/ai-pickup-decisions.sh around lines 689 - 752, In
_execute_pickup_decisions validate each AI decision against the provided
candidates and re-apply hard gates before executing: for each task_id (from
decisions) ensure it exists in candidates (use jq to select by .task_id) and if
not, log_warn and skip; fetch the candidate entry and enforce non-negotiable
gates (reject if assignee is non-empty, merged == true, db_status indicates
closed, blockers present, or tags include "needed" or other blocking tags) by
turning the decision into skip and logging the gate that blocked it; if a
candidate contains tag "plan_decomposition" force decision to "decompose";
validate inherit_model exists on the candidate before passing it to cmd_add
(otherwise ignore model_arg); only allow executing pickup/decompose/skip after
these checks and log when decisions are altered due to validation.
|
Superseded — this work was completed manually in a different PR. |
1 participant
Summary
Migrates ~460 lines of inline dispatch gating, blocked-by chain checking, and task selection decisions from
cmd_auto_pickup()incron.shto a newai-pickup-decisions.shmodule.Architecture
GATHER (shell) → JUDGE (AI) → RETURN (shell) with deterministic fallback.
What moved to AI (
ai-pickup-decisions.sh):-neededtags block?)What stays in shell:
is_task_blocked()/_check_and_skip_if_blocked()— data queries_is_cross_repo_misregistration()— data querycmd_add/cmd_batchexecution — side effectsdispatch_decomposition_worker()— executioncmd_cron,cmd_watch) — unchangedFeature flag
AI_PICKUP_DECISIONS_ENABLED(default:true). Set tofalsefor deterministic-only mode.Model
sonnet— fast and cheap enough for structured classification tasks.Files Changed
.agents/scripts/supervisor/ai-pickup-decisions.sh.agents/scripts/supervisor/cron.shcmd_auto_pickup()reduced to thin wrapper.agents/scripts/supervisor-helper.sh.agents/aidevops/supervisor-module-map.mdVerification
.shfilesRef #2215
Summary by CodeRabbit
New Features
Refactor
Chores