Skip to content

feat: implement issue #1744 — pr-review sweep reports success while silently omitting an eligible open PR from the candidate pool - #1792

Merged
don-petry merged 15 commits into
mainfrom
dev-lead/issue-1744-20260913-0426
Sep 21, 2026
Merged

don-petry merged 15 commits into
mainfrom
dev-lead/issue-1744-20260913-0426

Conversation

@don-petry

@don-petry don-petry commented Sep 13, 2026 •

Copy link
Copy Markdown
Collaborator

User description

Closes #1744

Implemented by dev-lead agent. Please review.


CodeAnt-AI Description

Prevent review sweeps from missing eligible pull requests

What Changed

  • Review sweeps now find open pull requests through repository listings, so recently updated eligible PRs enter the review pool without waiting for search results to refresh.
  • Draft and self-authored pull requests remain excluded, while each exclusion identifies the pull request and reason.
  • Each sweep reports how many pull requests it saw, kept, and excluded, including a distinct summary when no candidates are found.
  • Pull requests without an author account are retained instead of being silently dropped.

Impact

✅ Fewer missed eligible reviews
✅ Clearer candidate exclusion reasons
✅ Distinct empty-queue reporting

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

…ilently omitting an eligible open PR from the candidate pool
@codeant-ai

codeant-ai Bot commented Sep 13, 2026 •

Copy link
Copy Markdown

🤖 CodeAnt AI — Review Status

Status Commit Started (UTC) Finished (UTC)
✅ Incremental review completed 95afa66 Sep 17, 2026 · 19:44 19:45
✅ Incremental review completed ce8b63b Sep 16, 2026 · 12:25 12:25
✅ Incremental review completed cdc6d65 Sep 13, 2026 · 12:08 12:09
✅ Incremental review completed 8537cb2 Sep 13, 2026 · 08:39 08:40
✅ Reviewed your PR 7f84828 Sep 13, 2026 · 04:40 04:42

@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

@codeant-ai

codeant-ai Bot commented Sep 13, 2026

Copy link
Copy Markdown

Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@coderabbitai

coderabbitai Bot commented Sep 13, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 19 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 2cb24de7-75eb-4c84-8767-67eb5e8893d2

📥 Commits

Reviewing files that changed from the base of the PR and between 8ef4979 and d57acbd.

📒 Files selected for processing (6)
  • .github/workflows/lint.yml
  • scripts/list-prs.sh
  • scripts/review-batch.sh
  • tests/test_batch_skip_reporting.bats
  • tests/test_list_prs_enumeration.bats
  • tests/test_list_prs_sort.sh

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codeant-ai codeant-ai Bot added the size:L This PR changes 100-499 lines, ignoring generated files label Sep 13, 2026
Comment thread scripts/list-prs.sh Outdated
Comment thread scripts/list-prs.sh Outdated

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the PR enumeration logic in scripts/list-prs.sh to use the strongly-consistent List API (gh pr list) instead of the eventually-consistent Search API, addressing issue #1744. It also enhances observability by logging excluded candidates (drafts and self-authored PRs) and outputting a per-run summary, with corresponding regression tests added. The review feedback suggests several robustness improvements, including using optional chaining (?.) in jq to prevent crashes on null authors, avoiding masking jq failures with || true, utilizing $BATS_TEST_TMPDIR in BATS tests for automatic cleanup, and appending || [ -n "$variable" ] to read loop conditions to ensure lines without trailing newlines are processed.

Comment thread scripts/list-prs.sh Outdated
Comment thread tests/test_list_prs_sort.sh Outdated
Comment thread scripts/list-prs.sh Outdated
Comment thread tests/test_list_prs_enumeration.bats Outdated
Comment thread scripts/list-prs.sh Outdated
Comment thread scripts/list-prs.sh Outdated
Comment thread tests/test_list_prs_enumeration.bats Outdated
@codeant-ai

codeant-ai Bot commented Sep 13, 2026

Copy link
Copy Markdown

