From 3c32eb1c9be4f8efabeb8b9863b46e2eb5987bd4 Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Fri, 1 May 2026 20:24:18 +0200 Subject: [PATCH] feat: subscribe to pull_request.synchronize and .reopened (Bug #35) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: Probot/Octokit-expert agent flagged that the bot only listened to `pull_request.opened` and `.closed`. Real-world PR lifecycles include: - force-push / new commits → `synchronize` (bot was silent) - reopen-after-close → `reopened` (bot was silent) The bot couldn't re-review on force-push, which was the most-asked-for behaviour during rivet PR triage. What: - Extract `handlePullRequestOpenedOrReopened` so `.opened` and `.reopened` share one code path (auto-merge enable + AI review + dependabot config check). - New `pull_request.synchronize` handler runs only AI review. `reviewPullRequest` already calls `supersedePreviousReviews`, so the older bot review is hidden and the PR shows only the latest. - Bot-authored PRs (dependabot, etc.) are still skipped on synchronize just like on opened. Out of scope (deferred to a follow-up): pull_request_review, pull_request_review_comment, check_run.rerequested, installation.created, installation_repositories.added — these need non-trivial behaviour decisions and shouldn't be lumped in. Test plan: - 4 new tests in __tests__/integration/app.test.js covering: subscription registration, synchronize → review, synchronize-bot → skip, reopened → review. - Updated `app.on` call-count assertion 6 → 8. - npm test → 838 pass (was 834). - npm run lint → clean. Risk: low — handler shares the same code path as `.opened` for review, which is exercised in production daily. The synchronize event arrives on every push so the AI review queue load increases, but the existing `reviewPullRequest` already throttles via `_reviewTimestamps` and supersedes previous reviews. Co-Authored-By: Claude Opus 4.7 (1M context) --- __tests__/integration/app.test.js | 83 +++++++++++++++++++++++++++++-- src/app.js | 51 ++++++++++++++++--- 2 files changed, 125 insertions(+), 9 deletions(-) diff --git a/__tests__/integration/app.test.js b/__tests__/integration/app.test.js index da1cb5a..535a226 100644 --- a/__tests__/integration/app.test.js +++ b/__tests__/integration/app.test.js @@ -644,12 +644,89 @@ describe('app', () => { it('works without getRouter option', () => { const { app } = setupApp({ skipRouter: true }); - // 6 events: repository.created, issues.opened, issue_comment.created, - // pull_request.opened, pull_request.closed, push - expect(app.on).toHaveBeenCalledTimes(6); + // 8 events: repository.created, issues.opened, issue_comment.created, + // pull_request.opened, pull_request.reopened, + // pull_request.synchronize, pull_request.closed, push + expect(app.on).toHaveBeenCalledTimes(8); expect(app.onError).toHaveBeenCalledTimes(1); }); + it('subscribes to pull_request.opened, .reopened, .synchronize, .closed', () => { + const { app } = setupApp(); + expect(app.on).toHaveBeenCalledWith('pull_request.opened', expect.any(Function)); + expect(app.on).toHaveBeenCalledWith('pull_request.reopened', expect.any(Function)); + expect(app.on).toHaveBeenCalledWith('pull_request.synchronize', expect.any(Function)); + expect(app.on).toHaveBeenCalledWith('pull_request.closed', expect.any(Function)); + }); + + it('pull_request.synchronize re-runs reviewPullRequest when ai_review enabled', async () => { + _setConfigForTesting({ ai_review: { enabled: true } }); + reviewPullRequest.mockResolvedValue({ success: true }); + const { handlers } = setupApp(); + + const octokit = createMockOctokit(); + const context = { + id: 'delivery-sync-1', + log: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, + octokit, + payload: { + pull_request: { number: 99, node_id: 'PR_99', user: { login: 'human-author' } }, + repository: { + name: 'rivet', + owner: { login: 'pulseengine' }, + default_branch: 'main' + } + } + }; + + await handlers['pull_request.synchronize'](context); + + expect(reviewPullRequest).toHaveBeenCalledWith(octokit, 'pulseengine', 'rivet', 99); + }); + + it('pull_request.synchronize skips review for bot-authored PRs', async () => { + _setConfigForTesting({ ai_review: { enabled: true } }); + reviewPullRequest.mockClear(); + const { handlers } = setupApp(); + + const context = { + id: 'delivery-sync-2', + log: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, + octokit: createMockOctokit(), + payload: { + pull_request: { number: 100, node_id: 'PR_100', user: { login: 'dependabot[bot]' } }, + repository: { name: 'rivet', owner: { login: 'pulseengine' }, default_branch: 'main' } + } + }; + + await handlers['pull_request.synchronize'](context); + + expect(reviewPullRequest).not.toHaveBeenCalled(); + }); + + it('pull_request.reopened runs the same opened path (review + dependabot check)', async () => { + _setConfigForTesting({ ai_review: { enabled: true } }); + reviewPullRequest.mockClear(); + reviewPullRequest.mockResolvedValue({ success: true }); + const { handlers } = setupApp(); + + const octokit = createMockOctokit(); + const context = { + id: 'delivery-reopen-1', + log: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, + octokit, + payload: { + pull_request: { number: 50, node_id: 'PR_50', user: { login: 'human' } }, + repository: { name: 'rivet', owner: { login: 'pulseengine' }, default_branch: 'main' }, + sender: { login: 'human' } + } + }; + + await handlers['pull_request.reopened'](context); + + expect(reviewPullRequest).toHaveBeenCalledWith(octokit, 'pulseengine', 'rivet', 50); + }); + it('health endpoint returns correct response', () => { const { routeHandlers } = setupApp(); const mockRes = { status: jest.fn().mockReturnThis(), json: jest.fn() }; diff --git a/src/app.js b/src/app.js index db1446d..c46864b 100644 --- a/src/app.js +++ b/src/app.js @@ -836,8 +836,10 @@ function registerApp(app, options = {}) { } }); - // PR opened: auto-merge for bots, AI review, dependabot generation check - app.on('pull_request.opened', async (context) => { + // PR opened/reopened: auto-merge for bots, AI review, dependabot generation + // check. `.reopened` is symmetric with `.opened` — a closed PR being + // reopened should re-arm auto-merge and re-run the same checks. + async function handlePullRequestOpenedOrReopened(context, eventName) { if (context.log) setLogger(context.log); const config = getConfig(); const pr = context.payload.pull_request; @@ -856,7 +858,7 @@ function registerApp(app, options = {}) { if (isDependabot || isBotUser) { const mergeMethod = autoMerge.merge_method || "squash"; - getLogger().info({ pr: pr.number, sender, mergeMethod }, "Enabling auto-merge"); + getLogger().info({ pr: pr.number, sender, mergeMethod, event: eventName }, "Enabling auto-merge"); try { const query = `mutation($prId: ID!, $mergeMethod: PullRequestMergeMethod!) { @@ -892,12 +894,12 @@ function registerApp(app, options = {}) { try { const result = await reviewPullRequest(context.octokit, owner, repo, pr.number); if (result.success) { - getLogger().info({ pr: pr.number, repo: `${owner}/${repo}` }, 'Auto AI review posted'); + getLogger().info({ pr: pr.number, repo: `${owner}/${repo}`, event: eventName }, 'Auto AI review posted'); } else { - getLogger().warn({ pr: pr.number, error: result.error }, 'Auto AI review skipped'); + getLogger().warn({ pr: pr.number, error: result.error, event: eventName }, 'Auto AI review skipped'); } } catch (err) { - getLogger().warn({ pr: pr.number, err: err.message }, 'Auto AI review failed'); + getLogger().warn({ pr: pr.number, err: err.message, event: eventName }, 'Auto AI review failed'); } } @@ -926,6 +928,43 @@ function registerApp(app, options = {}) { } } } + } + + app.on('pull_request.opened', (context) => handlePullRequestOpenedOrReopened(context, 'opened')); + app.on('pull_request.reopened', (context) => handlePullRequestOpenedOrReopened(context, 'reopened')); + + // Re-review on force-push / new commits. supersedePreviousReviews inside + // reviewPullRequest hides the previous bot comment so the PR shows only + // the latest review. We deliberately skip the auto-merge and dependabot + // checks — those decisions don't change when the diff is updated, only + // when the PR is created or revived. + app.on('pull_request.synchronize', async (context) => { + if (context.log) setLogger(context.log); + const config = getConfig(); + const pr = context.payload.pull_request; + const { repository } = context.payload; + const owner = repository.owner.login; + const repo = repository.name; + const sender = pr.user?.login || ""; + + const isBotPR = sender === "dependabot[bot]" || + sender.endsWith("[bot]") || + (config?.auto_merge?.on_bot_users || []).some( + bot => sender === bot || sender === bot + "[bot]" + ); + + if (!config?.ai_review?.enabled || isBotPR) return; + + try { + const result = await reviewPullRequest(context.octokit, owner, repo, pr.number); + if (result.success) { + getLogger().info({ pr: pr.number, repo: `${owner}/${repo}` }, 'Re-reviewed PR after synchronize'); + } else { + getLogger().warn({ pr: pr.number, error: result.error }, 'Re-review skipped after synchronize'); + } + } catch (err) { + getLogger().warn({ pr: pr.number, err: err.message }, 'Re-review failed after synchronize'); + } }); app.on('pull_request.closed', async (context) => {