Fall back to the COMMENT path when GitHub refuses the APPROVE - #17
Conversation
An APPROVE clears the identity's earlier REQUEST_CHANGES only by superseding it, so when GitHub refuses the approval (the Actions approve setting is off, as on truenas-file-manager#1797) nothing was posted and the stale block stayed. A refused approval now posts the "would approve" COMMENT quoting the refusal and dismisses the run's own earlier state reviews.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| '', | ||
| 'Nothing blocks and no human review is needed.', | ||
| ]); | ||
| } catch (error) { |
There was a problem hiding this comment.
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:
| } 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)}`); |
| | 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 | |
There was a problem hiding this comment.
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.
|
Two findings: one HIGH, one LOW. A human should look at this one. The problem this fixes is real and correctly diagnosed: an APPROVE clears the HIGH — LOW — A human should look: |
## Summary Follow-up to #17, from its own review. - **HIGH:** the fallback treated every failure of the APPROVE POST as a refusal. A timeout or dropped connection after GitHub created the review would then post the COMMENT and dismiss the approval that had landed. Only an HTTP 422 now falls back; anything else goes to the outer catch, which warns and leaves the score's exit status, as before. - **LOW:** README said "the two COMMENT outcomes" after the table gained a third. Verified against a mocked API: a timeout on APPROVE warns and posts nothing; a 422 still falls back to the COMMENT and dismisses the run's own earlier request-changes. No Jira ticket on hand for the title; add one so `PR ticket` passes.
1 participant
Summary
On truenas-file-manager#1797 a clean run left an earlier request-changes review standing. The run tried to APPROVE (
approve-when-cleandefaults on) and GitHub answered:That repo has not enabled Allow GitHub Actions to create and approve pull requests. The refusal was a warning in the log, but the APPROVE path relied on the approval superseding the old request-changes, so with no approval landing nothing cleared it.
Now a refused APPROVE falls back to the "would approve" COMMENT, which quotes the refusal on the PR itself and dismisses the run's own earlier request-changes and approvals, the same as the other COMMENT paths. A successful APPROVE is unchanged.
Verified against a mocked API: refused approve → COMMENT + dismissal, exit 0; successful approve → APPROVE only.
No Jira ticket on hand for the title; add one so
PR ticketpasses.