Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 7 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@
| Only LOW or none, a human must look | COMMENT listing the reasons | passes, no approval |
| Only LOW or none, `approve-when-clean` off | COMMENT "would approve" | passes |
| Only LOW or none, `approve-when-clean` on | APPROVE | passes |
| Only LOW or none, but GitHub refuses the APPROVE | COMMENT quoting the refusal | passes |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LOW — this row makes a third COMMENT outcome, but the paragraph below still counts two: "REQUEST_CHANGES and APPROVE change the PR's state; the two COMMENT outcomes do not" (README.md:307-308). A reader counting rows in the table now finds three.

| No or unparseable output | nothing | fails |

"A human must look" is the reviewer's own answer to the *Does this need a
Expand All @@ -303,7 +304,7 @@

Each run adds a review; GitHub reviews are appended, not edited, so a PR with
ten pushes carries ten of them, and the newest is the one that describes the
head commit. REQUEST_CHANGES and APPROVE change the PR's state; the two

Check notice on line 307 in README.md

View workflow job for this annotation

GitHub Actions / claude-review / Automatic PR review

LOW: The new table row adds a third COMMENT outcome but the paragraph below still says the two COMMENT outcomes do not change the PR state
COMMENT outcomes do not.

Nothing is submitted when the reviewer crashed, on purpose: a changes-requested
Expand Down Expand Up @@ -335,9 +336,12 @@
re-reviewed, and `cancel-in-progress` makes that window real.
- **With the job token only:** enable *Allow GitHub Actions to create and
approve pull requests* in the repository's (or organisation's) Actions
settings, or APPROVE returns 422. A review the token cannot post is a
warning in the log, never a change to the check: the score decides the
exit status, so a clean PR stays green and simply gets no review.
settings, or APPROVE returns 422. A refused approval falls back to the
"would approve" COMMENT, which quotes the refusal and clears the run's own
earlier request-changes, so the PR is not left blocked by a round it has
since passed. Any other review the token cannot post is a warning in the
log, never a change to the check: the score decides the exit status, so a
clean PR stays green and simply gets no review.
- **CODEOWNERS:** if *Require review from Code Owners* is on, the approval only
satisfies it when the posting identity is a code owner.

Expand Down
51 changes: 37 additions & 14 deletions review/submit-verdict.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* clean, but a human must look COMMENT naming why, job passes
* clean, approve-when-clean off COMMENT "would approve", job passes
* clean, approve-when-clean on APPROVE, job passes
* clean, but GitHub refuses the APPROVE COMMENT saying so, job passes
* no or unparseable output nothing submitted, job fails
*
* Nothing is submitted on a crashed reviewer on purpose: a changes-requested
Expand Down Expand Up @@ -246,6 +247,17 @@
}
};

const approveHint = (error) => {
if (!/HTTP 422/.test(error.message)) return;
console.log(
'HTTP 422 on a review is usually one of two things: the job token is not allowed to ' +
'approve — enable "Allow GitHub Actions to create and approve pull requests" in the ' +
'repository or organisation Actions settings — or the owner of the github-token ' +
'secret authored this PR, which GitHub refuses to let anyone approve or request ' +
'changes on. Use a machine account or GitHub App rather than a person\'s token.'
);
};

const counts = ['BLOCKER', 'HIGH', 'MEDIUM', 'LOW']
.map((s) => [s, findings.filter((f) => f.severity === s).length])
.filter(([, n]) => n > 0)
Expand Down Expand Up @@ -289,11 +301,30 @@
]);
await dismissOwnStateReviews(own);
} else {
await submit('APPROVE', [
`**Approved.** ${countLine}`,
'',
'Nothing blocks and no human review is needed.',
]);
// An APPROVE supersedes the identity's earlier REQUEST_CHANGES only if
// it lands. Refused (the Actions approve setting, a token whose owner
// authored the PR), fall back to the COMMENT path so the stale block
// is still cleared and the refusal is on the PR, not only in the log.
try {
await submit('APPROVE', [
`**Approved.** ${countLine}`,
'',
'Nothing blocks and no human review is needed.',
]);
} catch (error) {

Check failure on line 314 in review/submit-verdict.mjs

View workflow job for this annotation

GitHub Actions / claude-review / Automatic PR review

HIGH: The APPROVE fallback catches every error, so a timed-out POST that GitHub processed gets its landed approval dismissed by dismissOwnStateReviews and replaced with a would-approve comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

HIGH — this catch treats every failure of the APPROVE POST as "GitHub refused the approval", but api() also throws when the request never got an answer: AbortSignal.timeout(15_000) fires, or the connection drops, after GitHub has already created the review. In that sequence the APPROVE lands (id A, this login, body carries MARKER), the client throws, the fallback posts the "would approve" COMMENT (id B), and dismissOwnStateReviews(B) then matches id A — state === 'APPROVED', different id, same login, marker present — and dismisses it. The PR ends the run with its approval actively dismissed and a comment quoting The operation was aborted due to timeout as a refusal. Before this change the timed-out approval was left standing.

The narrower non-422 cases are wrong in a smaller way: a 403 or a 5xx produces a PR comment asserting a refusal that did not happen, and pointing at the Actions approve setting, which is not the cause.

approveHint right above already has the right predicate — gating the fallback on it keeps the intended 422 behaviour and sends everything else to the outer catch, which is where "the environment could not post this review" was already handled:

Suggested change
} catch (error) {
} catch (error) {
// Only a refusal falls back. A request that never got an answer (the
// 15s timeout, a dropped connection) may have created the review
// anyway, and commenting then would dismiss the approval that landed.
if (!/HTTP 422/.test(error.message)) throw error;
console.log(`::warning::could not approve: ${escapeData(error.message)}`);

console.log(`::warning::could not approve: ${escapeData(error.message)}`);
approveHint(error);
const own = await submit('COMMENT', [
`**Would approve.** ${countLine}`,
'',
'Nothing blocks and no human review is needed, but GitHub refused the approval:',
'',
`> ${error.message.replace(/\s+/g, ' ').slice(0, 300)}`,
'',
'With the job token this needs *Allow GitHub Actions to create and approve pull requests* in the repository\'s Actions settings; otherwise pass a `github-token` from a machine account.',
]);
await dismissOwnStateReviews(own);
}
}
}
} catch (error) {
Expand All @@ -303,15 +334,7 @@
// red for good.
console.log(`::warning::could not submit the review: ${escapeData(error.message)}`);
console.log('The check still reports the score below; only the PR review is missing.');
if (/HTTP 422/.test(error.message)) {
console.log(
'HTTP 422 on a review is usually one of two things: the job token is not allowed to ' +
'approve — enable "Allow GitHub Actions to create and approve pull requests" in the ' +
'repository or organisation Actions settings — or the owner of the github-token ' +
'secret authored this PR, which GitHub refuses to let anyone approve or request ' +
'changes on. Use a machine account or GitHub App rather than a person\'s token.'
);
}
approveHint(error);
}

if (blocking.length === 0) {
Expand Down
Loading