Repository navigation
Conversation
…s are never retried — review-thread counterpart of #2017
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
This comment has been minimized.
This comment has been minimized.
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #2048 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but I can't mark it done yet: required checks |
|
No description provided. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| _bcr_n=$(scan_pr_for_undispositioned_bot_comments "$_OWNER_REPO" "$_bcr_pr") | ||
| echo "$_bcr_n" | ||
| if [ "${_bcr_n:-0}" = "0" ]; then |
There was a problem hiding this comment.
Suggestion: _bcr_n includes scanner diagnostics because of the outer 2>&1; it is not equal to 0, so thread retries are skipped.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Incorrect condition logic
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/review-one-pr.sh
**Line:** 509:511
**Comment:**
*Incorrect Condition Logic: `_bcr_n` includes scanner diagnostics because of the outer `2>&1`; it is not equal to `0`, so thread retries are skipped.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Skipped: false positive. _bcr_n=$(scan_pr_for_undispositioned_bot_comments ...) captures stdout only, and the function prints only the dispatch count there (diagnostics go to stderr). The outer 2>&1 applies to the subshell, not the command substitution.
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-03T15:08:45Z. |
| echo " [warn] bot-thread retry: could not render skip decisions for PR ${pr_number}" >&2 | ||
| fi | ||
|
|
||
| post_bot_thread_exhausted_notice "$repo" "$pr_number" "$decisions" |
There was a problem hiding this comment.
Suggestion: Concurrent scans call post_bot_thread_exhausted_notice before claiming a retry marker, so both can post duplicate visible exhaustion comments.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Race condition
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/dev-lead-retry.sh
**Line:** 876:876
**Comment:**
*Race Condition: Concurrent scans call `post_bot_thread_exhausted_notice` before claiming a retry marker, so both can post duplicate visible exhaustion comments.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Skipped: the exhaustion notice is a best-effort, informational comment whose hidden marker dedups later scans. A rare concurrent duplicate is harmless and the retry claim marker is not applicable (nothing is dispatched for exhausted threads).
![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 implements a mechanism to automatically retry lost "fix-reviews" passes for unreplied trusted-bot review threads, introducing paginated thread fetching and deduplicated retry logic. Feedback on the changes highlights an issue in the integration tests where guarding "grep -c" with "|| echo 0" causes duplicate output on no matches, and recommends using "|| true" instead. Additionally, for security-sensitive login comparisons in "jq" scripts, it is advised to use explicit string slicing instead of regex replacements and to pass variables as arguments rather than using direct string interpolation.
| local listing first_marker logins_jq pending | ||
| pending="${BOT_THREAD_RETRY_PENDING_SEC:-9000}" | ||
| [[ "$pending" =~ ^[0-9]+$ ]] || pending=9000 | ||
| logins_jq=$(jq -cn --arg a "$automation" '$a | split(",") | map(sub("\\[bot\\]$"; ""))') |
There was a problem hiding this comment.
For security-sensitive login comparisons in jq, prefer explicit string slicing (using endswith and string indexing) over regex replacements (such as sub) to ensure the code remains highly readable and easy to audit at a glance.
| logins_jq=$(jq -cn --arg a "$automation" '$a | split(",") | map(sub("\\[bot\\]$"; ""))') | |
| logins_jq=$(jq -cn --arg a "$automation" '$a | split(",") | map(if endswith("[bot]") then .[0:-5] else . end)') |
References
- For security-sensitive login comparisons in jq, prefer explicit string slicing (e.g., using endswith and string indexing) over regex replacements (e.g., sub) to ensure the code remains highly readable and easy to audit at a glance.
There was a problem hiding this comment.
Skipped: style-only. The sub("\\[bot\\]$"; "") normalization is covered by tests and equivalent to the suggested slicing.
| sleep "$BOT_COMMENT_RETRY_CLAIM_SETTLE_SEC" | ||
| fi | ||
| if ! listing=$(gh api --paginate "repos/${repo}/issues/${pr_number}/comments?per_page=100" \ | ||
| --jq '.[] | select((.user.login // "" | sub("\\[bot\\]$"; "")) as $l | '"${logins_jq}"' | index($l) != null) |
There was a problem hiding this comment.
For security-sensitive login comparisons in jq, prefer explicit string slicing (using endswith and string indexing) over regex replacements (such as sub) to ensure the code remains highly readable and easy to audit at a glance. Additionally, avoid direct string interpolation inside jq filter strings to prevent escaping issues or syntax errors; instead, pass variables as arguments (e.g., using --argjson) for safer variable handling.
| --jq '.[] | select((.user.login // "" | sub("\\[bot\\]$"; "")) as $l | '"${logins_jq}"' | index($l) != null) | |
| --argjson logins "${logins_jq}" --jq '.[] | select((.user.login // "" | if endswith("[bot]") then .[0:-5] else . end) as $l | $logins | index($l) != null)' |
References
- For security-sensitive login comparisons in jq, prefer explicit string slicing (e.g., using endswith and string indexing) over regex replacements (e.g., sub) to ensure the code remains highly readable and easy to audit at a glance.
- Avoid direct string interpolation inside jq filter strings to prevent escaping issues or syntax errors. Instead, pass variables as arguments using --arg for safer variable handling.
There was a problem hiding this comment.
Skipped: gh api --jq does not accept --argjson, so the suggestion would not run; the logins are produced by jq --arg and validated upstream.
|
Qodo trial-ended notice: acknowledged, no action needed in this PR. |
This comment has been minimized.
This comment has been minimized.
|
SonarCloud Quality Gate passed: informational, no action needed. |
Dev-Lead — fix-reviews (partial)A commit was pushed, but not every requested change was applied. Per requested item:
The unaddressed items above still need work. |
Superseded by automated re-review at
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-03T15:14:30Z. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0f4d65215
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _bcr_n=$(scan_pr_for_undispositioned_bot_comments "$_OWNER_REPO" "$_bcr_pr") | ||
| echo "$_bcr_n" | ||
| if [ "${_bcr_n:-0}" = "0" ]; then | ||
| scan_pr_for_unreplied_bot_threads "$_OWNER_REPO" "$_bcr_pr" | ||
| fi |
There was a problem hiding this comment.
This hook is nested inside the mc_gate_rc == 1 branch, so when an unreplied bot review thread is the only blocker and the PR has no undispositioned issue comment, the maintainer-comment gate succeeds and this scan is never called. dev-lead-resume.sh also invokes only scan_pr_for_rate_limits, leaving the new two-hour cron as the sole recovery path for the primary scenario; move the scan to a generally reachable review-event path so a dropped fix-reviews run is retried event-first rather than waiting on a best-effort timer.
AGENTS.md reference: AGENTS.md:L537-L546
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipped: the thread retry deliberately rides the existing maintainer-comment gate path of #2017 (shared claim marker with the comment retry); the cron covers PRs blocked only by review threads. Moving it to a separate review-event path is a design change for a follow-up.
| ${list} | ||
|
|
||
| $(btr_exhausted_marker "$ids")" | ||
| if ! gh api --method POST "repos/${repo}/issues/${pr_number}/comments" -f body="$body" >/dev/null 2>&1; then |
There was a problem hiding this comment.
When the cron and the pr-review hook scan an exhausted thread concurrently, both can read the same snapshot without an exhaustion marker and reach this unconditional POST. Exhausted scans create no retry claim marker, and the notice marker is only observed on a later scan, so the two callers post duplicate visible escalation comments despite the stated one-notice contract; re-read or claim the exhaustion marker before posting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipped: the exhaustion notice is informational and its hidden marker dedups later scans; a rare concurrent duplicate is harmless and nothing is dispatched on it.
Dev-Lead — fix-reviews (partial)A commit was pushed, but not every requested change was applied. Per requested item:
The unaddressed items above still need work. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Review — fix requested (cycle 2/3)The automated review identified the following issues. Please address each one: Findings to fixAutomated review — NEEDS HUMAN REVIEWRisk: MEDIUM SummaryBoth reviewers rate the PR MEDIUM risk and escalate, so they fully agree. Both found that CI is still queued or in progress at the head SHA and that bot review threads are unresolved. Both also flagged that fetch_open_review_threads is now unbounded, which makes the OPEN_THREADS_JSON env payload and prompt unbounded. The deep reviewer adds that this can exceed the 128KB per-env-var limit and abort the pass. The rubber duck uniquely raised the retry-until-cap cost for silently skipped threads, the '@mention dev-lead' text in the exhaustion notice, and the grep -c duplicate-0 check. Cross-engine agreementfull Findings
Reviewed by the PR-review cascade (triage: haiku 4.5 [sonnet 5.5, sonnet 5] → deep: opus 5.5 [opus 4.8, sonnet 5.5] + duck: gemini-3.8-flash [sonnet 5.5] → audit: opus 5.5 [opus 4.8, opus 4.7]). Reply if you need a human review. Additional tasks
The review cascade will automatically re-review after new commits are pushed. |
|
No description provided. |
|
No description provided. |
Dev-Lead — rate-limited (intent: fix-reviews)PR: #2048 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
CodeAnt PR Risk: Medium Risk
Assessed commit: |
|
|
No description provided. |
|
No description provided. |
|
No description provided. |
|
Auto-rebase failed — merge conflict — this branch has conflicts with dev-lead will attempt to resolve this automatically. If it cannot, a follow-up comment will explain what needs manual attention. To resolve manually instead: |
|
Automated rebase onto The branch conflicts with #2057 (issue #2056), which landed on
Keeping this PR's side would reintroduce the fail-open behaviour #2056 removed. Keeping 🤖 Generated with Claude Code |
Dev-Lead — rebase (failed)Post-conflict-resolution integrity check failed ( |
|
No description provided. |
|
Auto-rebase failed — merge conflict — this branch has conflicts with dev-lead will attempt to resolve this automatically. If it cannot, a follow-up comment will explain what needs manual attention. To resolve manually instead: |
|
Dev-lead rebase aborted: this branch ( |
Dev-Lead — rebase (failed)Post-conflict-resolution integrity check failed ( |
|
No description provided. |
Status on CodeRabbit findingsRate-limit notice: Informational — CodeRabbit review throttling does not prevent addressing other findings in this comment. Pre-merge checks (2 warnings):
Security Architecture Review (Retained concerns):
Action: The docstring coverage issue (80% threshold) is a concrete blocker and should be resolved before merge by adding missing docstrings to the 39 analyzed functions. The PR description's Interaction contract section is a pre-merge check warning; while not blocking CI, it should be updated to document the new retry scan's role in the existing timer mechanism. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
2 participants
Problem
dev-lead: unprocessed bot review threads are never retried — review-thread counterpart of #2017
From the issue: Bot review threads that dev-lead never processes are never retried. They sit with no reply and no resolution, and under
required_review_thread_resolutionthe PR cannot merge. This is the review-thread counterpart of #2017, whose fix (#2022) covers only undispositioned PR issue comments.Risk
Low — changes automation shell logic under scripts/, covered by shellcheck (--severity=warning) and the bats suite.
Test plan
Tests added/updated:
tests/dev-lead/integration/test_prompt_coverage.sh,tests/dev-lead/unit/test_bot_thread_retry.bats. Verification:bash scripts/dev-lead-lint.sh(shellcheck --severity=warning) ran pre-commit; the bats suite runs in CI.Rollback
Revert this PR. No non-revertible side effects (no tags, migrations, or external state).
Monitoring
This PR's Lint (shellcheck) and bats checks show pass/fail; watch subsequent dev-lead / pr-review runs for behavioral regressions.
Closes #2046