Fall back from a refused APPROVE only on 422 - #18
Conversation
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.
| // 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; |
There was a problem hiding this comment.
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.
|
One finding: one LOW. A human should look at this before it merges. The change is correct and the reasoning behind it holds. The directions this moves are both safe. A refusal that did not create a No regression for the narrowed cases. A The README is consistent with the result. The table row at line 290 is the The one finding is on the diff line: Unrelated to the diff: |
There was a problem hiding this comment.
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.
Summary
Follow-up to #17, from its own review.
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 ticketpasses.