fix: isolate each linked issue's processing when a PR closes multiple bounties - #81
Merged
chonilius merged 5 commits intoAug 17, 2026
Conversation
A PR body referencing the same issue twice (e.g. "Fixes MergeFi#12. Also resolves MergeFi#12 as discussed.") previously attempted to process the same bounty twice in one event — the second markMergedAndRelease call throws InvalidBountyTransitionError against the state the first call just left it in. Confirmed safe from double-payment either way (assertTransition blocks it), but it's a benign, false-alarm failure that shouldn't mark the webhook FAILED. Also adds the LinkedIssueOutcome type used by the per-issue isolation in the next commit (MergeFi#47).
handlePullRequest previously ran every linked issue's bounty processing in one unguarded loop — if markMergedAndRelease threw for the first linked issue (an invalid state transition, an escrow release failure, a Soroban error), the exception propagated straight out of the loop and every subsequent linked issue in the same PR was never attempted at all (MergeFi#47). Now returns a LinkedIssueOutcome per issue number (succeeded/skipped/ failed) instead of throwing on the first failure, so one bounty's problem no longer blocks the others linked from the same merged PR. handleEvent's use of this return value follows in the next commit.
Wires handlePullRequest's per-issue outcomes into a deliberate policy (applyPullRequestOutcomes, doc comment explains the choice among the three the issue names): event.status stays FAILED if any linked issue failed, but event.error now lists every failure by issue number instead of only whichever one happened to throw first. A PR where every linked issue succeeded or was skipped (no bounty) is PROCESSED, unchanged from before (MergeFi#47).
The issue's reproduction plan: a PR linking three issues where the middle one's markMergedAndRelease throws still releases the first and third bounties. event.status is FAILED, but event.error names only the failing issue (MergeFi#34) — the two successes aren't listed as failures.
The issue's other reproduction scenario: "Fixes MergeFi#42. This also resolves MergeFi#42 as discussed" only attempts bounty-42 once (de-duped), and the event stays PROCESSED rather than FAILED on the benign InvalidBountyTransitionError the second, redundant call would have thrown against the now-PAID bounty.
|
@boluwacodes is attempting to deploy a commit to the chonilius' projects Team on Vercel. A member of the Team first needs to authorize it. |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GithubWebhooksService.handlePullRequestprocessed every issue linked from a merged PR's body in one unguarded loop. IfmarkMergedAndReleasethrew for the first linked issue (an invalid state transition, an escrow release failure, a Soroban error), the exception propagated out of the loop and every subsequent linked issue in the same PR was never attempted — and the only error boundary (handleEvent's outer try/catch) marked the wholeWebhookEventFAILEDwith just the first error message, with no way to tell "only #12 failed" from "everything failed" fromevent.erroralone.There was also a more mundane trigger for the same failure mode:
extractLinkedIssueNumbersdidn't de-duplicate, so a PR body like "Fixes #12. This also resolves #12 as discussed." processed bounty #12 twice — the second call throwsInvalidBountyTransitionErroragainst the now-PAIDbounty, marking an otherwise-fully-successful mergeFAILED(confirmed safe from double-payment either way, but a real false-alarm cost for monitoring).succeeded/skipped/failedoutcome per issue instead of throwing on the first failure.applyPullRequestOutcomes):event.statusstaysFAILEDif any linked issue failed —FAILEDalready means "an operator should look at this," so it doesn't lose that signal — butevent.errornow lists every failure by issue number (#12: <message>; #56: <message>) instead of only whichever one happened to throw first. A PR where every linked issue succeeded or was skipped (no bounty) isPROCESSED, unchanged from before.Closes #47
Test plan
npm run lint— clean (0 errors; only pre-existing unrelated warnings intest/*.e2e-spec.ts)npx tsc --noEmit— cleannpm run build— succeedsnpm run test— 127/127 pass across 18 suites (7 pre-existing in this spec file + 2 new), including both scenarios from the issue's own reproduction plan:event.statusisFAILEDbutevent.errornames only the actual failure (#34), not the two successesPROCESSED, rather than falselyFAILEDon the benign duplicate-transition errornpm run test:cov— passes