t1263: Fix false-positive claim detection and add verified skip in auto-pickup#1979
Conversation
Three fixes to cmd_auto_pickup claim detection: 1. Tighten assignee:/started: regex to match metadata fields only, not description text. assignee: requires word chars after colon, started: requires ISO timestamp pattern. Previously, tasks with 'worker_never_started:no_sentinel' in descriptions were falsely skipped as 'already claimed'. 2. Add 'verified' to the silent-skip status list alongside 'complete' and 'cancelled'. Previously, verified tasks logged noisy 'already tracked' messages instead of being silently skipped. 3. Applied same fixes to all three grep locations (Strategy 1, Strategy 2, and subtask pickup).
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 addresses critical issues in the task auto-pickup system by refining how tasks are identified as claimed or in progress. It prevents tasks from being erroneously skipped due to keywords appearing in their descriptions and streamlines the process by silently ignoring tasks that have already been verified. The changes ensure that the auto-pickup mechanism accurately dispatches available tasks, improving overall system efficiency and preventing task queue stalls. 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
|
WalkthroughThis change refines the supervisor cron script's auto-pickup filtering logic by implementing stricter metadata-field pattern matching (replacing loose substring detection with specific regex patterns) and expanding terminal-status checks to recognize "verified" as a non-pickup state across multiple pickup strategies. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 19 22:28:03 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 effectively addresses a critical false-positive bug in task claim detection by tightening the grep regular expressions. The more specific patterns for assignee: and started: will prevent incorrect matches in task descriptions, as explained in the PR description. Adding verified to the list of skippable statuses is also a sensible improvement.
My review includes a few suggestions to further enhance the code by using bash's built-in features for better performance and readability. Specifically, I've recommended replacing echo | grep with the [[ ... =~ ... ]] construct for regex matching, and using case statements for multi-condition status checks.
| # Skip tasks with assignee: or started: metadata fields (t1062, t1263) | ||
| # Match actual metadata fields, not description text containing these words. | ||
| # assignee: must be followed by a username (word chars), started: by ISO timestamp. | ||
| if echo "$line" | grep -qE '(assignee:[a-zA-Z0-9_-]+|started:[0-9]{4}-[0-9]{2}-[0-9]{2}T)'; then |
There was a problem hiding this comment.
For performance and to reduce reliance on external processes, consider using bash's built-in regular expression matching instead of piping to grep. This avoids creating a subshell and invoking another process.
| if echo "$line" | grep -qE '(assignee:[a-zA-Z0-9_-]+|started:[0-9]{4}-[0-9]{2}-[0-9]{2}T)'; then | |
| if [[ "$line" =~ (assignee:[a-zA-Z0-9_-]+|started:[0-9]{4}-[0-9]{2}-[0-9]{2}T) ]]; then |
| if [[ "$existing" == "complete" || "$existing" == "cancelled" || "$existing" == "verified" ]]; then | ||
| continue | ||
| fi |
There was a problem hiding this comment.
To improve readability and make it easier to add more statuses in the future, consider using a case statement instead of a chain of || operators.
| if [[ "$existing" == "complete" || "$existing" == "cancelled" || "$existing" == "verified" ]]; then | |
| continue | |
| fi | |
| case "$existing" in | |
| complete|cancelled|verified) continue ;; | |
| esac |
| # Skip tasks with assignee: or started: metadata fields (t1062, t1263) | ||
| # Match actual metadata fields, not description text containing these words. | ||
| # assignee: must be followed by a username (word chars), started: by ISO timestamp. | ||
| if echo "$line" | grep -qE '(assignee:[a-zA-Z0-9_-]+|started:[0-9]{4}-[0-9]{2}-[0-9]{2}T)'; then |
There was a problem hiding this comment.
For performance and to reduce reliance on external processes, consider using bash's built-in regular expression matching instead of piping to grep. This avoids creating a subshell and invoking another process.
| if echo "$line" | grep -qE '(assignee:[a-zA-Z0-9_-]+|started:[0-9]{4}-[0-9]{2}-[0-9]{2}T)'; then | |
| if [[ "$line" =~ (assignee:[a-zA-Z0-9_-]+|started:[0-9]{4}-[0-9]{2}-[0-9]{2}T) ]]; then |
| if [[ "$existing" == "complete" || "$existing" == "cancelled" || "$existing" == "verified" ]]; then | ||
| continue | ||
| fi |
There was a problem hiding this comment.
To improve readability and make it easier to add more statuses in the future, consider using a case statement instead of a chain of || operators.
| if [[ "$existing" == "complete" || "$existing" == "cancelled" || "$existing" == "verified" ]]; then | |
| continue | |
| fi | |
| case "$existing" in | |
| complete|cancelled|verified) continue ;; | |
| esac |
| # Skip tasks with assignee: or started: fields | ||
| if echo "$sub_line" | grep -qE '(assignee:|started:)'; then | ||
| # Skip tasks with assignee: or started: metadata fields (t1263) | ||
| if echo "$sub_line" | grep -qE '(assignee:[a-zA-Z0-9_-]+|started:[0-9]{4}-[0-9]{2}-[0-9]{2}T)'; then |
There was a problem hiding this comment.
For performance and to reduce reliance on external processes, consider using bash's built-in regular expression matching instead of piping to grep. This avoids creating a subshell and invoking another process.
| if echo "$sub_line" | grep -qE '(assignee:[a-zA-Z0-9_-]+|started:[0-9]{4}-[0-9]{2}-[0-9]{2}T)'; then | |
| if [[ "$sub_line" =~ (assignee:[a-zA-Z0-9_-]+|started:[0-9]{4}-[0-9]{2}-[0-9]{2}T) ]]; then |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agents/scripts/supervisor/cron.sh (1)
688-692:⚠️ Potential issue | 🟡 Minor
"verified"silent-skip not applied to Strategy 3 (plan tasks) or the Strategy 4 subtask block.Strategies 1 and 2 now silently skip
"verified"tasks, but the identical status guards in Strategy 3 (line 688) and the Strategy 4 subtask path (line 784) still only checkcomplete/cancelled. Averifiedtask encountered by either of those code paths will fall through to the noisylog_info "... already tracked (status: verified)"log line instead of silently continuing — the exact behavior the PR set out to suppress.The AI summary also lists "Strategy 3 (PLANS.md plan tasks)" as one of the updated paths, which is inconsistent with the code: no
~-annotated lines appear in that block.🐛 Proposed fix — apply to both gaps
Line 688 (Strategy 3):
- if [[ "$existing" == "complete" || "$existing" == "cancelled" ]]; then + if [[ "$existing" == "complete" || "$existing" == "cancelled" || "$existing" == "verified" ]]; thenLine 784 (Strategy 4 subtask):
- if [[ "$existing" == "complete" || "$existing" == "cancelled" ]]; then + if [[ "$existing" == "complete" || "$existing" == "cancelled" || "$existing" == "verified" ]]; thenAlso applies to: 784-789
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/supervisor/cron.sh around lines 688 - 692, In the Strategy 3 block that checks existing task status (the if that currently tests "$existing" == "complete" || "$existing" == "cancelled" near where task_id is used) and in the Strategy 4 subtask path that logs "already tracked (status: ...)", add "$existing" == "verified" to the guard so verified tasks are silently continued; update the conditional(s) that control the continue (and remove the noisy log_info call for verified) so both the plan-task (Strategy 3) branch and the subtask branch behave like Strategies 1/2 and skip verified without logging.
🧹 Nitpick comments (1)
.agents/scripts/supervisor/cron.sh (1)
543-548: Core false-positive fix is correct; consider adding a field-boundary anchor for full hardening.The described bug (
worker_never_started:no_sentinelmatching the oldstarted:substring grep) is definitively fixed by requiring an ISO timestamp suffix. Good change.However, a compound token like
not_started:2026-01-01T12:00:00Zorpre_assignee:marcuswould still match because neither pattern has a boundary check before the field name. Adding(^|[[:space:]])before each field name guards against this entire class without relying on\b(which has uneven support across BSD/GNU grep on macOS vs. Linux).🔧 Proposed hardening for all three regex call sites
-if echo "$line" | grep -qE '(assignee:[a-zA-Z0-9_-]+|started:[0-9]{4}-[0-9]{2}-[0-9]{2}T)'; then +if echo "$line" | grep -qE '(^|[[:space:]])(assignee:[a-zA-Z0-9_-]+|started:[0-9]{4}-[0-9]{2}-[0-9]{2}T)'; thenApply the same diff to lines 623 and 770.
Also applies to: 620-626, 769-773
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/supervisor/cron.sh around lines 543 - 548, The current grep pattern used to detect task metadata (the if echo "$line" | grep -qE '(assignee:[a-zA-Z0-9_-]+|started:[0-9]{4}-[0-9]{2}-[0-9]{2}T)') can false-positive on tokens like "pre_assignee:" or "not_started:"; update the regex to require a field boundary by prefixing each alternative with '(^|[[:space:]])' so it only matches standalone fields (i.e., change to something like '(^|[[:space:]])assignee:...|(^|[[:space:]])started:...'), and apply the same hardened pattern to the other two grep sites in this script where similar checks occur; keep the surrounding logic (log_info " $task_id: already claimed..." and continue) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.agents/scripts/supervisor/cron.sh:
- Around line 688-692: In the Strategy 3 block that checks existing task status
(the if that currently tests "$existing" == "complete" || "$existing" ==
"cancelled" near where task_id is used) and in the Strategy 4 subtask path that
logs "already tracked (status: ...)", add "$existing" == "verified" to the guard
so verified tasks are silently continued; update the conditional(s) that control
the continue (and remove the noisy log_info call for verified) so both the
plan-task (Strategy 3) branch and the subtask branch behave like Strategies 1/2
and skip verified without logging.
---
Nitpick comments:
In @.agents/scripts/supervisor/cron.sh:
- Around line 543-548: The current grep pattern used to detect task metadata
(the if echo "$line" | grep -qE
'(assignee:[a-zA-Z0-9_-]+|started:[0-9]{4}-[0-9]{2}-[0-9]{2}T)') can
false-positive on tokens like "pre_assignee:" or "not_started:"; update the
regex to require a field boundary by prefixing each alternative with
'(^|[[:space:]])' so it only matches standalone fields (i.e., change to
something like '(^|[[:space:]])assignee:...|(^|[[:space:]])started:...'), and
apply the same hardened pattern to the other two grep sites in this script where
similar checks occur; keep the surrounding logic (log_info " $task_id: already
claimed..." and continue) unchanged.
1 participant
Summary
grep -qE '(assignee:|started:)'matching description text instead of metadata fieldsverifiedto silent-skip status list (was onlycompleteandcancelled)Root Cause
Tasks like t1113 contain
worker_never_started:no_sentinelin their description. The old regex matchedstarted:in that text and skipped the task as "already claimed". Similarly, t1263's description mentionsassignee:andstarted:when explaining the stale-claim problem.This caused 4 genuinely dispatchable aidevops tasks to be permanently skipped by auto-pickup, stalling the queue for 2+ days.
Fix
Tightened regex patterns:
assignee:must be followed by word characters (a username):assignee:[a-zA-Z0-9_-]+started:must be followed by an ISO timestamp:started:[0-9]{4}-[0-9]{2}-[0-9]{2}TAlso added
verifiedto the silent-skip list so verified tasks don't log noisy "already tracked" messages.Testing
assignee:usernameandstarted:2026-...fields still correctly skippedBroader Context
This is the immediate regex fix. The broader self-healing improvement (AI-driven stale-claim detection, TODO/DB reconciliation) remains as the t1263 TODO entry for autonomous dispatch.
Closes #1978
Summary by CodeRabbit