t1001: Add DB-TODO reconciliation pulse phase to supervisor#1275
t1001: Add DB-TODO reconciliation pulse phase to supervisor#1275marcusquinn merged 1 commit intomainfrom
Conversation
Adds Phase 7b to the supervisor pulse cycle — bidirectional DB<->TODO.md reconciliation that fills gaps not covered by Phase 7: 1. DB failed/blocked tasks with no annotation in TODO.md — annotates them 2. Tasks marked [x] in TODO.md but DB still in non-terminal state — transitions DB to complete 3. DB orphans with no TODO.md entry — logs warnings for human review New function cmd_reconcile_db_todo() in todo-sync.sh, invocable as supervisor-helper.sh reconcile-db-todo [--batch id] [--dry-run]. Runs during pulse when no workers are active to avoid mid-flight interference.
WalkthroughThis PR introduces a bidirectional database-to-TODO.md reconciliation command ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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: Thu Feb 12 16:57:52 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: 1
🤖 Fix all issues with AI agents
In @.agents/scripts/supervisor/todo-sync.sh:
- Around line 1016-1026: Gap 2's SQL list of terminal states omits "merged" (and
possibly related states), causing tasks marked [x] in TODO.md and with status
"merged" to be pulled as non-terminal and regress; update the NOT IN clause used
to populate all_db_tasks so it includes 'merged' (and, after review, consider
adding 'deploying', 'verifying', 'verify_failed' if those should be terminal) to
match how cmd_reconcile_todo and cmd_update_todo treat merged as a completed
state.
🧹 Nitpick comments (1)
.agents/scripts/supervisor/todo-sync.sh (1)
987-987: Unescaped dots in task IDs used as regex patterns.Task IDs can contain dots (e.g.,
t123.4for subtasks). When used directly ingrep -Epatterns, the dot matches any character — sot123.4would also matcht12304ort123x4. This could cause false positives in all three gap checks.The existing
update_todo_on_blocked(line 614) andupdate_todo_on_complete(line 418) have the same pattern, so this is a pre-existing issue, but since this is a new function it's worth hardening.🛡️ Proposed fix — escape dots in task IDs before grep
Add near line 970 (before the gap checks begin):
+ # Escape dots in task IDs for safe use in grep -E patterns + _escape_tid() { printf '%s' "$1" | sed 's/\./\\./g'; } + # --- Gap 1: DB failed/blocked but TODO.md has no annotation ---Then use it in each grep, e.g.:
- line_num=$(grep -nE "^[[:space:]]*- \[ \] ${tid}( |$)" "$todo_file" | head -1 | cut -d: -f1) + local etid; etid=$(_escape_tid "$tid") + line_num=$(grep -nE "^[[:space:]]*- \[ \] ${etid}( |$)" "$todo_file" | head -1 | cut -d: -f1)Apply similarly at lines 1033 and 1069.
Also applies to: 1033-1033, 1069-1069
| # --- Gap 2: TODO.md [x] but DB still in non-terminal state --- | ||
| # Terminal states: complete, deployed, verified, failed, blocked, cancelled | ||
| # Non-terminal: queued, dispatched, running, evaluating, retrying, | ||
| # pr_review, review_triage, merging, merged, deploying, verifying, verify_failed | ||
| local all_db_tasks | ||
| all_db_tasks=$(db -separator '|' "$SUPERVISOR_DB" " | ||
| SELECT t.id, t.status FROM tasks t | ||
| WHERE t.status NOT IN ('complete', 'deployed', 'verified', 'failed', 'blocked', 'cancelled') | ||
| $batch_filter | ||
| ORDER BY t.id; | ||
| ") |
There was a problem hiding this comment.
merged (and possibly deploying, verifying) may be incorrectly classified as non-terminal.
cmd_reconcile_todo (line 738) and cmd_update_todo (line 684) both treat merged as a completed/terminal state. However, Gap 2's SQL excludes only complete, deployed, verified, failed, blocked, cancelled — meaning a task in merged state that's [x] in TODO.md would be transitioned back to complete, which is likely a state regression.
Consider adding merged (and review whether deploying, verifying, verify_failed should also be terminal) to the exclusion list:
🐛 Proposed fix
- WHERE t.status NOT IN ('complete', 'deployed', 'verified', 'failed', 'blocked', 'cancelled')
+ WHERE t.status NOT IN ('complete', 'deployed', 'verified', 'merged', 'failed', 'blocked', 'cancelled')📝 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.
| # --- Gap 2: TODO.md [x] but DB still in non-terminal state --- | |
| # Terminal states: complete, deployed, verified, failed, blocked, cancelled | |
| # Non-terminal: queued, dispatched, running, evaluating, retrying, | |
| # pr_review, review_triage, merging, merged, deploying, verifying, verify_failed | |
| local all_db_tasks | |
| all_db_tasks=$(db -separator '|' "$SUPERVISOR_DB" " | |
| SELECT t.id, t.status FROM tasks t | |
| WHERE t.status NOT IN ('complete', 'deployed', 'verified', 'failed', 'blocked', 'cancelled') | |
| $batch_filter | |
| ORDER BY t.id; | |
| ") | |
| # --- Gap 2: TODO.md [x] but DB still in non-terminal state --- | |
| # Terminal states: complete, deployed, verified, failed, blocked, cancelled | |
| # Non-terminal: queued, dispatched, running, evaluating, retrying, | |
| # pr_review, review_triage, merging, merged, deploying, verifying, verify_failed | |
| local all_db_tasks | |
| all_db_tasks=$(db -separator '|' "$SUPERVISOR_DB" " | |
| SELECT t.id, t.status FROM tasks t | |
| WHERE t.status NOT IN ('complete', 'deployed', 'verified', 'merged', 'failed', 'blocked', 'cancelled') | |
| $batch_filter | |
| ORDER BY t.id; | |
| ") |
🤖 Prompt for AI Agents
In @.agents/scripts/supervisor/todo-sync.sh around lines 1016 - 1026, Gap 2's
SQL list of terminal states omits "merged" (and possibly related states),
causing tasks marked [x] in TODO.md and with status "merged" to be pulled as
non-terminal and regress; update the NOT IN clause used to populate all_db_tasks
so it includes 'merged' (and, after review, consider adding 'deploying',
'verifying', 'verify_failed' if those should be terminal) to match how
cmd_reconcile_todo and cmd_update_todo treat merged as a completed state.
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 enhances the robustness and consistency of task management by introducing a new reconciliation phase. It ensures that the database and TODO.md file accurately reflect each other's state, addressing several scenarios where discrepancies could arise. This improves the reliability of task tracking and reduces manual intervention for common synchronization issues, making the system more resilient to out-of-sync states. 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 introduces a new supervisor pulse phase (7b) and the reconcile-db-todo command for bidirectional reconciliation between the database and TODO.md, aiming to synchronize tasks and address discrepancies. My review identified a potential SQL injection risk, a couple of shell style guide violations regarding error suppression, and an opportunity to optimize the database access pattern.
| if [[ -n "$batch_id" ]]; then | ||
| local escaped_batch | ||
| escaped_batch=$(sql_escape "$batch_id") | ||
| batch_filter="AND EXISTS (SELECT 1 FROM batch_tasks bt WHERE bt.task_id = t.id AND bt.batch_id = '$escaped_batch')" |
There was a problem hiding this comment.
Constructing SQL queries by embedding variables, even after escaping, can be vulnerable to SQL injection. This pattern is used in three separate database queries within this function. If the db helper function supports parameterized queries, that would be a more secure approach. If not, consider adding strict validation for batch_id to ensure it only contains expected characters before it's used in the query.
References
- The style guide recommends using parameterized queries where possible to prevent SQL injection vulnerabilities. (link)
|
|
||
| # Determine repo path | ||
| if [[ -z "$repo_path" ]]; then | ||
| repo_path=$(db "$SUPERVISOR_DB" "SELECT DISTINCT repo FROM tasks LIMIT 1;" 2>/dev/null || echo "") |
There was a problem hiding this comment.
This line uses 2>/dev/null to suppress stderr, which violates the repository style guide. While || echo "" handles a non-zero exit code, suppressing the error message hides potentially useful diagnostic information. The error output should be allowed to pass through or be redirected to a log file like $SUPERVISOR_LOG.
| repo_path=$(db "$SUPERVISOR_DB" "SELECT DISTINCT repo FROM tasks LIMIT 1;" 2>/dev/null || echo "") | |
| repo_path=$(db "$SUPERVISOR_DB" "SELECT DISTINCT repo FROM tasks LIMIT 1;" || echo "") |
References
- The style guide prohibits blanket stderr suppression with
2>/dev/null, allowing it only when redirecting to a log file. (link)
| # --- Gap 1: DB failed/blocked but TODO.md has no annotation --- | ||
| local failed_tasks | ||
| failed_tasks=$(db -separator '|' "$SUPERVISOR_DB" " | ||
| SELECT t.id, t.status, t.error FROM tasks t | ||
| WHERE t.status IN ('failed', 'blocked') | ||
| $batch_filter | ||
| ORDER BY t.id; | ||
| ") | ||
|
|
||
| if [[ -n "$failed_tasks" ]]; then | ||
| while IFS='|' read -r tid tstatus terror; do | ||
| [[ -z "$tid" ]] && continue | ||
|
|
||
| # Check if task is open in TODO.md with no Notes annotation | ||
| local line_num | ||
| line_num=$(grep -nE "^[[:space:]]*- \[ \] ${tid}( |$)" "$todo_file" | head -1 | cut -d: -f1) | ||
| [[ -z "$line_num" ]] && continue | ||
|
|
||
| # Check if a Notes line already exists below | ||
| local next_line_num=$((line_num + 1)) | ||
| local next_line | ||
| next_line=$(sed -n "${next_line_num}p" "$todo_file" 2>/dev/null || echo "") | ||
|
|
||
| if echo "$next_line" | grep -qE "^[[:space:]]*- Notes:"; then | ||
| # Notes already present — skip | ||
| continue | ||
| fi | ||
|
|
||
| issue_count=$((issue_count + 1)) | ||
|
|
||
| if [[ "$dry_run" == "true" ]]; then | ||
| log_warn "[dry-run] $tid: DB status=$tstatus but TODO.md has no annotation" | ||
| else | ||
| log_info "Phase 7b: Annotating $tid ($tstatus) in TODO.md" | ||
| local reason="${terror:-no error details}" | ||
| update_todo_on_blocked "$tid" "$reason" 2>>"${SUPERVISOR_LOG:-/dev/null}" || { | ||
| log_warn "Phase 7b: Failed to annotate $tid" | ||
| continue | ||
| } | ||
| fixed_count=$((fixed_count + 1)) | ||
| fi | ||
| done <<<"$failed_tasks" | ||
| fi | ||
|
|
||
| # --- Gap 2: TODO.md [x] but DB still in non-terminal state --- | ||
| # Terminal states: complete, deployed, verified, failed, blocked, cancelled | ||
| # Non-terminal: queued, dispatched, running, evaluating, retrying, | ||
| # pr_review, review_triage, merging, merged, deploying, verifying, verify_failed | ||
| local all_db_tasks | ||
| all_db_tasks=$(db -separator '|' "$SUPERVISOR_DB" " | ||
| SELECT t.id, t.status FROM tasks t | ||
| WHERE t.status NOT IN ('complete', 'deployed', 'verified', 'failed', 'blocked', 'cancelled') | ||
| $batch_filter | ||
| ORDER BY t.id; | ||
| ") | ||
|
|
||
| if [[ -n "$all_db_tasks" ]]; then | ||
| while IFS='|' read -r tid tstatus; do | ||
| [[ -z "$tid" ]] && continue | ||
|
|
||
| # Check if this task is marked [x] in TODO.md | ||
| if grep -qE "^[[:space:]]*- \[x\] ${tid}( |$)" "$todo_file"; then | ||
| issue_count=$((issue_count + 1)) | ||
|
|
||
| if [[ "$dry_run" == "true" ]]; then | ||
| log_warn "[dry-run] $tid: marked [x] in TODO.md but DB status=$tstatus" | ||
| else | ||
| log_info "Phase 7b: Transitioning $tid from $tstatus to complete (TODO.md shows [x])" | ||
| cmd_transition "$tid" "complete" \ | ||
| --reason "Reconciled: TODO.md marked [x] but DB was $tstatus (t1001)" \ | ||
| 2>>"${SUPERVISOR_LOG:-/dev/null}" || { | ||
| log_warn "Phase 7b: Failed to transition $tid to complete" | ||
| continue | ||
| } | ||
| fixed_count=$((fixed_count + 1)) | ||
| fi | ||
| fi | ||
| done <<<"$all_db_tasks" | ||
| fi | ||
|
|
||
| # --- Gap 3: DB orphans — tasks in DB with no TODO.md entry at all --- | ||
| local orphan_tasks | ||
| orphan_tasks=$(db -separator '|' "$SUPERVISOR_DB" " | ||
| SELECT t.id, t.status FROM tasks t | ||
| WHERE t.status NOT IN ('cancelled') | ||
| $batch_filter | ||
| ORDER BY t.id; | ||
| ") | ||
|
|
||
| local orphan_count=0 | ||
| local orphan_ids="" | ||
|
|
||
| if [[ -n "$orphan_tasks" ]]; then | ||
| while IFS='|' read -r tid tstatus; do | ||
| [[ -z "$tid" ]] && continue | ||
|
|
||
| # Check if task ID appears anywhere in TODO.md (open, closed, or in notes) | ||
| if ! grep -qE "(^|[[:space:]])${tid}([[:space:]]|$)" "$todo_file"; then | ||
| orphan_count=$((orphan_count + 1)) | ||
| orphan_ids="${orphan_ids}${orphan_ids:+, }${tid}(${tstatus})" | ||
| fi | ||
| done <<<"$orphan_tasks" | ||
| fi | ||
|
|
||
| if [[ "$orphan_count" -gt 0 ]]; then | ||
| issue_count=$((issue_count + orphan_count)) | ||
| log_warn "Phase 7b: $orphan_count DB orphan(s) with no TODO.md entry: $orphan_ids" | ||
| # Orphans are logged but not auto-fixed — they may be from other repos | ||
| # or manually managed tasks. The warning enables human review. | ||
| fi |
There was a problem hiding this comment.
This function executes three separate queries against the database to handle the three reconciliation gaps. This could be optimized by fetching all potentially relevant tasks in a single query and then processing them in memory within Bash. This would reduce the number of round-trips to the database and could improve performance, especially if the tasks table is large.
For example:
Fetch all non-cancelled tasks once
all_tasks=$(db -separator '|' "$SUPERVISOR_DB" "
SELECT t.id, t.status, t.error FROM tasks t
WHERE t.status NOT IN ('cancelled')
$batch_filter
ORDER BY t.id;
")
Then process all gaps in one or two passes in Bash
...
| # Check if a Notes line already exists below | ||
| local next_line_num=$((line_num + 1)) | ||
| local next_line | ||
| next_line=$(sed -n "${next_line_num}p" "$todo_file" 2>/dev/null || echo "") |
There was a problem hiding this comment.
This line uses 2>/dev/null to suppress stderr from sed, which violates the repository style guide. While || echo "" handles a non-zero exit code, suppressing the error message hides potentially useful diagnostic information, for example if sed fails for some reason.
| next_line=$(sed -n "${next_line_num}p" "$todo_file" 2>/dev/null || echo "") | |
| next_line=$(sed -n "${next_line_num}p" "$todo_file" || echo "") |
References
- The style guide prohibits blanket stderr suppression with
2>/dev/null, allowing it only when redirecting to a log file. (link)
1 participant
Summary
cmd_reconcile_db_todo()intodo-sync.sh, also available as CLI commandsupervisor-helper.sh reconcile-db-todo [--batch id] [--dry-run]Gaps Filled
failed/blockedbut TODO.md has no annotationupdate_todo_on_blocked()to add Notes line[x]but DB still in non-terminal state (queued/running/etc.)completewith reconciliation reasonDesign Decisions
total_running == 0 && total_queued == 0to avoid interfering with active workers (chose same guard as Phase 7 minus thetotal_complete > 0requirement since Gap 1 targets failed/blocked tasks)update_todo_on_blocked()andcmd_transition()rather than duplicating logicRef #1274
Summary by CodeRabbit
reconcile-db-todocommand for bidirectional database and TODO.md file synchronization.