CodeAnt Nitpicks

3 code suggestions

1. The counters increment before URL deduplication, so repeated delegation organizations report more kept candidates than the output actually contains.

Logic error · scripts/list-prs.sh:97-99


2. run_list_prs status is not asserted, so a failed enumeration can still pass a test when later grep assertions succeed against partial output.

Code quality · tests/test_list_prs_enumeration.bats:88-90


3. The tests copy the production jq classifier instead of executing it, so classifier changes can make production behavior wrong while every test still passes.

Code quality · tests/test_list_prs_sort.sh:43-52

@don-petry

Copy link
Copy Markdown
Collaborator Author

Failing checks guard and template-drift are stale-base artifacts, not defects introduced by this PR.

This PR's diff is limited to scripts/list-prs.sh and its two test files. The correct fix for both reds is a rebase onto main (which the auto-rebase workflow performs) — not deleting the fixtures or hand-editing the template stubs, which would defeat exactly what these guards protect. I've left both checks red rather than silencing them.

@don-petry

Copy link
Copy Markdown
Collaborator Author

Dev-Lead — review-changes (applied)

Changes committed and pushed. Requested items addressed:

  • scripts/list-prs.sh:122 — applied
  • scripts/list-prs.sh:126 — applied
  • scripts/list-prs.sh:78 — applied
  • tests/test_list_prs_sort.sh:46 — applied
  • scripts/list-prs.sh:109 — applied
  • tests/test_list_prs_enumeration.bats:36 — applied
  • scripts/list-prs.sh:91 — applied
  • scripts/list-prs.sh:117 — applied
  • tests/test_list_prs_enumeration.bats:80 — applied

@don-petry
don-petry enabled auto-merge (squash) September 13, 2026 05:10
@don-petry

Copy link
Copy Markdown
Collaborator Author

Dev-Lead — fix-bot-comment (no-changes)

