feat(maintenance): auto-dismiss stale CHANGES_REQUESTED bot reviews#3744
Merged
Conversation
Closes #3732. PRs get permanently BLOCKED when protoquinn[bot] or coderabbitai[bot] leave a CHANGES_REQUESTED review on commit A, the issues get fixed on commit B, and the bot re-reviews B as COMMENTED instead of APPROVED. COMMENTED doesn't clear CHANGES_REQUESTED, so the PR sits indefinitely until a maintainer manually dismisses. Adds a new full-tier MaintenanceCheck that runs every 6h. For each open PR with mergeStateStatus=BLOCKED and reviewDecision=CHANGES_REQUESTED: 1. Group reviews by bot author (login ends with `[bot]`). 2. Find any CHANGES_REQUESTED whose commit is NOT the current head. 3. Confirm the same bot has a later COMMENTED or APPROVED review. 4. Confirm all CI checks on head are COMPLETED + SUCCESS/SKIPPED. When all four hold, dismiss the stale review with a message citing the follow-up review and head SHA. The PR transitions out of BLOCKED on the next reviewDecision recomputation. Repo coords come from GITHUB_REPO_OWNER / GITHUB_REPO_NAME env vars, defaulting to protoLabsAI/protoMaker so a fresh install works. 7/7 new tests cover: - Happy path: dismiss when superseded by COMMENTED on later commit - Skip: CHANGES_REQUESTED is on current head (not stale) - Skip: follow-up is from a different bot - Skip: blocking review is from a human (no `[bot]` suffix) - Skip: CI has a failure on head - Skip: CI still IN_PROGRESS (wait for stable state) - Skip: PR is not BLOCKED or has no CHANGES_REQUESTED decision Full server suite: 3449/3449.
📝 WalkthroughWalkthroughThis PR adds a new maintenance check that automatically dismisses stale bot ChangesStale bot review dismissal check
Sequence Diagram(s)sequenceDiagram
participant Check as AutoDismissStaleBotReviewsCheck
participant GitHub as GitHub API (gh)
participant Result as MaintenanceIssue[]
Check->>GitHub: listOpenPRs() - fetch up to 50 open PRs
GitHub-->>Check: PR list with mergeStateStatus and reviewDecision
Check->>Check: filter BLOCKED + CHANGES_REQUESTED
loop per PR
Check->>GitHub: listReviews() - fetch PR review history
GitHub-->>Check: all reviews ordered by submitted_at
Check->>Check: processPR - identify stale bot CHANGES_REQUESTED
Check->>Check: verify later COMMENTED/APPROVED from same bot
Check->>Check: allChecksGreen() - validate CI status
alt all conditions met
Check->>GitHub: dismiss via gh api - remove stale review
GitHub-->>Check: dismissal success
Check->>Result: record MaintenanceIssue
else condition failed
Check->>Check: skip PR
end
end
Check-->>Result: return aggregated issues
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: one or more packages not found in the registry. Comment |
Contributor
Code Review — ? finding(s)
protoLabs Code Review Report
No findings recorded. |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/server/tests/unit/services/maintenance/auto-dismiss-stale-bot-reviews-check.test.ts (1)
14-14: ⚡ Quick winUse relative import for server-internal class in unit test
For this internal server implementation, prefer a relative path import to keep tests aligned with server-internal boundary conventions.
Based on learnings, "for tests verifying internal server implementation classes, import via relative paths within apps/server rather than shared/public-style aliases."
🤖 Prompt for 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. In `@apps/server/tests/unit/services/maintenance/auto-dismiss-stale-bot-reviews-check.test.ts` at line 14, Replace the aliased import of AutoDismissStaleBotReviewsCheck with a relative-path import that points to the server-internal implementation used by this unit test; update the import line importing AutoDismissStaleBotReviewsCheck so it uses a relative path within the apps/server tree instead of the '`@/`...' alias to keep the test aligned with server-internal boundaries.
🤖 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 `@apps/server/src/services/maintenance.module.ts`:
- Around line 203-204: Replace the hardcoded fallback values for repoOwner and
repoName (the constants currently assigned in maintenance.module.ts) so they are
sourced only from configuration/settings or environment without embedded
defaults; read them via the app configuration provider or process.env (but do
not provide 'protoLabsAI'/'protoMaker' as fallbacks), validate that the values
exist at startup (throw or log and fail fast if missing), and update any code
that references repoOwner/repoName to rely on the validated configuration values
rather than hardcoded defaults.
In
`@apps/server/src/services/maintenance/checks/auto-dismiss-stale-bot-reviews-check.ts`:
- Around line 149-153: The follow-up selection currently only checks state and
timestamp and can match a later review on the same commit; update the followUp
predicate (the const followUp = list.find(...) block) to also require the
follow-up review be for a different commit than latestStale by comparing commit
identifiers (e.g. r.commit_id or r.commit_sha) to
latestStale.commit_id/latestStale.commit_sha and treat missing values safely
(use nullish coalescing) so the condition becomes: state is COMMENTED/APPROVED,
submitted_at is later, and the commit id/sha is different from latestStale.
---
Nitpick comments:
In
`@apps/server/tests/unit/services/maintenance/auto-dismiss-stale-bot-reviews-check.test.ts`:
- Line 14: Replace the aliased import of AutoDismissStaleBotReviewsCheck with a
relative-path import that points to the server-internal implementation used by
this unit test; update the import line importing AutoDismissStaleBotReviewsCheck
so it uses a relative path within the apps/server tree instead of the '`@/`...'
alias to keep the test aligned with server-internal boundaries.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e4306b12-90fc-438e-8070-51db3b567f50
📒 Files selected for processing (3)
apps/server/src/services/maintenance.module.tsapps/server/src/services/maintenance/checks/auto-dismiss-stale-bot-reviews-check.tsapps/server/tests/unit/services/maintenance/auto-dismiss-stale-bot-reviews-check.test.ts
Comment on lines
+203
to
+204
| const repoOwner = process.env.GITHUB_REPO_OWNER || 'protoLabsAI'; | ||
| const repoName = process.env.GITHUB_REPO_NAME || 'protoMaker'; |
There was a problem hiding this comment.
Avoid hardcoded repo defaults in server maintenance wiring
Line 203-204 hardcodes repo owner/name fallbacks. In this path, repo/workflow coordinates should come from configuration only (env/settings), not embedded defaults.
As per coding guidelines, "apps/server/src/**/*.{ts,tsx} must never hardcode workflow-specific values ... These must come from settings or configuration."
🤖 Prompt for 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.
In `@apps/server/src/services/maintenance.module.ts` around lines 203 - 204,
Replace the hardcoded fallback values for repoOwner and repoName (the constants
currently assigned in maintenance.module.ts) so they are sourced only from
configuration/settings or environment without embedded defaults; read them via
the app configuration provider or process.env (but do not provide
'protoLabsAI'/'protoMaker' as fallbacks), validate that the values exist at
startup (throw or log and fail fast if missing), and update any code that
references repoOwner/repoName to rely on the validated configuration values
rather than hardcoded defaults.
Comment on lines
+149
to
+153
| const followUp = list.find( | ||
| (r) => | ||
| (r.state === 'COMMENTED' || r.state === 'APPROVED') && | ||
| (r.submitted_at ?? '').localeCompare(latestStale.submitted_at ?? '') > 0 | ||
| ); |
There was a problem hiding this comment.
Follow-up review must prove commit progression before dismissing
At Line 149, the follow-up check only validates review type + later timestamp. This can dismiss a still-valid CHANGES_REQUESTED if the bot later comments on the same commit. Add a commit guard so the follow-up is on a different (newer) commit than the stale review.
Proposed fix
const followUp = list.find(
(r) =>
(r.state === 'COMMENTED' || r.state === 'APPROVED') &&
- (r.submitted_at ?? '').localeCompare(latestStale.submitted_at ?? '') > 0
+ (r.submitted_at ?? '').localeCompare(latestStale.submitted_at ?? '') > 0 &&
+ !!r.commit_id &&
+ r.commit_id !== latestStale.commit_id
);🤖 Prompt for 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.
In
`@apps/server/src/services/maintenance/checks/auto-dismiss-stale-bot-reviews-check.ts`
around lines 149 - 153, The follow-up selection currently only checks state and
timestamp and can match a later review on the same commit; update the followUp
predicate (the const followUp = list.find(...) block) to also require the
follow-up review be for a different commit than latestStale by comparing commit
identifiers (e.g. r.commit_id or r.commit_sha) to
latestStale.commit_id/latestStale.commit_sha and treat missing values safely
(use nullish coalescing) so the condition becomes: state is COMMENTED/APPROVED,
submitted_at is later, and the commit id/sha is different from latestStale.
This was referenced May 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #3732. Adds a maintenance check that detects and dismisses bot CHANGES_REQUESTED reviews that have been superseded by later COMMENTED/APPROVED reviews on subsequent commits, when CI is green on the current head.
Why
GitHub branch protection only clears a CHANGES_REQUESTED review when the reviewer dismisses it explicitly OR submits a fresh APPROVED. Our review bots routinely re-review fix commits with COMMENTED (acknowledging the fix without escalating), which does NOT clear the blocking state.
Today (session), we hit this on three PRs (#3660, #3691, plus the crew dispatch flow) and had to manually dismiss each blocking review. This check eliminates that toil.
Detection criteria — all must hold
mergeStateStatus: BLOCKED,reviewDecision: CHANGES_REQUESTED[bot]userWhen all five hold, dismiss with a message citing the follow-up review and head SHA.
Test plan
7 new unit tests in
apps/server/tests/unit/services/maintenance/auto-dismiss-stale-bot-reviews-check.test.ts:Full server suite: 3449/3449 pass.
Summary by CodeRabbit
New Features
Tests