diff --git a/src/github/github-webhooks.service.spec.ts b/src/github/github-webhooks.service.spec.ts index cc4d331..fe5bb14 100644 --- a/src/github/github-webhooks.service.spec.ts +++ b/src/github/github-webhooks.service.spec.ts @@ -136,6 +136,115 @@ describe('GithubWebhooksService', () => { expect(bountiesService.markMergedAndRelease).not.toHaveBeenCalled(); }); + describe('per-linked-issue isolation on a merged PR (#47)', () => { + function mockIssueAndBounty( + byNumber: Record, + ) { + issueRepo.findOne.mockImplementation( + ({ where }: { where: { number: number } }) => { + const entry = byNumber[where.number]; + return Promise.resolve( + entry + ? { id: `issue-${where.number}`, bounty: { id: entry.bountyId } } + : null, + ); + }, + ); + bountyRepo.findOne.mockImplementation( + ({ where }: { where: { id: string } }) => { + const entry = Object.values(byNumber).find( + (e) => e.bountyId === where.id, + ); + return Promise.resolve( + entry ? { id: where.id, status: entry.status } : null, + ); + }, + ); + } + + it("processes the first and third linked issues even when the middle one's bounty processing throws", async () => { + mockIssueAndBounty({ + 12: { bountyId: 'bounty-12', status: 'claimed' }, + 34: { bountyId: 'bounty-34', status: 'claimed' }, + 56: { bountyId: 'bounty-56', status: 'claimed' }, + }); + bountiesService.markMergedAndRelease.mockImplementation((id: string) => { + if (id === 'bounty-34') { + return Promise.reject(new Error('escrow release failed')); + } + return Promise.resolve(undefined); + }); + + const payload = { + action: 'closed', + number: 9, + pull_request: { + html_url: 'https://github.com/acme/repo/pull/9', + number: 9, + merged: true, + body: 'Fixes #12. Also fixes #34. Also fixes #56.', + }, + repository: { id: 999, full_name: 'acme/repo' }, + }; + + const event = await service.handleEvent( + 'pull_request', + 'delivery-partial-failure', + payload, + true, + ); + + // Both the working bounties were still released — #34's failure + // didn't abort the loop before reaching #56. + expect(bountiesService.markMergedAndRelease).toHaveBeenCalledWith( + 'bounty-12', + ); + expect(bountiesService.markMergedAndRelease).toHaveBeenCalledWith( + 'bounty-34', + ); + expect(bountiesService.markMergedAndRelease).toHaveBeenCalledWith( + 'bounty-56', + ); + expect(event.status).toBe(WebhookEventStatus.FAILED); + expect(event.error).toContain('#34'); + expect(event.error).toContain('escrow release failed'); + // The successful ones aren't mentioned as failures. + expect(event.error).not.toContain('#12'); + expect(event.error).not.toContain('#56'); + }); + + it('does not mark the event FAILED for a benign duplicate issue reference in the PR body', async () => { + mockIssueAndBounty({ + 42: { bountyId: 'bounty-42', status: 'claimed' }, + }); + bountiesService.markMergedAndRelease.mockResolvedValue(undefined); + + const payload = { + action: 'closed', + number: 10, + pull_request: { + html_url: 'https://github.com/acme/repo/pull/10', + number: 10, + merged: true, + body: 'Fixes #42. This also resolves #42 as discussed in review.', + }, + repository: { id: 999, full_name: 'acme/repo' }, + }; + + const event = await service.handleEvent( + 'pull_request', + 'delivery-duplicate-ref', + payload, + true, + ); + + // De-duplicated before processing — only attempted once, not twice. + expect(bountiesService.markMergedAndRelease).toHaveBeenCalledTimes(1); + expect(event.status).toBe(WebhookEventStatus.PROCESSED); + expect(event.error).toBeUndefined(); + }); + }); + describe('"issues" webhook events (#24)', () => { const payload = { action: 'edited', diff --git a/src/github/github-webhooks.service.ts b/src/github/github-webhooks.service.ts index 20646b3..ea950c6 100644 --- a/src/github/github-webhooks.service.ts +++ b/src/github/github-webhooks.service.ts @@ -32,6 +32,18 @@ interface GithubIssuesEventPayload { const CLOSING_KEYWORD_RE = /\b(close[sd]?|fix(e[sd])?|resolve[sd]?)\b\s*:?\s*(?:[\w.-]+\/[\w.-]+)?#(\d+)/gi; +/** + * The outcome of processing one issue number linked from a merged PR's + * body — tracked per-issue rather than collapsing a whole PR's several + * linked bounties into one pass/fail, so a failure on one doesn't hide + * what happened to the others (#47). + */ +interface LinkedIssueOutcome { + issueNumber: number; + outcome: 'succeeded' | 'skipped' | 'failed'; + error?: string; +} + @Injectable() export class GithubWebhooksService { private readonly logger = new Logger(GithubWebhooksService.name); @@ -83,16 +95,19 @@ export class GithubWebhooksService { try { if (eventType === 'pull_request') { - await this.handlePullRequest( + const outcomes = await this.handlePullRequest( payload as unknown as GithubPullRequestPayload, ); - } else if (eventType === 'issues') { - await this.handleIssueEvent( - payload as unknown as GithubIssuesEventPayload, - ); + this.applyPullRequestOutcomes(event, outcomes); + } else { + if (eventType === 'issues') { + await this.handleIssueEvent( + payload as unknown as GithubIssuesEventPayload, + ); + } + event.status = WebhookEventStatus.PROCESSED; + event.processedAt = new Date(); } - event.status = WebhookEventStatus.PROCESSED; - event.processedAt = new Date(); } catch (err) { event.status = WebhookEventStatus.FAILED; event.error = (err as Error).message; @@ -104,48 +119,114 @@ export class GithubWebhooksService { return this.webhookEventRepo.save(event); } + /** + * Decides `event.status`/`event.error` from a merged PR's per-linked- + * issue outcomes (#47) — a deliberate choice among the three the issue + * names as options: + * + * `event.status` stays FAILED whenever at least one linked issue's + * bounty processing failed, even if others succeeded. FAILED already + * means "an operator should look at this," so keeping it doesn't lose + * that signal — but `event.error` now lists every failure by issue + * number (`#12: ; #56: `) instead of only whichever + * one happened to throw first and abort the old unguarded loop, so an + * operator can see exactly which of the PR's several linked bounties + * actually need attention without re-deriving it from the PR body and + * bounty statuses by hand. A PR where every linked issue either + * succeeded or had no bounty to process (skipped) is PROCESSED, same + * as before this fix. + */ + private applyPullRequestOutcomes( + event: WebhookEvent, + outcomes: LinkedIssueOutcome[], + ): void { + const failures = outcomes.filter((o) => o.outcome === 'failed'); + if (failures.length === 0) { + event.status = WebhookEventStatus.PROCESSED; + event.processedAt = new Date(); + return; + } + + event.status = WebhookEventStatus.FAILED; + event.error = failures + .map((f) => `#${f.issueNumber}: ${f.error}`) + .join('; '); + this.logger.error( + `Partial failure processing linked issues for a merged PR: ${event.error}`, + ); + } + private async handlePullRequest( payload: GithubPullRequestPayload, - ): Promise { + ): Promise { if (payload.action !== 'closed' || !payload.pull_request.merged) { - return; + return []; } - const issueNumbers = this.extractLinkedIssueNumbers( - payload.pull_request.body ?? '', - ); + // De-duplicated so a PR body referencing the same issue twice (e.g. + // "Fixes #12. Also resolves #12 as discussed.") doesn't attempt to + // process the same bounty twice in one event — the second call would + // otherwise throw InvalidBountyTransitionError against the state the + // first call just left it in, which is a benign, false-alarm failure + // rather than a real one (#47). + const issueNumbers = [ + ...new Set( + this.extractLinkedIssueNumbers(payload.pull_request.body ?? ''), + ), + ]; if (issueNumbers.length === 0) { this.logger.warn( `PR #${payload.number} in ${payload.repository.full_name} merged but references no issue`, ); - return; + return []; } + // Each linked issue is processed in its own try/catch so one bounty's + // failure — an invalid state transition, an escrow release failure, + // a Soroban error — doesn't abort processing of every other bounty + // linked from the same merged PR (#47). + const outcomes: LinkedIssueOutcome[] = []; for (const number of issueNumbers) { - const issue = await this.issueRepo.findOne({ - where: { - number, - repository: { githubRepoId: String(payload.repository.id) }, - }, - relations: { repository: true, bounty: true }, - }); - if (!issue?.bounty) continue; - - // Mark in_review first if it hadn't been (idempotent no-op if already there). - const bounty = await this.bountyRepo.findOne({ - where: { id: issue.bounty.id }, - }); - if (!bounty) continue; - - if (bounty.status === BountyStatus.CLAIMED) { - await this.bountiesService.markInReview( - bounty.id, - payload.pull_request.html_url, - payload.pull_request.number, - ); + try { + const issue = await this.issueRepo.findOne({ + where: { + number, + repository: { githubRepoId: String(payload.repository.id) }, + }, + relations: { repository: true, bounty: true }, + }); + if (!issue?.bounty) { + outcomes.push({ issueNumber: number, outcome: 'skipped' }); + continue; + } + + // Mark in_review first if it hadn't been (idempotent no-op if already there). + const bounty = await this.bountyRepo.findOne({ + where: { id: issue.bounty.id }, + }); + if (!bounty) { + outcomes.push({ issueNumber: number, outcome: 'skipped' }); + continue; + } + + if (bounty.status === BountyStatus.CLAIMED) { + await this.bountiesService.markInReview( + bounty.id, + payload.pull_request.html_url, + payload.pull_request.number, + ); + } + await this.bountiesService.markMergedAndRelease(bounty.id); + outcomes.push({ issueNumber: number, outcome: 'succeeded' }); + } catch (err) { + outcomes.push({ + issueNumber: number, + outcome: 'failed', + error: (err as Error).message, + }); } - await this.bountiesService.markMergedAndRelease(bounty.id); } + return outcomes; } /**