t318.2: Verify supervisor worker PRs include task ID#1283
Conversation
- Fix run_pr_create_phase() PR title format: use "tNNN: description" instead of "feat: description (tNNN)" — matches MANDATORY PR title format - Add validate_pr_task_id() function to warn when PR title lacks task ID - Add warning when branch name has no task ID pattern (no-task-ID fallback) - Add "3b. PR title MUST contain task ID" to worker efficiency prompt in supervisor dispatch — reinforces existing instruction at step 2 - auto_create_pr_for_task() already uses correct format (no change needed) - validate_pr_belongs_to_task() already validates task ID (no change needed)
WalkthroughThe PR enhances PR management automation by introducing task-ID validation in PR titles through a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 17:30:33 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/full-loop-helper.sh:
- Around line 554-569: The pr_url variable is being assigned with stderr merged
into stdout via "gh pr create ... 2>&1", which can pollute pr_url with warnings
and break the PR number extraction that uses grep on pr_url; change the gh pr
create invocation so pr_url captures only stdout (no 2>&1), and redirect stderr
separately (e.g., to a log file or a separate variable) or discard it, then keep
the fallback logic that calls gh pr view into pr_url and the later grep -oE
'[0-9]+$' on pr_url unchanged; update any error logging to use the separate
stderr capture if you need to preserve warnings for debugging.
🧹 Nitpick comments (6)
.agents/scripts/full-loop-helper.sh (6)
174-191: Clean validation function — minor robustness nit withecho.If
pr_titleever starts with a dash (e.g.,-fix: something),echomay interpret it as a flag.printf '%s\n'is the standard portable-safe alternative.♻️ Suggested hardening
- if echo "$pr_title" | grep -qE '\bt[0-9]+(\.[0-9]+)*\b'; then + if printf '%s\n' "$pr_title" | grep -qE '\bt[0-9]+(\.[0-9]+)*\b'; then
435-435: Redundant stderr redirection.
&>/dev/nullalready redirects both stdout and stderr to/dev/null. The trailing2>&1is a no-op.♻️ Minor cleanup
- if gh auth status &>/dev/null 2>&1; then + if gh auth status &>/dev/null; then
158-169: Guard removal awk is coupled to the exact indentation of the heredoc.The
^fi$anchor only works because the innerfiis indented with spaces. If the guard format ever changes (e.g., tabs, or a refactor flattens the nesting), this removal logic silently leaves orphaned guard fragments in the hook file.Consider using a paired end-marker comment (e.g.,
# t173-headless-todo-guard-end) for unambiguous block removal withsed:♻️ Proposed marker-based approach
Add
# t173-headless-todo-guard-endafter the closingfiin both heredocs (install), then simplify removal:- awk '/# t173-headless-todo-guard/{skip=1} /^fi$/ && skip{skip=0; next} !skip' "$hook_file" >"$tmp_file" + sed '/# t173-headless-todo-guard/,/# t173-headless-todo-guard-end/d' "$hook_file" >"$tmp_file"This decouples removal logic from internal indentation details.
246-262:load_statesilently leaves globals empty when fields are missing from the state file.If a state file is written by an older version (or gets corrupted),
grepreturns empty and variables likeCURRENT_PHASEbecome empty strings. Thecmd_resumecase statement would then fall into*)and error — which is acceptable. ButSTARTED_AT,SAVED_PROMPT, etc. being silently empty could cause subtle bugs insave_statecalls that propagate empty values forward (e.g., line 574:save_state "$PHASE_PR_REVIEW" "$SAVED_PROMPT" "$pr_number" "$STARTED_AT").Consider adding a guard after parsing the required field:
🛡️ Proposed guard for required fields
CURRENT_PHASE=$(grep '^phase:' "$STATE_FILE" | cut -d: -f2 | tr -d ' "') + if [[ -z "$CURRENT_PHASE" ]]; then + print_error "Corrupt state file: missing 'phase' field" + return 1 + fi
516-519: SQL value derived from regex-constrained input — safe, but prefer parameterized queries as a habit.
$task_id_matchis safe here becausegrep -oE 't[0-9]+(\.[0-9]+)*'restricts it tot, digits, and dots. However, embedding shell variables directly into SQL strings is a code smell that can become a real vulnerability if the input source changes.If the supervisor DB helper supports parameterized queries or a safe quoting function, prefer that. Otherwise, this is acceptable as-is given the input constraint.
88-90: Predicate functions lack explicitreturnstatements.
is_headless,is_loop_active, andis_on_feature_branchrely on the implicit exit code of their last command. Per the project's coding guidelines, shell functions should include explicit returns. A simplereturn $?suffix or anif/then/returnpattern would satisfy this.As per coding guidelines, "Include explicit returns in shell scripts".
♻️ Example for is_headless
is_headless() { - [[ "$HEADLESS" == "true" ]] + if [[ "$HEADLESS" == "true" ]]; then + return 0 + fi + return 1 }Also applies to: 272-274, 306-310
| # Create PR with proper title and body | ||
| print_info "Creating pull request..." | ||
| local pr_url | ||
| pr_url=$(gh pr create --title "$pr_title" --body "$pr_body" 2>&1) || { | ||
| # PR might already exist | ||
| pr_url=$(gh pr view --json url --jq '.url' 2>/dev/null || echo "") | ||
| if [[ -z "$pr_url" ]]; then | ||
| print_error "Failed to create PR" | ||
| return 1 | ||
| fi | ||
| print_info "PR already exists: $pr_url" | ||
| } | ||
|
|
||
| # Extract PR number | ||
| local pr_number | ||
| pr_number=$(echo "$pr_url" | grep -oE '[0-9]+$' || gh pr view --json number --jq '.number') |
There was a problem hiding this comment.
2>&1 in gh pr create captures stderr warnings into pr_url, which may corrupt the URL.
If gh pr create emits any stderr warnings (e.g., deprecation notices), they get mixed into pr_url. The subsequent grep -oE '[0-9]+$' extraction on line 569 would then potentially match digits from warning text rather than the PR number.
Redirect stderr to the log or discard it separately so pr_url contains only the URL.
🐛 Proposed fix
- pr_url=$(gh pr create --title "$pr_title" --body "$pr_body" 2>&1) || {
+ pr_url=$(gh pr create --title "$pr_title" --body "$pr_body" 2>/dev/null) || {Or, if you want to preserve stderr for debugging:
- pr_url=$(gh pr create --title "$pr_title" --body "$pr_body" 2>&1) || {
+ pr_url=$(gh pr create --title "$pr_title" --body "$pr_body") || {📝 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.
| # Create PR with proper title and body | |
| print_info "Creating pull request..." | |
| local pr_url | |
| pr_url=$(gh pr create --title "$pr_title" --body "$pr_body" 2>&1) || { | |
| # PR might already exist | |
| pr_url=$(gh pr view --json url --jq '.url' 2>/dev/null || echo "") | |
| if [[ -z "$pr_url" ]]; then | |
| print_error "Failed to create PR" | |
| return 1 | |
| fi | |
| print_info "PR already exists: $pr_url" | |
| } | |
| # Extract PR number | |
| local pr_number | |
| pr_number=$(echo "$pr_url" | grep -oE '[0-9]+$' || gh pr view --json number --jq '.number') | |
| # Create PR with proper title and body | |
| print_info "Creating pull request..." | |
| local pr_url | |
| pr_url=$(gh pr create --title "$pr_title" --body "$pr_body" 2>/dev/null) || { | |
| # PR might already exist | |
| pr_url=$(gh pr view --json url --jq '.url' 2>/dev/null || echo "") | |
| if [[ -z "$pr_url" ]]; then | |
| print_error "Failed to create PR" | |
| return 1 | |
| fi | |
| print_info "PR already exists: $pr_url" | |
| } | |
| # Extract PR number | |
| local pr_number | |
| pr_number=$(echo "$pr_url" | grep -oE '[0-9]+$' || gh pr view --json number --jq '.number') |
🤖 Prompt for AI Agents
In @.agents/scripts/full-loop-helper.sh around lines 554 - 569, The pr_url
variable is being assigned with stderr merged into stdout via "gh pr create ...
2>&1", which can pollute pr_url with warnings and break the PR number extraction
that uses grep on pr_url; change the gh pr create invocation so pr_url captures
only stdout (no 2>&1), and redirect stderr separately (e.g., to a log file or a
separate variable) or discard it, then keep the fallback logic that calls gh pr
view into pr_url and the later grep -oE '[0-9]+$' on pr_url unchanged; update
any error logging to use the separate stderr capture if you need to preserve
warnings for debugging.
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 enhances the traceability and adherence to naming conventions for worker-generated Pull Requests. It standardizes the PR title format to ensure task IDs are always present, introduces a validation step to warn about missing task IDs before PR creation, and updates worker instructions to reinforce this mandatory requirement. The changes aim to improve auditability and consistency across the development workflow. 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 successfully refactors the PR title generation to prepend the task ID, which is a great improvement for auditability and traceability. The addition of the validate_pr_task_id function provides a helpful, non-blocking warning for PRs that might be missing this crucial information. The changes are well-implemented and align with the repository's conventions. I have one suggestion to improve the robustness of a script used for git hook cleanup.
| _save_cleanup_scope | ||
| trap '_run_cleanups' RETURN | ||
| push_cleanup "rm -f '${tmp_file}'" | ||
| awk '/# t173-headless-todo-guard/{skip=1} /^fi$/ && skip{skip=0; next} !skip' "$hook_file" >"$tmp_file" |
There was a problem hiding this comment.
The awk command used to remove the pre-commit hook guard is a bit brittle. It relies on matching a line containing only fi to determine the end of the block. If the guard's logic were ever extended to include a nested if...fi block where the inner fi is on its own line, this awk script would fail and only partially remove the guard.
A more robust approach would be to use explicit start and end markers. For example, in install_headless_todo_guard:
# START t173-headless-todo-guard
...
# END t173-headless-todo-guardThen you could safely remove the block with sed '/# START t173-headless-todo-guard/,/# END t173-headless-todo-guard/d', which would be more resilient to future changes.
1 participant
Summary
Audit and fix PR title generation to ensure all worker PRs include task IDs for audit traceability.
Changes
run_pr_create_phase()PR title format: Changed fromfeat: description (tNNN)totNNN: description— matches the MANDATORY PR title format documented in AGENTS.md and git-workflow.mdvalidate_pr_task_id()function: Pre-creation validation that warns when a PR title lacks a task ID pattern (tNNN)Audit Results
run_pr_create_phase()(full-loop)feat: desc (tNNN)— Fixed totNNN: descauto_create_pr_for_task()(supervisor)${task_id}: ${task_desc}gh pr createinstruction<task-id>: <description>validate_pr_belongs_to_task()Ref #1236
Summary by CodeRabbit
New Features
Documentation
Chores