Skip to content

Fall back from a refused APPROVE only on 422 - #18

Merged
william-gr merged 1 commit into
masterfrom
approve-fallback-422
Sep 13, 2026
Merged

william-gr merged 1 commit into
masterfrom
approve-fallback-422

Conversation

@william-gr

Copy link
Copy Markdown
Contributor

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.

A timed-out or dropped request may have created the approval; commenting
then would dismiss it. Anything but a refusal goes to the outer catch, as
before. README no longer counts two COMMENT outcomes.
@william-gr
william-gr merged commit 4d64a98 into master Sep 13, 2026
6 of 7 checks passed
@william-gr
william-gr deleted the approve-fallback-422 branch September 13, 2026 16:26
Comment thread review/submit-verdict.mjs
// Only a refusal falls back. A request that got no 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;

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 — "is this a refusal" is now the literal /HTTP 422/ in two places: here, and inside approveHint at line 251. Because line 320 is only reached once this test has already passed, approveHint's own guard is a no-op on this path — it still earns its keep at line 341, where the outer catch calls it on arbitrary errors.

The cost is that the two have to move together. If the refusal set ever widens — GitHub returning 403 for the Actions-approve setting, say — widening only this one gives a fallback COMMENT with no hint explaining it, and widening only approveHint prints the hint on a path that never falls back. A shared predicate says once what a refusal is:

const isRefusal = (error) => /HTTP 422/.test(error.message);

used by both approveHint and the guard here. Not blocking; the behaviour today is right.

@github-actions

Copy link
Copy Markdown

One finding: one LOW. A human should look at this before it merges.

The change is correct and the reasoning behind it holds. api() builds its
message as ${method} ${path} -> HTTP ${status} …, so /HTTP 422/ matches
exactly the responses GitHub actually refuses a review with, and the two
documented refusals — the Actions create and approve setting off, and a
token whose owner authored the PR — are both 422, which is already what
approveHint keys on. A TimeoutError from AbortSignal.timeout(15_000)
or a TypeError from a dropped connection carries no such text and now
reaches the outer catch, which is the point: the old code answered an
unanswered POST by posting a COMMENT and calling dismissOwnStateReviews,
and if the APPROVE had in fact landed that dismissal took it back out.

The directions this moves are both safe. A refusal that did not create a
review still falls back, still clears the run's own earlier
request-changes, and still puts the reason on the PR rather than only in
the log. A failure that may have created one now submits nothing, the score
still decides the exit status, and a stale request-changes is left standing
for a person rather than cleared on a guess — which is the same way the
rest of this script resolves "I don't know what happened".

No regression for the narrowed cases. A pull-requests: read token gets
403 on the APPROVE; the fallback COMMENT would have taken the same 403, so
dropping it loses a duplicate warning and nothing else.

The README is consistent with the result. The table row at line 290 is the
refusal, which the bullet at line 339 already ties to 422, and "any other
review the token cannot post is a warning in the log, never a change to the
check" at line 342 was written for the outer catch and now covers the
timed-out APPROVE too. The the two COMMENT outcomes correction matches
the three COMMENT rows in the table above it.

The one finding is on the diff line: /HTTP 422/ is now the refusal
predicate in two places and they have to stay in sync. It is a LOW, and
the check is not red for it.

Unrelated to the diff: check-ticket is failing because the PR title
carries no ticket, as the description notes.

@github-actions github-actions Bot left a 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.

Needs a human review. 1 finding(s): 1 LOW.

Nothing blocks, but this change is one a person should decide on:

  • review/submit-verdict.mjs:318 changes which review the workflow publishes on a failed APPROVE, and therefore whether a landed approval is preserved or dismissed toward a required-approval merge gate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant