t3667: refactor supervisor cron.sh — extract _should_skip_task helper and fix parent_ids extraction#4653
t3667: refactor supervisor cron.sh — extract _should_skip_task helper and fix parent_ids extraction#4653marcusquinn wants to merge 1 commit intomainfrom
Conversation
…_ids extraction t3667: Address medium review feedback from PR #1616 (Gemini + CodeRabbit). Two changes: 1. Fix parent_ids extraction bug in Strategy 4 — the previous pipeline used 'grep -oE t[0-9]+' which grabbed ALL task IDs in a line (including those embedded in descriptions like 'blocked-by:t1234'). Replace with sed to extract only the leading task ID after the checklist prefix. 2. Extract _should_skip_task() helper — the guard sequence (assignee/started check → blocked-by check → -needed blocker check → DB lookup → cross-repo guard → merged-PR check) was duplicated across Strategies 1, 2, and 4 (~60 lines of copy-paste). Centralise into a single helper with a check_needed_blockers flag (true for Strategies 1 & 2, false for Strategy 4 subtasks). Strategy 3 retains its own guards due to different logic flow. Closes #3667
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
PR is CONFLICTING after recent merges. Closing — issue #3667 will be relabeled status:available for re-dispatch with a fresh branch. |
Summary
Addresses medium review feedback from PR #1616 (Gemini Code Assist + CodeRabbit).
Changes
1. Fix
parent_idsextraction bug in Strategy 4 (cron.sh:821-822)The previous pipeline:
extracted all
tNNNtokens from each matching line — including IDs embedded in descriptions (e.g.blocked-by:t1234,see t999). This could cause non-parent task IDs to be treated as parents, leading to spurious subtask lookups.Fixed by replacing the second
grep -oEwith a targetedsedthat extracts only the leading task ID after the checklist prefix:2. Extract
_should_skip_task()helper (cron.sh:482-537)The guard sequence (assignee/started → blocked-by → -needed blockers → DB lookup → cross-repo guard → merged-PR check) was copy-pasted across Strategies 1, 2, and 4 (~60 lines of duplication). Centralised into a single
_should_skip_task()helper with acheck_needed_blockersflag:"true"— Strategies 1 & 2 (top-level tasks, check-neededblocker tags)"false"— Strategy 4 subtasks (parent already passed that gate)Strategy 3 retains its own guards due to its different logic flow (subtask-existence check before any guard, no assignee/blocked-by checks).
Net: -50 lines (91 insertions, 141 deletions).
Verification
Closes #3667