From 5130ca66331d6c8f423f77848787b81a398d39df Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Tue, 8 Sep 2026 19:27:43 +0200 Subject: [PATCH] fix(pr-bot): fail fast when the review model's budget is spent Claude Code answers an exhausted model budget INSIDE the turn ("You've reached your Fable limit. Run /usage-credits to continue or switch models with /model.") and then sits there with nothing to write. The reviewer never produces a report, so `runTurn` waited out its full 40-minute deadline and reported a bare "timed out after 40 min without a report", which reads as a hung reviewer rather than an account that needs attention. Measured on 2026-09-08: #388, #393, #394 and #377 each lost 40 minutes this way, and because every attempt counted, all four reached MAX_AUTO_RETRIES and would NOT have been picked up again once the budget returned. One spent afternoon quietly took the whole queue out of service. `findModelLimitNotice()` reads the notice off the pane and `runTurn` returns a new `limit` outcome instead of waiting. It is consulted in exactly two places, both of which mean "the turn produced nothing": on a stop where `isDone()` is still false, and on each timed-out wait slice. A review that merely discusses usage limits in its own findings therefore cannot be mistaken for one that hit the wall, and the pattern matches neither the model name nor a straight apostrophe, since the pane renders a typographic one and every model prints the same sentence. A spent budget is an account condition, not a bad PR, so it no longer spends the per-head retry budget: the queue resumes by itself when the budget does. Telegram now names the cause and the file to change. Tests use the pane captured verbatim off the run that lost the 40 minutes. Co-Authored-By: Claude Opus 5 (1M context) --- docs/pr-bot.md | 2 +- scripts/pr-bot/bot.ts | 24 ++++++++++++--- scripts/pr-bot/codeman-client.ts | 52 ++++++++++++++++++++++++++++++-- scripts/pr-bot/worktree.ts | 2 +- test/pr-bot-report.test.ts | 34 ++++++++++++++++++++- 5 files changed, 104 insertions(+), 10 deletions(-) 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');