Skip to content

feat: subscribe to pull_request.synchronize and .reopened (Bug #35) #50

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
avrabe wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: subscribe to pull_request.synchronize and .reopened (Bug #35) #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 80 additions & 3 deletions __tests__/integration/app.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() };
Expand Down
51 changes: 45 additions & 6 deletions src/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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!) {
Expand Down Expand Up @@ -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');
}
}

Expand Down Expand Up @@ -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) => {
Expand Down
Loading