diff --git a/docs/pr-bot.md b/docs/pr-bot.md index 04117576..1bf91c77 100644 --- a/docs/pr-bot.md +++ b/docs/pr-bot.md @@ -90,7 +90,7 @@ has to be copied; set them here to use a different bot. | `PR_BOT_POLL_INTERVAL` | `600` | Seconds between GitHub polls (minimum 60). | | `PR_BOT_MAIN_CHECKOUT` | the repo this script is in | The repository the clones share objects with and fetch from. | | `PR_BOT_DATA_DIR` | `~/.codeman/pr-bot` | State, briefs, reports, clones. | -| `PR_BOT_MODEL` | unset (the session default) | Codeman `modelOverride` for the review sessions, e.g. `claude-fable-5-1`. | +| `PR_BOT_MODEL` | unset (the session default) | Codeman `modelOverride` for the review sessions, e.g. `opus[1m]`. ⚠️ Pick a model whose budget can absorb a re-review of every open PR on every head commit: when it runs out, Claude Code answers the limit **inside the turn** and the reviewer has nothing to write. The bot now names that failure in seconds (`findModelLimitNotice`) instead of burning the whole `PR_BOT_REVIEW_TIMEOUT`, and a limit does not spend the per-head retry budget, so the queue resumes by itself once the budget does. | | `PR_BOT_EFFORT` | unset | Codeman `effort` for the review sessions. | | `PR_BOT_REVIEW_TIMEOUT` | `40` | Minutes before a review is abandoned. | | `PR_BOT_FOLLOWUP_TIMEOUT` | `20` | Minutes before a follow-up is abandoned. | diff --git a/scripts/pr-bot/bot.ts b/scripts/pr-bot/bot.ts index 1526eb22..2e8bd2f4 100644 --- a/scripts/pr-bot/bot.ts +++ b/scripts/pr-bot/bot.ts @@ -19,7 +19,7 @@ import { randomBytes } from 'crypto'; import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'fs'; import { join } from 'path'; -import { CodemanClient, stripAnsi, type TurnOutcome } from './codeman-client.js'; +import { CodemanClient, ModelLimitError, stripAnsi, type TurnOutcome } from './codeman-client.js'; import type { PrBotConfig } from './config.js'; import { approveWorkflowRun, @@ -434,7 +434,10 @@ export class PrBot { if (report && !existsSync(reportMdPath)) writeFileSync(reportMdPath, last); } await this.recordClaudeSessionId(sessionId, rec); - if (!report) throw new Error(await this.describeFailure(sessionId, outcome, started, last)); + if (!report) { + const why = await this.describeFailure(sessionId, outcome, started, last); + throw outcome.kind === 'limit' ? new ModelLimitError(why) : new Error(why); + } const durationMin = Math.max(1, Math.round((Date.now() - started) / 60_000)); Object.assign(rec, { @@ -460,9 +463,15 @@ 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; + // A spent model budget is an account condition, not a bad PR, so it must not + // spend the per-head retry budget: otherwise one exhausted afternoon marks every + // open PR "not retrying on my own" and none of them come back when credits do. + const accountCondition = err instanceof ModelLimitError; + if (!accountCondition) { + rec.failedAttempts = rec.failedSha === rec.headSha ? (rec.failedAttempts ?? 0) + 1 : 1; + rec.failedSha = rec.headSha; + } + const givingUp = !accountCondition && (rec.failedAttempts ?? 0) >= MAX_AUTO_RETRIES; await this.telegram .sendMessage( formatReviewFailure(rec, reason) + @@ -523,6 +532,11 @@ export class PrBot { } case 'exit': return 'the session exited before writing a report'; + case 'limit': + return ( + `the model budget for these review sessions is spent, so the reviewer never started:\n${outcome.message}\n` + + 'Point PR_BOT_MODEL in ~/.codeman/pr-bot.env at a model with headroom and restart codeman-pr-bot.' + ); case 'timeout': return `timed out after ${minutes} min without a report`; default: diff --git a/scripts/pr-bot/codeman-client.ts b/scripts/pr-bot/codeman-client.ts index 0efde9c5..86511def 100644 --- a/scripts/pr-bot/codeman-client.ts +++ b/scripts/pr-bot/codeman-client.ts @@ -50,7 +50,42 @@ export interface SessionRecord { mode: string; } -export type TurnOutcome = { kind: 'stop' } | { kind: 'blocked' } | { kind: 'exit' } | { kind: 'timeout' }; +export type TurnOutcome = + | { kind: 'stop' } + | { kind: 'blocked' } + | { kind: 'exit' } + | { kind: 'timeout' } + | { kind: 'limit'; message: string }; + +/** + * Claude Code answers a spent model budget INSIDE the turn ("You've reached your Fable + * limit. Run /usage-credits to continue or switch models with /model.") and then simply + * sits there with nothing to write. Measured 2026-09-08: four reviews each burned their + * whole 40-minute deadline and reported a bare "timed out without a report", which reads + * as a hung reviewer rather than an account that needs attention, and the retries spent + * the per-head budget so the PRs would not have been picked up again once credits + * returned. Matching the notice turns 40 silent minutes into a named failure in seconds. + * + * Deliberately model-agnostic: the same sentence is printed for every model, and the + * apostrophe is typographic on the pane, so neither the model name nor `'` is matched. + */ +const MODEL_LIMIT_PATTERN = /reached your [^\n]{0,40}?\blimit\b|\/usage-credits/i; + +/** + * Thrown instead of a plain Error when a review died on a spent model budget, so the + * caller can tell an account condition apart from a review that genuinely failed. + */ +export class ModelLimitError extends Error { + override readonly name = 'ModelLimitError'; +} + +/** The limit notice as one clean line, or undefined if the screen does not carry it. */ +export function findModelLimitNotice(screen: string): string | undefined { + const line = stripAnsi(screen) + .split('\n') + .find((l) => MODEL_LIMIT_PATTERN.test(l)); + return line?.replace(/^[\s>|]*(?:\u23bf|\u2514|\u256d|\u2570|\u23a2|\u2502|\u23bd)?\s*/u, '').trim() || undefined; +} const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); @@ -241,9 +276,22 @@ export class CodemanClient { if (!r.delivered) throw new Error('the prompt was not delivered (pane dead?)'); let wait = r.wait; let nudged = false; + // Only consulted when the turn produced nothing, so a review that merely QUOTES the + // notice in its report cannot be mistaken for one that hit it. + const limitNotice = async (): Promise => + findModelLimitNotice(await this.terminalText(id).catch(() => '')); while (true) { - if (wait && !wait.timedOut) return toOutcome(wait); + if (wait && !wait.timedOut) { + const outcome = toOutcome(wait); + if (outcome.kind === 'stop' && !opts.isDone?.()) { + const limit = await limitNotice(); + if (limit) return { kind: 'limit', message: limit }; + } + return outcome; + } if (opts.isDone?.()) return { kind: 'stop' }; + const limit = await limitNotice(); + if (limit) return { kind: 'limit', message: limit }; const remaining = opts.deadlineMs - (Date.now() - started); if (remaining <= 0) return { kind: 'timeout' }; if (!nudged) { diff --git a/scripts/pr-bot/worktree.ts b/scripts/pr-bot/worktree.ts index a8b23787..9e09b4b8 100644 --- a/scripts/pr-bot/worktree.ts +++ b/scripts/pr-bot/worktree.ts @@ -11,7 +11,7 @@ * worktree's project settings through the git common dir, i.e. the MAIN checkout's * `.claude/settings.local.json`, whose model pin then silently overrides anything * written into the worktree (measured 2026-09-05: a worktree pinned to - * `claude-fable-5-1` reported `claude-opus-5[1m]`). A shared clone has its own + * `claude-fable-5-1` reported `claude-opus-5[1m]`, the main checkout's pin). A shared clone has its own * project root, so Codeman's `modelOverride` and hooks land where the CLI reads them, * while `objects/info/alternates` keeps the object store shared (no duplication). * diff --git a/test/pr-bot-report.test.ts b/test/pr-bot-report.test.ts index 0981fe73..e891ad4c 100644 --- a/test/pr-bot-report.test.ts +++ b/test/pr-bot-report.test.ts @@ -20,7 +20,7 @@ import { } from '../scripts/pr-bot/report.js'; import { classifyCi, latestRunPerWorkflow, type PrSummary, type WorkflowRun } from '../scripts/pr-bot/github.js'; import { parseCallback, parseCommand, prNumberFromMessageText } from '../scripts/pr-bot/telegram.js'; -import { trustDialogKey } from '../scripts/pr-bot/codeman-client.js'; +import { findModelLimitNotice, trustDialogKey } from '../scripts/pr-bot/codeman-client.js'; import { buildConfig, parseEnvFile } from '../scripts/pr-bot/config.js'; import { buildReviewBrief } from '../scripts/pr-bot/review-task.js'; @@ -302,6 +302,38 @@ describe('trustDialogKey', () => { }); }); +describe('findModelLimitNotice', () => { + // Captured off prbot-394's pane on 2026-09-08, the run that lost 40 minutes: Claude + // Code answers a spent budget inside the turn and then simply sits there. + const SPENT_PANE = [ + '\x1b[38;5;153m\u276f\x1b[39m Read /home/arkon/.codeman/pr-bot/jobs/pr-394/brief.md and do the review.', + " \u23bf You've reached your Fable limit. Run /usage-credits to continue or switch models with /model.", + '\u273b Saut\u00e9ed for 1s \u00b7 done 7:16 PM', + ].join('\n'); + + it('finds the notice on a real pane, ANSI and gutter glyph stripped', () => { + expect(findModelLimitNotice(SPENT_PANE)).toBe( + "You've reached your Fable limit. Run /usage-credits to continue or switch models with /model." + ); + }); + + it('is not tied to one model name or to a straight apostrophe', () => { + // The pane renders a typographic apostrophe, and every model prints this sentence. + expect( + findModelLimitNotice(' \u23bf You\u2019ve reached your Opus limit. Run /usage-credits to continue.') + ).toContain('reached your Opus limit'); + expect(findModelLimitNotice('You have reached your Sonnet 5 limit.')).toContain('Sonnet 5'); + }); + + it('says nothing about an ordinary working pane', () => { + expect(findModelLimitNotice('\u273b Actualizing\u2026 (13m 23s \u00b7 esc to interrupt)')).toBeUndefined(); + expect(findModelLimitNotice('')).toBeUndefined(); + // The bare word is not the notice: a review whose own findings discuss usage limits + // must not be reported as an exhausted account. + expect(findModelLimitNotice('the usage limit parser handles the 5-hour reset')).toBeUndefined(); + }); +}); + describe('config', () => { it('parses env files with quotes, comments and export prefixes', () => { const env = parseEnvFile('# c\nexport A="x y"\nB=\'z\'\nC=plain\nbad line\n=nokey\n');