feat: implement issue #806 — dev-lead fix-review addresses advisory findings in code but never resolves the review thread → PRs stall on the advisory gate - #840
Conversation
…indings in code but never resolves the review thread → PRs stall on the advisory gate
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR changes PR auto-review readiness gating from unresolved-thread counts to blocking-thread counts. Threads block dispatch when unresolved and not explicitly outdated; missing or null ChangesBlocking review-thread gate
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PRWorkflow
participant GitHubGraphQL
participant BlockingThreadHelper
participant ReadinessCheck
PRWorkflow->>GitHubGraphQL: Query review threads with isResolved and isOutdated
GitHubGraphQL-->>PRWorkflow: Return review thread nodes
PRWorkflow->>BlockingThreadHelper: Count unresolved, non-outdated threads
BlockingThreadHelper-->>PRWorkflow: Return BLOCKING_THREAD_COUNT
PRWorkflow->>ReadinessCheck: Evaluate readiness with blocking count
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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 introduces the pr_auto_review_blocking_thread_count function to count blocking review threads (unresolved and not outdated) to address issue #806, along with documentation and comprehensive tests. Feedback suggests using the optional/try operator ? in the jq filter to prevent potential 'Cannot index null' errors when parsing nested JSON structures.
Dev-Lead — review-changes (applied)Changes committed and pushed. |
There was a problem hiding this comment.
Pull request overview
Updates the org-level PR auto-review readiness gate to treat unresolved-but-outdated review threads as non-blocking, preventing PRs from stalling when advisory findings are fixed in follow-up commits but the original review threads are never explicitly resolved (issue #806).
Changes:
- Add a pure helper (
pr_auto_review_blocking_thread_count) that counts only blocking review threads (unresolved AND not outdated), with bats coverage. - Update the reusable workflow to fetch
isOutdatedvia GraphQL and use the pure helper for the unresolved-thread gate. - Document the new “blocking thread” semantics in the pr-auto-review script README and workflow header.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
test/workflows/pr-auto-review/blocking-threads.bats |
Adds unit tests covering blocking vs non-blocking (outdated) review thread counting behavior. |
.github/workflows/pr-auto-review-reusable.yml |
Switches from “unresolved threads” to “blocking threads” by querying isOutdated and delegating counting to the pure helper. |
.github/scripts/pr-auto-review/README.md |
Documents the new unresolved-thread semantics and the new helper function contract. |
.github/scripts/pr-auto-review/lib/ready-check.sh |
Introduces pr_auto_review_blocking_thread_count (jq-based) for consistent, unit-testable blocking-thread counting. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #840 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
|
CI checks on this PR are still running. Once they complete, re-mention Posted by the donpetry-bot PR-review cascade. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
|
|
Note @don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
|
CI checks on this PR are still running. Once they complete, re-mention Posted by the donpetry-bot PR-review cascade. |
|
@donpetry-bot review — all required checks are green (SonarCloud, CodeQL, agent-shield/AgentShield, Detect ecosystems, Lint, ShellCheck, Agent Security Scan). The only non-passing context is CodeRabbit, a non-required check stuck on a per-developer rate limit; its last completed review at this head was clean. Please review and merge (fix-loop repair for #806). |
|
@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes. |
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #840 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@donpetry-bot review — CodeRabbit is now green and all required checks pass; please review and merge. |
|
@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: cf0a4a013d0fec2a8867e847fc4071ec05dcc6b4
Review mode: triage-approved (single reviewer)
Summary
Implements the consumer-side fix from issue #806: the pr-auto-review readiness gate now blocks only on review threads that are unresolved AND not outdated, so dev-lead PRs whose advisory findings were fixed in code (making the threads outdated) no longer stall on manual thread resolution. Adds a pure, unit-tested helper (pr_auto_review_blocking_thread_count), 15 bats tests covering blocking/outdated/fail-safe/malformed-payload cases, and updated docs. Verified the jq logic locally: fail-safe holds (null/absent isOutdated on an unresolved thread still blocks) and GraphQL error bodies yield 0 while the transport remains fail-closed under set -e.
Linked issue analysis
Issue #806 (open) proposed two fixes; this PR delivers option 2 (consumer-side, defense-in-depth) exactly as specified, including the fail-safe that only an explicit isOutdated == true is non-blocking. The producer-side fix (dev-lead resolving threads it fixes) remains open work under #806, which the PR docs acknowledge. Substantively addressed for the consumer side.
Findings
- (Info, accepted) The change deliberately loosens the review gate: isOutdated is a diff-anchor heuristic, so an unresolved substantive concern whose flagged line changed without being fixed would no longer block. This is the explicit intent of #806, is documented as a heuristic (not a guarantee) in README/workflow/lib comments after Copilot review pushback, and the fail-safe prevents silent drops on unknown staleness. 2. (Nit) package-lock.json and node_modules/.package-lock.json name churn (pr-837 → pr-840) is unrelated mechanical noise from the dev-lead process; pre-existing pattern, no dependency changes. 3. Workflow change reviewed for Actions security smells: static single-quoted GraphQL query with variables passed via -f/-F (no shell/GraphQL injection), no permission changes, transport kept fail-closed (no || true). None found. 4. Secret scan: run_secret_scanning MCP tool unavailable in this environment; gitleaks CI check is green and the diff contains no credential-like content. 5. All 8 review threads (Gemini, Copilot x6, CodeRabbit) are resolved; CodeRabbit's earlier CHANGES_REQUESTED was dismissed and superseded.
CI status
All required and advisory checks green at cf0a4a0: Lint, ShellCheck, bats, Lint and bats, CodeQL (actions), SonarCloud, Agent Security Scan, agent-shield, Secret scan (gitleaks), npm audit, CodeRabbit. Ecosystem-conditional checks (pip-audit, cargo audit, govulncheck, pnpm audit, dependabot-automerge) skipped as expected. Mergeable; BLOCKED only on the pending review this verdict satisfies.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Closes #806
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit
New Features
Documentation
Tests