fix: prevent false task completion cascade (t163)#622
Conversation
Add 4 guards to prevent tasks being marked [x] without real deliverables: 1. AGENTS.md: Task completion rules - NEVER mark [x] without merged PR 2. issue-sync-helper.sh: cmd_close() requires merged PR or verified: field before closing GitHub issues (--force flag for override) 3. pre-commit-hook.sh: Warns when TODO.md tasks go [ ] -> [x] without evidence of a merged PR or verified: field 4. supervisor-helper.sh: verify_task_deliverables() checks PR is merged with substantive file changes before update_todo_on_complete() marks [x] Root cause: supervisor marked tasks 'deployed' in DB, then update_todo_on_complete() blindly marked [x] in TODO.md, which triggered issue-sync GHA to auto-close 38 GitHub issues without real work done.
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 a robust, multi-layered defense system to prevent the erroneous automatic closure of GitHub issues due to prematurely marked tasks in 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
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds multi-layer verification for marking tasks complete: documents rules in AGENTS.md, validates TODO.md [x] transitions in the pre-commit hook, prevents issue-closing when completion evidence is missing (unless forced), and requires supervisor verification of PR deliverables before updating TODO.md completions. Changes
Sequence Diagram(s)sequenceDiagram
actor Developer as Developer
participant GitHook as Pre-commit Hook\n(validate_todo_completions)
participant TODO as TODO.md
participant IssueSync as Issue Sync\n(issue-sync-helper.sh)
participant Supervisor as Supervisor\n(supervisor-helper.sh)
participant GH as GitHub API
Developer->>GitHook: git commit (marks task [x])
GitHook->>TODO: Inspect staged TODO.md diffs
GitHook->>GH: Search for merged PR or verified date
alt Evidence found
GH-->>GitHook: PR merged / verified ✓
GitHook-->>Developer: Validation passed
else No evidence
GitHook-->>Developer: ⚠️ Warning: missing verified: or merged PR
end
Developer->>IssueSync: run issue-sync (cmd_close)
IssueSync->>TODO: Read task completion states
IssueSync->>IssueSync: task_has_completion_evidence()
alt Evidence verified
IssueSync->>GH: Close associated issue
GH-->>IssueSync: Issue closed ✓
IssueSync-->>Developer: Close complete: X closed
else No evidence
IssueSync-->>Developer: ⚠️ Skipped (no evidence) — use --force to override
end
Note over Supervisor,GH: After PR merge
Supervisor->>GH: gh pr view / gh pr view --json files
alt PR merged & substantive changes
GH-->>Supervisor: PR merged, files changed ✓
Supervisor->>TODO: update_todo_on_complete (mark [x])
Supervisor-->>Developer: Deliverable verified
else Missing substantive changes
GH-->>Supervisor: Only TODO/workflows or not merged ✗
Supervisor-->>Developer: ⚠️ Manual verification needed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 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: Sun Feb 8 14:41:57 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
![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 several layers of protection to prevent tasks from being marked as complete without sufficient evidence, which was causing a cascade of incorrect issue closures. The changes include new documentation rules, guards in the issue-sync and supervisor scripts, and a pre-commit hook to warn developers.
My review focuses on improving the robustness and consistency of these new guards. I've identified a potential bug in how task notes are parsed, an inconsistency in the evidence-checking logic between two scripts, and a minor code clarity issue with an unused parameter. The proposed suggestions aim to make these new checks more reliable.
.agents/scripts/issue-sync-helper.sh
Outdated
| task_with_notes="$task_line" | ||
| local notes_line | ||
| notes_line=$(grep -A1 "^- \[x\] ${task_id} " "$todo_file" | tail -1 || echo "") | ||
| if [[ "$notes_line" =~ ^[[:space:]]+- ]]; then |
There was a problem hiding this comment.
The regex ^[[:space:]]+- is too broad and can incorrectly match a subtask as a note line. This could lead to incorrect validation if a subtask's description contains words like "merged". The logic should be more specific to avoid matching subtasks.
Additionally, this logic only checks the single line following the task. If evidence for completion exists on a second or later line of notes, it will be missed. A more robust solution would be to parse all note lines for a given task.
| if [[ "$notes_line" =~ ^[[:space:]]+- ]]; then | |
| if [[ "$notes_line" =~ ^[[:space:]]+- && ! "$notes_line" =~ " t[0-9]+\." ]]; then |
| if echo "$line" | grep -qE 'verified:[0-9]{4}-[0-9]{2}-[0-9]{2}'; then | ||
| has_evidence=true | ||
| fi | ||
|
|
There was a problem hiding this comment.
The evidence checking logic here is inconsistent with the task_has_completion_evidence function in issue-sync-helper.sh. This function is missing the check for a merged PR reference in the task line itself (e.g., PR #123 merged).
This could lead to false warnings from the pre-commit hook if the only evidence is text in the task line. To maintain consistency, the check should be added here as well.
| if [[ "$has_evidence" == "false" ]] && echo "$line" | grep -qiE 'PR #[0-9]+ merged|PR.*merged'; then | |
| has_evidence=true | |
| fi | |
![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
🤖 Fix all issues with AI agents
In @.agents/scripts/supervisor-helper.sh:
- Around line 5261-5266: The substantive-file filtering currently uses grep -vE
with pattern '^(TODO\.md|todo/|\.github/workflows/)$' which only matches exact
names and misses nested paths; update the regex used when setting
substantive_files to exclude any file paths that begin with those
planning/workflow prefixes (e.g., match TODO.md exactly or any path starting
with todo/ or .github/workflows/), so modify the pattern in the
substantive_files assignment (the grep -vE call that reads from changed_files)
to anchor the start and allow prefixes rather than requiring the entire path to
match.
In `@TODO.md`:
- Around line 310-317: The entries for tasks t013, t016, t017 (and t031) in
TODO.md have completed: timestamps but lack the verified: field, which will
trigger warnings from the validate_todo_completions() hook; update each task
block (t013, t016, t017, t031) to include a verified: YYYY-MM-DD (or appropriate
verifier signature) line or modify the validate_todo_completions() logic to
accept notes-as-evidence (e.g., allowlist these IDs) — locate the task lines in
TODO.md and add the verified field for consistency or change
validate_todo_completions() to treat Notes containing deliverables as satisfied.
🧹 Nitpick comments (5)
.agents/AGENTS.md (1)
107-113: Consider moving detailed bullets to a subdoc if the root instruction budget is exceeded.These additions are important, but root AGENTS.md is constrained to ~50–100 instructions with >80% task relevance; if this pushes the budget, prefer a short summary here with a pointer to a subdoc for full detail.
As per coding guidelines: “Organize agent documentation with instructions budget of ~50-100 max in root AGENTS.md, using progressive disclosure with pointers to subagents” and “Keep every instruction in root AGENTS.md relevant to >80% of tasks.”.agents/scripts/pre-commit-hook.sh (2)
164-226:violationscounter is dead code — never incremented, always returns 0.
violationsis initialized at line 167 but never modified. Warnings go towarn_countinstead. Since this is intentionally warning-only (line 222), thereturn $violationson line 225 is misleading. Either removeviolationsor justreturn 0explicitly.♻️ Simplify return
- local violations=0 - # Only check if TODO.md is staged ... - return $violations + return 0
176-182: Only top-level task completions are detected; subtask[ ] → [x]transitions are missed.The pattern
^\+- \[x\] t[0-9]+requires no leading whitespace, so indented subtasks like- [x] t163.1in the diff won't match. If subtask completion validation is desired, broaden the regex:- newly_completed=$(git diff --cached -U0 TODO.md | grep -E '^\+- \[x\] t[0-9]+' | sed 's/^\+//' || true) + newly_completed=$(git diff --cached -U0 TODO.md | grep -E '^\+\s*- \[x\] t[0-9]+' | sed 's/^\+//' || true)If this is intentional (subtasks inherit evidence from parent), a brief comment would help future readers.
.agents/scripts/issue-sync-helper.sh (2)
1037-1055: Notes extraction only captures one line after the task (grep -A1).Tasks can have multiple indented lines (subtasks, multiple Notes). If "PR
#NNNmerged" appears on a deeper Notes line, it won't be captured. Consider usingextract_task_block(already defined in this script) for a more complete check:- local task_with_notes - task_with_notes="$task_line" - local notes_line - notes_line=$(grep -A1 "^- \[x\] ${task_id} " "$todo_file" | tail -1 || echo "") - if [[ "$notes_line" =~ ^[[:space:]]+- ]]; then - task_with_notes="$task_line $notes_line" - fi + local task_with_notes + task_with_notes=$(extract_task_block "$task_id" "$todo_file")This would search the entire task block (including all Notes and subtask lines) for evidence, making the check more robust. The function already exists at line 153.
1275-1278:FORCE_CLOSEvariable not initialized with other config vars at the top of the file.
VERBOSE,DRY_RUN, andREPO_SLUGare initialized in the Configuration section (lines 36-38).FORCE_CLOSEshould follow the same pattern for consistency and to allow env-var override without--force:VERBOSE="${VERBOSE:-false}" DRY_RUN="${DRY_RUN:-false}" REPO_SLUG="" +FORCE_CLOSE="${FORCE_CLOSE:-false}"This also makes
local force="${FORCE_CLOSE:-false}"on line 1002 redundant — it could just use$FORCE_CLOSEdirectly.
| # Check PR has substantive file changes (not just TODO.md or planning files) | ||
| local changed_files | ||
| changed_files=$(gh pr view "$pr_number" --repo "$repo_slug" --json files --jq '.files[].path' 2>/dev/null || echo "") | ||
| local substantive_files | ||
| substantive_files=$(echo "$changed_files" | grep -vE '^(TODO\.md|todo/|\.github/workflows/)$' || true) | ||
|
|
There was a problem hiding this comment.
Fix substantive-file filtering — current regex misses nested paths.
grep -vE '^(TODO\.md|todo/|\.github/workflows/)$' only matches the exact directory names, so files like .github/workflows/ci.yml or todo/notes.md are treated as substantive. That can falsely validate a task with only planning/workflow changes.
🛠️ Suggested fix
- substantive_files=$(echo "$changed_files" | grep -vE '^(TODO\.md|todo/|\.github/workflows/)$' || true)
+ substantive_files=$(echo "$changed_files" | grep -vE '^(TODO\.md|todo/|\.github/workflows/)' || true)🤖 Prompt for AI Agents
In @.agents/scripts/supervisor-helper.sh around lines 5261 - 5266, The
substantive-file filtering currently uses grep -vE with pattern
'^(TODO\.md|todo/|\.github/workflows/)$' which only matches exact names and
misses nested paths; update the regex used when setting substantive_files to
exclude any file paths that begin with those planning/workflow prefixes (e.g.,
match TODO.md exactly or any path starting with todo/ or .github/workflows/), so
modify the pattern in the substantive_files assignment (the grep -vE call that
reads from changed_files) to anchor the start and allow prefixes rather than
requiring the entire path to match.
| - [x] t013 Image SEO Enhancement with AI Vision #plan → [todo/PLANS.md#image-seo-enhancement-with-ai-vision] ~45m (ai:25m test:10m read:10m) ref:GH#507 logged:2025-12-21 completed:2026-02-08 | ||
| - Notes: Created 3 subagents: seo/moondream.md (Moondream 3 vision API - caption, query, detect, point, segment with SEO-specific prompts), seo/image-seo.md (orchestrator for alt text, filename, tag generation with WCAG guidelines and WordPress integration), seo/upscale.md (Real-ESRGAN local, Replicate cloud, Cloudflare CDN, Sharp format conversion). Updated seo.md with subagent references and Image SEO workflow section. | ||
| - [x] t014 Document RapidFuzz library for fuzzy string matching #tools #context ~5m (ai:4m read:1m) logged:2025-12-21 completed:2026-02-07 | ||
| - Notes: tools/context/rapidfuzz.md created (122 lines). Covers core functions, process module, distance functions, performance tips, and aidevops integration patterns. | ||
| - [x] t015 Add MinerU subagent as alternative to Pandoc for PDF conversion #tools #conversion ~15m actual:15m (ai:10m read:5m) logged:2025-12-21 completed:2026-02-06 | ||
| - [ ] t016 Uncloud Integration for aidevops #plan → [todo/PLANS.md#uncloud-integration-for-aidevops] ~45m (ai:25m test:10m read:10m) ref:GH#508 logged:2025-12-21 | ||
| - [x] t016 Uncloud Integration for aidevops #plan → [todo/PLANS.md#uncloud-integration-for-aidevops] ~45m (ai:25m test:10m read:10m) ref:GH#508 logged:2025-12-21 completed:2026-02-08 | ||
| - Notes: Created uncloud.md subagent (deployment provider guide with Quick Reference, CLI examples, Compose format, architecture diagram), uncloud-helper.sh (CLI wrapper with 20 commands, zero ShellCheck violations), uncloud-config.json.txt (cluster config template). Updated subagent-index.toon. Uncloud v0.16.0, 4.6k stars, Apache-2.0, active development. | ||
| - [ ] t017 SEO Machine Integration for aidevops #plan → [todo/PLANS.md#seo-machine-integration-for-aidevops] ~1.5h (ai:45m test:30m read:15m) ref:GH#509 logged:2025-12-21 | ||
| - [x] t017 SEO Machine Integration for aidevops #plan → [todo/PLANS.md#seo-machine-integration-for-aidevops] ~1.5h (ai:45m test:30m read:15m) ref:GH#509 logged:2025-12-21 completed:2026-02-08 |
There was a problem hiding this comment.
Newly completed tasks (t013, t016, t017, t031) have completed: timestamps but no verified: field.
These will trigger warnings from the new validate_todo_completions() hook. Since the hook is warning-only and these tasks have Notes with deliverable evidence, this is low risk — but worth noting for consistency with the new rules.
🤖 Prompt for AI Agents
In `@TODO.md` around lines 310 - 317, The entries for tasks t013, t016, t017 (and
t031) in TODO.md have completed: timestamps but lack the verified: field, which
will trigger warnings from the validate_todo_completions() hook; update each
task block (t013, t016, t017, t031) to include a verified: YYYY-MM-DD (or
appropriate verifier signature) line or modify the validate_todo_completions()
logic to accept notes-as-evidence (e.g., allowlist these IDs) — locate the task
lines in TODO.md and add the verified field for consistency or change
validate_todo_completions() to treat Notes containing deliverables as satisfied.
…tion - supervisor: fix grep regex so nested paths (todo/notes.md) are excluded - pre-commit: remove dead violations var, add PR merged text check, clarify subtask inheritance - issue-sync: init FORCE_CLOSE config var, use extract_task_block for notes, remove dead local var
🔍 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: Sun Feb 8 14:49:52 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.agents/scripts/supervisor-helper.sh:
- Around line 5221-5276: Summary: gh CLI errors are being suppressed in
verify_task_deliverables, hiding auth/API failures and causing misleading "not
merged" or "no substantive files" results; surface and log these errors and fail
fast. Update the verify_task_deliverables function to stop discarding stderr on
gh invocations (the gh pr view calls used to fetch state and files), instead
append stderr to SUPERVISOR_LOG (use 2>>"$SUPERVISOR_LOG"), and check each gh
command's exit status: if gh returns non-zero, log an error via log_error (or
log_warn) with context (task_id, pr_number) and return 1 immediately; mirror the
pre-flight gh availability/auth pattern used elsewhere in the script. Ensure
both the state fetch and files fetch follow this pattern so authentication/API
failures are recorded and cause the function to fail rather than silently
treating the PR as unmerged or empty.
- Add pre-flight gh availability and auth checks - Log gh errors to SUPERVISOR_LOG instead of suppressing with 2>/dev/null - Fail fast on API errors instead of silent fallthrough
🔍 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: Sun Feb 8 15:00:55 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
1 participant
Summary
Adds 4 layers of protection against false task completions that cascade into auto-closed GitHub issues:
[x]without a merged PR containing real deliverablescmd_close()now requires a merged PR orverified:YYYY-MM-DDfield before closing issues.--forceflag available for override[ ]->[x]without evidence of a merged PRverify_task_deliverables()checks PR is merged with substantive file changes beforeupdate_todo_on_complete()marks[x]Root Cause
The supervisor marked tasks "deployed" in its DB after running the post-PR lifecycle, then
update_todo_on_complete()blindly marked[x]in TODO.md. Theissue-syncGitHub Action then auto-closed 38 issues. Many of those tasks hadno_prortask_onlystatus — meaning no real work was done.Changes
.agents/AGENTS.md.agents/scripts/issue-sync-helper.shtask_has_completion_evidence()+ guard incmd_close()+--forceflag.agents/scripts/pre-commit-hook.shvalidate_todo_completions()check for[ ]->[x]transitions.agents/scripts/supervisor-helper.shverify_task_deliverables()+ guard inupdate_todo_on_complete()Testing
shellcheck -S errorwith zero violationsbash -nsyntax check*-diag-*) are exempted from verification (they fix process, not deliverables)Closes #618
Summary by CodeRabbit
Bug Fixes
New Features
Chores