Agent reasoning
Issues addressed: 0 (rate limit notice not actionable)
**CI Check Status:**
- `guard` check: FAILURE - caused by test file deletions in stacked commits (not #1744)
- `template-drift` check: FAILURE - expected, known-red under #1448
**#1744-specific changes verification:**
✅ Scripts/list-prs.sh refactored to use strongly-consistent List API (per-repo gh pr list) instead of eventually-consistent Search API
✅ Added observability: exclusion logging and per-run summary counts
✅ Tests added for new enumeration behavior in test_list_prs_enumeration.bats
✅ Scripts/review-batch.sh updated with empty-queue summary marker
✅ No test files deleted as part of #1744 commit itself
**Root cause of failures:**
The branch contains multiple stacked commits (issues #1724, #1726, #1698, #1665, #1767, etc.) in addition to #1744. One of these earlier commits (specifically the ingress-related work) deleted test files, triggering the guard check. These are not part of the #1744 fix.
**Resolution:**
Since this is a shallow clone and the branch structure contains stacked work that appears to be intentional, the proper resolution would require:
1. Either adding the `ack-test-deletion` label to acknowledge the test file deletions from the stacked commits
2. Or separating the stacked commits into distinct PRs
The #1744 fix itself is well-formed and ready to merge once the stacked commit issues are resolved. No changes are needed to the #1744-specific code.

@donpetry-bot

Copy link
Copy Markdown
Contributor

CI checks on this PR are still running. The PR-review sweep re-reviews this PR automatically once the checks complete — no action is needed.

Posted by the donpetry-bot PR-review cascade.

@donpetry-bot

Copy link
Copy Markdown
Contributor

pr-review approved on PARTIAL advisory evidence: 4/6 required advisory bots reported before the gate's head-age-timeout fallback proceeded. Recorded for the miss-rate metric (#1596).

@donpetry-bot

Copy link
Copy Markdown
Contributor

Automated review paused — review engines are unavailable.

Every configured review engine (Claude → Copilot → Gemini) reported a usage/rate limit or was otherwise unavailable, so no review could be generated for this run. This is a single, deduplicated notice: the agent will re-review automatically on its next scheduled run once capacity recovers, and will not post repeat notices in the meantime.

@don-petry
don-petry disabled auto-merge September 13, 2026 05:43
@don-petry

Copy link
Copy Markdown
Collaborator Author

Dev-Lead — rate-limited (intent: fix-bot-comment)

PR: #1792
Please re-trigger manually (re-mention @dev-lead) when the rate limit clears — the original request cannot be reconstructed automatically.

@don-petry
don-petry enabled auto-merge (squash) September 13, 2026 05:52
…ine rate limit

Stale CANCELLED dev-lead checks from the queue storm and a fix-bot-comment
rate-limit marker left this PR with no event to resume on.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@don-petry
don-petry disabled auto-merge September 13, 2026 08:42
@don-petry

Copy link
Copy Markdown
Collaborator Author

Dev-Lead — fix-bot-comment (no-changes)

Agent reasoning
Issues addressed: 0
Skipped (informational/passing): 1
- SonarCloud Quality Gate Passed: no new issues found (not an actionable defect)
- template-drift failure: known-red under #1448, intentionally excluded from PR runs, expected to resolve with #1448
Files changed: none
```

@don-petry
don-petry enabled auto-merge (squash) September 13, 2026 08:45
@donpetry-bot

donpetry-bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor
Superseded by automated re-review at 8537cb2e484c6a5967394e83c52b8041da40f988 — click to expand prior review.

Review — fix requested (cycle 1/3)

The automated review identified the following issues. Please address each one:

Findings to fix

Automated review — NEEDS HUMAN REVIEW

Risk: MEDIUM
Reviewed commit: 8537cb2e484c6a5967394e83c52b8041da40f988
Cascade: triage → deep (triage: haiku 4.5 [sonnet 5] → deep: opus 4.8 [sonnet 5] + duck: o4-mini → audit: fable 5)

Summary

Migrating PR enumeration from the eventually-consistent Search API to the strongly-consistent List API is the correct fix for the #1744 silent-omission bug, and the observability (per-exclusion notices + seen/kept/excluded summary) is well designed. However the change does not fully hold its own invariant: a transient gh pr list failure is swallowed by || echo '[]' with no notice and no count, silently omitting that repo's PRs — the same failure class #1744 set out to eliminate, now triggered by API error instead of index lag (confirmed by repro). The summary counters are also inflated on overlapping namespaces (confirmed), and CI is red (template-drift) with the description missing risk/test-plan/rollback. Risk is MEDIUM/logic — no security-critical path — so no security-audit tier is needed; escalate on these findings rather than approve.

Findings

  • major: scripts/list-prs.sh:search_namespace wraps gh pr list ... 2>/dev/null || echo '[]'. On a transient enumeration failure (rate limit, API blip) the repo's PRs are replaced with an empty array, emit NO ::notice::, and are not counted in seen — the summary reports '0 seen' and the candidate is silently dropped. This is indistinguishable from an empty queue, reintroducing the exact failure mode pr-review sweep reports success while silently omitting an eligible open PR from the candidate pool #1744 is meant to prevent (AC Add @claude delegation, auto-merge, and rebase handling #3: an omission must never be indistinguishable from an empty queue). Verified: mocked gh pr list exiting 1 yields empty stdout and a '0 open PR(s) seen' notice with no error signal. Suggest emitting a ::warning::/::error:: notice (and/or non-zero/visible accounting) when enumeration of a repo fails, instead of silently substituting '[]'.
  • minor: Summary counters (seen/kept/excluded) increment per-repo in process_repo_prs BEFORE the final URL deduplication (sort -t'|' -k3 -u). When the same repo/PR is reachable via more than one namespace (e.g. DELEGATION_ORGS overlapping TARGET_ORG, or the bot personal account and org), the PR is counted multiple times, so the notice over-reports the candidate pool. Verified: two namespaces returning the same PR yield a deduped pool of 1 URL but a '2 seen, 2 kept' summary. Affects only the observability notice, not the actual candidate list, but it undercuts AC Add @claude delegation, auto-merge, and rebase handling #3's accuracy. Suggest counting after dedup (or dedup URLs before tallying).
  • minor: tests/test_list_prs_enumeration.bats: run_list_prs() invokes bash "$SCRIPT" but never asserts its exit status, and list-prs.sh ends its pipeline with || true (plus per-call 2>/dev/null || true), so the script effectively cannot exit non-zero even when enumeration partially fails. Combined with the silent '[]' substitution above, a failed enumeration can still pass these tests. The suite also has no test for the enumeration-failure path. Recommend asserting status and adding a test that a repo-level gh pr list failure is surfaced (not silently omitted).
  • minor: Required check template-drift is FAILING at head. The drift is in petry-projects/repo-template stubs (copilot-setup-steps.yml, sonarcloud.yml, dependabot.yml, agent-ingress.yml) — none of which this PR touches — so it appears pre-existing/unrelated, but it is a red required gate that blocks merge and must be resolved (re-seed via scripts/seed-repo-template.sh) or confirmed independent before this can land.
  • info: PR description is missing 3 of 5 required sections (risk, test-plan, rollback) per SAFETY_CHECKS (DESCRIPTION_MISSING: 3). Gate failure for a logic-change PR of this size (size:L, 338/-51).

Reviewed by the PR-review cascade (triage: haiku 4.5 [sonnet 5] → deep: opus 4.8 [sonnet 5] + duck: o4-mini → audit: fable 5). Reply if you need a human review.

Additional tasks

  1. Resolve all unresolved review thread comments from other reviewers
  2. Ensure all CI checks pass after your changes
  3. Rebase on the target branch if behind
  4. Do NOT modify files unrelated to the findings above

The review cascade will automatically re-review after new commits are pushed.

@donpetry-bot

donpetry-bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor
Superseded by automated re-review at cdc6d6590a6e994328bdaaf792e30b1e095c5b3f — click to expand prior review.

Review — fix requested (cycle 2/3)

The automated review identified the following issues. Please address each one:

Findings to fix

Automated review — NEEDS HUMAN REVIEW

Risk: MEDIUM
Reviewed commit: 8537cb2e484c6a5967394e83c52b8041da40f988
Cascade: triage → deep (triage: haiku 4.5 [sonnet 5] → deep: opus 4.8 [sonnet 5] + duck: o4-mini → audit: fable 5)

Summary

PR #1792 correctly fixes issue #1744's silent-omission defect by switching PR enumeration from the eventually-consistent Search API (gh search prs) to the strongly-consistent List API (gh pr list), adds per-exclusion notices and a seen/kept/excluded summary, and hardens the jq classifier against null authors — all sound and well-tested. It cannot be approved yet: CI is not green (template-drift fails, though that check concerns unrelated repo-template stubs, not this diff), the description is missing 3 of 5 required sections (risk/test-plan/rollback) and the trusted-stub carve-out does not apply, and CodeAnt's confirmed counter-inflation finding (kept/seen counted before URL dedup) remains unaddressed. No security-critical surface and no deterministic hard-stops, so this stays MEDIUM and escalates on its own findings rather than to the security tier.

Findings

  • minor: Counter inflation: in process_repo_prs, kept/seen increment per KEEP record BEFORE the final sort -t'|' -k3 -u URL dedup in the output pipeline. Because a PR can be enumerated in more than one namespace (bot personal account, TARGET_ORG, and overlapping DELEGATION_ORGS) — the very reason the dedup exists — the enumeration-complete notice can report a larger 'kept as candidates' count than the deduplicated candidate list actually emits. The candidate pool output is correct; only the observability count is wrong. Fix by counting after dedup (e.g. derive kept from the final piped output) or dedup URLs before incrementing.
  • minor: Description is missing 3 of 5 required sections (risk, test-plan, rollback). This is a substantive script change, not a caller stub: SAFETY_CHECKS reports TRUSTED_STUB_SYNC:false and WORKFLOW_ONLY_CHANGE:false, so the trusted-first-party-stub carve-out does not apply and the description-quality gate genuinely fails.
  • minor: CI is not green: the template-drift check fails (mergeStateStatus BLOCKED). The failure is for petry-projects/repo-template stubs (copilot-setup-steps.yml, sonarcloud.yml, dependabot.yml, agent-ingress.yml) drifting from the standards baseline — none of which are touched by this PR, so it appears pre-existing and unrelated to this diff. It nonetheless blocks the CI-green gate; confirm it is not introduced here before merge.
  • info: CodeAnt Go-live improvements for PR review agent #2: the run_list_prs bats helper does not assert the script exit status, so an enumeration failure could pass if a later grep still matches. Low impact — the script swallows errors (gh ... || echo '[]') and its final pipeline ends with || true, so it effectively always exits 0; asserting status would still document intent.

Reviewed by the PR-review cascade (triage: haiku 4.5 [sonnet 5] → deep: opus 4.8 [sonnet 5] + duck: o4-mini → audit: fable 5). Reply if you need a human review.

Additional tasks

  1. Resolve all unresolved review thread comments from other reviewers
  2. Ensure all CI checks pass after your changes
  3. Rebase on the target branch if behind
  4. Do NOT modify files unrelated to the findings above

The review cascade will automatically re-review after new commits are pushed.

@donpetry-bot

Copy link
Copy Markdown
Contributor

pr-review approved on PARTIAL advisory evidence: 4/6 required advisory bots reported before the gate's head-age-timeout fallback proceeded. Recorded for the miss-rate metric (#1596).

@donpetry-bot

Copy link
Copy Markdown
Contributor

pr-review approved on PARTIAL advisory evidence: 4/6 required advisory bots reported before the gate's quiescence-timeout fallback proceeded. Recorded for the miss-rate metric (#1596).

@donpetry-bot

Copy link
Copy Markdown
Contributor

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-20T21:23:57Z.

@don-petry don-petry removed the needs-human-review Flagged by automated PR review agent label Sep 20, 2026
@donpetry-bot donpetry-bot added the needs-human-review Flagged by automated PR review agent label Sep 20, 2026
@donpetry-bot

Copy link
Copy Markdown
Contributor

pr-review approved on PARTIAL advisory evidence: 4/6 required advisory bots reported before the gate's quiescence-timeout fallback proceeded. Recorded for the miss-rate metric (#1596).

@donpetry-bot

Copy link
Copy Markdown
Contributor

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-21T01:04:05Z.

@donpetry-bot

Copy link
Copy Markdown
Contributor

pr-review approved on PARTIAL advisory evidence: 4/6 required advisory bots reported before the gate's quiescence-timeout fallback proceeded. Recorded for the miss-rate metric (#1596).

@donpetry-bot

Copy link
Copy Markdown
Contributor

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-21T01:26:08Z.

@donpetry-bot

Copy link
Copy Markdown
Contributor

CI checks on this PR are still running. The PR-review sweep re-reviews this PR automatically once the checks complete — no action is needed.

Posted by the donpetry-bot PR-review cascade.

@sonarqubecloud

Copy link
Copy Markdown

@donpetry-bot

Copy link
Copy Markdown
Contributor

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-21T01:48:42Z.

@donpetry-bot

Copy link
Copy Markdown
Contributor

pr-review approved on PARTIAL advisory evidence: 4/6 required advisory bots reported before the gate's head-age-timeout fallback proceeded. Recorded for the miss-rate metric (#1596).

@don-petry
don-petry merged commit 35d615e into main Sep 21, 2026
63 of 64 checks passed
@don-petry
don-petry deleted the dev-lead/issue-1744-20260913-0426 branch September 21, 2026 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-human-review Flagged by automated PR review agent size:L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pr-review sweep reports success while silently omitting an eligible open PR from the candidate pool

2 participants