feat: implement issue #1408 — [Phase 4] Narrow pr-review-sweep's cron scope to genuinely un-eventable cases and surface a sweep-hit-rate metric in daily health - #1504
Conversation
… scope to genuinely un-eventable cases and surface a sweep-hit-rate metric in daily health
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR classifies CI checks by workflow eventability, limits scheduled sweeps to uneventable or mixed checks, and adds scheduled sweep hit-rate telemetry to daily health reporting. It also updates the interaction contract and extends Bats coverage. ChangesSweep eventability and telemetry
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant sweep_stuck_reviews.sh
participant classify_rollup_eventability
participant ReviewDispatch
Scheduler->>sweep_stuck_reviews.sh: Start scheduled sweep
sweep_stuck_reviews.sh->>classify_rollup_eventability: Classify pull request checks
classify_rollup_eventability-->>sweep_stuck_reviews.sh: Return eventability result
sweep_stuck_reviews.sh->>ReviewDispatch: Dispatch uneventable or mixed candidate
sequenceDiagram
participant pr_review_health.sh
participant GitHubAPI
participant pr_review_sweep_metrics.sh
participant DailyHealthReport
pr_review_health.sh->>GitHubAPI: Fetch scheduled runs and logs
GitHubAPI-->>pr_review_health.sh: Return run data and log text
pr_review_health.sh->>pr_review_sweep_metrics.sh: Calculate hit rate
pr_review_sweep_metrics.sh-->>pr_review_health.sh: Return metrics and coverage
pr_review_health.sh->>DailyHealthReport: Write sweep metrics
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
codeant-ai
Bot
added
the
size:XL
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-08-12T19:29:43Z. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
![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 narrows the scheduled pr-review-sweep cron path to target only un-eventable checks (such as SonarCloud) that do not emit workflow_run events, leaving eventable GitHub Actions checks to the fast path. It also introduces a deterministic sweep-hit-rate metric to the daily health report and adds corresponding unit tests. The review feedback focuses on improving shell script robustness and performance, suggesting guards for grep pipelines under set -e, optimizing loop-based jq invocations using positional arguments, defensively handling missing JSON properties with optional chaining, and refining test assertions to check for specific exit codes.
Superseded by automated re-review at
|
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: b0c8736edfdbe0b93257e0ff979568f11924f0e3
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)
Summary
Feature PR (#1408) narrows the pr-review-sweep cron to genuinely un-eventable checks (SonarCloud-style, no workflowName) and adds a deterministic sweep-hit-rate metric to the daily health report, with new pure helpers and thorough bats coverage. The triage's central HIGH signal — an unguarded grep pipeline in pr-review-sweep-metrics.sh crashing pr_review_health.sh under set -euo pipefail — is a verified FALSE POSITIVE: the function's last command is always a successful printf, and set -e is ignored inside a function invoked via command-substitution assignment (the exact production call pattern), so it returns 0 and cannot abort the caller (reproduced on bash 5.2). No auth/secrets/crypto/migrations/CI-scanner/Actions-security surface is touched; CodeQL, SonarCloud, gitleaks, shellcheck, bats and unit-tests are all green. Downstream: 5 consumers pin pr-review.yml (informational; this PR does not alter that workflow's interface).
Downstream impact
This change is consumed by 5 downstream repo(s) that pin the affected reusable workflow / lib / prompt. Impacted consumers:
Impacted shared surfaces:
- .github/workflows/pr-review.yml
Impacted consumers (5, fetching up to 10):
- petry-projects/.github-private (pins .github/workflows/pr-review.yml)
.github/workflows/pr-review-canary.yml
.github/workflows/pr-review-trigger.yml
.github/workflows/test-dev-lead.yml
- petry-projects/ContentTwin (pins .github/workflows/pr-review.yml)
.github/workflows/pr-review.yml
- petry-projects/bmad-bgreat-suite (pins .github/workflows/pr-review.yml)
.github/workflows/pr-review.yml
- petry-projects/google-app-scripts (pins .github/workflows/pr-review.yml)
.github/workflows/pr-review.yml
- petry-projects/markets (pins .github/workflows/pr-review.yml)
.github/workflows/pr-review.yml
Findings
- INFO [correctness] (
scripts/lib/pr-review-sweep-metrics.sh:63): Triage HIGH signal disproven. pr_review_sweep_dispatched_from_log ends in a successful printf, so its return status is always 0; and set -e is ignored for the function body when it is called as dispatched=$(...) (command-substitution assignment) at pr_review_health.sh:317. Empirically reproduced under set -euo pipefail on bash 5.2: no-match input yields '0' and the script reaches its end (exit 0). The bats test 'no summary line yields 0' also exercises this and passes in CI. - MINOR [maintainability] (
scripts/lib/pr-review-sweep-metrics.sh:63): Non-blocking robustness nit (Gemini flagged 'high', but impact is low given the above): the grep pipeline is only crash-safe because it runs in a set-e-ignored command-substitution context and ends in printf. If the function were ever called as a bare statement or as the last element of a pipeline, a no-match grep (exit 1) could abort under pipefail. Appending '|| true' (or ': ${count:=0}') would make the safety explicit and future-proof. Optional. - INFO [correctness] (
scripts/lib/ci-status.sh:73): ci-status.sh refactor extracts the shared is_own_check jq into _CI_STATUS_JQ_IS_OWN_CHECK and switches compute_ci_status to a double-quoted jq program with escaped $ jq-vars and " literals; new classify_rollup_eventability reuses the same def. No untrusted data is interpolated into the jq program (data is passed via <<< heredoc), so no jq/shell injection. Behavior preservation is corroborated by the existing test_ci_status.bats suite plus new eventability tests, all green in CI. - INFO [correctness] (
scripts/pr_review_health.sh:300): pr_review_health.sh sweep telemetry loop uses only internal constants in the gh api URL (WORKFLOW_REPO, SWEEP_WORKFLOW_FILE) and numeric sweep_id from the runs API; --argjson d "${dispatched:-0}" is fed a digits-only value from the ^[0-9]+ grep, so argjson cannot receive malformed JSON. SWEEP_MAX_LOG_FETCH bounds per-run log I/O. No injection or unbounded-cost concern. - INFO [downstream-impact]: Downstream-impact block lists 5 consumers pinning .github/workflows/pr-review.yml (petry-projects/.github-private, ContentTwin, bmad-bgreat-suite, google-app-scripts, markets). This PR does NOT modify pr-review.yml or any consumed interface — it changes internal sweep/health scripts and the interaction-contract doc. Informational only; no interface break, so not an escalation trigger.
- MINOR [maintainability] (
scripts/sweep-stuck-reviews.sh:258): Remaining advisory nits are non-blocking and optional: defensive jq optional-chaining (.statusCheckRollup?, .workflow_runs[]?) at sweep-stuck-reviews.sh:258 / pr_review_health.sh:100, jq -n '$ARGS.positional' to avoid per-iteration jq spawns at pr_review_health.sh:116, and asserting exit status == 1 (not just non-zero) in the negative grep bats assertions. None affect correctness or security. Codex posted a usage-limit notice (not a finding); SonarCloud quality gate passed. - INFO [process]: MCP run_secret_scanning tool was not available in this environment (GitHub Secret Protection MCP not exposed); the gitleaks CI check passed and the diff introduces no secret-like material (pure shell logic, tests, docs).
Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
7b908ed
|
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. |
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #1504 |
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@interaction-contracts/pr-review.yml`:
- Around line 17-24: Update the documentation comment describing uneventable
checks to define eventability by membership in pr-review-sweep.yml’s
workflow_run.workflows list. Document checks with unsupported workflowName
values as uneventable alongside checks with no workflowName, and align the
scheduled-sweep description with this classifier behavior.
In `@scripts/lib/ci-status.sh`:
- Around line 124-129: Update the CI status validation around is_uneventable and
the workflow configuration to add an automated allowlist drift check that
compares the classifier’s workflow-name list against the names configured for
workflow_run. Fail validation when either list differs, using the actual
configuration as the source rather than duplicating the same hardcoded names in
the test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f51c591c-50b7-4374-9ae0-ac305c7f7247
📒 Files selected for processing (9)
.github/workflows/lint.ymlinteraction-contracts/pr-review.ymlscripts/lib/ci-status.shscripts/lib/pr-review-sweep-metrics.shscripts/pr_review_health.shscripts/sweep-stuck-reviews.shtests/pr_review_sweep_metrics.batstests/test_ci_status.batstests/test_sweep_stuck_reviews.bats
Superseded by automated re-review at
|
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
Auto-dismissed (#617): coderabbitai[bot] CHANGES_REQUESTED on a superseded commit. The bot re-reviews the new head automatically — a valid concern will return as a fresh review.
|
Dev-Lead — waiting on PR blockers (intent: fix-reviews)PR: #1504 |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-08-12T20:05:26Z. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 3ce15799fa94799e01108d88a9e35977824c0599
Review mode: triage-approved (single reviewer)
Summary
Re-review at 3ce1579 confirms all prior findings are resolved. The prior MAJOR finding (claiming pr-review-sweep.yml has no workflow_run trigger) was factually incorrect: the file at head defines workflow_run with workflows [CI, Tests, Holdout Guard, SonarCloud Analysis, Lint], matching issue #1408's Dev Notes. The fix commit extracted the eventable allowlist into _CI_STATUS_EVENTABLE_WORKFLOWS, added a bats drift-guard test that parses the real trigger list from pr-review-sweep.yml, and tightened the interaction-contract docs to define eventability by allowlist membership. coderabbitai has APPROVED at head and all review threads are resolved. CI is fully green.
Linked issue analysis
Closes #1408. All five acceptance criteria are substantively addressed: (1) scheduled sweep narrowed to un-eventable residue in sweep-stuck-reviews.sh with the workflow_run fast path explicitly unchanged (tested); (2) sweep-hit-rate computed in pr_review_health.sh via pure helpers in pr-review-sweep-metrics.sh; (3) interpretation note (low = exception-only, rising = eventable leak) rendered with the metric; (4) no new cron, idempotent, cron minutes untouched; (5) interaction contract timers[] updated and validate-interaction-contracts is green. The residual risk of a dropped workflow_run event stranding an eventable-only PR is the design tradeoff the issue explicitly requests, and the new hit-rate metric exists to detect exactly that leakage.
Findings
- RESOLVED (prior MAJOR, correctness): the workflow_run fast path exists at head (pr-review-sweep.yml triggers on workflows [CI, Tests, Holdout Guard, SonarCloud Analysis, Lint]); docs now describe eventability as allowlist membership rather than mere presence of a workflowName.
- RESOLVED (prior MINOR, maintainability): hardcoded list replaced with _CI_STATUS_EVENTABLE_WORKFLOWS plus a drift-guard bats test comparing it against the actual workflow_run.workflows line in pr-review-sweep.yml.
- RESOLVED (prior MINOR, process): coderabbitai APPROVED at head 3ce1579; all 11 review threads resolved; no CHANGES_REQUESTED remains.
- INFO (downstream): ci-status.sh / sweep-stuck-reviews.sh are shared surfaces pinned by 5 consumers; compute_ci_status refactor is behavior-preserving (identical defs, single-to-double-quote conversion with correct escaping) and existing bats + ShellCheck pass.
- INFO (secret scan): run_secret_scanning MCP tool not exposed in this environment; gitleaks CI check passed and manual diff review found no secrets.
CI status
All checks green at head: shellcheck, bats (including the new drift-guard and narrowing tests), unit-tests, actionlint, CodeQL (actions+python), SonarCloud quality gate, gitleaks, agent-shield, validate-interaction-contracts, and all governance checks SUCCESS. Remaining CANCELLED entries are superseded dev-lead orchestration/review runs (non-blocking per issue #608); their latest runs are SUCCESS.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
2 participants
User description
Closes #1408
Implemented by dev-lead agent. Please review.
CodeAnt-AI Description
Limit scheduled PR review sweeps to checks without an event trigger and track sweep activity
What Changed
Impact
✅ Fewer redundant PR reviews✅ Faster event-driven reviews✅ Clearer sweep exception monitoring💡 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:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
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:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests