Skip to content

Commit 9e9539e

Browse files
matejclaude
andauthored
Only dismiss reviews posted by this action (#14)
## Summary Fixes a bug where `dismissStaleReviews()` would dismiss ALL bot reviews, including those from Dependabot, Renovate, or other important bot reviewers. ## Problem Previously, the code checked only: - `review.user.type === 'Bot'` - `review.state === 'APPROVED' || 'CHANGES_REQUESTED'` This meant any bot's approval (e.g., Dependabot security review) would be dismissed when our action posted a new review. ## Solution Added `isOwnReview()` function that checks the review body for our signature patterns: - "No issues found. Changes look good." - "Found X issues..." - "Please address the high-severity issues..." - "Consider addressing the suggestions..." - "Minor suggestions noted..." Only reviews matching these patterns will be dismissed. ## Test plan - [x] Updated test to include Dependabot/Renovate mock reviews - [x] Verified only our reviews (101, 102) are dismissed - [x] Verified other bot reviews (103, 104) are NOT dismissed - [x] All 17 tests pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 parent e16daf3 commit 9e9539e

2 files changed

Lines changed: 44 additions & 9 deletions

File tree

scripts/comment-pr-findings.bun.test.js

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -849,7 +849,7 @@ describe('comment-pr-findings.js', () => {
849849
});
850850

851851
describe('Stale Review Handling', () => {
852-
test('should dismiss stale reviews when DISMISS_STALE_REVIEWS is true', async () => {
852+
test('should only dismiss own bot reviews when DISMISS_STALE_REVIEWS is true', async () => {
853853
process.env.DISMISS_STALE_REVIEWS = 'true';
854854

855855
const mockFindings = [{
@@ -863,10 +863,16 @@ describe('comment-pr-findings.js', () => {
863863
const mockPrFiles = [{ filename: 'test.py', patch: '@@ -10,1 +10,1 @@' }];
864864

865865
const mockReviews = [
866-
{ id: 101, state: 'CHANGES_REQUESTED', user: { type: 'Bot' } },
867-
{ id: 102, state: 'APPROVED', user: { type: 'Bot' } },
868-
{ id: 103, state: 'COMMENTED', user: { type: 'Bot' } }, // Should not be dismissed
869-
{ id: 104, state: 'CHANGES_REQUESTED', user: { type: 'User' } } // Should not be dismissed
866+
// Our reviews - should be dismissed
867+
{ id: 101, state: 'CHANGES_REQUESTED', user: { type: 'Bot' }, body: 'Found 3 security issues. Please address the high-severity issues before merging.' },
868+
{ id: 102, state: 'APPROVED', user: { type: 'Bot' }, body: 'No issues found. Changes look good.' },
869+
// Other bot reviews - should NOT be dismissed
870+
{ id: 103, state: 'APPROVED', user: { type: 'Bot' }, body: 'Dependabot has approved this PR.' },
871+
{ id: 104, state: 'CHANGES_REQUESTED', user: { type: 'Bot' }, body: 'Renovate: This PR has conflicts.' },
872+
// COMMENTED state - should NOT be dismissed
873+
{ id: 105, state: 'COMMENTED', user: { type: 'Bot' }, body: 'Found 1 security issue.' },
874+
// User review - should NOT be dismissed
875+
{ id: 106, state: 'CHANGES_REQUESTED', user: { type: 'User' }, body: 'Please fix the typo.' }
870876
];
871877

872878
readFileSyncSpy.mockImplementation((path) => {
@@ -907,10 +913,14 @@ describe('comment-pr-findings.js', () => {
907913

908914
await import('./comment-pr-findings.js');
909915

916+
// Only our reviews should be dismissed
910917
expect(dismissedReviews).toContain(101);
911918
expect(dismissedReviews).toContain(102);
912-
expect(dismissedReviews).not.toContain(103); // COMMENTED state
913-
expect(dismissedReviews).not.toContain(104); // User review
919+
// Other bot reviews should NOT be dismissed
920+
expect(dismissedReviews).not.toContain(103); // Dependabot
921+
expect(dismissedReviews).not.toContain(104); // Renovate
922+
expect(dismissedReviews).not.toContain(105); // COMMENTED state
923+
expect(dismissedReviews).not.toContain(106); // User review
914924
expect(consoleLogSpy).toHaveBeenCalledWith('Dismissed stale review 101');
915925
expect(consoleLogSpy).toHaveBeenCalledWith('Dismissed stale review 102');
916926
});

scripts/comment-pr-findings.js

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,31 @@ function addReactionsToReview(reviewId) {
8888
}
8989
}
9090

91-
// Helper function to dismiss stale bot reviews
91+
// Check if a review was posted by this action
92+
function isOwnReview(review) {
93+
if (!review.body) return false;
94+
95+
// Check for our review summary patterns
96+
const ownPatterns = [
97+
'No issues found. Changes look good.',
98+
/^Found \d+ .+ issues?\./,
99+
'Please address the high-severity issues before merging.',
100+
'Consider addressing the suggestions in the comments.',
101+
'Minor suggestions noted in comments.'
102+
];
103+
104+
for (const pattern of ownPatterns) {
105+
if (pattern instanceof RegExp) {
106+
if (pattern.test(review.body)) return true;
107+
} else {
108+
if (review.body.includes(pattern)) return true;
109+
}
110+
}
111+
112+
return false;
113+
}
114+
115+
// Helper function to dismiss stale bot reviews from this action only
92116
function dismissStaleReviews() {
93117
try {
94118
const reviews = ghApi(`/repos/${context.repo.owner}/${context.repo.repo}/pulls/${context.issue.number}/reviews`);
@@ -101,8 +125,9 @@ function dismissStaleReviews() {
101125
for (const review of reviews) {
102126
const isDismissible = review.state === 'APPROVED' || review.state === 'CHANGES_REQUESTED';
103127
const isBot = review.user && review.user.type === 'Bot';
128+
const isOwn = isOwnReview(review);
104129

105-
if (isBot && isDismissible) {
130+
if (isBot && isDismissible && isOwn) {
106131
try {
107132
ghApi(
108133
`/repos/${context.repo.owner}/${context.repo.repo}/pulls/${context.issue.number}/reviews/${review.id}/dismissals`,

0 commit comments

Comments
 (0)