Skip to content

fix: return no-op for stale review_id 404 in dismiss_pull_request_review #49745

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 5 commits into from
Aug 2, 2026
+60 −6
Merged

fix: return no-op for stale review_id 404 in dismiss_pull_request_review #49745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d768f17
Initial plan
Copilot Aug 2, 2026
9f3c78b
fix: catch getReview 404 in dismiss_pull_request_review and return no-op
Copilot Aug 2, 2026
2a92b66
Merge branch 'main' into copilot/aw-failures-dismiss-pull-request-review
github-actions[bot] Aug 2, 2026
6aa73e4
fix: tighten dismiss review stale-id coverage
Copilot Aug 2, 2026
879bf63
Merge branch 'main' into copilot/aw-failures-dismiss-pull-request-review
github-actions[bot] Aug 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions actions/setup/js/dismiss_pull_request_review.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,12 +225,31 @@ async function main(config = {}) {
};
}

const { data: review } = await githubClient.rest.pulls.getReview({
owner,
repo,
pull_number: pullRequestNumber,
review_id: reviewId,
});
let review;
try {
const { data } = await githubClient.rest.pulls.getReview({
owner,
repo,
pull_number: pullRequestNumber,
review_id: reviewId,
});
review = data;
} catch (getReviewError) {
if (getReviewError?.status === 404) {
return {
success: true,
skipped: true,
reason: "review no longer exists",
review_id: reviewId,
pull_request_number: pullRequestNumber,
repo: `${owner}/${repo}`,
};
}
if (getReviewError && typeof getReviewError.message === "string") {
getReviewError.message = `Failed to fetch review ${reviewId} on ${owner}/${repo}#${pullRequestNumber}: ` + getReviewError.message;
}
throw getReviewError;

@github-actions github-actions Bot Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non-404 errors are re-thrown without added context, making them harder to diagnose in workflow logs when many reviews/PRs are processed.

💡 Details

When getReviewError.status is anything other than 404 (403, 500, rate-limit, network error), it propagates via throw getReviewError unchanged. The outer catch (error) at the bottom of main only records getErrorMessage(error), so the resulting failure message won't mention which review_id/pull_request_number the lookup was for. In a workflow that fans out over multiple dismiss requests, this makes triage harder.

Suggested fix:

} catch (getReviewError) {
  if (getReviewError?.status === 404) {
    return { success: true, skipped: true, reason: "review no longer exists", review_id: reviewId, pull_request_number: pullRequestNumber, repo: `${owner}/${repo}` };
  }
  getReviewError.message = `Failed to fetch review ${reviewId} on ${owner}/${repo}#${pullRequestNumber}: ${getReviewError.message}`;
  throw getReviewError;
}

Rationale: preserving the original error type/stack while enriching the message keeps the fatal-failure behavior intact but makes logs actionable.

Copilot AI Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 6aa73e4. /home/runner/work/gh-aw/gh-aw/actions/setup/js/dismiss_pull_request_review.cjs now enriches non-404 getReview failures with the review_id and owner/repo#pull_request_number context before rethrowing, so fatal safe-output logs stay actionable.

}

const reviewAuthorLogin = review?.user?.login;
const reviewAuthorType = typeof review?.user?.type === "string" ? review.user.type.trim() : "";
Expand Down
35 changes: 35 additions & 0 deletions actions/setup/js/dismiss_pull_request_review.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,4 +365,39 @@ describe("dismiss_pull_request_review", () => {
expect(result.error).toContain("truncated");
expect(mockListReviews).toHaveBeenCalledTimes(10);
});

it("returns skipped no-op when getReview returns 404 for an explicit review_id", async () => {
const notFoundError = Object.assign(new Error("Not Found"), { status: 404 });
mockGetReview.mockRejectedValueOnce(notFoundError);

const result = await handler({
type: "dismiss_pull_request_review",
review_id: 123,
justification: "This stale review no longer reflects the updated implementation.",
});

expect(result.success).toBe(true);
expect(result.skipped).toBe(true);
expect(result.reason).toContain("review no longer exists");
expect(result.review_id).toBe(123);
expect(result.pull_request_number).toBe(42);
expect(result.repo).toBe("test-owner/test-repo");
expect(mockDismissReview).not.toHaveBeenCalled();
});

it("fails with review context when getReview returns a non-404 error", async () => {
const serverError = Object.assign(new Error("Internal Server Error"), { status: 500 });
mockGetReview.mockRejectedValueOnce(serverError);

const result = await handler({
type: "dismiss_pull_request_review",
review_id: 123,
justification: "This stale review no longer reflects the updated implementation.",
});

expect(result.success).toBe(false);
expect(result.error).toContain("Failed to fetch review 123 on test-owner/test-repo#42");
expect(result.error).toContain("Internal Server Error");
expect(mockDismissReview).not.toHaveBeenCalled();
});

@github-actions github-actions Bot Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No test asserts that non-404 errors from getReview (e.g. 500 or transient network failures) still propagate as failures rather than being swallowed.

💡 Details

The new test only exercises the happy path for the 404 no-op. There is no negative test verifying that a non-404 error (e.g. status: 500 or an error without a status field) still causes handler to return success: false (or throw, per the outer catch). Without this guard test, a future refactor that widens the getReviewError?.status === 404 check (e.g. to >= 400 or a truthy check) could silently convert real failures into false skipped: true no-ops, and CI wouldn't catch the regression.

Suggested addition:

it("still fails when getReview returns a non-404 error", async () => {
  const serverError = Object.assign(new Error("Internal Server Error"), { status: 500 });
  mockGetReview.mockRejectedValueOnce(serverError);

  const result = await handler({
    type: "dismiss_pull_request_review",
    review_id: 123,
    justification: "...",
  });

  expect(result.success).toBe(false);
  expect(mockDismissReview).not.toHaveBeenCalled();
});

Copilot AI Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 6aa73e4. I added a non-404 regression in /home/runner/work/gh-aw/gh-aw/actions/setup/js/dismiss_pull_request_review.test.cjs that keeps getReview 500s fatal and verifies dismissReview is not called.

});

@github-actions github-actions Bot Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] The test doesn't assert pull_request_number or repo fields on the returned no-op — these are part of the documented return shape and missing assertions allow silent regressions if the structure changes.

💡 Suggested additions 
expect(result.pull_request_number).toBeDefined();
expect(result.repo).toMatch(/\//);

All fields of the contract surface should be covered so a future refactor can't silently drop them.

@copilot please address this.

Copilot AI Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 6aa73e4. The 404 no-op regression now also asserts pull_request_number and repo in /home/runner/work/gh-aw/gh-aw/actions/setup/js/dismiss_pull_request_review.test.cjs so the full return shape stays covered.

Loading