From 9f5010aa51c6891a90e1a00417c0ed1a4e8e82a3 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Sun, 6 Sep 2026 21:09:47 +0200 Subject: [PATCH] fix(pr-bot): announce a bot-made merge once, cap automatic retries, show why a review failed Observed on the first live merge (#383 via the Telegram button): runConfirmed announced the merge and the scan five seconds later announced it again as a closed PR. The scan now stays quiet for PRs the bot itself merged or closed, and a merge of a `merge-with-fixes` verdict reminds that merging applies none of the listed fixes. A failed review used to be re-queued on every scan with no limit (two PRs failed once each and were retried fine, but a head that keeps failing would cost a session every ten minutes forever): three failures on one head now stop the automatic retries until /review N or a new push. The failure notice carries the reviewer's last message, so "finished without writing report.json" says what it wrote instead. Co-Authored-By: Claude Fable 5.1 --- scripts/pr-bot/bot.ts | 47 ++++++++++++++++++++++++++++++------ scripts/pr-bot/state.ts | 3 +++ test/pr-bot-commands.test.ts | 16 ++++++++++-- 3 files changed, 56 insertions(+), 10 deletions(-) diff --git a/scripts/pr-bot/bot.ts b/scripts/pr-bot/bot.ts index 7d48750d..1526eb22 100644 --- a/scripts/pr-bot/bot.ts +++ b/scripts/pr-bot/bot.ts @@ -1,7 +1,7 @@ /** * @fileoverview The PR bot: polls the repository's open pull requests, reviews each * one (once per head commit) in a Codeman claude session running in a private - * worktree, and reports to the maintainer over Telegram with a verdict, the ranked + * clone, and reports to the maintainer over Telegram with a verdict, the ranked * findings, a recommendation and action buttons. * * Three rules shape everything here: @@ -80,6 +80,8 @@ export interface PrBotDeps { } const CONFIRM_TTL_MS = 15 * 60_000; +/** A head that failed this many times is left alone until /review N or a new push. */ +const MAX_AUTO_RETRIES = 3; const MAX_FOLLOWUPS = 2; const REPORT_INLINE_MAX = 3000; @@ -135,6 +137,8 @@ export class PrBot { private reviewQueue: number[] = []; private readonly busy = new Set(); private readonly createdSessions = new Set(); + /** PRs the bot itself merged or closed: the scan's close notice would repeat what runConfirmed already said. */ + private readonly selfClosed = new Set(); private followupsRunning = 0; private stopped = false; private scanning = false; @@ -248,7 +252,9 @@ export class PrBot { } if (rec.status === 'skipped') rec.status = rec.reviewedSha ? 'reviewed' : 'new'; const needsReview = rec.reviewedSha !== pr.headSha; - if (needsReview && !this.busy.has(pr.number) && !this.reviewQueue.includes(pr.number)) candidates.push(pr); + const gaveUp = (rec.failedAttempts ?? 0) >= MAX_AUTO_RETRIES && rec.failedSha === pr.headSha; + if (needsReview && !gaveUp && !this.busy.has(pr.number) && !this.reviewQueue.includes(pr.number)) + candidates.push(pr); } if (this.cfg.autoReview && !this.store.state.paused) { for (const pr of orderBacklog(candidates)) { @@ -292,6 +298,7 @@ export class PrBot { prNumber: rec.number, log: (m) => this.log(`[#${rec.number}] ${m}`), }).catch((err) => this.log(`worktree cleanup #${rec.number}: ${errText(err)}`)); + if (this.selfClosed.delete(rec.number)) return; // announced by runConfirmed already await this.telegram .sendMessage( `${merged ? '๐ŸŽ‰ Merged' : '๐Ÿ”’ Closed'} #${rec.number} ยท ${escapeHtml(rec.title)} (${escapeHtml(rec.author)})` @@ -419,18 +426,21 @@ export class PrBot { } let report: ReviewReport | null = null; + let last = ''; if (existsSync(reportJsonPath)) report = parseReport(extractJsonObject(readFileSync(reportJsonPath, 'utf8'))); if (!report) { - const last = await this.pollLastResponse(sessionId); + last = await this.pollLastResponse(sessionId); report = parseReport(extractJsonObject(last)); if (report && !existsSync(reportMdPath)) writeFileSync(reportMdPath, last); } await this.recordClaudeSessionId(sessionId, rec); - if (!report) throw new Error(await this.describeFailure(sessionId, outcome, started)); + if (!report) throw new Error(await this.describeFailure(sessionId, outcome, started, last)); const durationMin = Math.max(1, Math.round((Date.now() - started) / 60_000)); Object.assign(rec, { status: 'reviewed', + failedAttempts: 0, + failedSha: undefined, reviewedSha: detail.headSha, reviewedAt: new Date().toISOString(), reviewDurationMin: durationMin, @@ -450,8 +460,16 @@ export class PrBot { if (rec) { rec.status = 'failed'; rec.lastError = reason; + rec.failedAttempts = rec.failedSha === rec.headSha ? (rec.failedAttempts ?? 0) + 1 : 1; + rec.failedSha = rec.headSha; + const givingUp = rec.failedAttempts >= MAX_AUTO_RETRIES; await this.telegram - .sendMessage(formatReviewFailure(rec, reason)) + .sendMessage( + formatReviewFailure(rec, reason) + + (givingUp + ? `\n\nThat was attempt ${rec.failedAttempts}; not retrying this head on my own.` + : ' (retrying on the next scan)') + ) .catch((e) => log(`failure notice: ${errText(e)}`)); } } finally { @@ -489,8 +507,14 @@ export class PrBot { return text; } - private async describeFailure(sessionId: string, outcome: TurnOutcome, started: number): Promise { + private async describeFailure( + sessionId: string, + outcome: TurnOutcome, + started: number, + lastText = '' + ): Promise { const minutes = Math.round((Date.now() - started) / 60_000); + const said = lastText.trim() ? `\nIts last message:\n${lastText.trim().slice(-900)}` : ''; switch (outcome.kind) { case 'blocked': { const screen = stripAnsi(await this.codeman.terminalText(sessionId).catch(() => '')); @@ -502,7 +526,7 @@ export class PrBot { case 'timeout': return `timed out after ${minutes} min without a report`; default: - return 'the session finished without writing report.json'; + return `the session finished after ${minutes} min without writing report.json${said}`; } } @@ -820,6 +844,7 @@ export class PrBot { return; } } + rec.failedAttempts = 0; const result = this.enqueueReview(n, { front: true }); if (result === 'busy') await this.telegram.sendMessage(`#${n} is being reviewed right now.`); else { @@ -1027,12 +1052,18 @@ export class PrBot { switch (pending.action) { case 'merge': { await mergePr(this.cfg.githubRepo, n); - await this.telegram.sendMessage(`๐ŸŽ‰ Merged #${n}${rec ? ` ยท ${escapeHtml(rec.title)}` : ''}.`); + this.selfClosed.add(n); + const fixes = + rec?.verdict === 'merge-with-fixes' + ? ' The review listed fixes to apply at merge time; they are not applied by merging (see /report).' + : ''; + await this.telegram.sendMessage(`๐ŸŽ‰ Merged #${n}${rec ? ` ยท ${escapeHtml(rec.title)}` : ''}.${fixes}`); this.scheduleScan(5000); return; } case 'close': { await closePr(this.cfg.githubRepo, n, pending.reason ?? ''); + this.selfClosed.add(n); await this.telegram.sendMessage(`๐Ÿ”’ Closed #${n}${rec ? ` ยท ${escapeHtml(rec.title)}` : ''}.`); this.scheduleScan(5000); return; diff --git a/scripts/pr-bot/state.ts b/scripts/pr-bot/state.ts index 4ce8db0c..c87078c7 100644 --- a/scripts/pr-bot/state.ts +++ b/scripts/pr-bot/state.ts @@ -41,6 +41,9 @@ export interface PrRecord { worktreeDir?: string; telegramMessageId?: number; lastError?: string; + /** Consecutive failed attempts at `failedSha`; the scan stops auto-retrying at MAX_AUTO_RETRIES. */ + failedAttempts?: number; + failedSha?: string; closedAs?: 'merged' | 'closed'; updatedAt: string; } diff --git a/test/pr-bot-commands.test.ts b/test/pr-bot-commands.test.ts index e4fef134..8b5b7337 100644 --- a/test/pr-bot-commands.test.ts +++ b/test/pr-bot-commands.test.ts @@ -134,7 +134,7 @@ describe('PrBot commands', () => { tg = new FakeTelegram(); bot = new PrBot(cfg, { telegram: tg, codeman: {} as CodemanClient, log: () => undefined }); const rec = bot.store.upsertPr(detail()); - Object.assign(rec, { status: 'reviewed', reviewedSha: 'abc123abc123', verdict: 'merge', report }); + Object.assign(rec, { status: 'reviewed', reviewedSha: 'abc123abc123', verdict: 'merge-with-fixes', report }); bot.store.save(); }); @@ -176,6 +176,18 @@ describe('PrBot commands', () => { expect(tg.last()).toContain('no longer valid'); }); + it('announces a bot-made merge once: the next scan retires the PR silently', async () => { + await bot.handleUpdate(msg('/merge 381')); + await bot.handleUpdate(cb(tg.confirmData())); + const merged = tg.sent.filter((s) => s.text.includes('Merged #381')); + expect(merged).toHaveLength(1); + expect(merged[0].text).toContain('fixes to apply at merge time'); // the verdict on the seeded record is merge-with-fixes below + const result = await bot.scanOnce('test'); // listOpenPrs is mocked to []: 381 is gone + expect(result.closed).toEqual([381]); + expect(bot.store.pr(381)?.closedAs).toBe('merged'); + expect(tg.sent.filter((s) => s.text.includes('Merged #381'))).toHaveLength(1); + }); + it('ignores a confirmation tap from a foreign chat', async () => { await bot.handleUpdate(msg('/merge 381')); await bot.handleUpdate(cb(tg.confirmData(), 2)); @@ -232,6 +244,6 @@ describe('PrBot commands', () => { it('reports status with verdict icons', async () => { await bot.handleUpdate(msg('/status')); - expect(tg.last()).toContain('โœ… #381'); + expect(tg.last()).toContain('๐ŸŸข #381'); }); });