GH#18539: fix review-followup findings in new-task-helper.sh#18725
GH#18539: fix review-followup findings in new-task-helper.sh#18725marcusquinn merged 1 commit intomainfrom
Conversation
Completion Summary
Why fresh PR instead of rebasing #18604PR #18604 was created before two substantial refactors landed on main: #18724 ( Bash regex fixWhile implementing, hit Bash Pitfall #46 — inline regex with backslash-brackets (
Risk: low (batch task creation helper, no critical paths, planning-only script)
Chose variable regexes over inline ones because inline |
|
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)
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 SonarCloud: 0 bugs, 0 vulnerabilities, 226 code smells Mon Apr 13 21:09:37 UTC 2026: Code review monitoring started 📈 Current Quality Metrics
Generated on: Mon Apr 13 21:09:40 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
Up to standards ✅🟢 Issues
|
|
Extends cmd_merge arg parser to accept --admin and --auto flags and pass them through to gh pr merge. Enables workers and interactive sessions to use gh's native branch-protection escape hatches without bypassing the sanctioned merge wrapper. Design: - Explicit --admin / --auto skip the error-retry fallback (intent is already set; retrying --admin when --admin was requested is a no-op, retrying --admin when --auto was requested would clobber queue intent). - No-flag path preserves the GH#18538 / PR #18748 / PR #18750 branch- protection error-retry fallback verbatim. - --auto success reports 'queued for auto-merge' (correct semantics: gh doesn't merge now, it queues when required checks pass). - Bash 3.2 safe: empty array expansion uses ${arr[@]+"${arr[@]}"} — plain "${arr[@]}" under set -u raises 'unbound variable' on macOS default bash. Verification: - shellcheck clean (SC1091 info only, pre-existing source directive). - bash -n syntax clean. - 21/21 runtime assertions pass in /tmp/test-cmd-merge-parsing.sh covering: * bash 3.2 empty-array crash regression * --admin, --auto, --admin --auto, --rebase --admin pass-through * --admin failure suppresses retry (no duplicate gh call) * no-flag branch-protection failure still triggers fallback (#18748/#18750) * unknown flag rejection - show_help usage line updated. Closes #18731. Context: #18725 (fix that surfaced this), t1382 review bot gate, GH#17541 sanctioned merge path, #18748 (--admin fallback), #18750 (set -e capture fix), #18732 (superseded stale PR on same branch).
…31) (#18757) Extends cmd_merge arg parser to accept --admin and --auto flags and pass them through to gh pr merge. Enables workers and interactive sessions to use gh's native branch-protection escape hatches without bypassing the sanctioned merge wrapper. Design: - Explicit --admin / --auto skip the error-retry fallback (intent is already set; retrying --admin when --admin was requested is a no-op, retrying --admin when --auto was requested would clobber queue intent). - No-flag path preserves the GH#18538 / PR #18748 / PR #18750 branch- protection error-retry fallback verbatim. - --auto success reports 'queued for auto-merge' (correct semantics: gh doesn't merge now, it queues when required checks pass). - Bash 3.2 safe: empty array expansion uses ${arr[@]+"${arr[@]}"} — plain "${arr[@]}" under set -u raises 'unbound variable' on macOS default bash. Verification: - shellcheck clean (SC1091 info only, pre-existing source directive). - bash -n syntax clean. - 21/21 runtime assertions pass in /tmp/test-cmd-merge-parsing.sh covering: * bash 3.2 empty-array crash regression * --admin, --auto, --admin --auto, --rebase --admin pass-through * --admin failure suppresses retry (no duplicate gh call) * no-flag branch-protection failure still triggers fallback (#18748/#18750) * unknown flag rejection - show_help usage line updated. Closes #18731. Context: #18725 (fix that surfaced this), t1382 review bot gate, GH#17541 sanctioned merge path, #18748 (--admin fallback), #18750 (set -e capture fix), #18732 (superseded stale PR on same branch).
1 participant
Summary
Addresses all 4 actionable review bot findings from PR #18416 on
.agents/scripts/new-task-helper.sh, re-applied against the post-refactor structure from #18724.HIGH — TODO.md insertion position:
_append_todo_entrywas appending to EOF with>>, landing new tasks after## Doneand TOON blocks. Now scans for## Ready(falling back to## Backlog), tracks the last- [ ]task line within that section, and inserts after it via awk. Empty-section, no-header (EOF fallback with warning), and Backlog-only cases are all handled.MEDIUM — Destructive
#stripping:_read_titles_streamused${line%%#*}which destroyed titles like "Fix bug feat(plan+): add granular bash permissions for file discovery #123" or "Add PR fix(worktree): prevent removal of unpushed branches and uncommitted changes #42 handler". Changed to skip only lines that START with#(after whitespace trim), preserving inline#references.MEDIUM — Redundant TODO.md check: The file existence check was inside
_append_todo_entry(called once per task). Moved tocmd_batch— validated once before the allocation loop starts._append_todo_entrynow assumes the caller validated (documented in the header comment).MEDIUM — Suppressed stderr:
2>/dev/nullonclaim-task-id.shhid auth failures, network errors, and lock contention. Removed so allocation diagnostics surface properly.Why fresh PR instead of rebasing #18604
PR #18604 was created before two substantial refactors landed on main: #18724 (
cmd_batchdecomposed into focused helpers under 100 lines) and #18607+#18610 (post-merge-review-scanner.shgained full inline bodies, file:line refs,[View inline comment]links, comprehensive Worker Guidance, andneeds-maintainer-reviewgating). The rebase conflicts were too intrusive to resolve cleanly, and the scanner changes in #18604 were obsolete relative to current main. Closed #18604 with explanation; this PR applies just the 4new-task-helper.shfixes to the current refactored structure, no obsolete scanner changes.Bash regex fix
While implementing, hit Bash Pitfall #46 — inline regex with backslash-brackets (
\\[[[:space:]]\\]) parses ambiguously against POSIX char classes. Moved all three regexes (header_re,next_header_re,task_re) into variables, per the standard workaround. Smoke-tested with 4 TODO.md layouts (populated Ready, Backlog fallback, empty section, no matching header).Files Changed
.agents/scripts/new-task-helper.sh
Runtime Testing
shellcheck .agents/scripts/new-task-helper.sh— cleanbash -nsyntax check — clean_append_todo_entry: Ready/Backlog/empty-section/EOF-fallback all pass_read_titles_stream: inline#123,#42,#issue-18539all preserved; full-line and indented#comments all skippedbatch --dry-runagainst a throwaway repo: allocates, prints summary, preserves inline#in titlesRisk: low (batch task creation helper, no critical paths, planning-only script)
Verification:
self-assessedResolves #18539
aidevops.sh v3.8.8 plugin for OpenCode v1.4.3 with claude-opus-4-6 spent 10m and 1,175 tokens on this as a headless worker.