Handle degenerate REVIEW_REQUIRED PR state in dismiss_pull_request_review#43657
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
dismiss_pull_request_review
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43657 does not have the 'implementation' label and has only 1 new line of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR hardens the dismiss_pull_request_review safe-output handler to handle PRs that appear “Review required” in some clients but have no dismissible requested-review entry, by adding an "auto" review-id resolution path and better error reporting.
Changes:
- Added
review_id: "auto"support to resolve the latest dismissible (APPROVED/CHANGES_REQUESTED) review authored by the current workflow actor. - Added degenerate-state detection for the “blocked + no requested reviewers/teams” case, returning a dedicated error.
- Improved robustness by paginating review enumeration (bounded) and updating tool/schema/type descriptions to document
"auto".
Show a summary per file
| File | Description |
|---|---|
| schemas/agent-output.json | Documents review_id accepting "auto" for dismiss review output items. |
| pkg/workflow/js/safe_outputs_tools.json | Updates safe-output tool schema description to mention "auto" resolution. |
| actions/setup/js/types/safe-outputs.d.ts | Updates TypeScript doc comment to mention "auto" behavior. |
| actions/setup/js/safe_outputs_tools.json | Updates setup-side safe-output tool schema description to mention "auto" resolution. |
| actions/setup/js/dismiss_pull_request_review.cjs | Implements "auto" resolution, degenerate-state detection, and bounded pagination. |
| actions/setup/js/dismiss_pull_request_review.test.cjs | Adds test coverage for "auto" resolution, degenerate-state detection, pagination, and actor matching. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/6 changed files
- Comments generated: 4
- Review effort level: Low
| "review_id": { | ||
| "type": ["number", "string"], | ||
| "description": "The numeric review ID to dismiss." | ||
| "description": "The numeric review ID to dismiss, or 'auto' to resolve the latest dismissible review authored by the current workflow actor." | ||
| }, |
| async function listAllPullRequestReviews(githubClient, owner, repo, pullRequestNumber) { | ||
| const all = []; | ||
| const maxPages = 10; | ||
| let page = 1; | ||
| while (page <= maxPages) { | ||
| const { data } = await githubClient.rest.pulls.listReviews({ | ||
| owner, | ||
| repo, | ||
| pull_number: pullRequestNumber, | ||
| per_page: 100, | ||
| page, | ||
| }); | ||
| if (!Array.isArray(data) || data.length === 0) break; | ||
| all.push(...data); | ||
| if (data.length < 100) break; | ||
| page++; | ||
| } | ||
| return all; | ||
| } |
| "review_id": { | ||
| "type": ["number", "string"], | ||
| "description": "Numeric pull request review ID to dismiss (for example, 123456789).", | ||
| "description": "Numeric pull request review ID to dismiss (for example, 123456789), or 'auto' to resolve the latest dismissible review authored by the current workflow actor.", | ||
| "x-synonyms": ["reviewId"] | ||
| }, |
| "review_id": { | ||
| "type": ["number", "string"], | ||
| "description": "Numeric pull request review ID to dismiss (for example, 123456789).", | ||
| "description": "Numeric pull request review ID to dismiss (for example, 123456789), or 'auto' to resolve the latest dismissible review authored by the current workflow actor.", | ||
| "x-synonyms": ["reviewId"] | ||
| }, |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 93/100 — Excellent
📊 Metrics (9 tests)
Score formula: Verdict
References: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
|
Investigated PR #43649. It's blocked by two The current PR #43649 also has The two blocking reviews flag real issues still present in the code:
Would you like me to fix those issues on PR #43649's branch so the bot reviews can re-run and clear, or is there a different aspect of the degenerate state you'd like me to address? |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting with actionable issues; not blocking merge.
📋 Key Themes & Highlights
Key Themes
mergeable_statereliability: The degenerate-state detection heuristic (mergeable_state === "blocked") uses an undocumented, internal API field. A PR blocked for unrelated reasons (missing status checks, branch protection, dirty branch) will trigger the misleading "degenerate review-required state" error.- Staged mode contract gap: When
review_id: "auto"is used in staged mode, the response returnsreview_id: "auto"instead of the resolved numeric ID. Live runs return a number; staged runs return a string — asymmetric contract. - Test isolation: The
GITHUB_ACTORenv mutation in one test isn't cleaned up with afinallyblock, making the suite subtly order-dependent. - Weak pagination test assertion: The pagination-cap test verifies call count but doesn't assert the error message, so it can't distinguish the "no match after full pagination" path from the "degenerate state" path.
Positive Highlights
- ✅ Excellent coverage breadth — 6 new test cases cover auto-resolution, degenerate detection, pagination, actor matching, and edge cases
- ✅ Bounded pagination (max 10 pages × 100/page) is a good defensive pattern
- ✅
findLatestDismissibleReviewForActoris a clean, well-typed pure function with a clear sort strategy and ID tie-breaker - ✅ The
Promise.allto fetch PR metadata and reviews concurrently is a nice performance touch - ✅ Schema/type docs kept in sync across all 4 locations
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 64.5 AIC · ⌖ 10.7 AIC · ⊞ 6.7K
Comment /matt to run again
|
|
||
| const autoReview = findLatestDismissibleReviewForActor(reviews, expectedAuthor); | ||
| if (!autoReview || !Number.isInteger(autoReview.id) || autoReview.id <= 0) { | ||
| const isBlockedWithoutReviewers = hasNoRequestedReviewersOrTeams(pullRequest) && pullRequest?.mergeable_state === "blocked"; |
There was a problem hiding this comment.
[/diagnosing-bugs] The degenerate-state check relies on mergeable_state === "blocked" — an undocumented, internal GitHub API field not in the REST contract. A PR can be blocked for many other reasons (missing status checks, branch protection rules, merge conflicts), so this condition may misidentify normal blocked PRs as the degenerate "no-reviewer" case.
💡 Suggested improvement
Consider narrowing the heuristic or documenting its limitations explicitly:
// mergeable_state is undocumented — valid known values include:
// "clean", "blocked", "behind", "unstable", "draft", "dirty"
// We rely on it being "blocked" alongside empty reviewers/teams as
// a proxy for the REVIEW_REQUIRED degenerate state.Alternatively, always return the actor-specific error (review_id=auto did not find a dismissible review authored by ...) regardless, so callers don't get a false-positive "degenerate state" diagnosis on a legitimately blocked PR.
@copilot please address this.
| @@ -99,19 +172,47 @@ | |||
| if (filterResult) return filterResult; | |||
|
|
|||
| if (isStaged) { | |||
There was a problem hiding this comment.
[/diagnosing-bugs] In staged mode, review_id: "auto" is returned verbatim in the response without resolving the actual review ID. This means the caller gets no useful preview of which review would be dismissed — the staged output has a different shape than the live output ("auto" vs a numeric ID). This breaks the contract symmetry between staged and live runs.
💡 Suggested fix
For staged mode, still fetch and resolve the review (or at least validate that a dismissible review exists), so the preview includes the real numeric review_id. This makes staged previews trustworthy and catches misconfiguration early:
if (isStaged) {
// Resolve auto even in staged mode so the preview shows the real review ID
if (useAutoReviewId) {
const reviews = await listAllPullRequestReviews(...);
const autoReview = findLatestDismissibleReviewForActor(reviews, expectedAuthor);
if (!autoReview) return { success: false, error: '...' };
reviewId = autoReview.id;
}
logStagedPreviewInfo(`Would dismiss review #${reviewId} ...`);
...
}@copilot please address this.
| expect(mockDismissReview).toHaveBeenCalledWith(expect.objectContaining({ review_id: 777 })); | ||
| }); | ||
|
|
||
| it("caps review pagination to avoid excessive API calls", async () => { |
There was a problem hiding this comment.
[/tdd] The pagination cap test verifies that mockListReviews is called exactly 10 times, but it doesn't assert the error message or confirm that success: false is due to exhausted pagination specifically versus finding no matching review. Both cases return success: false and could mask regression.
💡 Suggested addition
Add a more specific assertion to distinguish the "no match after full pagination" case:
expect(result.success).toBe(false);
expect(result.error).toContain("did not find a dismissible review"); // not "degenerate"
expect(mockListReviews).toHaveBeenCalledTimes(10);This ensures the pagination-exhaustion path returns the right error, not the degenerate-state error.
@copilot please address this.
| ); | ||
| }); | ||
|
|
||
| it("uses GITHUB_ACTOR for review_id=auto actor matching", async () => { |
There was a problem hiding this comment.
[/tdd] The GITHUB_ACTOR test modifies process.env.GITHUB_ACTOR and calls vi.resetModules() + require() to reload the module, but it doesn't restore process.env.GITHUB_ACTOR after the test. This leaks the "custom-bot" actor into subsequent tests if test order changes.
💡 Suggested fix
Add cleanup in the test or rely on the beforeEach that already sets process.env.GITHUB_ACTOR = "github-actions[bot]". Since beforeEach runs before each test and calls vi.resetModules(), the leak is currently harmless — but it's fragile. Consider using afterEach or a scoped approach:
it('uses GITHUB_ACTOR for review_id=auto actor matching', async () => {
const origActor = process.env.GITHUB_ACTOR;
try {
process.env.GITHUB_ACTOR = 'custom-bot';
// ... test body ...
} finally {
process.env.GITHUB_ACTOR = origActor;
}
});@copilot please address this.
|
@copilot update safe outputs to dismiss all review request from the request user to deal with these cases. |
There was a problem hiding this comment.
Review: Handle degenerate REVIEW_REQUIRED PR state in dismiss_pull_request_review
The approach is well-structured and the new test coverage is thorough. Found 4 non-blocking issues worth addressing:
-
Unnecessary
Promise.allon happy path (line 190, main file) —pullRequestis only needed in the failure branch, so thepulls.getcall is wasted on every successful auto-resolution. Lazy fetch is the fix. -
GITHUB_ACTORenv var leaks between tests (line 259, test file) — no teardown restores the value after thecustom-bottest, risking ordering-dependent test failures. -
mergeable_state === "blocked"is over-broad (line 201, main file) — many things causeblockedstate beyond the degenerate review-required case. The error message should be hedged as a best-guess diagnosis. -
Staged mode bypasses auto-resolution (line 174, main file) — a staged run with
review_id=autoalways returnssuccess: truewithout checking whether a dismissible review exists. Staged previews should surface the same failure paths as live runs.
All four inline comments include specific suggestions. None block correctness of the happy path.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 99.6 AIC · ⌖ 8.75 AIC · ⊞ 4.9K
|
|
||
| try { | ||
| if (useAutoReviewId) { | ||
| const [{ data: pullRequest }, reviews] = await Promise.all([ |
There was a problem hiding this comment.
Unnecessary eager fetch of PR details on happy path. pullRequest is only used in the failure branch (when no dismissible review is found), yet it is fetched unconditionally in a Promise.all on every review_id=auto call. On the common happy path this is a wasted API call.
Suggest fetching it lazily:
const reviews = await listAllPullRequestReviews(githubClient, owner, repo, pullRequestNumber);
const autoReview = findLatestDismissibleReviewForActor(reviews, expectedAuthor);
if (!autoReview || !Number.isInteger(autoReview.id) || autoReview.id <= 0) {
const { data: pullRequest } = await githubClient.rest.pulls.get({ owner, repo, pull_number: pullRequestNumber });
...
}@copilot please address this.
| }); | ||
|
|
||
| it("uses GITHUB_ACTOR for review_id=auto actor matching", async () => { | ||
| process.env.GITHUB_ACTOR = "custom-bot"; |
There was a problem hiding this comment.
GITHUB_ACTOR env var leak across tests. This test sets process.env.GITHUB_ACTOR = "custom-bot" but there is no afterEach or afterAll that restores the original value. Because vi.resetModules() is called in beforeEach but the env var is not reset, any test that runs after this one (e.g., in the same file with a different order) would unexpectedly use custom-bot as the actor.
Add cleanup in an afterEach or restore at the end of this test:
const previousActor = process.env.GITHUB_ACTOR;
process.env.GITHUB_ACTOR = "custom-bot";
// ... test body ...
process.env.GITHUB_ACTOR = previousActor;@copilot please address this.
|
|
||
| const autoReview = findLatestDismissibleReviewForActor(reviews, expectedAuthor); | ||
| if (!autoReview || !Number.isInteger(autoReview.id) || autoReview.id <= 0) { | ||
| const isBlockedWithoutReviewers = hasNoRequestedReviewersOrTeams(pullRequest) && pullRequest?.mergeable_state === "blocked"; |
There was a problem hiding this comment.
mergeable_state === "blocked" is too broad for degenerate-state detection. A PR can be blocked for many reasons: failing required status checks, a required review is pending from a different reviewer, branch protection mismatches, etc. None of those are the degenerate review-required state this code is trying to detect.
The combination of blocked + no requested_reviewers + no requested_teams + no dismissible actor review is a reasonable heuristic, but the code and error message present it as a definitive detection. Consider softening the message to reflect this is a best-guess diagnosis:
error: "no dismissible actor-authored review was found; PR may be in a degenerate REVIEW_REQUIRED state (blocked with no pending reviewer requests)"This avoids misleading callers who hit blocked for unrelated reasons.
@copilot please address this.
| @@ -99,19 +172,47 @@ | |||
| if (filterResult) return filterResult; | |||
|
|
|||
| if (isStaged) { | |||
There was a problem hiding this comment.
Staged mode with review_id=auto reports success: true without verifying a dismissible review exists. In staged/dry-run mode the function returns early (line 178) with review_id: "auto" in the payload, bypassing the auto-resolution logic entirely. This means a staged run will always succeed even when the same payload would fail at runtime (e.g., no actor-authored review, or degenerate state).
Consider running the review lookup in staged mode too (skip only the actual dismissReview call), so that staged previews surface failures before they reach production.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — three correctness issues need fixing before merge.
### Blocking findings (3)
-
mergeable_staterace condition (line 201): GitHub computesmergeable_stateasynchronously; it returnsnullor"unknown"immediately after pushes/review events. The=== "blocked"gate reliably misses the degenerate state in those windows, defeating the core purpose of this PR. -
hasNoRequestedReviewersOrTeamsfalse-negative when fields are absent (line 63):Array.isArray(undefined)isfalse, so the function returnsfalsewhen the API response omits either field — degenerate-state detection silently never triggers. -
Staged+auto path emits
review_id: "auto"(string) (line 175): every non-staged path returns a numericreview_id; this inconsistency will cause schema/type mismatches for downstream consumers.
Non-blocking findings (1)
- Silent 1000-review cap (line 77): no warning is emitted when pagination hits
maxPages, so the wrong (non-latest) review may be silently selected on PRs with long review histories.
🔎 Code quality review by PR Code Quality Reviewer · 155.9 AIC · ⌖ 8.55 AIC · ⊞ 5.4K
Comment /review to run again
|
|
||
| const autoReview = findLatestDismissibleReviewForActor(reviews, expectedAuthor); | ||
| if (!autoReview || !Number.isInteger(autoReview.id) || autoReview.id <= 0) { | ||
| const isBlockedWithoutReviewers = hasNoRequestedReviewersOrTeams(pullRequest) && pullRequest?.mergeable_state === "blocked"; |
There was a problem hiding this comment.
mergeable_state race condition silently defeats degenerate-state detection: GitHub computes mergeable_state asynchronously and commonly returns null or "unknown" immediately after pushes/review events, so === "blocked" will miss the degenerate state during those windows.
💡 Suggested fix
The degenerate condition is really no requested reviewers/teams — mergeable_state should not be the gating signal:
// Before (fragile — misses null/"unknown" during async recompute)
const isBlockedWithoutReviewers =
hasNoRequestedReviewersOrTeams(pullRequest) &&
pullRequest?.mergeable_state === "blocked";
// After (reliable — only checks the structural invariant)
const isBlockedWithoutReviewers = hasNoRequestedReviewersOrTeams(pullRequest);If the intent is to distinguish a truly blocked PR from one that just has no pending reviewers for other reasons, document the exact conditions under which mergeable_state can be trusted and add a test for null/"unknown". Otherwise drop the mergeable_state guard entirely; the requested_reviewers/requested_teams check alone describes the misconfigured state the PR describes.
| * @returns {boolean} | ||
| */ | ||
| function hasNoRequestedReviewersOrTeams(pullRequest) { | ||
| const hasNoRequestedReviewers = Array.isArray(pullRequest?.requested_reviewers) && pullRequest.requested_reviewers.length === 0; |
There was a problem hiding this comment.
hasNoRequestedReviewersOrTeams silently returns false when fields are absent: if the pulls.get response omits requested_reviewers or requested_teams (e.g. a minimal/partial response), Array.isArray(undefined) is false, so both local variables are false and the function returns false — degenerate-state detection never fires.
💡 Suggested fix
Treat absent fields as empty (zero reviewers), which is the safe default for this invariant:
function hasNoRequestedReviewersOrTeams(pullRequest) {
const reviewers = pullRequest?.requested_reviewers;
const teams = pullRequest?.requested_teams;
const hasNoReviewers = !Array.isArray(reviewers) || reviewers.length === 0;
const hasNoTeams = !Array.isArray(teams) || teams.length === 0;
return hasNoReviewers && hasNoTeams;
}This way a missing field is treated as an empty list (no reviewers requested) rather than silently suppressing the detection path.
|
|
||
| if (isStaged) { | ||
| logStagedPreviewInfo(`Would dismiss review #${reviewId} on PR #${pullRequestNumber} (${owner}/${repo}) as ${dismisser}`); | ||
| const previewReviewId = useAutoReviewId ? "auto" : reviewId; |
There was a problem hiding this comment.
Staged+auto path returns review_id: "auto" (string) instead of a resolved integer: every non-staged code path returns review_id as a number, but the staged+auto early-return emits the string "auto". Downstream consumers (schema validation, workflow steps reading the output) that expect a numeric ID will receive a type mismatch with no indication that resolution was skipped.
💡 Suggested fix
Document the intentional difference or normalise the output. At minimum, add a review_id_resolved: false flag to the staged response so consumers can distinguish it from a real ID:
return {
success: true,
staged: true,
review_id: previewReviewId, // still "auto" — resolution skipped in staged mode
review_id_resolved: !useAutoReviewId,
pull_request_number: pullRequestNumber,
repo: `${owner}/${repo}`,
author: expectedAuthor,
};Or if a string "auto" is intentionally allowed in the output schema, update the TypeScript type and JSON schema to reflect that.
| */ | ||
| async function listAllPullRequestReviews(githubClient, owner, repo, pullRequestNumber) { | ||
| const all = []; | ||
| const maxPages = 10; |
There was a problem hiding this comment.
Silent truncation at 1000 reviews can select the wrong (non-latest) review to dismiss: maxPages=10 with per_page=100 caps total fetched reviews at 1000 with no warning. On PRs with >1000 reviews, findLatestDismissibleReviewForActor sorts over an incomplete set and may select a stale review, silently dismissing the wrong one.
💡 Suggested fix
At minimum, log a warning when the cap is reached so the caller is not blindsided:
if (page > maxPages) {
// Fetched maximum reviewable pages; actor's latest review may be
// in the unfetched tail. Log and surface this to the caller.
console.warn(`[listAllPullRequestReviews] Reached max page limit (${maxPages}); review list may be incomplete.`);
}
return all;A stronger fix would be to short-circuit once a page containing an actor-authored dismissible review is found (since GitHub returns reviews in ascending chronological order, scan from the last page backward). That avoids fetching the entire history and eliminates the silent-truncation risk.
…he latest Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Key changes in commit
|
|
@copilot auto is the default value of review_id is not provided |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done.
|
|
@copilot please run the Remaining feedback to close out:
Run: https://github.com/github/gh-aw/actions/runs/28764674525
|
|
🎉 This pull request is included in a new release. Release: |
Some PRs surface as Review required in mobile iOS while having no dismissible requested-review entry in the web UX. This change hardens
dismiss_pull_request_reviewso it can detect and handle that degenerate state instead of failing with an opaque “review not found” path.Auto review resolution for dismissals
review_id: "auto"support to resolve the latest actor-authored dismissible review (APPROVEDorCHANGES_REQUESTED) before callingdismissReview.Degenerate-state detection
review_id: "auto"finds no actor-authored dismissible review, the handler now checks PR state and returns a dedicated error for the blocked/no-requested-reviewers-no-requested-teams case.Review lookup robustness
Contract/schema sync
review_idaccepts either a numeric ID or"auto".{ "type": "dismiss_pull_request_review", "pull_request_number": 43495, "review_id": "auto", "justification": "Dismissing stale actor-authored review after follow-up changes." }